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

[Marc-André Lureau]

Hi
On Fri, Aug 24, 2018 at 10:12 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(): now assert() on NULL.
>
> Why is it not such a good idea?
>
> Is it unsafe?  Unclean?  Ugly?
>
> If unsafe, can you point to actual problems, present or past?
>
> If you consider it unclean or ugly, can you point to established
> convention?

In general, a function/method shouldn't accept NULL as argument, as it
is usually a programmer mistake.

free() / unref() are exceptions, for convenience.

fwiw, g_object_ref/unref() do not accept NULL. However,
g_clear_object() was introduced later to simply clearing an object
reference (unref and set to NULL, checks NULL).

>
> > Some code relied on that behaviour, but it's best to be explicit that
> > NULL is accepted.  We have to rely on testing, and manual inspection
> > of qobject_ref() usage:
> >
> > * block.c:
> >  - bdrv_refresh_filename(): guarded
> >  - append_open_options(): it depends if qdict values could be NULL,
> >    handled for extra safety, might be unnecessary
> >
> > * block/blkdebug.c:
> >  - blkdebug_refresh_filename(): depends if qdict values could be NULL,
> >    full_open_options could be NULL apparently, handled
> >
> > * block/blkverify.c: guarded
> >
> > * block/{null,nvme}.c: guarded, previous qdict_del() (actually
> >   qdict_find()) guarantee non-NULL)
> >
> > * block/quorum.c: full_open_options could be NULL, handled for extra
> >   safety, might be unnecessary
> >
> > * monitor: events have associated qdict, but may not have 'data' dict
> >   entry. Command 'id' is already guarded. A queued response is
> >   non-NULL.
> >
> > * 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()
> >   qobject_output_complete(): guarded by pre-condition assert()
> >
> > * 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? no test?
> >
> >   Some code, such as qdict_flatten_qdict(), assume the value is always
> >   non-NULL for example.
> >
> > - tests/*: considered to be covered by make check, not critical
> >
> > - util/keyval.c: guarded, if (!elt[i]) before
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
>
> That's a lot of analysis to support a change...
>
> > ---
> >  include/qapi/qmp/qobject.h | 7 ++++---
> >  block.c                    | 6 ++++--
> >  block/blkdebug.c           | 3 ++-
> >  block/quorum.c             | 3 ++-
> >  monitor.c                  | 2 +-
> >  5 files changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
> > index fcfd549220..2fe5b42579 100644
> > --- a/include/qapi/qmp/qobject.h
> > +++ b/include/qapi/qmp/qobject.h
> > @@ -74,9 +74,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++;
> >  }
> >
> >  /**
> > @@ -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.
> > @@ -116,6 +116,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 child type instance (can be NULL)
> >   */
> >  #define qobject_unref(obj) qobject_unref_impl(QOBJECT(obj))
> >
> > diff --git a/block.c b/block.c
> > index 6161dbe3eb..f1e35c3c1e 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -5154,6 +5154,8 @@ static bool append_open_options(QDict *d, 
> > BlockDriverState *bs)
> >      for (entry = qdict_first(bs->options); entry;
> >           entry = qdict_next(bs->options, entry))
> >      {
> > +        QObject *val;
> > +
> >          /* Exclude node-name references to children */
> >          QLIST_FOREACH(child, &bs->children, next) {
> >              if (!strcmp(entry->key, child->name)) {
> > @@ -5174,8 +5176,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);
> >          found_any = true;
> >      }
> >
> > diff --git a/block/blkdebug.c b/block/blkdebug.c
> > index 0759452925..062263f7e1 100644
> > --- a/block/blkdebug.c
> > +++ b/block/blkdebug.c
> > @@ -846,7 +846,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);
> >
> >      for (e = qdict_first(options); e; e = qdict_next(options, e)) {
> >          if (strcmp(qdict_entry_key(e), "x-image")) {
> > diff --git a/block/quorum.c b/block/quorum.c
> > index 9152da8c58..96cd094ede 100644
> > --- a/block/quorum.c
> > +++ b/block/quorum.c
> > @@ -1089,7 +1089,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);
> >      }
> >
> >      opts = qdict_new();
> > diff --git a/monitor.c b/monitor.c
> > index eab2dc7b7b..be4bcd82a0 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -675,7 +675,7 @@ monitor_qapi_event_queue_no_reenter(QAPIEvent event, 
> > QDict *qdict)
> >
> >              evstate = g_new(MonitorQAPIEventState, 1);
> >              evstate->event = event;
> > -            evstate->data = qobject_ref(data);
> > +            evstate->data = data ? qobject_ref(data) : NULL;
> >              evstate->qdict = NULL;
> >              evstate->timer = timer_new_ns(monitor_get_event_clock(),
> >                                            monitor_qapi_event_handler,
>
> ... that only complicates code.  By not much, granted.  My problem with
> it is that the commit message doesn't convince me it's an improvement.
>


-- 
Marc-André Lureau