| Message ID | 20180419150145.24795-6-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
Hi On Thu, Apr 19, 2018 at 5:39 PM, Eric Blake <eblake@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.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. 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.
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h index 25a83aa619..fe8643634c 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++; return obj; } @@ -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) */ #define qobject_unref(obj) qobject_unref_impl(QOBJECT(obj)) diff --git a/block.c b/block.c index 676e57f562..9f4d7c79af 100644 --- a/block.c +++ 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); found_any = true; } diff --git a/block/blkdebug.c b/block/blkdebug.c index 053372c22e..1876a42c0e 100644 --- a/block/blkdebug.c +++ 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); 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 a5051da56e..0304f248ed 100644 --- a/block/quorum.c +++ 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); } opts = qdict_new(); diff --git a/monitor.c b/monitor.c index 46814af533..5dc0c34799 100644 --- a/monitor.c +++ b/monitor.c @@ -4187,7 +4187,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) req_obj = g_new0(QMPRequest, 1); req_obj->mon = mon; - req_obj->id = qobject_ref(id); + req_obj->id = id ? qobject_ref(id) : NULL; req_obj->req = req; req_obj->need_resume = false;
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: * 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: optional "id" case must be handled, in 2 places - monitor_json_emitter(): only called with data != NULL - monitor_qapi_event_queue(): called from func_emit, by qapi events, assert earlier during construction in qobject_output_complete() * 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 | 9 +++++---- block/blkdebug.c | 3 ++- block/quorum.c | 3 ++- monitor.c | 2 +- 5 files changed, 14 insertions(+), 10 deletions(-)