| Message ID | 20170711004303.3902-4-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 10 Jul 2017 21:43:03 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > This reverts commit 0bcba41fe379e4c6834adcf1456d9099db31a5b2. > > The bug addressed by that commit is now fixed in a better way by the > commit "qdev: fix the order compat and global properties are applied". > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > hw/core/machine.c | 26 +++----------------------- > 1 file changed, 3 insertions(+), 23 deletions(-) Acked-by: Cornelia Huck <cohuck@redhat.com>
On Mon, 10 Jul 2017 21:43:03 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > This reverts commit 0bcba41fe379e4c6834adcf1456d9099db31a5b2. > > The bug addressed by that commit is now fixed in a better way by the > commit "qdev: fix the order compat and global properties are applied". > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > hw/core/machine.c | 26 +++----------------------- > 1 file changed, 3 insertions(+), 23 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index ecb5552..1d10b01 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -770,18 +770,11 @@ static void machine_class_finalize(ObjectClass *klass, void *data) > g_free(mc->name); > } > > -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); > int i; > GlobalProperty *p; > - ObjectClass *oc; > > if (!mc->compat_props) { > return; > @@ -789,22 +782,9 @@ void machine_register_compat_props(MachineState *machine) > > for (i = 0; i < mc->compat_props->len; i++) { > p = g_array_index(mc->compat_props, GlobalProperty *, i); > - oc = object_class_by_name(p->driver); > - if (oc && object_class_is_abstract(oc)) { > - /* temporary hack to make sure we do not override > - * globals set explicitly on -global: if an abstract class > - * is on compat_props, register globals for all its > - * non-abstract subtypes instead. > - * > - * This doesn't solve the problem for cases where > - * a non-abstract typename mentioned on compat_props > - * has subclasses, like spapr-pci-host-bridge. > - */ > - object_class_foreach(machine_register_compat_for_subclass, > - p->driver, false, p); > - } else { > - register_compat_prop(p->driver, p->property, p->value); > - } > + /* Machine compat_props must never cause errors: */ > + p->errp = &error_abort; > + qdev_prop_register_global(p); > } > } >
On 07/11/2017 02:43 AM, Eduardo Habkost wrote: > This reverts commit 0bcba41fe379e4c6834adcf1456d9099db31a5b2. > > The bug addressed by that commit is now fixed in a better way by the > commit "qdev: fix the order compat and global properties are applied". > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com> Note: It is not like the effect of commit 0bcba41fe3 is canceled out with your first patch in place. It depends on the client code (the implementation of the individual devices) wether this patch changes something or not. I did not check myself. So the did you verify that nothing breaks with this change applies here too. > --- > hw/core/machine.c | 26 +++----------------------- > 1 file changed, 3 insertions(+), 23 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index ecb5552..1d10b01 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -770,18 +770,11 @@ static void machine_class_finalize(ObjectClass *klass, void *data) > g_free(mc->name); > } > > -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); > int i; > GlobalProperty *p; > - ObjectClass *oc; > > if (!mc->compat_props) { > return; > @@ -789,22 +782,9 @@ void machine_register_compat_props(MachineState *machine) > > for (i = 0; i < mc->compat_props->len; i++) { > p = g_array_index(mc->compat_props, GlobalProperty *, i); > - oc = object_class_by_name(p->driver); > - if (oc && object_class_is_abstract(oc)) { > - /* temporary hack to make sure we do not override > - * globals set explicitly on -global: if an abstract class > - * is on compat_props, register globals for all its > - * non-abstract subtypes instead. > - * > - * This doesn't solve the problem for cases where > - * a non-abstract typename mentioned on compat_props > - * has subclasses, like spapr-pci-host-bridge. > - */ > - object_class_foreach(machine_register_compat_for_subclass, > - p->driver, false, p); > - } else { > - register_compat_prop(p->driver, p->property, p->value); > - } > + /* Machine compat_props must never cause errors: */ > + p->errp = &error_abort; > + qdev_prop_register_global(p); > } > } >
On Wed, Jul 12, 2017 at 07:49:22PM +0200, Halil Pasic wrote: > On 07/11/2017 02:43 AM, Eduardo Habkost wrote: > > This reverts commit 0bcba41fe379e4c6834adcf1456d9099db31a5b2. > > > > The bug addressed by that commit is now fixed in a better way by the > > commit "qdev: fix the order compat and global properties are applied". > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com> > > Note: It is not like the effect of commit 0bcba41fe3 is canceled > out with your first patch in place. It depends on the client > code (the implementation of the individual devices) wether > this patch changes something or not. I did not check myself. > So the did you verify that nothing breaks with this change applies > here too. I don't get this part. I don't see how individual devices implementation will be able to affect the outcome after this patch is applied. GlobalProperty::driver is not used as input for object_property_parse() at all (see qdev_prop_set_globals()). This means exactly the same property setter is invoked when registering "<superclass>.<property>" or "<subclass>.<property>". The only difference introduced by this series is in the ordering of the object_property_parse() calls. And even the object_property_parse() call ordering is not affected by this patch at all, because of patch 1/3. Patch 1/3 will ensure the properties will be applied in exactly the same order they were registered, so this patch should introduce absolutely no behavior change on any device.
On 07/12/2017 09:20 PM, Eduardo Habkost wrote: > On Wed, Jul 12, 2017 at 07:49:22PM +0200, Halil Pasic wrote: >> On 07/11/2017 02:43 AM, Eduardo Habkost wrote: >>> This reverts commit 0bcba41fe379e4c6834adcf1456d9099db31a5b2. >>> >>> The bug addressed by that commit is now fixed in a better way by the >>> commit "qdev: fix the order compat and global properties are applied". >>> >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >> >> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> >> Note: It is not like the effect of commit 0bcba41fe3 is canceled >> out with your first patch in place. It depends on the client >> code (the implementation of the individual devices) wether >> this patch changes something or not. I did not check myself. >> So the did you verify that nothing breaks with this change applies >> here too. > > I don't get this part. I don't see how individual devices > implementation will be able to affect the outcome after this > patch is applied. > > GlobalProperty::driver is not used as input for > object_property_parse() at all (see qdev_prop_set_globals()). > This means exactly the same property setter is invoked when > registering "<superclass>.<property>" or "<subclass>.<property>". > The only difference introduced by this series is in the ordering > of the object_property_parse() calls. > > And even the object_property_parse() call ordering is not > affected by this patch at all, because of patch 1/3. Patch 1/3 > will ensure the properties will be applied in exactly the same > order they were registered, so this patch should introduce > absolutely no behavior change on any device. > Resolving this disagreement IMHO depends on resolving our disagreement about patch 1. Let us postpone this until we have an agreement there. Cheers, Halil
On Thu, Jul 13, 2017 at 02:11:09PM +0200, Halil Pasic wrote: > > > On 07/12/2017 09:20 PM, Eduardo Habkost wrote: > > On Wed, Jul 12, 2017 at 07:49:22PM +0200, Halil Pasic wrote: > >> On 07/11/2017 02:43 AM, Eduardo Habkost wrote: > >>> This reverts commit 0bcba41fe379e4c6834adcf1456d9099db31a5b2. > >>> > >>> The bug addressed by that commit is now fixed in a better way by the > >>> commit "qdev: fix the order compat and global properties are applied". > >>> > >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > >> > >> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com> > >> > >> Note: It is not like the effect of commit 0bcba41fe3 is canceled > >> out with your first patch in place. It depends on the client > >> code (the implementation of the individual devices) wether > >> this patch changes something or not. I did not check myself. > >> So the did you verify that nothing breaks with this change applies > >> here too. > > > > I don't get this part. I don't see how individual devices > > implementation will be able to affect the outcome after this > > patch is applied. > > > > GlobalProperty::driver is not used as input for > > object_property_parse() at all (see qdev_prop_set_globals()). > > This means exactly the same property setter is invoked when > > registering "<superclass>.<property>" or "<subclass>.<property>". > > The only difference introduced by this series is in the ordering > > of the object_property_parse() calls. > > > > And even the object_property_parse() call ordering is not > > affected by this patch at all, because of patch 1/3. Patch 1/3 > > will ensure the properties will be applied in exactly the same > > order they were registered, so this patch should introduce > > absolutely no behavior change on any device. > > > > Resolving this disagreement IMHO depends on resolving our disagreement > about patch 1. Let us postpone this until we have an agreement > there. Your observation on patch 1 was correct, so the ordering of object_property_parse() calls will change if using pseries <= 2.3 and "-global spapr-pci-vfio-host-bridge".
diff --git a/hw/core/machine.c b/hw/core/machine.c index ecb5552..1d10b01 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -770,18 +770,11 @@ static void machine_class_finalize(ObjectClass *klass, void *data) g_free(mc->name); } -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); int i; GlobalProperty *p; - ObjectClass *oc; if (!mc->compat_props) { return; @@ -789,22 +782,9 @@ void machine_register_compat_props(MachineState *machine) for (i = 0; i < mc->compat_props->len; i++) { p = g_array_index(mc->compat_props, GlobalProperty *, i); - oc = object_class_by_name(p->driver); - if (oc && object_class_is_abstract(oc)) { - /* temporary hack to make sure we do not override - * globals set explicitly on -global: if an abstract class - * is on compat_props, register globals for all its - * non-abstract subtypes instead. - * - * This doesn't solve the problem for cases where - * a non-abstract typename mentioned on compat_props - * has subclasses, like spapr-pci-host-bridge. - */ - object_class_foreach(machine_register_compat_for_subclass, - p->driver, false, p); - } else { - register_compat_prop(p->driver, p->property, p->value); - } + /* Machine compat_props must never cause errors: */ + p->errp = &error_abort; + qdev_prop_register_global(p); } }
This reverts commit 0bcba41fe379e4c6834adcf1456d9099db31a5b2. The bug addressed by that commit is now fixed in a better way by the commit "qdev: fix the order compat and global properties are applied". Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- hw/core/machine.c | 26 +++----------------------- 1 file changed, 3 insertions(+), 23 deletions(-)