Re: [PATCH 00/16] qom/object: Deprecate type_register()

[Zhao Liu]

Hi Paolo and Dainel,

Kindly ping. Do you agree with this idea?

Thanks,
Zhao

On Tue, Oct 29, 2024 at 04:59:18PM +0800, Zhao Liu wrote:
> Date: Tue, 29 Oct 2024 16:59:18 +0800
> From: Zhao Liu <zhao1.liu@intel.com>
> Subject: [PATCH 00/16] qom/object: Deprecate type_register()
> X-Mailer: git-send-email 2.34.1
> 
> Hi maintainers,
> 
> This series is trying to deprecate type_register() and just keep
> type_register_static() to register QOM type.
> 
> This series chosen to deprecate type_register() since changes required
> to deprecate type_register() are smaller. (type_register_static() needs
> 1000+ LOC changes.)
> 
> The two main changes are patch 15 and 16, while the others are trivial
> replacements.
> 
> This series is based on commit fdf250e5a37830 ("Merge tag
> 'pull-maintainer-oct-misc-241024-1' of https://gitlab.com/stsquad/qemu
> into staging").
> 
> 
> Introduction
> ============
> 
> The type_register() and type_register_static() have existed since the
> birth of QOM (commit 2f28d2ff9dce ("qom: add the base Object class
> (v2)")).
> 
> In the code implementation, type_register_static() has always been just
> a wrapper around type_register(), and they are essentially the same.
> 
> The only difference between them is described in the documentation 
> (include/qom/object.h):
> 
> * type_register_static()
> 
> > @info and all of the strings it points to should exist for the life time
> > that the type is registered.
> 
> * type_register()
> 
> > Unlike type_register_static(), this call does not require @info or its
> > string members to continue to exist after the call returns.
> 
> From the documentation, the difference between these two interfaces
> arises from the lifetime of 2 cases:
> 
> 1. the strings contained in the TypeInfo parameter.
> 
> The *_static variant requires the strings to have a long lifetime
> (static).
> 
> I reviewed the discussions on the mailing list about the QOM v1 patch
> [1], and I understand that the consideration for static is due to
> Anthony's idea that in certain cases, the string field could be "const
> char *", allowing the address to be directly copied to the created
> Type/TypeImpl.
> 
> However, this consideration seems unnecessary in the merged v2 version,
> as Anthony followed Paolo's suggestion to pass all string fields by
> copying them via g_strdup() to the created TypeImple. This remains true
> to this day.
> 
> [1]: https://lore.kernel.org/qemu-devel/4EF1EEA4.40209@us.ibm.com/
> 
> 
> 2. the function pointer and a special item called "class_data" in
> TypeInfo
> 
> I suppose that there are currently no lifetime issues about these items
> in QEMU, as neither type_register() nor type_register_static()
> explicitly checks whether the parameters are static. If there were any
> issues, they would likely be easily detected.
> 
> Furthermore, I haven't seen any preference for these items in the usage
> of type_register() and type_register_static().
> 
> 
> Based on points 1 and 2, I think it is sufficient to explain that
> type_register() and type_register_static() are redundant in usage and do
> not require distinction. Additionally, since they are consistent in the
> code, it is safe to deprecate either one.
> 
> Since the changes required to deprecate type_register() are smaller,
> choose to deprecate type_register() and delete the requirement about
> string lifetime from the documentation.
> 
> 
> Thanks and Best Regards,
> Zhao
> ---
> Zhao Liu (16):
>   arm: Replace type_register() with type_register_static()
>   hw/block: Replace type_register() with type_register_static()
>   hw/net: Replace type_register() with type_register_static()
>   ppc: Replace type_register() with type_register_static()
>   hw/rtc: Replace type_register() with type_register_static()
>   hw/scsi: Replace type_register() with type_register_static()
>   hw/sensor: Replace type_register() with type_register_static()
>   hw/usb: Replace type_register() with type_register_static()
>   hw/virtio: Replace type_register() with type_register_static()
>   i386: Replace type_register() with type_register_static()
>   target/mips: Replace type_register() with type_register_static()
>   target/sparc: Replace type_register() with type_register_static()
>   target/xtensa: Replace type_register() with type_register_static()
>   ui: Replace type_register() with type_register_static()
>   script/codeconverter/qom_type_info: Deprecate MakeTypeRegisterStatic
>     and MakeTypeRegisterNotStatic
>   qom/object: Deprecate type_register()
> 
>  hw/arm/armsse.c                               |  2 +-
>  hw/arm/smmuv3.c                               |  4 ++--
>  hw/block/m25p80.c                             |  2 +-
>  hw/net/e1000.c                                |  2 +-
>  hw/net/eepro100.c                             |  2 +-
>  hw/ppc/spapr.c                                |  2 +-
>  hw/rtc/m48t59-isa.c                           |  2 +-
>  hw/rtc/m48t59.c                               |  2 +-
>  hw/scsi/megasas.c                             |  2 +-
>  hw/scsi/mptsas.c                              |  2 +-
>  hw/sensor/tmp421.c                            |  2 +-
>  hw/usb/hcd-ehci-pci.c                         |  2 +-
>  hw/usb/hcd-uhci.c                             |  2 +-
>  hw/virtio/virtio-pci.c                        |  8 ++++----
>  include/hw/i386/pc.h                          |  4 ++--
>  include/qom/object.h                          | 14 -------------
>  qom/object.c                                  |  7 +------
>  .../codeconverter/qom_type_info.py            | 20 -------------------
>  target/arm/cpu.c                              |  2 +-
>  target/arm/cpu64.c                            |  2 +-
>  target/i386/cpu.c                             |  2 +-
>  target/mips/cpu.c                             |  2 +-
>  target/ppc/kvm.c                              |  2 +-
>  target/sparc/cpu.c                            |  2 +-
>  target/xtensa/helper.c                        |  2 +-
>  ui/console-vc.c                               |  2 +-
>  ui/dbus.c                                     |  2 +-
>  ui/gtk.c                                      |  2 +-
>  ui/spice-app.c                                |  2 +-
>  29 files changed, 32 insertions(+), 71 deletions(-)
> 
> -- 
> 2.34.1
>