Re: [Qemu-devel] [PATCH 1/5] qom: add a generic mechanism to resolve paths

[Paolo Bonzini]

Il 17/06/2014 16:18, Andreas Färber ha scritto:
> >  /**
> > + * ObjectPropertyResolve:
> > + * @obj: the object that owns the property
> > + * @opaque: the opaque registered with the property
> > + * @part: the name of the property
> > + *
> > + * If @path is the path that led to @obj, the function should
> > + * return the Object corresponding to "@path/@part".  If #NULL
> > + * is returned, "@path/@part" is not a valid object path.
> > + *
> > + * The returned object can also be used as a starting point
> > + * to resolve a relative path starting with "@part".
>
> Minor: I would propose something like:
>
>  * Resolves the #Object corresponding to property @part.
>  *
>  * The returned object can also be used as a starting point
>  * to resolve a relative path starting with "@part".
>  *
>  * Returns: If @path is the path that led to @obj, the function
>  * returns the #Object corresponding to "@path/@part".
>  * If "@path/@part" is not a valid object path, it returns #NULL.
>
> I.e. inverting the two sentences to fit into a gtk-doc "Returns:"
> section, and either Object -> #Object or object.

Good suggestion.
> > +void object_property_add_full(Object *obj, const char *name, const char *type,
> > +                              ObjectPropertyAccessor *get,
> > +                              ObjectPropertyAccessor *set,
> > +                              ObjectPropertyResolve *resolve,
> > +                              ObjectPropertyRelease *release,
> > +                              void *opaque, Error **errp);
>
> I'm a bit concerned about the duplication going on here, e.g. of the
> forbidden characters. I think we should either just add the new argument
> to object_property_add() and add NULL arguments at old call sites as
> done before, or we should (to avoid future _really_full,
> _really_really_full versions) return the resulting ObjectProperty * for
> modification by the caller (in this case: ->resolve = foo).

The reason I went with "_full" is that the new argument is really needed 
only in a minority of cases.  There are ~50 occurrences right now, and I 
expect only a handful of them to need a ->resolve callback (and so far 
all of them would be in qom/object.c).

There are many examples in glib's GSource (g_timeout_add_full, 
g_main_context_invoke_full, etc.) or elsewhere in glib (g_format_size_full).

Since we do not have an ABI to follow, we could add arguments to the 
_full routine while keeping the shorthand version as is.

I can change the 50 occurrences if you want though.

Paolo

> > -void object_property_add(Object *obj, const char *name, const char *type,
> > +void object_property_add_full(Object *obj, const char *name, const char *type,
> >                           ObjectPropertyAccessor *get,
> >                           ObjectPropertyAccessor *set,
> > +                         ObjectPropertyResolve *resolve,
> >                           ObjectPropertyRelease *release,
> >                           void *opaque, Error **errp)
> >  {
> > @@ -751,12 +747,23 @@ void object_property_add(Object *obj, const char *name, 
> > const char *type,
> >
> >      prop->get = get;
> >      prop->set = set;
> > +    prop->resolve = resolve;
> >      prop->release = release;
> >      prop->opaque = opaque;
> >
> >      QTAILQ_INSERT_TAIL(&obj->properties, prop, node);
> >  }
> >
> > +void object_property_add(Object *obj, const char *name, const char *type,
> > +                         ObjectPropertyAccessor *get,
> > +                         ObjectPropertyAccessor *set,
> > +                         ObjectPropertyRelease *release,
> > +                         void *opaque, Error **errp)
> > +{
> > +    object_property_add_full(obj, name, type, get, set, NULL, release,
> > +                             opaque, errp);
> > +}
> > +
> >  ObjectProperty *object_property_find(Object *obj, const char *name,
> >                                       Error **errp)
> >  {
> > @@ -993,6 +1000,11 @@ static void object_get_child_property(Object *obj, 
> > Visitor *v, void *opaque,
> >      g_free(path);
> >  }
> >
> > +static Object *object_resolve_child_property(Object *parent, void *opaque, 
> > const gchar *part)
> > +{
> > +    return opaque;
> > +}
> > +
> >  static void object_finalize_child_property(Object *obj, const char *name,
> >                                             void *opaque)
> >  {
> > @@ -1009,8 +1021,9 @@ void object_property_add_child(Object *obj, const char 
> > *name,
> >
> >      type = g_strdup_printf("child<%s>", object_get_typename(OBJECT(child)));
> >
> > -    object_property_add(obj, name, type, object_get_child_property, NULL,
> > -                        object_finalize_child_property, child, &local_err);
> > +    object_property_add_full(obj, name, type, object_get_child_property, NULL,
> > +                             object_resolve_child_property,
> > +                             object_finalize_child_property, child, 
> > &local_err);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> >          goto out;
> > @@ -1128,6 +1141,12 @@ static void object_set_link_property(Object *obj, 
> > Visitor *v, void *opaque,
> >      }
> >  }
> >
> > +static Object *object_resolve_link_property(Object *parent, void *opaque, 
> > const gchar *part)
> > +{
> > +    LinkProperty *lprop = opaque;
> > +    return *lprop->child;
> > +}
> > +
> >  static void object_release_link_property(Object *obj, const char *name,
> >                                           void *opaque)
> >  {
> > @@ -1156,12 +1175,13 @@ void object_property_add_link(Object *obj, const char 
> > *name,
> >
> >      full_type = g_strdup_printf("link<%s>", type);
> >
> > -    object_property_add(obj, name, full_type,
> > -                        object_get_link_property,
> > -                        check ? object_set_link_property : NULL,
> > -                        object_release_link_property,
> > -                        prop,
> > -                        &local_err);
> > +    object_property_add_full(obj, name, full_type,
> > +                             object_get_link_property,
> > +                             check ? object_set_link_property : NULL,
> > +                             object_resolve_link_property,
> > +                             object_release_link_property,
> > +                             prop,
> > +                             &local_err);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> >          g_free(prop);
> > @@ -1225,11 +1245,8 @@ Object *object_resolve_path_component(Object *parent, 
> > const gchar *part)
> >          return NULL;
> >      }
> >
> > -    if (object_property_is_link(prop)) {
> > -        LinkProperty *lprop = prop->opaque;
> > -        return *lprop->child;
> > -    } else if (object_property_is_child(prop)) {
> > -        return prop->opaque;
> > +    if (prop->resolve) {
> > +        return prop->resolve(parent, prop->opaque, part);
> >      } else {
> >          return NULL;
> >      }
>
> The _add vs. _add_full issue aside, the implementation looks good.
>
> Regards,
> Andreas