Handle calls of object_property_set_globals() with any object type,
but only apply globals to TYPE_DEVICE & TYPE_USER_CREATABLE.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
qom/globals.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/qom/globals.c b/qom/globals.c
index 587f4a1b5c..8664baebe0 100644
--- a/qom/globals.c
+++ b/qom/globals.c
@@ -15,22 +15,28 @@ void object_property_register_global(GlobalProperty *prop)
void object_property_set_globals(Object *obj)
{
- DeviceState *dev = DEVICE(obj);
GList *l;
+ DeviceState *dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE);
+
+ if (!dev && !IS_USER_CREATABLE(obj)) {
+ /* only TYPE_DEVICE and TYPE_USER_CREATABLE support globals */
+ return;
+ }
for (l = global_props; l; l = l->next) {
GlobalProperty *prop = l->data;
Error *err = NULL;
- if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) {
+ if (object_dynamic_cast(obj, prop->driver) == NULL) {
continue;
}
prop->used = true;
- object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
+ object_property_parse(obj, prop->value, prop->property, &err);
if (err != NULL) {
error_prepend(&err, "can't apply global %s.%s=%s: ",
prop->driver, prop->property, prop->value);
- if (!dev->hotplugged && prop->errp) {
+
+ if (dev && !dev->hotplugged && prop->errp) {
error_propagate(prop->errp, err);
} else {
assert(prop->user_provided);
@@ -56,15 +62,15 @@ int object_property_check_globals(void)
continue;
}
oc = object_class_by_name(prop->driver);
- oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
- if (!oc) {
+ dc = (DeviceClass *)object_class_dynamic_cast(oc, TYPE_DEVICE);
+ if (!IS_USER_CREATABLE_CLASS(oc) && !dc) {
warn_report("global %s.%s has invalid class name",
prop->driver, prop->property);
ret = 1;
continue;
}
- dc = DEVICE_CLASS(oc);
- if (!dc->hotpluggable && !prop->used) {
+
+ if (dc && !dc->hotpluggable) {
warn_report("global %s.%s=%s not used",
prop->driver, prop->property, prop->value);
ret = 1;
--
2.19.0.271.gfe8321ec05
On Tue, Oct 30, 2018 at 07:04:48PM +0400, Marc-André Lureau wrote:
> Handle calls of object_property_set_globals() with any object type,
> but only apply globals to TYPE_DEVICE & TYPE_USER_CREATABLE.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qom/globals.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/qom/globals.c b/qom/globals.c
> index 587f4a1b5c..8664baebe0 100644
> --- a/qom/globals.c
> +++ b/qom/globals.c
> @@ -15,22 +15,28 @@ void object_property_register_global(GlobalProperty *prop)
>
> void object_property_set_globals(Object *obj)
> {
> - DeviceState *dev = DEVICE(obj);
> GList *l;
> + DeviceState *dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE);
> +
> + if (!dev && !IS_USER_CREATABLE(obj)) {
> + /* only TYPE_DEVICE and TYPE_USER_CREATABLE support globals */
> + return;
> + }
This is core QOM code, ideally type-specific code doesn't belong
here.
This also duplicates the purpose of the ObjectClass::set_globals
flag you have added on another patch, doesn't it? I suggest just
dropping this hunk, and letting callers decide if it's
appropriate to call object_property_set_globals() or not.
>
> for (l = global_props; l; l = l->next) {
> GlobalProperty *prop = l->data;
> Error *err = NULL;
>
> - if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) {
> + if (object_dynamic_cast(obj, prop->driver) == NULL) {
> continue;
> }
> prop->used = true;
> - object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
> + object_property_parse(obj, prop->value, prop->property, &err);
> if (err != NULL) {
> error_prepend(&err, "can't apply global %s.%s=%s: ",
> prop->driver, prop->property, prop->value);
> - if (!dev->hotplugged && prop->errp) {
> +
> + if (dev && !dev->hotplugged && prop->errp) {
Hmm, more type-specific code. Can't we get rid of the
dev->hotplugged check here?
Maybe changing the function signature to:
void object_property_set_globals(Object *obj, bool propagate_errors);
and let the caller decide?
Or we could try to find a way to get rid of prop->errp. I never
really liked that hack, anyway.
Anyway, I won't mind keeping this code as-is if the solution is
too complex.
> error_propagate(prop->errp, err);
> } else {
> assert(prop->user_provided);
> @@ -56,15 +62,15 @@ int object_property_check_globals(void)
> continue;
> }
> oc = object_class_by_name(prop->driver);
> - oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
> - if (!oc) {
> + dc = (DeviceClass *)object_class_dynamic_cast(oc, TYPE_DEVICE);
> + if (!IS_USER_CREATABLE_CLASS(oc) && !dc) {
This could use the ObjectClass::set_globals flag you have added.
> warn_report("global %s.%s has invalid class name",
> prop->driver, prop->property);
> ret = 1;
> continue;
> }
> - dc = DEVICE_CLASS(oc);
> - if (!dc->hotpluggable && !prop->used) {
> +
> + if (dc && !dc->hotpluggable) {
I wonder how we could get rid of this type-specific check. Maybe
a ObjectClass::only_init_time_globals flag, automatically
initialized by TYPE_DEVICE based on DeviceClass::hotpluggable?
In this case, I'm not sure it would be worth the extra
complexity. The type-specific code is just for a warning,
anyway, so it's not a big deal.
> warn_report("global %s.%s=%s not used",
> prop->driver, prop->property, prop->value);
> ret = 1;
> --
> 2.19.0.271.gfe8321ec05
>
>
--
Eduardo
On Wed, 31 Oct 2018 17:12:56 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Oct 30, 2018 at 07:04:48PM +0400, Marc-André Lureau wrote:
> > Handle calls of object_property_set_globals() with any object type,
> > but only apply globals to TYPE_DEVICE & TYPE_USER_CREATABLE.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > qom/globals.c | 22 ++++++++++++++--------
> > 1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/qom/globals.c b/qom/globals.c
> > index 587f4a1b5c..8664baebe0 100644
> > --- a/qom/globals.c
> > +++ b/qom/globals.c
> > @@ -15,22 +15,28 @@ void object_property_register_global(GlobalProperty *prop)
> >
> > void object_property_set_globals(Object *obj)
> > {
> > - DeviceState *dev = DEVICE(obj);
> > GList *l;
> > + DeviceState *dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE);
> > +
> > + if (!dev && !IS_USER_CREATABLE(obj)) {
> > + /* only TYPE_DEVICE and TYPE_USER_CREATABLE support globals */
> > + return;
> > + }
>
> This is core QOM code, ideally type-specific code doesn't belong
> here.
>
> This also duplicates the purpose of the ObjectClass::set_globals
> flag you have added on another patch, doesn't it? I suggest just
> dropping this hunk, and letting callers decide if it's
> appropriate to call object_property_set_globals() or not.
>
> >
> > for (l = global_props; l; l = l->next) {
> > GlobalProperty *prop = l->data;
> > Error *err = NULL;
> >
> > - if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) {
> > + if (object_dynamic_cast(obj, prop->driver) == NULL) {
> > continue;
> > }
> > prop->used = true;
> > - object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
> > + object_property_parse(obj, prop->value, prop->property, &err);
> > if (err != NULL) {
> > error_prepend(&err, "can't apply global %s.%s=%s: ",
> > prop->driver, prop->property, prop->value);
> > - if (!dev->hotplugged && prop->errp) {
> > +
> > + if (dev && !dev->hotplugged && prop->errp) {
>
> Hmm, more type-specific code. Can't we get rid of the
> dev->hotplugged check here?
>
> Maybe changing the function signature to:
> void object_property_set_globals(Object *obj, bool propagate_errors);
> and let the caller decide?
>
> Or we could try to find a way to get rid of prop->errp. I never
> really liked that hack, anyway.
>
> Anyway, I won't mind keeping this code as-is if the solution is
> too complex.
>
>
> > error_propagate(prop->errp, err);
> > } else {
> > assert(prop->user_provided);
> > @@ -56,15 +62,15 @@ int object_property_check_globals(void)
> > continue;
> > }
> > oc = object_class_by_name(prop->driver);
> > - oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
> > - if (!oc) {
> > + dc = (DeviceClass *)object_class_dynamic_cast(oc, TYPE_DEVICE);
> > + if (!IS_USER_CREATABLE_CLASS(oc) && !dc) {
>
> This could use the ObjectClass::set_globals flag you have added.
>
> > warn_report("global %s.%s has invalid class name",
> > prop->driver, prop->property);
> > ret = 1;
> > continue;
> > }
> > - dc = DEVICE_CLASS(oc);
> > - if (!dc->hotpluggable && !prop->used) {
> > +
> > + if (dc && !dc->hotpluggable) {
>
> I wonder how we could get rid of this type-specific check. Maybe
> a ObjectClass::only_init_time_globals flag, automatically
> initialized by TYPE_DEVICE based on DeviceClass::hotpluggable?
in v1, I've suggested to add a hook something like
ObjectClass::set_globals(Object *obj)
that will handle type specific code and probably
drop instance_post_init() hook.
that would be better than adding boolean ObjectClass::set_globals
as it could serve as both a flag and a type specific handler.
So in the end we would end up replacing instance_post_init()
with set_globals() hook.
> In this case, I'm not sure it would be worth the extra
> complexity. The type-specific code is just for a warning,
> anyway, so it's not a big deal.
>
>
> > warn_report("global %s.%s=%s not used",
> > prop->driver, prop->property, prop->value);
> > ret = 1;
> > --
> > 2.19.0.271.gfe8321ec05
> >
> >
>
On Thu, Nov 01, 2018 at 11:18:42AM +0100, Igor Mammedov wrote:
> On Wed, 31 Oct 2018 17:12:56 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Tue, Oct 30, 2018 at 07:04:48PM +0400, Marc-André Lureau wrote:
> > > Handle calls of object_property_set_globals() with any object type,
> > > but only apply globals to TYPE_DEVICE & TYPE_USER_CREATABLE.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > > qom/globals.c | 22 ++++++++++++++--------
> > > 1 file changed, 14 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/qom/globals.c b/qom/globals.c
> > > index 587f4a1b5c..8664baebe0 100644
> > > --- a/qom/globals.c
> > > +++ b/qom/globals.c
> > > @@ -15,22 +15,28 @@ void object_property_register_global(GlobalProperty *prop)
> > >
> > > void object_property_set_globals(Object *obj)
> > > {
> > > - DeviceState *dev = DEVICE(obj);
> > > GList *l;
> > > + DeviceState *dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE);
> > > +
> > > + if (!dev && !IS_USER_CREATABLE(obj)) {
> > > + /* only TYPE_DEVICE and TYPE_USER_CREATABLE support globals */
> > > + return;
> > > + }
> >
> > This is core QOM code, ideally type-specific code doesn't belong
> > here.
> >
> > This also duplicates the purpose of the ObjectClass::set_globals
> > flag you have added on another patch, doesn't it? I suggest just
> > dropping this hunk, and letting callers decide if it's
> > appropriate to call object_property_set_globals() or not.
> >
> > >
> > > for (l = global_props; l; l = l->next) {
> > > GlobalProperty *prop = l->data;
> > > Error *err = NULL;
> > >
> > > - if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) {
> > > + if (object_dynamic_cast(obj, prop->driver) == NULL) {
> > > continue;
> > > }
> > > prop->used = true;
> > > - object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
> > > + object_property_parse(obj, prop->value, prop->property, &err);
> > > if (err != NULL) {
> > > error_prepend(&err, "can't apply global %s.%s=%s: ",
> > > prop->driver, prop->property, prop->value);
> > > - if (!dev->hotplugged && prop->errp) {
> > > +
> > > + if (dev && !dev->hotplugged && prop->errp) {
> >
> > Hmm, more type-specific code. Can't we get rid of the
> > dev->hotplugged check here?
> >
> > Maybe changing the function signature to:
> > void object_property_set_globals(Object *obj, bool propagate_errors);
> > and let the caller decide?
> >
> > Or we could try to find a way to get rid of prop->errp. I never
> > really liked that hack, anyway.
> >
> > Anyway, I won't mind keeping this code as-is if the solution is
> > too complex.
> >
> >
> > > error_propagate(prop->errp, err);
> > > } else {
> > > assert(prop->user_provided);
> > > @@ -56,15 +62,15 @@ int object_property_check_globals(void)
> > > continue;
> > > }
> > > oc = object_class_by_name(prop->driver);
> > > - oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
> > > - if (!oc) {
> > > + dc = (DeviceClass *)object_class_dynamic_cast(oc, TYPE_DEVICE);
> > > + if (!IS_USER_CREATABLE_CLASS(oc) && !dc) {
> >
> > This could use the ObjectClass::set_globals flag you have added.
> >
> > > warn_report("global %s.%s has invalid class name",
> > > prop->driver, prop->property);
> > > ret = 1;
> > > continue;
> > > }
> > > - dc = DEVICE_CLASS(oc);
> > > - if (!dc->hotpluggable && !prop->used) {
> > > +
> > > + if (dc && !dc->hotpluggable) {
> >
> > I wonder how we could get rid of this type-specific check. Maybe
> > a ObjectClass::only_init_time_globals flag, automatically
> > initialized by TYPE_DEVICE based on DeviceClass::hotpluggable?
> in v1, I've suggested to add a hook something like
>
> ObjectClass::set_globals(Object *obj)
>
> that will handle type specific code and probably
> drop instance_post_init() hook.
>
> that would be better than adding boolean ObjectClass::set_globals
> as it could serve as both a flag and a type specific handler.
> So in the end we would end up replacing instance_post_init()
> with set_globals() hook.
That could work, but:
I really have the impression we're overengineering this. We can
get rid of most complexity if we don't try to make -global work
with TYPE_USER_CREATABLE too.
We don't need to support -global for backend objects, and most of
the complexity in this code exists only because of -global.
There's no reason to make the complexity of -global sneak into
the core QOM code.
It would be much simpler if we just make device_post_init() do:
Error *global_errors = NULL;
/* machine_register_compat_props() won't exist anymore. */
object_apply_props(current_machine->compat_props, &error_abort);
/* accel_register_compat_props() won't exist anymore. */
object_apply_props(current_machine->accel->compat_props, &error_abort);
/* This will apply only -global options, _not_ compat props: */
object_apply_props(qdev_global_properties, &global_errors);
if (global_errors) {
//TODO: find a way to report error back to device_add or to main()
warn_report_err(global_errors);
}
...and make user_creatable_post_init() just do:
/* This will apply only compat_props: */
object_apply_props(current_machine->compat_props, &error_abort);
...with no support for -global.
This would also let us get rid of GlobalProperty::errp, which is
a hack.
And it would make the code less sensitive to subtle
initialization ordering changes, which is a problem with the
current code.
But, as I said on the reply to 10/10, for QEMU 3.1 I'd prefer to
simply add a:
bool MachineClass::canonical_path_for_ramblock_id
field, because we're past soft freeze, and deal with
global/compat_props cleanups after QEMU 3.1.
>
>
> > In this case, I'm not sure it would be worth the extra
> > complexity. The type-specific code is just for a warning,
> > anyway, so it's not a big deal.
> >
> >
> > > warn_report("global %s.%s=%s not used",
> > > prop->driver, prop->property, prop->value);
> > > ret = 1;
> > > --
> > > 2.19.0.271.gfe8321ec05
> > >
> > >
> >
>
--
Eduardo
On Thu, 1 Nov 2018 12:27:45 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Thu, Nov 01, 2018 at 11:18:42AM +0100, Igor Mammedov wrote:
> > On Wed, 31 Oct 2018 17:12:56 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > > On Tue, Oct 30, 2018 at 07:04:48PM +0400, Marc-André Lureau wrote:
> > > > Handle calls of object_property_set_globals() with any object type,
> > > > but only apply globals to TYPE_DEVICE & TYPE_USER_CREATABLE.
> > > >
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > ---
> > > > qom/globals.c | 22 ++++++++++++++--------
> > > > 1 file changed, 14 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/qom/globals.c b/qom/globals.c
> > > > index 587f4a1b5c..8664baebe0 100644
> > > > --- a/qom/globals.c
> > > > +++ b/qom/globals.c
> > > > @@ -15,22 +15,28 @@ void object_property_register_global(GlobalProperty *prop)
> > > >
> > > > void object_property_set_globals(Object *obj)
> > > > {
> > > > - DeviceState *dev = DEVICE(obj);
> > > > GList *l;
> > > > + DeviceState *dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE);
> > > > +
> > > > + if (!dev && !IS_USER_CREATABLE(obj)) {
> > > > + /* only TYPE_DEVICE and TYPE_USER_CREATABLE support globals */
> > > > + return;
> > > > + }
> > >
> > > This is core QOM code, ideally type-specific code doesn't belong
> > > here.
> > >
> > > This also duplicates the purpose of the ObjectClass::set_globals
> > > flag you have added on another patch, doesn't it? I suggest just
> > > dropping this hunk, and letting callers decide if it's
> > > appropriate to call object_property_set_globals() or not.
> > >
> > > >
> > > > for (l = global_props; l; l = l->next) {
> > > > GlobalProperty *prop = l->data;
> > > > Error *err = NULL;
> > > >
> > > > - if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) {
> > > > + if (object_dynamic_cast(obj, prop->driver) == NULL) {
> > > > continue;
> > > > }
> > > > prop->used = true;
> > > > - object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
> > > > + object_property_parse(obj, prop->value, prop->property, &err);
> > > > if (err != NULL) {
> > > > error_prepend(&err, "can't apply global %s.%s=%s: ",
> > > > prop->driver, prop->property, prop->value);
> > > > - if (!dev->hotplugged && prop->errp) {
> > > > +
> > > > + if (dev && !dev->hotplugged && prop->errp) {
> > >
> > > Hmm, more type-specific code. Can't we get rid of the
> > > dev->hotplugged check here?
> > >
> > > Maybe changing the function signature to:
> > > void object_property_set_globals(Object *obj, bool propagate_errors);
> > > and let the caller decide?
> > >
> > > Or we could try to find a way to get rid of prop->errp. I never
> > > really liked that hack, anyway.
> > >
> > > Anyway, I won't mind keeping this code as-is if the solution is
> > > too complex.
> > >
> > >
> > > > error_propagate(prop->errp, err);
> > > > } else {
> > > > assert(prop->user_provided);
> > > > @@ -56,15 +62,15 @@ int object_property_check_globals(void)
> > > > continue;
> > > > }
> > > > oc = object_class_by_name(prop->driver);
> > > > - oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
> > > > - if (!oc) {
> > > > + dc = (DeviceClass *)object_class_dynamic_cast(oc, TYPE_DEVICE);
> > > > + if (!IS_USER_CREATABLE_CLASS(oc) && !dc) {
> > >
> > > This could use the ObjectClass::set_globals flag you have added.
> > >
> > > > warn_report("global %s.%s has invalid class name",
> > > > prop->driver, prop->property);
> > > > ret = 1;
> > > > continue;
> > > > }
> > > > - dc = DEVICE_CLASS(oc);
> > > > - if (!dc->hotpluggable && !prop->used) {
> > > > +
> > > > + if (dc && !dc->hotpluggable) {
> > >
> > > I wonder how we could get rid of this type-specific check. Maybe
> > > a ObjectClass::only_init_time_globals flag, automatically
> > > initialized by TYPE_DEVICE based on DeviceClass::hotpluggable?
> > in v1, I've suggested to add a hook something like
> >
> > ObjectClass::set_globals(Object *obj)
> >
> > that will handle type specific code and probably
> > drop instance_post_init() hook.
> >
> > that would be better than adding boolean ObjectClass::set_globals
> > as it could serve as both a flag and a type specific handler.
> > So in the end we would end up replacing instance_post_init()
> > with set_globals() hook.
>
> That could work, but:
>
> I really have the impression we're overengineering this. We can
> get rid of most complexity if we don't try to make -global work
> with TYPE_USER_CREATABLE too.
>
> We don't need to support -global for backend objects, and most of
> the complexity in this code exists only because of -global.
> There's no reason to make the complexity of -global sneak into
> the core QOM code.
>
> It would be much simpler if we just make device_post_init() do:
>
> Error *global_errors = NULL;
> /* machine_register_compat_props() won't exist anymore. */
> object_apply_props(current_machine->compat_props, &error_abort);
>
> /* accel_register_compat_props() won't exist anymore. */
> object_apply_props(current_machine->accel->compat_props, &error_abort);
>
> /* This will apply only -global options, _not_ compat props: */
> object_apply_props(qdev_global_properties, &global_errors);
> if (global_errors) {
> //TODO: find a way to report error back to device_add or to main()
> warn_report_err(global_errors);
> }
>
> ...and make user_creatable_post_init() just do:
>
> /* This will apply only compat_props: */
> object_apply_props(current_machine->compat_props, &error_abort);
>
> ...with no support for -global.
that's looks promising and may actually be simpler,
all I need is to allow compat props affect backends for cases where backend changes affect ABI.
> This would also let us get rid of GlobalProperty::errp, which is
> a hack.
>
> And it would make the code less sensitive to subtle
> initialization ordering changes, which is a problem with the
> current code.
>
> But, as I said on the reply to 10/10, for QEMU 3.1 I'd prefer to
> simply add a:
> bool MachineClass::canonical_path_for_ramblock_id
> field, because we're past soft freeze, and deal with
> global/compat_props cleanups after QEMU 3.1.
it's not really bug fix, more like a feature to let switching backends
on destination host i.e. (ram on src and memfd on dst).
Lets just postpone it for 3.2 instead of adding another hack
Mark,
are you fine with moving it post 3.1 timeframe?
© 2016 - 2025 Red Hat, Inc.