[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 5/5] qobject: modify qobject_ref() to assert
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v6 5/5] qobject: modify qobject_ref() to assert on NULL |
Date: |
Thu, 19 Apr 2018 10:39:35 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 |
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.
> ---
> 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
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v6 1/5] qobject: ensure base is at offset 0, (continued)
Re: [Qemu-devel] [PATCH v6 1/5] qobject: ensure base is at offset 0, Markus Armbruster, 2018/04/27
[Qemu-devel] [PATCH v6 2/5] qobject: use a QObjectBase_ struct, Marc-André Lureau, 2018/04/19
[Qemu-devel] [PATCH v6 5/5] qobject: modify qobject_ref() to assert on NULL, Marc-André Lureau, 2018/04/19
- Re: [Qemu-devel] [PATCH v6 5/5] qobject: modify qobject_ref() to assert on NULL,
Eric Blake <=
[Qemu-devel] [PATCH v6 4/5] qobject: modify qobject_ref() to return obj, Marc-André Lureau, 2018/04/19
[Qemu-devel] [PATCH v6 3/5] qobject: replace qobject_incref/QINCREF qobject_decref/QDECREF, Marc-André Lureau, 2018/04/19
Re: [Qemu-devel] [PATCH v6 0/5] Simplify qobject refcount, Eric Blake, 2018/04/19