Re: [Qemu-devel] [PATCH v6 5/5] qobject: modify qobject_ref() to assert on NULL

[Marc-André Lureau]

Hi

On Thu, Apr 19, 2018 at 5:39 PM, Eric Blake  wrote:
> On 04/19/2018 10:01 AM, Marc-André Lureau wrote:
>> While it may be convenient to accept NULL value in
>> qobject_unref() (for similar reasons as free() accepts NULL), it is
>> not such a good idea for qobject_ref(): now assert() on NULL.
>>
>> Some places relied on that behaviour (the monitor request id for
>> example), and it's best to be explicit that NULL is accepted there.
>> We have to rely on testing, and manual inspection of qobject_ref()
>> usage:
>>
>
>> Signed-off-by: Marc-André Lureau 
>> Reviewed-by: Eric Blake 
>
> Again, you've made a substantial change since v5 (more hunks added, and
> justification in the commit message that needs double-checking that your
> audit was sane), so I would have dropped R-b.

ok

>
>> ---
>>  include/qapi/qmp/qobject.h | 7 ---
>>  block.c| 9 +
>>  block/blkdebug.c   | 3 ++-
>>  block/quorum.c | 3 ++-
>>  monitor.c  | 2 +-
>>  5 files changed, 14 insertions(+), 10 deletions(-)
>>
>
>> @@ -104,6 +103,7 @@ static inline void qobject_unref_impl(QObject *obj)
>>
>>  /**
>>   * qobject_ref(): Increment QObject's reference count
>> + * @obj: a #QObject or child type instance (must not be NULL)
>>   *
>>   * Returns: the same @obj. The type of @obj will be propagated to the
>>   * return type.
>> @@ -117,6 +117,7 @@ static inline void qobject_unref_impl(QObject *obj)
>>  /**
>>   * qobject_unref(): Decrement QObject's reference count, deallocate
>>   * when it reaches zero
>> + * @obj: a #QObject or children type instance (can be NULL)
>
> s/children/child/
>
>> +++ b/block.c
>> @@ -5110,8 +5110,9 @@ static bool append_open_options(QDict *d, 
>> BlockDriverState *bs)
>>  const char *p;
>>
>>  for (entry = qdict_first(bs->options); entry;
>> - entry = qdict_next(bs->options, entry))
>> -{
>> + entry = qdict_next(bs->options, entry)) {
>> +QObject *val;
>> +
>>  /* Exclude options for children */
>>  QLIST_FOREACH(child, >children, next) {
>>  if (strstart(qdict_entry_key(entry), child->name, )
>> @@ -5134,8 +5135,8 @@ static bool append_open_options(QDict *d, 
>> BlockDriverState *bs)
>>  continue;
>>  }
>>
>> -qdict_put_obj(d, qdict_entry_key(entry),
>> -  qobject_ref(qdict_entry_value(entry)));
>> +val = qdict_entry_value(entry);
>> +qdict_put_obj(d, qdict_entry_key(entry), val ? qobject_ref(val) : 
>> NULL);
>
> I don't think we allow pushing NULL into qdict; we should probably beef
> up the testsuite and/or add asserts to qdict_put_obj(), at which point
> this hunk is spurious.
>
>> +++ b/block/blkdebug.c
>> @@ -845,7 +845,8 @@ static void blkdebug_refresh_filename(BlockDriverState 
>> *bs, QDict *options)
>>  opts = qdict_new();
>>  qdict_put_str(opts, "driver", "blkdebug");
>>
>> -qdict_put(opts, "image", qobject_ref(bs->file->bs->full_open_options));
>> +qdict_put(opts, "image", bs->file->bs->full_open_options ?
>> +  qobject_ref(bs->file->bs->full_open_options) : NULL);
>
> Likewise, this hunk is spurious if we can't push NULL into a QDict.
>
>> +++ b/block/quorum.c
>> @@ -1083,7 +1083,8 @@ static void quorum_refresh_filename(BlockDriverState 
>> *bs, QDict *options)
>>  children = qlist_new();
>>  for (i = 0; i < s->num_children; i++) {
>>  qlist_append(children,
>> - qobject_ref(s->children[i]->bs->full_open_options));
>> + s->children[i]->bs->full_open_options ?
>> + qobject_ref(s->children[i]->bs->full_open_options) : 
>> NULL);
>
> And again, but for QList.


Yes, for now I stayed on the safe side. Open-questions in the commit message.

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v6 5/5] qobject: modify qobject_ref() to assert on NULL

