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

[Eric Blake]

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 <address@hidden>
> ---

> @@ -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