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

[Marc-André Lureau]

Hi

On Thu, Apr 19, 2018 at 8:18 AM, Markus Armbruster <address@hidden> wrote:
> Marc-André Lureau <address@hidden> writes:
>
>> 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(), assert() on NULL. One place
>> relied on that behaviour (the monitor request id), and it's best to be
>> explicit that NULL is accepted there.
>>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> ---
>>  include/qapi/qmp/qobject.h | 7 ++++---
>>  monitor.c                  | 2 +-
>>  2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
>> index befc945504..a0b2affb2c 100644
>> --- a/include/qapi/qmp/qobject.h
>> +++ b/include/qapi/qmp/qobject.h
>> @@ -73,9 +73,8 @@ static inline void qobject_init(QObject *obj, QType type)
>>
>>  static inline void *qobject_ref_impl(QObject *obj)
>>  {
>> -    if (obj) {
>> -        obj->base.refcnt++;
>> -    }
>> +    assert(obj);
>> +    obj->base.refcnt++;
>>      return obj;
>>  }
>>
>> @@ -103,6 +102,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.
>> @@ -112,6 +112,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)
>>   */
>>  #define qobject_unref(obj) qobject_unref_impl(QOBJECT(obj))
>>
>> diff --git a/monitor.c b/monitor.c
>> index 7dbc1f74b8..3a750208fd 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -4188,7 +4188,7 @@ static void handle_qmp_command(JSONMessageParser 
>> *parser, GQueue *tokens)
>>
>>      req_obj = g_new0(QMPRequest, 1);
>>      req_obj->mon = mon;
>> -    req_obj->id = qobject_ref(id);
>> +    req_obj->id = id ? qobject_ref(id) : NULL;
>>      req_obj->req = req;
>>      req_obj->need_resume = false;
>
> What's your argument that this is the only use of qobject_ref() that
> needs to be guarded now?

Right, that's the only one that is obvious, as it fails in test, and
the code is pretty obvious about id being optional.

For the rest, we have to rely on testing, and manual inspection of
qobject_ref() usage:

- block.c: guarded in bdrv_refresh_filename(), more tricky for
append_open_options(), as it depends if qdict values could be NULL
- block/blkdebug.c: depends if qdict values could be NULL,
full_open_options could NULL apparently, so should probably be handled
FIXED
- block/blkverify.c: guarded
- block/{null,nvme}.c: guaded, previous qdict_del() (actually
qdict_find()) guarantee non-NULL)
- block/quorum.c: if full_open_options can be NULL, we should handle it. FIXED
- monitor: optional "id" case must be handled, done in 2 places
  monitor_json_emitter(): only called with data != NULL, we could add
an earlier assert
  monitor_qapi_event_queue(): called from func_emit, by qapi events,
assert earlier during construction in qobject_output_complete()
- qapi/qmp-dispatch.c: if "arguments" exists, it can't be NULL during
json parsing
- qapi/qobject-input-visitor.c: guarded by assert in visit_type_any()
- qapi/qobject-output-visitor.c: guarded by assert in visit_type_any()
for qobject_output_type_any, guarded by assert() in
qobject_output_complete
- qmp.c: guarded, error out before if NULL
- qobject/q{list,dict}.c: can accept NULL values apparently, what's
the reason? how are you supposed to handle that?
- tests/*: let's consider this is covered by make check
- util/keyval.c: guarded, if (!elt[i]) before the ref

Except a very few corner cases, it looks quite safe to me. But, if
necessary, we can add if obj ? obj_ref(obj) : NULL for the dubious
cases.

-- 
Marc-André Lureau