Re: [Qemu-devel] [PATCH v4 3/4] qobject: replace qobject_incref/QINCREF qobject_decref/QDECREF

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Now that we can safely call QOBJECT() on QObject * and children types,
> we can have a single macro to ref/unref the object.
>
> Change the incref/decref prefix name for the more common ref/unref.
>
> Note that before the patch, "QDECREF(obj)" was problematic because it
> would expand to "obj ? obj : ...", which could evaluate "obj" multiple
> times.

Problematic just in theory, or have you seen problematic uses?

Perhaps what you want to say is that spelling your new macros lower case
is okay, because they evaluate their argument just like a function.

Suggest

  Now that we can safely call QOBJECT() on QObject * as well as its
  subtypes, we can have macros qobject_ref() / qobject_unref() that work
  everywhere instead of having to use QINCREF() / QDECREF() for QObject
  and qobject_incref() / qobject_decref() for its subtypes.

  Note that the new macros evalcuate their argument exactly once, thus
  no need to shout them.

>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  scripts/qapi/events.py              |   2 +-
>  include/qapi/qmp/qnull.h            |   2 +-
>  include/qapi/qmp/qobject.h          |  36 +++++-----
>  block.c                             |  78 +++++++++++-----------
>  block/blkdebug.c                    |   4 +-
>  block/blkverify.c                   |   4 +-
>  block/crypto.c                      |   4 +-
>  block/gluster.c                     |   4 +-
>  block/iscsi.c                       |   2 +-
>  block/nbd.c                         |   4 +-
>  block/nfs.c                         |   4 +-
>  block/null.c                        |   2 +-
>  block/nvme.c                        |   2 +-
>  block/parallels.c                   |   4 +-
>  block/qapi.c                        |   2 +-
>  block/qcow.c                        |   8 +--
>  block/qcow2.c                       |   8 +--
>  block/qed.c                         |   4 +-
>  block/quorum.c                      |   2 +-
>  block/rbd.c                         |  14 ++--
>  block/sheepdog.c                    |  12 ++--
>  block/snapshot.c                    |   4 +-
>  block/ssh.c                         |   4 +-
>  block/vdi.c                         |   2 +-
>  block/vhdx.c                        |   4 +-
>  block/vpc.c                         |   4 +-
>  block/vvfat.c                       |   2 +-
>  block/vxhs.c                        |   2 +-
>  blockdev.c                          |  16 ++---
>  hw/i386/acpi-build.c                |  12 ++--
>  hw/ppc/spapr_drc.c                  |   2 +-
>  hw/usb/xen-usb.c                    |   4 +-
>  migration/migration.c               |   4 +-
>  migration/qjson.c                   |   2 +-
>  monitor.c                           |  50 +++++++-------
>  qapi/qapi-dealloc-visitor.c         |   4 +-
>  qapi/qmp-dispatch.c                 |   6 +-
>  qapi/qobject-input-visitor.c        |   8 +--
>  qapi/qobject-output-visitor.c       |   8 +--
>  qemu-img.c                          |  18 ++---
>  qemu-io.c                           |   6 +-
>  qga/main.c                          |  12 ++--
>  qmp.c                               |   4 +-
>  qobject/json-parser.c               |  10 +--
>  qobject/qdict.c                     |  38 +++++------
>  qobject/qjson.c                     |   2 +-
>  qobject/qlist.c                     |   4 +-
>  qom/object.c                        |  16 ++---
>  qom/object_interfaces.c             |   2 +-
>  target/ppc/translate_init.c         |   2 +-
>  target/s390x/cpu_models.c           |   2 +-
>  tests/ahci-test.c                   |   6 +-
>  tests/check-qdict.c                 | 100 ++++++++++++++--------------
>  tests/check-qjson.c                 |  84 +++++++++++------------
>  tests/check-qlist.c                 |   8 +--
>  tests/check-qlit.c                  |  10 +--
>  tests/check-qnull.c                 |  10 +--
>  tests/check-qnum.c                  |  28 ++++----
>  tests/check-qobject.c               |   2 +-
>  tests/check-qstring.c               |  10 +--
>  tests/cpu-plug-test.c               |   4 +-
>  tests/device-introspect-test.c      |  24 +++----
>  tests/drive_del-test.c              |   4 +-
>  tests/libqos/libqos.c               |   8 +--
>  tests/libqos/pci-pc.c               |   2 +-
>  tests/libqtest.c                    |  24 +++----
>  tests/machine-none-test.c           |   2 +-
>  tests/migration-test.c              |  24 +++----
>  tests/numa-test.c                   |  16 ++---
>  tests/pvpanic-test.c                |   2 +-
>  tests/q35-test.c                    |   2 +-
>  tests/qmp-test.c                    |  38 +++++------
>  tests/qom-test.c                    |   8 +--
>  tests/tco-test.c                    |  12 ++--
>  tests/test-char.c                   |   2 +-
>  tests/test-keyval.c                 |  82 +++++++++++------------
>  tests/test-netfilter.c              |  26 ++++----
>  tests/test-qemu-opts.c              |  14 ++--
>  tests/test-qga.c                    |  76 ++++++++++-----------
>  tests/test-qmp-cmds.c               |  24 +++----
>  tests/test-qmp-event.c              |   2 +-
>  tests/test-qobject-input-visitor.c  |  10 +--
>  tests/test-qobject-output-visitor.c |  18 ++---
>  tests/test-visitor-serialization.c  |   6 +-
>  tests/test-x86-cpuid-compat.c       |  14 ++--
>  tests/tmp105-test.c                 |   4 +-
>  tests/vhost-user-test.c             |   6 +-
>  tests/virtio-net-test.c             |   6 +-
>  tests/vmgenid-test.c                |   2 +-
>  tests/wdt_ib700-test.c              |  14 ++--
>  util/keyval.c                       |  12 ++--
>  util/qemu-config.c                  |   4 +-
>  docs/devel/qapi-code-gen.txt        |   2 +-
>  scripts/coccinelle/qobject.cocci    |   8 +--
>  94 files changed, 606 insertions(+), 610 deletions(-)

