On Thu, 1 Nov 2018 12:27:45 -0300
Eduardo Habkost <ehabk...@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 <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.
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?
