[2/2] qobject: modify qobject_ref() to assert on NULL

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 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 <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 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(-)

On Fri 17 Aug 2018 07:19:32 PM CEST, Marc-André Lureau wrote:
> 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();

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Marc-André Lureau <marcandre.lureau@redhat.com> 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?

> 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 <marcandre.lureau@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

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.

Hi
On Fri, Aug 24, 2018 at 10:12 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> 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 <marcandre.lureau@redhat.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
>
> 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 <marcandre.lureau@gmail.com> writes:

> Hi
> On Fri, Aug 24, 2018 at 10:12 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> 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.

There are certainly plenty of specific functions where the claim is
true.  But a blanket claim like yours need to be supported by evidence.

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

Here's the argument for cleanup functions accepting null pointers.

Having cleanup functions reject null pointers complicates cleanup for no
gain.  Examples of this are everywhere.  Common pattern:

    T1 *v1 = NULL;
    ...
    Tn *vn = NULL;

    ... do stuff, create v1, ..., vn, goto out on error ...

out:
    T1_cleanup(v1);
    ...
    Tn_cleanup(vn);

Guarding the Ti_cleanup() with if (vi) clutters the code.  Moving the
guard into the Ti_cleanup() is an obvious improvement.

Conversely, examples where a cleanup function rejecting null would catch
actual mistakes do not come to mind.

qobject_unref() is an instance of this pattern.

qobject_ref() isn't, but there are similarities.  Again, examples exist
where rejecting null buys us nothing but additional code to guard the
call.  These are visible in your patch.  And again, examples where
rejecting null would catch mistakes do not come to (my) mind.  That's
why I'm asking for them.

> 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).

[...]

Hi
On Fri, Aug 24, 2018 at 3:24 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>
> > Hi
> > On Fri, Aug 24, 2018 at 10:12 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Marc-André Lureau <marcandre.lureau@redhat.com> 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.
>
> There are certainly plenty of specific functions where the claim is
> true.  But a blanket claim like yours need to be supported by evidence.

There is no evidence for such claim, only idiomatic code or common
patterns. It's clear when the caller handles/expects NULL cases.
That's what the patch does, there are a few places where this is made
explicit, and imho that improves readability.

For ex:
            evstate->data = qobject_ref(data);

You might assume, by just reading that line, that evstate->data != NULL.

But if you rewrite:

            evstate->data = data ? qobject_ref(data) : NULL;

Then it's clear that field accpets both cases, and programmer has
thought about that case, it's not handled by mistake (where it could
be a bug).


>
> > free() / unref() are exceptions, for convenience.
>
> Here's the argument for cleanup functions accepting null pointers.
>
> Having cleanup functions reject null pointers complicates cleanup for no
> gain.  Examples of this are everywhere.  Common pattern:
>
>     T1 *v1 = NULL;
>     ...
>     Tn *vn = NULL;
>
>     ... do stuff, create v1, ..., vn, goto out on error ...
>
> out:
>     T1_cleanup(v1);
>     ...
>     Tn_cleanup(vn);
>
> Guarding the Ti_cleanup() with if (vi) clutters the code.  Moving the
> guard into the Ti_cleanup() is an obvious improvement.
>
> Conversely, examples where a cleanup function rejecting null would catch
> actual mistakes do not come to mind.
>
> qobject_unref() is an instance of this pattern.
>
> qobject_ref() isn't, but there are similarities.  Again, examples exist
> where rejecting null buys us nothing but additional code to guard the
> call.  These are visible in your patch.  And again, examples where
> rejecting null would catch mistakes do not come to (my) mind.  That's
> why I'm asking for them.
>
> > 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).
>
> [...]