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.
> 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
> >
> >
>
