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

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

Hi

On Thu, Mar 29, 2018 at 6:10 PM, Eric Blake <eblake@redhat.com> wrote:
> 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.
>

ok

>> +++ 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?

Manual review (there are not that many qobject_ref())

thanks