On Thu, Nov 01, 2018 at 11:18:42AM +0100, Igor Mammedov wrote:
> On Wed, 31 Oct 2018 17:12:56 -0300
> Eduardo Habkost <ehabk...@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.lur...@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
