| Message ID | 20161206193159.GE4027@thinpad.lan.raisama.net |
|---|---|
| State | New |
| Headers | show |
On Tue, 6 Dec 2016 17:31:59 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Dec 06, 2016 at 05:19:52PM -0200, Eduardo Habkost wrote: > > On Tue, Dec 06, 2016 at 06:50:47PM +0100, Greg Kurz wrote: > > > Since commit "9a4c0e220d8a hw/virtio-pci: fix virtio behaviour", passing > > > -device virtio-blk-pci.disable-modern=off has no effect on 2.6 machine > > > types because the internal virtio-pci.disable-modern=on compat property > > > always prevail. > > > > > > This should ideally be fixed in the qdev properties core code, but it is > > > too late in the QEMU 2.8 schedule. So this patch fixes the issue by setting > > > the compat properties for every virtio-*-pci subtypes instead of the base > > > virtio-pci type. > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > So, it looks like the bug is present in many other cases... > > > > I have hacked QEMU to print a warning in case the driver name in > > compat_props refer to an abstract class or a class that have any > > subclasses. The results are below: > > > > apic-common.legacy-instance-id set for abstract class > > apic-common.vapic set for abstract class > > i386-cpu.arat set for abstract class > > i386-cpu.check set for abstract class > > i386-cpu.cpuid-0xb set for abstract class > > i386-cpu.fill-mtrr-mask set for abstract class > > i386-cpu.full-cpuid-auto-level set for abstract class > > i386-cpu.l3-cache set for abstract class > > i386-cpu.pmu set for abstract class > > pci-device.command_serr_enable set for abstract class > > pci-device.rombar set for abstract class > > pci-device.x-pcie-lnksta-dllla set for abstract class > > powerpc64-cpu.pre-2.8-migration set for abstract class > > s390-skeys.migration-enabled set for abstract class > > spapr-pci-host-bridge.ddw set for superclass > > spapr-pci-host-bridge.dynamic-reconfiguration set for superclass > > spapr-pci-host-bridge.mem64_win_size set for superclass > > spapr-pci-host-bridge.mem_win_size set for superclass > > spapr-pci-host-bridge.pre-2.8-migration set for superclass > > usb-device.full-path set for abstract class > > usb-device.msos-desc set for abstract class > > virtio-pci.disable-legacy set for abstract class > > virtio-pci.disable-modern set for abstract class > > virtio-pci.migrate-extra set for abstract class > > virtio-pci.page-per-vq set for abstract class > > virtio-pci.virtio-pci-bus-master-bug-migration set for abstract class > > virtio-pci.x-disable-pcie set for abstract class > > x86_64-cpu.arat set for abstract class > > x86_64-cpu.check set for abstract class > > x86_64-cpu.cpuid-0xb set for abstract class > > x86_64-cpu.fill-mtrr-mask set for abstract class > > x86_64-cpu.full-cpuid-auto-level set for abstract class > > x86_64-cpu.l3-cache set for abstract class > > x86_64-cpu.pmu set for abstract class > > > > I believe the cases where we are likely to cause real-world bugs > > are virtio-pci and the *-cpu classes (because -cpu is translated > > to -global). > > > > I'm not sure what should be the right fix in 2.8. I am > > considering a temporary hack to translate abstract class names in > > compat_props to global properties for all subclasses, in case > > they refer to an abstract class. This way we fix the bug where > > -global doesn't override compat_props properly, but keep the > > rules for -global untouched. > > What about this? > Indeed that would have been a clever workaround but it is too late anyway. :) BTW, that makes me think about Halil's comment in another mail, even if it is not strictly related to the comapt issue (but rather because I mentioned it in the changelog of my first patch): "So I can't even tell if -global virtio-pci.disable-modern=off is even legit on the command line." And indeed, -device doesn't accept abstract classes... shouldn't -global prohibit setting properties for them ? > (untested) > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > hw/core/machine.c | 35 ++++++++++++++++++++++++++++++++--- > 1 file changed, 32 insertions(+), 3 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index b0fd91f..49f9007 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -554,6 +554,25 @@ static void machine_class_finalize(ObjectClass *klass, void *data) > g_free(mc->name); > } > > +static void register_compat_prop(const char *driver, > + const char *property, > + const char *value) > +{ > + GlobalProperty *p = g_new0(GlobalProperty, 1); > + /* Machine compat_props must never cause errors: */ > + p->errp = &error_abort; > + p->driver = driver; > + p->property = property; > + p->value = value; > + qdev_prop_register_global(p); > +} > + > +static void machine_register_compat_for_subclass(ObjectClass *oc, void *opaque) > +{ > + GlobalProperty *p = opaque; > + register_compat_prop(object_class_get_name(oc), p->property, p->value); > +} > + > void machine_register_compat_props(MachineState *machine) > { > MachineClass *mc = MACHINE_GET_CLASS(machine); > @@ -565,10 +584,20 @@ void machine_register_compat_props(MachineState *machine) > } > > for (i = 0; i < mc->compat_props->len; i++) { > + ObjectClass *oc; > p = g_array_index(mc->compat_props, GlobalProperty *, i); > - /* Machine compat_props must never cause errors: */ > - p->errp = &error_abort; > - qdev_prop_register_global(p); > + oc = object_class_by_name(p->driver); > + if (oc && object_class_is_abstract(oc)) { > + /* temporary hack to make sure we will never override > + * globals set explicitly on -global: if an abstract class > + * is on compat_props, register globals for each of their > + * subclasses instead. > + */ > + object_class_foreach(machine_register_compat_for_subclass, > + p->driver, false, p); > + } else { > + register_compat_prop(p->driver, p->property, p->value); > + } > } > } >
On Wed, Dec 07, 2016 at 02:39:25PM +0100, Greg Kurz wrote: > On Tue, 6 Dec 2016 17:31:59 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Tue, Dec 06, 2016 at 05:19:52PM -0200, Eduardo Habkost wrote: > > > On Tue, Dec 06, 2016 at 06:50:47PM +0100, Greg Kurz wrote: > > > > Since commit "9a4c0e220d8a hw/virtio-pci: fix virtio behaviour", passing > > > > -device virtio-blk-pci.disable-modern=off has no effect on 2.6 machine > > > > types because the internal virtio-pci.disable-modern=on compat property > > > > always prevail. > > > > > > > > This should ideally be fixed in the qdev properties core code, but it is > > > > too late in the QEMU 2.8 schedule. So this patch fixes the issue by setting > > > > the compat properties for every virtio-*-pci subtypes instead of the base > > > > virtio-pci type. > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > > > So, it looks like the bug is present in many other cases... > > > > > > I have hacked QEMU to print a warning in case the driver name in > > > compat_props refer to an abstract class or a class that have any > > > subclasses. The results are below: > > > > > > apic-common.legacy-instance-id set for abstract class > > > apic-common.vapic set for abstract class > > > i386-cpu.arat set for abstract class > > > i386-cpu.check set for abstract class > > > i386-cpu.cpuid-0xb set for abstract class > > > i386-cpu.fill-mtrr-mask set for abstract class > > > i386-cpu.full-cpuid-auto-level set for abstract class > > > i386-cpu.l3-cache set for abstract class > > > i386-cpu.pmu set for abstract class > > > pci-device.command_serr_enable set for abstract class > > > pci-device.rombar set for abstract class > > > pci-device.x-pcie-lnksta-dllla set for abstract class > > > powerpc64-cpu.pre-2.8-migration set for abstract class > > > s390-skeys.migration-enabled set for abstract class > > > spapr-pci-host-bridge.ddw set for superclass > > > spapr-pci-host-bridge.dynamic-reconfiguration set for superclass > > > spapr-pci-host-bridge.mem64_win_size set for superclass > > > spapr-pci-host-bridge.mem_win_size set for superclass > > > spapr-pci-host-bridge.pre-2.8-migration set for superclass > > > usb-device.full-path set for abstract class > > > usb-device.msos-desc set for abstract class > > > virtio-pci.disable-legacy set for abstract class > > > virtio-pci.disable-modern set for abstract class > > > virtio-pci.migrate-extra set for abstract class > > > virtio-pci.page-per-vq set for abstract class > > > virtio-pci.virtio-pci-bus-master-bug-migration set for abstract class > > > virtio-pci.x-disable-pcie set for abstract class > > > x86_64-cpu.arat set for abstract class > > > x86_64-cpu.check set for abstract class > > > x86_64-cpu.cpuid-0xb set for abstract class > > > x86_64-cpu.fill-mtrr-mask set for abstract class > > > x86_64-cpu.full-cpuid-auto-level set for abstract class > > > x86_64-cpu.l3-cache set for abstract class > > > x86_64-cpu.pmu set for abstract class > > > > > > I believe the cases where we are likely to cause real-world bugs > > > are virtio-pci and the *-cpu classes (because -cpu is translated > > > to -global). > > > > > > I'm not sure what should be the right fix in 2.8. I am > > > considering a temporary hack to translate abstract class names in > > > compat_props to global properties for all subclasses, in case > > > they refer to an abstract class. This way we fix the bug where > > > -global doesn't override compat_props properly, but keep the > > > rules for -global untouched. > > > > What about this? > > > > Indeed that would have been a clever workaround but it is too late anyway. :) > > BTW, that makes me think about Halil's comment in another mail, even if it > is not strictly related to the comapt issue (but rather because I mentioned > it in the changelog of my first patch): > > "So I can't even tell if -global virtio-pci.disable-modern=off is even legit > on the command line." > > And indeed, -device doesn't accept abstract classes... shouldn't -global > prohibit setting properties for them ? I'm not sure. Setting disable-modern for all virtio-pci devices with a single -global option sounds very useful, for example. Also, if we start rejecting it we risk breaking existing configurations. We would need to deprecate it first (making it print a warning) and reject it a few releases later.
On Wed, 7 Dec 2016 11:59:17 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Dec 07, 2016 at 02:39:25PM +0100, Greg Kurz wrote: > > On Tue, 6 Dec 2016 17:31:59 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Tue, Dec 06, 2016 at 05:19:52PM -0200, Eduardo Habkost wrote: > > > > On Tue, Dec 06, 2016 at 06:50:47PM +0100, Greg Kurz wrote: > > > > > Since commit "9a4c0e220d8a hw/virtio-pci: fix virtio behaviour", passing > > > > > -device virtio-blk-pci.disable-modern=off has no effect on 2.6 machine > > > > > types because the internal virtio-pci.disable-modern=on compat property > > > > > always prevail. > > > > > > > > > > This should ideally be fixed in the qdev properties core code, but it is > > > > > too late in the QEMU 2.8 schedule. So this patch fixes the issue by setting > > > > > the compat properties for every virtio-*-pci subtypes instead of the base > > > > > virtio-pci type. > > > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > > > > > So, it looks like the bug is present in many other cases... > > > > > > > > I have hacked QEMU to print a warning in case the driver name in > > > > compat_props refer to an abstract class or a class that have any > > > > subclasses. The results are below: > > > > > > > > apic-common.legacy-instance-id set for abstract class > > > > apic-common.vapic set for abstract class > > > > i386-cpu.arat set for abstract class > > > > i386-cpu.check set for abstract class > > > > i386-cpu.cpuid-0xb set for abstract class > > > > i386-cpu.fill-mtrr-mask set for abstract class > > > > i386-cpu.full-cpuid-auto-level set for abstract class > > > > i386-cpu.l3-cache set for abstract class > > > > i386-cpu.pmu set for abstract class > > > > pci-device.command_serr_enable set for abstract class > > > > pci-device.rombar set for abstract class > > > > pci-device.x-pcie-lnksta-dllla set for abstract class > > > > powerpc64-cpu.pre-2.8-migration set for abstract class > > > > s390-skeys.migration-enabled set for abstract class > > > > spapr-pci-host-bridge.ddw set for superclass > > > > spapr-pci-host-bridge.dynamic-reconfiguration set for superclass > > > > spapr-pci-host-bridge.mem64_win_size set for superclass > > > > spapr-pci-host-bridge.mem_win_size set for superclass > > > > spapr-pci-host-bridge.pre-2.8-migration set for superclass > > > > usb-device.full-path set for abstract class > > > > usb-device.msos-desc set for abstract class > > > > virtio-pci.disable-legacy set for abstract class > > > > virtio-pci.disable-modern set for abstract class > > > > virtio-pci.migrate-extra set for abstract class > > > > virtio-pci.page-per-vq set for abstract class > > > > virtio-pci.virtio-pci-bus-master-bug-migration set for abstract class > > > > virtio-pci.x-disable-pcie set for abstract class > > > > x86_64-cpu.arat set for abstract class > > > > x86_64-cpu.check set for abstract class > > > > x86_64-cpu.cpuid-0xb set for abstract class > > > > x86_64-cpu.fill-mtrr-mask set for abstract class > > > > x86_64-cpu.full-cpuid-auto-level set for abstract class > > > > x86_64-cpu.l3-cache set for abstract class > > > > x86_64-cpu.pmu set for abstract class > > > > > > > > I believe the cases where we are likely to cause real-world bugs > > > > are virtio-pci and the *-cpu classes (because -cpu is translated > > > > to -global). > > > > > > > > I'm not sure what should be the right fix in 2.8. I am > > > > considering a temporary hack to translate abstract class names in > > > > compat_props to global properties for all subclasses, in case > > > > they refer to an abstract class. This way we fix the bug where > > > > -global doesn't override compat_props properly, but keep the > > > > rules for -global untouched. > > > > > > What about this? > > > > > > > Indeed that would have been a clever workaround but it is too late anyway. :) > > > > BTW, that makes me think about Halil's comment in another mail, even if it > > is not strictly related to the comapt issue (but rather because I mentioned > > it in the changelog of my first patch): > > > > "So I can't even tell if -global virtio-pci.disable-modern=off is even legit > > on the command line." > > > > And indeed, -device doesn't accept abstract classes... shouldn't -global > > prohibit setting properties for them ? > > I'm not sure. Setting disable-modern for all virtio-pci devices > with a single -global option sounds very useful, for example. > Indeed but, unless I've missed something *again*, you cannot know about it without reading the code... maybe this calls for a '-global help' option to list them all ? > Also, if we start rejecting it we risk breaking existing > configurations. We would need to deprecate it first (making it > print a warning) and reject it a few releases later. > This would be an undocumented configuration but I get your point, so it's probably better to keep it... especially if it's useful :) Cheers. -- Greg
On Wed, Dec 07, 2016 at 02:39:25PM +0100, Greg Kurz wrote: > On Tue, 6 Dec 2016 17:31:59 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Tue, Dec 06, 2016 at 05:19:52PM -0200, Eduardo Habkost wrote: > > > On Tue, Dec 06, 2016 at 06:50:47PM +0100, Greg Kurz wrote: > > > > Since commit "9a4c0e220d8a hw/virtio-pci: fix virtio behaviour", passing > > > > -device virtio-blk-pci.disable-modern=off has no effect on 2.6 machine > > > > types because the internal virtio-pci.disable-modern=on compat property > > > > always prevail. > > > > > > > > This should ideally be fixed in the qdev properties core code, but it is > > > > too late in the QEMU 2.8 schedule. So this patch fixes the issue by setting > > > > the compat properties for every virtio-*-pci subtypes instead of the base > > > > virtio-pci type. > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > > > So, it looks like the bug is present in many other cases... > > > > > > I have hacked QEMU to print a warning in case the driver name in > > > compat_props refer to an abstract class or a class that have any > > > subclasses. The results are below: > > > [...] > > > > > > I believe the cases where we are likely to cause real-world bugs > > > are virtio-pci and the *-cpu classes (because -cpu is translated > > > to -global). > > > > > > I'm not sure what should be the right fix in 2.8. I am > > > considering a temporary hack to translate abstract class names in > > > compat_props to global properties for all subclasses, in case > > > they refer to an abstract class. This way we fix the bug where > > > -global doesn't override compat_props properly, but keep the > > > rules for -global untouched. > > > > What about this? > > > > Indeed that would have been a clever workaround but it is too late anyway. :) Now my question is: what should we do in 2.8.1? 1) Change the rules and apply "[PATCH] qdev: fix the order compat and global properties are applied" 2) Fix only virtio-pci (apply a new version of "[PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers") 3) Apply this hack so we fix all compat_props cases without changing ordering rules. (And then change the ordering rules in 2.9)
On Fri, 9 Dec 2016 18:06:50 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > Now my question is: what should we do in 2.8.1? > > 1) Change the rules and apply "[PATCH] qdev: fix the order compat > and global properties are applied" I don't think changing the rules is something that should be done for a stable release. > 2) Fix only virtio-pci (apply a new version of "[PATCH] virtio: > fix HW_COMPAT_2_6 macro for virtio-*-pci drivers") This has the benefit of fixing exactly one problem in an easily reviewable patch... > 3) Apply this hack so we fix all compat_props cases without > changing ordering rules. (And then change the ordering rules > in 2.9) ...while this has the benefit of fixing the complete class of problems. I'd say 2 or 3, but I'd favour 3. 2.8.1 will not be released next week, so we have enough time to verify that we don't introduce any new breakage.
On Fri, 9 Dec 2016 18:06:50 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Dec 07, 2016 at 02:39:25PM +0100, Greg Kurz wrote: > > On Tue, 6 Dec 2016 17:31:59 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Tue, Dec 06, 2016 at 05:19:52PM -0200, Eduardo Habkost wrote: > > > > On Tue, Dec 06, 2016 at 06:50:47PM +0100, Greg Kurz wrote: > > > > > Since commit "9a4c0e220d8a hw/virtio-pci: fix virtio behaviour", passing > > > > > -device virtio-blk-pci.disable-modern=off has no effect on 2.6 machine > > > > > types because the internal virtio-pci.disable-modern=on compat property > > > > > always prevail. > > > > > > > > > > This should ideally be fixed in the qdev properties core code, but it is > > > > > too late in the QEMU 2.8 schedule. So this patch fixes the issue by setting > > > > > the compat properties for every virtio-*-pci subtypes instead of the base > > > > > virtio-pci type. > > > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > > > > > So, it looks like the bug is present in many other cases... > > > > > > > > I have hacked QEMU to print a warning in case the driver name in > > > > compat_props refer to an abstract class or a class that have any > > > > subclasses. The results are below: > > > > > [...] > > > > > > > > I believe the cases where we are likely to cause real-world bugs > > > > are virtio-pci and the *-cpu classes (because -cpu is translated > > > > to -global). > > > > > > > > I'm not sure what should be the right fix in 2.8. I am > > > > considering a temporary hack to translate abstract class names in > > > > compat_props to global properties for all subclasses, in case > > > > they refer to an abstract class. This way we fix the bug where > > > > -global doesn't override compat_props properly, but keep the > > > > rules for -global untouched. > > > > > > What about this? > > > > > > > Indeed that would have been a clever workaround but it is too late anyway. :) > > Now my question is: what should we do in 2.8.1? > > 1) Change the rules and apply "[PATCH] qdev: fix the order compat > and global properties are applied" Would it be more acceptable to change the rules in 2.8.1 than it is for 2.8 ? > 2) Fix only virtio-pci (apply a new version of "[PATCH] virtio: > fix HW_COMPAT_2_6 macro for virtio-*-pci drivers") The patch is huge, but we're sure that it doesn't break anything else. > 3) Apply this hack so we fix all compat_props cases without > changing ordering rules. (And then change the ordering rules > in 2.9) > FWIW, I think this patch is ok, and not that hacky: it simply implements the idea that 'compat properties of abstract classes are actually handled at the non-abstract sub-classes level'. Of course, it would be great to document this behavior somewhere. :) Cheers. -- Greg
On Tue, 6 Dec 2016 17:31:59 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > What about this? > > (untested) Have not tested it either, but I think it looks good. Some nits below. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > hw/core/machine.c | 35 ++++++++++++++++++++++++++++++++--- > 1 file changed, 32 insertions(+), 3 deletions(-) > > @@ -565,10 +584,20 @@ void machine_register_compat_props(MachineState *machine) > } > > for (i = 0; i < mc->compat_props->len; i++) { > + ObjectClass *oc; Move the declaration out of the loop? > p = g_array_index(mc->compat_props, GlobalProperty *, i); > - /* Machine compat_props must never cause errors: */ > - p->errp = &error_abort; > - qdev_prop_register_global(p); > + oc = object_class_by_name(p->driver); > + if (oc && object_class_is_abstract(oc)) { > + /* temporary hack to make sure we will never override > + * globals set explicitly on -global: if an abstract class > + * is on compat_props, register globals for each of their > + * subclasses instead. > + */ I think this should not just be a 'temporary hack'... rather document this behaviour for abstract classes? > + object_class_foreach(machine_register_compat_for_subclass, > + p->driver, false, p); > + } else { > + register_compat_prop(p->driver, p->property, p->value); > + } > } > } >
On 12/12/2016 01:25 PM, Cornelia Huck wrote: >> + if (oc && object_class_is_abstract(oc)) { >> + /* temporary hack to make sure we will never override ~~~~~~~~~~~~~~~~~~~~~~ I would say this is not enough to provide that guarantee. Inheriting from non-abstract, or something like I described below, would still get us in trouble. Or am I wrong? How about s/we will never/we do not/? >> + * globals set explicitly on -global: if an abstract class >> + * is on compat_props, register globals for each of their >> + * subclasses instead. I'm not sure if this is clear enough regarding multilevel inheritance. I would prefer "subtype" or "class derived from it". >> + */ > I think this should not just be a 'temporary hack'... rather document > this behaviour for abstract classes? > Connie, I have to disagree with you on this. I'm fine with this as a temporary hack provided it remedies the acute problem, but I would this becoming state of art. The reason for this how abstract class is usually understood in OOP: it's like a normal class except you can not instantiate it. Adding an extra property rule, and especially such a convoluted one, could result in a conflict between intuition and reality. And it's not like we have no plan how to do this cleanly. Why do I say convoluted: * Inheriting form abstract and from non-abstract behaves differently. * Theoretically we have something like non-abstract (C_3) inherits for abstract (AC_2) inherits from non-abstract (C_1) inherits from abstract (AC_0), and we set the property P both via AC_0 and AC_2 for an instance of C1 we would/could end up having the same problem as we have now (via AC_0 taking precedence over AC_2) -- at least I think so. I did not test it either, but it looks sane to me. Regardless of the comments on the comment (if you are going to consider them or not) you can take my r-b. Cheers, Halil
On Mon, Dec 12, 2016 at 01:25:09PM +0100, Cornelia Huck wrote: > On Tue, 6 Dec 2016 17:31:59 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > What about this? > > > > (untested) > > Have not tested it either, but I think it looks good. Some nits below. Thanks! > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > hw/core/machine.c | 35 ++++++++++++++++++++++++++++++++--- > > 1 file changed, 32 insertions(+), 3 deletions(-) > > > > > @@ -565,10 +584,20 @@ void machine_register_compat_props(MachineState *machine) > > } > > > > for (i = 0; i < mc->compat_props->len; i++) { > > + ObjectClass *oc; > > Move the declaration out of the loop? I will do it on v2. > > > p = g_array_index(mc->compat_props, GlobalProperty *, i); > > - /* Machine compat_props must never cause errors: */ > > - p->errp = &error_abort; > > - qdev_prop_register_global(p); > > + oc = object_class_by_name(p->driver); > > + if (oc && object_class_is_abstract(oc)) { > > + /* temporary hack to make sure we will never override > > + * globals set explicitly on -global: if an abstract class > > + * is on compat_props, register globals for each of their > > + * subclasses instead. > > + */ > > I think this should not just be a 'temporary hack'... rather document > this behaviour for abstract classes? I'm not sure. I believe the translation from abstract superclass to subclasses just emulates the behavior implemented by Greg Kurz at "qdev: fix the order compat and global properties are applied" in a more complicated way (and only for compat_props, not for -global). This hack specifically would not be enough because it affects only compat_props, and still behaves in a weird way when non-abstract classes have subclasses. > > > + object_class_foreach(machine_register_compat_for_subclass, > > + p->driver, false, p); > > + } else { > > + register_compat_prop(p->driver, p->property, p->value); > > + } > > } > > } > > >
On Mon, Dec 12, 2016 at 06:13:50PM +0100, Halil Pasic wrote: > > > On 12/12/2016 01:25 PM, Cornelia Huck wrote: > >> + if (oc && object_class_is_abstract(oc)) { > >> + /* temporary hack to make sure we will never override > ~~~~~~~~~~~~~~~~~~~~~~ > > I would say this is not enough to provide that guarantee. Inheriting > from non-abstract, or something like I described below, would still > get us in trouble. Or am I wrong? Yes, inheriting from non-abstract would still be broken, so this hack is not enough for some cases. In the current tree, it won't fix the problem for the spapr-pci-host-bridge properties. > > How about s/we will never/we do not/? I will change it. > > >> + * globals set explicitly on -global: if an abstract class > >> + * is on compat_props, register globals for each of their > >> + * subclasses instead. > > I'm not sure if this is clear enough regarding multilevel inheritance. > I would prefer "subtype" or "class derived from it". I can rewrite it as "for all of their subtypes". > > >> + */ > > I think this should not just be a 'temporary hack'... rather document > > this behaviour for abstract classes? > > > > Connie, I have to disagree with you on this. I'm fine with this as a > temporary hack provided it remedies the acute problem, but I would this > becoming state of art. > > The reason for this how abstract class is usually understood in OOP: > it's like a normal class except you can not instantiate it. Adding an > extra property rule, and especially such a convoluted one, could result > in a conflict between intuition and reality. And it's not like we have > no plan how to do this cleanly. > > Why do I say convoluted: > * Inheriting form abstract and from non-abstract behaves differently. > * Theoretically we have something like non-abstract (C_3) inherits for > abstract (AC_2) inherits from non-abstract (C_1) inherits from abstract > (AC_0), and we set the property P both via AC_0 and AC_2 for an instance > of C1 we would/could end up having the same problem as we have now (via > AC_0 taking precedence over AC_2) -- at least I think so. Well, we could change the code to: 1) Not check object_class_is_abstract(), and simply register the globals for all subtypes; 2) Call qdev_prop_set_globals_for_type() only for object_class_get_name(), instead of doing it for all parent classes in qdev_prop_set_globals(). But this sounds like a more complicated way of implementing exactly the same behavior implemented by Greg Kurz in "qdev: fix the order compat and global properties are applied". > > I did not test it either, but it looks sane to me. Regardless of the > comments on the comment (if you are going to consider them or not) you > can take my r-b. Thanks!
On 12/12/2016 06:47 PM, Eduardo Habkost wrote: >>>> + */ >>> I think this should not just be a 'temporary hack'... rather document >>> this behaviour for abstract classes? >>> >> Connie, I have to disagree with you on this. I'm fine with this as a >> temporary hack provided it remedies the acute problem, but I would this >> becoming state of art. >> >> The reason for this how abstract class is usually understood in OOP: >> it's like a normal class except you can not instantiate it. Adding an >> extra property rule, and especially such a convoluted one, could result >> in a conflict between intuition and reality. And it's not like we have >> no plan how to do this cleanly. >> >> Why do I say convoluted: >> * Inheriting form abstract and from non-abstract behaves differently. >> * Theoretically we have something like non-abstract (C_3) inherits for >> abstract (AC_2) inherits from non-abstract (C_1) inherits from abstract >> (AC_0), and we set the property P both via AC_0 and AC_2 for an instance >> of C1 we would/could end up having the same problem as we have now (via >> AC_0 taking precedence over AC_2) -- at least I think so. > Well, we could change the code to: > 1) Not check object_class_is_abstract(), and simply register the > globals for all subtypes; > 2) Call qdev_prop_set_globals_for_type() only for > object_class_get_name(), instead of doing it for all > parent classes in qdev_prop_set_globals(). > > But this sounds like a more complicated way of implementing > exactly the same behavior implemented by Greg Kurz in "qdev: fix > the order compat and global properties are applied". > Yeah. This is what I meant with "And it's not like we have no plan how to do this cleanly.", was trying to convince Connie that this called a hack for a reason. I think we are on the same page. I think this is a really good way to solve the acute problem with a small LOC number. Halil
diff --git a/hw/core/machine.c b/hw/core/machine.c index b0fd91f..49f9007 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -554,6 +554,25 @@ static void machine_class_finalize(ObjectClass *klass, void *data) g_free(mc->name); } +static void register_compat_prop(const char *driver, + const char *property, + const char *value) +{ + GlobalProperty *p = g_new0(GlobalProperty, 1); + /* Machine compat_props must never cause errors: */ + p->errp = &error_abort; + p->driver = driver; + p->property = property; + p->value = value; + qdev_prop_register_global(p); +} + +static void machine_register_compat_for_subclass(ObjectClass *oc, void *opaque) +{ + GlobalProperty *p = opaque; + register_compat_prop(object_class_get_name(oc), p->property, p->value); +} + void machine_register_compat_props(MachineState *machine) { MachineClass *mc = MACHINE_GET_CLASS(machine); @@ -565,10 +584,20 @@ void machine_register_compat_props(MachineState *machine) } for (i = 0; i < mc->compat_props->len; i++) { + ObjectClass *oc; p = g_array_index(mc->compat_props, GlobalProperty *, i); - /* Machine compat_props must never cause errors: */ - p->errp = &error_abort; - qdev_prop_register_global(p); + oc = object_class_by_name(p->driver); + if (oc && object_class_is_abstract(oc)) { + /* temporary hack to make sure we will never override + * globals set explicitly on -global: if an abstract class + * is on compat_props, register globals for each of their + * subclasses instead. + */ + object_class_foreach(machine_register_compat_for_subclass, + p->driver, false, p); + } else { + register_compat_prop(p->driver, p->property, p->value); + } } }