It's a lot of churn.  I'm willing to accept the one-time pain because
the result is easier on the eyes.

> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> index 3dc523cf39..4426861ff1 100644
> --- a/scripts/qapi/events.py
> +++ b/scripts/qapi/events.py
> @@ -142,7 +142,7 @@ out:
>  ''')
>      ret += mcgen('''
>      error_propagate(errp, err);
> -    QDECREF(qmp);
> +    qobject_unref(qmp);
>  }
>  ''')
>      return ret
> diff --git a/include/qapi/qmp/qnull.h b/include/qapi/qmp/qnull.h
> index e8ea2c315a..75b29c6a39 100644
> --- a/include/qapi/qmp/qnull.h
> +++ b/include/qapi/qmp/qnull.h
> @@ -23,7 +23,7 @@ extern QNull qnull_;
>  
>  static inline QNull *qnull(void)
>  {
> -    QINCREF(&qnull_);
> +    qobject_ref(&qnull_);
>      return &qnull_;
>  }
>  
> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
> index 7e6fed242e..fce4a1cd75 100644
> --- a/include/qapi/qmp/qobject.h
> +++ b/include/qapi/qmp/qobject.h
> @@ -16,16 +16,16 @@
>   *
>   *  - Returning references: A function that returns an object may
>   *  return it as either a weak or a strong reference.  If the reference
> - *  is strong, you are responsible for calling QDECREF() on the reference
> + *  is strong, you are responsible for calling qobject_unref() on the 
> reference

Long line.

>   *  when you are done.
>   *
>   *  If the reference is weak, the owner of the reference may free it at
>   *  any time in the future.  Before storing the reference anywhere, you
> - *  should call QINCREF() to make the reference strong.
> + *  should call qobject_ref() to make the reference strong.
>   *
>   *  - Transferring ownership: when you transfer ownership of a reference
>   *  by calling a function, you are no longer responsible for calling
> - *  QDECREF() when the reference is no longer needed.  In other words,
> + *  qobject_unref() when the reference is no longer needed.  In other words,
>   *  when the function returns you must behave as if the reference to the
>   *  passed object was weak.
>   */
[...]