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

[Paolo Bonzini <pbonzini@redhat.com>]

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
>