Hi On Thu, Apr 19, 2018 at 5:39 PM, Eric Blake <ebl...@redhat.com> wrote: > 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.lur...@redhat.com> >> Reviewed-by: Eric Blake <ebl...@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.
ok > >> --- >> 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. Yes, for now I stayed on the safe side. Open-questions in the commit message. -- Marc-André Lureau