On Thu, Mar 30, 2017 at 10:59:10AM +0200, Paolo Bonzini wrote: > > > On 29/03/2017 21:41, Eduardo Habkost wrote: > > QOM has a data model where class struct data is static: class > > structs are initialized at class_init, and never changed again. > > Isn't that just the way class data is being used? There's no reason for > class data to be static. It happens to be that way because our > hierarchies are pretty shallow and global static variables are used > instead of class data. But it doesn't _have_ to be like that.
I disagree. I believe this is how class data is meant to be used. if we change per-class data at runtime outside class_init, we have a hard time providing querying and introspection mechanisms for types. If something really needs to be changed at runtime outside class_init, it belongs to object instance fields, not class data fields. e.g. the main benefit we got from eliminating the compat_* global variables in machine/pc code wasn't simply the global variable elimination: it was making the compat data static and available at the MachineClass structs, instead of burying it inside machine_init functions. If we kept the old model where class-specific data was initialized at machine_init-time, extending query-machines with that info, and writing new machine data queries would still be impossible. For reference, this is how I think we could have dealt with the existing cases of non-const class variables: > > qbus_realize(), code added by: > > commit 61de36761b565a4138d8ad7ec75489ab28fe84b6 > > Date: Thu Feb 6 16:08:15 2014 +0100 > > qdev: Keep global allocation counter per bus This one might make sense: it's easier to keep data in a BusClass field than a automatic_id[bus_type] dictionary somewhere else. But I would still prefer to have a automatic_id[bus_type] dictionary inside MachineState instead of this. > > > > 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 IMO, a simple MIPSCPU::ignore_invalid_exec_access boolean flag would be better than hijacking TYPE_MIPS_CPU's do_unassigned_access() method on the fly. > > > > 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 Here, setting has_dynamic_sysbus for all PC classes would be better, so a future supported-device-types/device-slot querying mechanism wouldn't incorrectly report xen-backend as unsupported. But there are additional problems I want to sove regarding sysbus before doing that. BTW, this is the case that prompted me to write this series in the first place. I believe changing MachineClass::has_dynamic_sysbus at runtime was a mistake, and a const object_get_class() would have helped us catch that. > > > > 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 Here, I see no reason for a per-cpu-class field at all. Even a static next_cpu_id variable wouldn't hurt. Also, this is the only remaining code still using cpu_exists(), and the same logic probably can be implemented using MachineClass::possible_cpu_arch_ids() instead. > > That said, the QUALIFIED_CAST concept is very interesting and I'd even > extend it to OBJECT_CHECK, object_dynamic_cast and > object_dynamic_cast_assert. I'm just not sure about returning const > from object_get_class()/*_GET_CLASS, which ironically is the thing that > prompted you to write the series. :) Makes sense. I will send extra patches to change the object cast macros, too. > > Paolo > -- Eduardo
