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 <agraf@suse.de>
> > Cc: Hervé Poussineau <hpoussin@reactos.org>
> > Cc: Juergen Gross <jgross@suse.com>
> > Cc: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Cc: Andreas Färber <afaerber@suse.de>
> >
> > 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