Hi,

This series aims to get rid of the distinction between QObject, that
must use qobject_incref/qobject_decref and its various derived types
that have to use QINCREF/QDECREF. Instead, replace it with
qobject_ref/qobject_unref for all types.

v5: after Markus review
- various commit message & comments update
- split the object_ref() patch to assert() on NULL, and return obj
- drop RFC from cover letter

v4:
- rename QObjectCommon->QObjectBase_
- add back qobject_ref_impl/qobject_unref_impl
- add extra parenthesis for qobject_ref() cast
- commit message tweaks

v3: after v2 review with Eric and Paolo
- fix clang ubsan warning when a null pointer is given to QOBJECT.
- add a patch to make qobject_ref() assert on null pointer, and return
  the same pointer, simplifying some code.

v2:
- use the QObjectCommon base approach suggested by Paolo and Eric
- remove need for QEMU_GENERIC

Marc-André Lureau (5):
  qobject: ensure base is at offset 0
  qobject: use a QObjectBase_ struct
  qobject: replace qobject_incref/QINCREF qobject_decref/QDECREF
  qobject: modify qobject_ref() to return obj
  qobject: modify qobject_ref() to assert on NULL

 scripts/qapi/events.py              |   2 +-
 include/qapi/qmp/qbool.h            |   2 +-
 include/qapi/qmp/qdict.h            |   2 +-
 include/qapi/qmp/qlist.h            |   2 +-
 include/qapi/qmp/qnull.h            |   5 +-
 include/qapi/qmp/qnum.h             |   2 +-
 include/qapi/qmp/qobject.h          |  74 ++++++++++---------
 include/qapi/qmp/qstring.h          |   2 +-
 block.c                             |  82 ++++++++++-----------
 block/blkdebug.c                    |   7 +-
 block/blkverify.c                   |   8 +--
 block/crypto.c                      |   4 +-
 block/gluster.c                     |   4 +-
 block/iscsi.c                       |   2 +-
 block/nbd.c                         |   4 +-
 block/nfs.c                         |   4 +-
 block/null.c                        |   3 +-
 block/nvme.c                        |   3 +-
 block/parallels.c                   |   4 +-
 block/qapi.c                        |   2 +-
 block/qcow.c                        |   8 +--
 block/qcow2.c                       |   8 +--
 block/qed.c                         |   4 +-
 block/quorum.c                      |   4 +-
 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                           |  57 +++++++--------
 qapi/qapi-dealloc-visitor.c         |   4 +-
 qapi/qmp-dispatch.c                 |   6 +-
 qapi/qobject-input-visitor.c        |  10 ++-
 qapi/qobject-output-visitor.c       |  11 ++-
 qemu-img.c                          |  18 ++---
 qemu-io.c                           |   6 +-
 qga/main.c                          |  12 ++--
 qmp.c                               |   4 +-
 qobject/json-parser.c               |  10 +--
 qobject/qdict.c                     |  49 +++++--------
 qobject/qjson.c                     |   2 +-
 qobject/qlist.c                     |   4 +-
 qobject/qobject.c                   |  21 ++++--
 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                 | 106 ++++++++++++++--------------
 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 +--
 100 files changed, 663 insertions(+), 669 deletions(-)

-- 
2.17.0.rc1.36.gcedb63ea2f

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi,
>
> This series aims to get rid of the distinction between QObject, that
> must use qobject_incref/qobject_decref and its various derived types
> that have to use QINCREF/QDECREF. Instead, replace it with
> qobject_ref/qobject_unref for all types.

This is a lovely improvement.

However, I think the code can't quite decide whether to embrace or
eschew type casts between QObject and its subtypes.

On the one hand, we provide safe conversion macros QOBJECT() and
qobject_to().  By the way, shouting one but not the other is a bit ugly.

On the other hand, we still use type casts, and enforce "base comes
first" to make them work.

If we want to eschew type casts, we should clean up the remaining ones.
The case for "base must come first" is quite weak then.  Not worth the
bother, I think.

I doubt we want to embrace type casts here.  If we wanted to, I'd go all
the way and unwrap base in QObjects and its subtypes with suitable "must
match QObject" comments.  Alternatively, use an unnamed member.  Then
convert with type casts rather than container_of() and &->base.

Hi

On Thu, Apr 19, 2018 at 8:44 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> Hi,
>>
>> This series aims to get rid of the distinction between QObject, that
>> must use qobject_incref/qobject_decref and its various derived types
>> that have to use QINCREF/QDECREF. Instead, replace it with
>> qobject_ref/qobject_unref for all types.
>
> This is a lovely improvement.

That's a good sign.

> However, I think the code can't quite decide whether to embrace or
> eschew type casts between QObject and its subtypes.

It uses both, for different reasons

> On the one hand, we provide safe conversion macros QOBJECT() and
> qobject_to().  By the way, shouting one but not the other is a bit ugly.

QOBJECT is static upcast, the compiler will shout.
qobject_to() is dynamic downcast, the runtime shout.

> On the other hand, we still use type casts, and enforce "base comes
> first" to make them work.
>
> If we want to eschew type casts, we should clean up the remaining ones.
> The case for "base must come first" is quite weak then.  Not worth the
> bother, I think.
>
> I doubt we want to embrace type casts here.  If we wanted to, I'd go all
> the way and unwrap base in QObjects and its subtypes with suitable "must
> match QObject" comments.  Alternatively, use an unnamed member.  Then
> convert with type casts rather than container_of() and &->base.
>

Can you make a different patch? Can it be done after? Should we add a
note there for future improvements?

I don't like to spent too much time on this, yet as you said, this is
a lovely improvement, so is there any real blocker left?

-- 
Marc-André Lureau

Hi

On Thu, Apr 19, 2018 at 12:09 PM, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>> On the one hand, we provide safe conversion macros QOBJECT() and
>> qobject_to().  By the way, shouting one but not the other is a bit ugly.
>
> QOBJECT is static upcast, the compiler will shout.
> qobject_to() is dynamic downcast, the runtime shout.

Actually it doesn't shout, it merely tries and return NULL if it can't
do it. So I understand what you mean now. So it's a balance, but so
far, it seems those 2 fit our needs.

> I don't like to spent too much time on this, yet as you said, this is
> a lovely improvement, so is there any real blocker left?

I'll try to address your comments from v5 patches and send a v6.


-- 
Marc-André Lureau

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Thu, Apr 19, 2018 at 12:09 PM, Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
>>> On the one hand, we provide safe conversion macros QOBJECT() and
>>> qobject_to().  By the way, shouting one but not the other is a bit ugly.
>>
>> QOBJECT is static upcast, the compiler will shout.
>> qobject_to() is dynamic downcast, the runtime shout.
>
> Actually it doesn't shout, it merely tries and return NULL if it can't
> do it. So I understand what you mean now. So it's a balance, but so
> far, it seems those 2 fit our needs.

Misunderstanding?  By "shouting", I meant spelling all-caps.

The convention to shout macros makes sense because you generally need to
know when something's a macro.  In cases where you don't need to know,
it's okay not to shout.

>> I don't like to spent too much time on this, yet as you said, this is
>> a lovely improvement, so is there any real blocker left?
>
> I'll try to address your comments from v5 patches and send a v6.

Thanks!