[Eric Blake]

On 04/19/2018 10:01 AM, Marc-André Lureau wrote:
> While it may be convenient to accept NULL value in
> qobject_unref() (for similar reasons as free() accepts NULL), it is
> not such a good idea for qobject_ref(): now assert() on NULL.
> 
> Some places relied on that behaviour (the monitor request id for
> example), and it's best to be explicit that NULL is accepted there.
> We have to rely on testing, and manual inspection of qobject_ref()
> usage:
> 

> Signed-off-by: Marc-André Lureau 
> Reviewed-by: Eric Blake 

Again, you've made a substantial change since v5 (more hunks added, and
justification in the commit message that needs double-checking that your
audit was sane), so I would have dropped R-b.

> ---
>  include/qapi/qmp/qobject.h | 7 ---
>  block.c| 9 +
>  block/blkdebug.c   | 3 ++-
>  block/quorum.c | 3 ++-
>  monitor.c  | 2 +-
>  5 files changed, 14 insertions(+), 10 deletions(-)
> 

> @@ -104,6 +103,7 @@ static inline void qobject_unref_impl(QObject *obj)
>  
>  /**
>   * qobject_ref(): Increment QObject's reference count
> + * @obj: a #QObject or child type instance (must not be NULL)
>   *
>   * Returns: the same @obj. The type of @obj will be propagated to the
>   * return type.
> @@ -117,6 +117,7 @@ static inline void qobject_unref_impl(QObject *obj)
>  /**
>   * qobject_unref(): Decrement QObject's reference count, deallocate
>   * when it reaches zero
> + * @obj: a #QObject or children type instance (can be NULL)

s/children/child/

> +++ b/block.c
> @@ -5110,8 +5110,9 @@ static bool append_open_options(QDict *d, 
> BlockDriverState *bs)
>  const char *p;
>  
>  for (entry = qdict_first(bs->options); entry;
> - entry = qdict_next(bs->options, entry))
> -{
> + entry = qdict_next(bs->options, entry)) {
> +QObject *val;
> +
>  /* Exclude options for children */
>  QLIST_FOREACH(child, >children, next) {
>  if (strstart(qdict_entry_key(entry), child->name, )
> @@ -5134,8 +5135,8 @@ static bool append_open_options(QDict *d, 
> BlockDriverState *bs)
>  continue;
>  }
>  
> -qdict_put_obj(d, qdict_entry_key(entry),
> -  qobject_ref(qdict_entry_value(entry)));
> +val = qdict_entry_value(entry);
> +qdict_put_obj(d, qdict_entry_key(entry), val ? qobject_ref(val) : 
> NULL);

I don't think we allow pushing NULL into qdict; we should probably beef
up the testsuite and/or add asserts to qdict_put_obj(), at which point
this hunk is spurious.

> +++ b/block/blkdebug.c
> @@ -845,7 +845,8 @@ static void blkdebug_refresh_filename(BlockDriverState 
> *bs, QDict *options)
>  opts = qdict_new();
>  qdict_put_str(opts, "driver", "blkdebug");
>  
> -qdict_put(opts, "image", qobject_ref(bs->file->bs->full_open_options));
> +qdict_put(opts, "image", bs->file->bs->full_open_options ?
> +  qobject_ref(bs->file->bs->full_open_options) : NULL);

Likewise, this hunk is spurious if we can't push NULL into a QDict.

> +++ b/block/quorum.c
> @@ -1083,7 +1083,8 @@ static void quorum_refresh_filename(BlockDriverState 
> *bs, QDict *options)
>  children = qlist_new();
>  for (i = 0; i < s->num_children; i++) {
>  qlist_append(children,
> - qobject_ref(s->children[i]->bs->full_open_options));
> + s->children[i]->bs->full_open_options ?
> + qobject_ref(s->children[i]->bs->full_open_options) : 
> NULL);

And again, but for QList.

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



signature.asc
Description: OpenPGP digital signature