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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3830 bytes --]

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

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, &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.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]