Re: [Qemu-devel] [RFC v2 0/9] qom: Make object_get_class()/*_GET_CLASS return const pointers

[Eduardo Habkost]

On Wed, Mar 29, 2017 at 09:56:54PM +0200, Laszlo Ersek wrote:
> On 03/29/17 21:41, Eduardo Habkost wrote:
> > The problem
> > -----------
> > 
> > QOM has a data model where class struct data is static: class
> > structs are initialized at class_init, and never changed again.
> > 
> > ...except for a few rare cases where class data is changed
> > outside class_init. Those cases are:
> > 
> >   qbus_realize(), code added by:
> >   commit 61de36761b565a4138d8ad7ec75489ab28fe84b6
> >   Date:   Thu Feb 6 16:08:15 2014 +0100
> >       qdev: Keep global allocation counter per bus
> > 
> >   mips_jazz_init(), code added by:
> >   commit 54e755588cf1e90f0b1460c4e8e6b6a54b6d3a32
> >   Date:   Mon Nov 4 23:26:17 2013 +0100
> >       mips jazz: do not raise data bus exception when accessing invalid 
> > addresses
> > 
> >   xen_set_dynamic_sysbus(), code added by:
> >   commit 3a6c9172ac5951e6dac2b3f6cbce3cfccdec5894
> >   Date:   Tue Nov 22 07:10:58 2016 +0100
> >       xen: create qdev for each backend device
> > 
> >   s390_cpu_realizefn(), code added by:
> >   commit c6644fc88bed1176f67b4b7a9df691bcf25c0c5b
> >   Date:   Fri Mar 4 12:34:31 2016 -0500
> >       s390x/cpu: Get rid of side effects when creating a vcpu
> > 
> > I want to prevent that from happening again.
> > 
> > Proposal
> > --------
> > 
> > I propose we make object_get_class() and *_GET_CLASS macros
> > return const pointers. This way, anybody willing to change class
> > structs outside class_init will (hopefully) be aware that they
> > are doing something unexpected.
> > 
> > This would be very simple and trivial, except that:
> > * OBJECT_CLASS_CHECK cast its return value to a different
> >   (non-const) pointer type.
> > * OBJECT_CLASS_CHECK-based macros are used everywhere to
> >   cast class pointers to different class types.
> > 
> > I have addressed this problem using C11's _Generic keyword.
> > _Generic allows us to make OBJECT_CLASS_CHECK and other macros
> > return a const pointer if its argument is a const pointer, and a
> > non-const pointer otherwise.
> > 
> > This series changes OBJECT_CLASS, OBJECT_CLASS_CHECK,
> > object_class_dynamic_cast_assert(), object_class_dynamic_cast(),
> > and object_class_get_parent() to exhibit this const-aware
> > behavior.
> > 
> > If the compiler doesn't support _Generic, we keep the old
> > behavior, and the cast macros will keep returning non-const
> > pointers.
> > 
> > ---
> > Cc: Alexander Graf <address@hidden>
> > Cc: Hervé Poussineau <address@hidden>
> > Cc: Juergen Gross <address@hidden>
> > Cc: Matthew Rosato <address@hidden>
> > Cc: Laszlo Ersek <address@hidden>
> > Cc: Paolo Bonzini <address@hidden>
> > Cc: "Dr. David Alan Gilbert" <address@hidden>
> > Cc: Igor Mammedov <address@hidden>
> > Cc: Andreas Färber <address@hidden>
> > 
> > Eduardo Habkost (9):
> >   configure: test if _Generic works as expected
> >   Simplify code using *MACHINE_GET_CLASS
> >   qom: QUALIFIED_CAST helper macro
> >   qom: Make object_class_get_parent() const-aware
> >   Make class parameter const at some functions
> >   Explicitly cast the *_GET_CLASS() value when we break the rules
> >   Use const variables for *_GET_CLASS values
> >   qom: Make class cast macros/functions const-aware
> >   qom: Make object_get_class() return const pointer
> 
> Only superficial comments:
> 
> (1) HACKING has some notes on C99 and C11 usage; I think you should
> update them as well.

I will take a look, thanks!

> 
> (2) Can you CC the maintainers of the code being modified on each patch?

I can, on the shorter ones, but this will be difficult on patch
7, which is composed mostly of mechanical changes generated using
Coccinelle. I didn't want to generate a huge CC list just because
of those mechanical changes.

> 
> (3) (Corollary of the former -- I'm not seeing patch 7 yet, which I
> assume is the big one, from the cumulative diffstat) -- is it possible
> to split up patch 7 more fine-grained? To help reviewers. (I don't
> maintain anything in QEMU, so this is just a generic suggestion;
> everyone feel free to disagree.)

The coccinelle-generated parts of patch 7 will be hard to split,
but I can try to split the rest of the changes later.

I tried to keep the patch count smaller on purpose on the RFC,
because first I want to hear what people think of this proposal.
If people like it, I will split a few patches, especially:

* [RFC v2 2/9] Simplify code using *MACHINE_GET_CLASS
  (split the pc, machine core, and pci changes into separate patches)
* [RFC v2 5/9] Make class parameter const at some functions
  (split into one patch for each subsystem being changed)
* [RFC v2 6/9] Explicitly cast the *_GET_CLASS() value when we break the rules
  (not sure if I I will split it, but I will surely CC the
  maintainers for each subsystem)
* [RFC v2 7/9] Use const variables for *_GET_CLASS values
  (probably I will separate the coccinelle-generated changes from the others,
  and split the manual changes per subsystem)

-- 
Eduardo