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