Re: [Qemu-devel] [PATCH v3 4/4] qobject: modify qobject_ref() to assert on NULL and return obj

From: Eric Blake <eblake@redhat.com>
To: "Marc-André Lureau" <marcandre.lureau@redhat.com>, qemu-devel@nongnu.org
Cc: berrange@redhat.com, armbru@redhat.com, pbonzini@redhat.com
Subject: Re: [Qemu-devel] [PATCH v3 4/4] qobject: modify qobject_ref() to assert on NULL and return obj
Date: Thu, 29 Mar 2018 11:10:47 -0500	[thread overview]
Message-ID: <49475649-b8d8-e09a-8e95-7ce36c7396f2@redhat.com> (raw)
In-Reply-To: <20180329154833.566-5-marcandre.lureau@redhat.com>

On 03/29/2018 10:48 AM, Marc-André Lureau wrote:
> Following a discussion on the mailing list: while it may be convenient
> to accept NULL value in qobject_unref() (for similar reasons as free()
> accepts NULL), it is a probably a bad idea to accept NULL argument in
> qobject_ref().
> 
> Furthermore, it is convenient and more clear to call qobject_ref() at
> the time when the reference is associated with a variable, or
> argument. For this reason, make qobject_ref() return the same pointer
> as given.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---

> @@ -101,13 +101,18 @@ static inline void qobject_unref(QObject *obj)
>   
>   /**
>    * qobject_ref(): Increment QObject's reference count
> + * @obj: a #QObject or children type instance (must not be NULL)

s/children/child/

> + *
> + * Returns: the same @obj. The type of @obj will be propagated to the
> + * return type.
>    */
>   #define qobject_ref(obj)                        \
> -    qobject_ref(QOBJECT(obj))
> +    (typeof(obj)) qobject_ref(QOBJECT(obj))

You're missing outer ().  There are cases where '(cast) (expr)' and 
'((cast)(expr))' can behave differently when combined with surrounding 
syntax; for example, -> has higher precedence than cast.  Consider:

qobject_ref(my_qdict)->size;

As you wrote it, you would attempt to dereference the 'size' member of 
void* (compile error), prior to casting that result to QDict*; with the 
parenthesis, you get the intended dereference of the size member of the 
QDict pointer.

> +++ b/block.c
> @@ -5134,8 +5134,8 @@ static bool append_open_options(QDict *d, BlockDriverState *bs)
>               continue;
>           }
>   
> -        qobject_ref(qdict_entry_value(entry));
> -        qdict_put_obj(d, qdict_entry_key(entry), qdict_entry_value(entry));
> +        qdict_put_obj(d, qdict_entry_key(entry),
> +                      qobject_ref(qdict_entry_value(entry)));

However, I like this simplification that your patch enables.  How did 
you find candidate spots? Manually, or via Coccinelle?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org