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

[Markus Armbruster]

Marc-André Lureau <address@hidden> 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(), assert() on NULL. One place
> relied on that behaviour (the monitor request id), and it's best to be
> explicit that NULL is accepted there.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  include/qapi/qmp/qobject.h | 7 ++++---
>  monitor.c                  | 2 +-
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
> index befc945504..a0b2affb2c 100644
> --- a/include/qapi/qmp/qobject.h
> +++ b/include/qapi/qmp/qobject.h
> @@ -73,9 +73,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;
>  }
>  
> @@ -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.
> @@ -112,6 +112,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/monitor.c b/monitor.c
> index 7dbc1f74b8..3a750208fd 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4188,7 +4188,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;

What's your argument that this is the only use of qobject_ref() that
needs to be guarded now?