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 <address@hidden> 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 <address@hidden>
>> Reviewed-by: Eric Blake <address@hidden>
>
> 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, &bs->children, next) {
>>              if (strstart(qdict_entry_key(entry), child->name, &p)
>> @@ -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