Re: [Qemu-devel] [PATCH] Add object_property_get_child().

[Alexander Barabash <alexander_barabash@mentor.com>]

On 02/17/2012 12:17 PM, Paolo Bonzini wrote:
> Besides the non-triviality of the patch, how is this different from
> object_property_get_link?  Perhaps we should just rename that one to
> object_property_get_obj, or add a function that is just a synonym.
As for the patch's (non-)triviality, I just wanted to check what counts 
as trivial here.
I agree that was a bit over-reaching.

The proposed object_property_get_child() may return either
the direct child with the specified name in the composition tree,
or the value of the link with the specified name, as
object_property_get_link() indeed does.

It has exactly the same semantics as:

Object *object_property_get_child(Object *obj, const char *name,
                                   struct Error **errp)
{
     Object * result;
     bool ambigious;
     gchar *child_path;

     /* If name contains a '/' in it, report an error and return NULL. */

     child_path = g_strdup_printf("%s/%s", 
object_get_canonical_path(obj), name);
     result = object_resolve_path(child_path, &ambigious);
     g_free(child_path);
     if (result == NULL) {
        /* Report an error. */
        return NULL;
     }
     if (ambigious) {
        /* Report an error. */
        return NULL;
     }
     return result;
}

but does it in a more straightforward way.

>
>> + */
>> +Object *object_property_get_child(Object *obj, const char *name,
>> +                                  struct Error **errp);
>> +
>> +/**
>>    * object_property_set:
>>    * @obj: the object
>>    * @v: the visitor that will be used to write the property value.  This
>> should
>> diff --git a/qom/object.c b/qom/object.c
>> index 2de6eaf..61e775b 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -297,12 +297,86 @@ static void object_property_del_all(Object *obj)
>>       }
>>   }
>>
>> +/*
>> + * To ensure correct format checking,
>> + * this function should be used only via REPORT_OBJECT_ERROR() macro.
>> + *
>> + * The first argument after 'obj' should be of type 'const char *'.
>> + * It is ignored, and replaced by the canonical path of 'obj'.
>> + */
>> +static void report_object_error(Error **errp, const char *fmt, Object
>> *obj, ...)
>> +    GCC_FMT_ATTR(2, 4);
>> +static void report_object_error(Error **errp, const char *fmt, Object
>> *obj, ...)
>> +{
>> +    gchar *path;
>> +    va_list ap;
>> +
>> +    if (errp != NULL) {
>> +        path = object_get_canonical_path(obj);
>> +        va_start(ap, obj);
>> +        va_arg(ap, const char *); /* Ignore the dummy string. */
> Why do you need it at all?

I think this is needed to avoid replicating code and have an easy tool 
to report
object's path in errors.

>
>> +        error_set(errp, fmt, path,&ap);
>> +        va_end(ap);
>> +        g_free(path);
>> +    }
>> +}
>> +#define REPORT_OBJECT_ERROR(errp, fmt, obj, ...)                 \
>> +    do {                                                         \
>> +        report_object_error(errp, fmt, obj, "", ## __VA_ARGS__); \
>> +    } while (0)
>> +#define CHILD_PROPERTY_TYPE_PREFIX "child<"
>> +#define CHILD_PROPERTY_TYPE_SUFFIX ">"
>> +#define LINK_PROPERTY_TYPE_PREFIX "link<"
>> +#define LINK_PROPERTY_TYPE_SUFFIX ">"
>> +
>> +static bool object_property_is_child(ObjectProperty *prop)
>> +{
>> +    return (strstart(prop->type, CHILD_PROPERTY_TYPE_PREFIX, NULL) != 0);
>> +}
>> +
>> +static bool object_property_is_link(ObjectProperty *prop)
>> +{
>> +    return (strstart(prop->type, LINK_PROPERTY_TYPE_PREFIX, NULL) != 0);
>> +}
>> +
>> +/* Go from LINK_PROPERTY_TYPE_PREFIX FOO LINK_PROPERTY_TYPE_SUFFIX to
>> FOO.  */
>> +static gchar *link_type_to_type(const gchar *type)
>> +{
>> +    return g_strndup(&type[sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1],
>> +                     strlen(type)
>> +                     - (sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1)
>> +                     - (sizeof(LINK_PROPERTY_TYPE_SUFFIX) - 1));
>> +}
>> +
>> +static Object *object_property_extract_child(ObjectProperty *prop,
>> +                                             bool *p_is_invalid_type)
>> +{
>> +    if (p_is_invalid_type != NULL) {
>> +        *p_is_invalid_type = false;
>> +    }
>> +    if (object_property_is_child(prop)) {
>> +        return (Object *)prop->opaque;
>> +    } else if (object_property_is_link(prop)) {
>> +        if (prop->opaque != NULL) {
>> +            return *(Object **)prop->opaque;
>> +        } else {
>> +            return NULL;
>> +        }
>> +    } else {
>> +        if (p_is_invalid_type != NULL) {
>> +            *p_is_invalid_type = true;
>> +        }
>> +        return NULL;
>> +    }
>> +}
>> +
> object_property_get_child should never be on a fast path.  We should
> avoid as much as possible peeking in the opaque.  Instead, you should
> just use a visitor like object_property_get_link does.

I am not sure what do you mean by "should never be on a fast path".
Anyway, I did not add any peeking in the opaque. I just moved the code
from object_resolve_abs_path() into a subroutine. Fairly enough, we could
avoid looking into the link property's opaque by calling 
object_property_get_link()
instead. I'll prepare a revision of the patch to this end.


>
> Everything starting here should be a separate patch.  But if you remove
> object_property_extract_child, I doubt it makes as much sense to
> refactor this.

Obviously, the refactoring was needed to add code. If we don't,
no refactoring is required.

>
>>   static void object_property_del_child(Object *obj, Object *child, Error
>> **errp)
>>   {
>>       ObjectProperty *prop;
>>
>>       QTAILQ_FOREACH(prop,&obj->properties, node) {
>> -        if (!strstart(prop->type, "child<", NULL)) {
>> +        if (!object_property_is_child(prop)) {
>>               continue;
>>           }
>>
>> @@ -799,6 +873,27 @@ Object *object_get_root(void)
>>       return root;
>>   }
>>
>> +Object *object_property_get_child(Object *obj, const char *name,
>> +                                  struct Error **errp) {
>> +    Object *result = NULL;
>> +    ObjectProperty *prop = object_property_find(obj, name);
>> +    bool is_invalid_type;
>> +
>> +    if (prop == NULL) {
>> +        REPORT_OBJECT_ERROR(errp, QERR_OBJECT_PROPERTY_NOT_FOUND, obj,
>> name);
>> +        return NULL;
>> +    }
>> +
>> +    result = object_property_extract_child(prop,&is_invalid_type);
>> +    if (is_invalid_type) {
>> +        REPORT_OBJECT_ERROR(errp, QERR_OBJECT_PROPERTY_INVALID_TYPE,
>> +                            obj, name, "child");
>> +        return NULL;
>> +    }
>> +
>> +    return result;
>> +}
>> +
>>   static void object_get_child_property(Object *obj, Visitor *v, void
>> *opaque,
>>                                         const char *name, Error **errp)
>>   {
>> @@ -829,7 +924,10 @@ void object_property_add_child(Object *obj, const
>> char *name,
>>        */
>>       assert(!object_is_type(obj, type_interface));
>>
>> -    type = g_strdup_printf("child<%s>",
>> object_get_typename(OBJECT(child)));
>> +    type = g_strdup_printf(CHILD_PROPERTY_TYPE_PREFIX
>> +                           "%s"
>> +                           CHILD_PROPERTY_TYPE_SUFFIX,
>> +                           object_get_typename(OBJECT(child)));
>>
>>       object_property_add(obj, name, type, object_get_child_property,
>>                           NULL, object_finalize_child_property, child,
>> errp);
>> @@ -878,8 +976,7 @@ static void object_set_link_property(Object *obj,
>> Visitor *v, void *opaque,
>>       if (strcmp(path, "") != 0) {
>>           Object *target;
>>
>> -        /* Go from link<FOO>  to FOO.  */
>> -        target_type = g_strndup(&type[5], strlen(type) - 6);
>> +        target_type = link_type_to_type(type);
>>           target = object_resolve_path_type(path, target_type,&ambiguous);
>>
>>           if (ambiguous) {
>> @@ -907,7 +1004,9 @@ void object_property_add_link(Object *obj, const
>> char *name,
>>   {
>>       gchar *full_type;
>>
>> -    full_type = g_strdup_printf("link<%s>", type);
>> +    full_type = g_strdup_printf(LINK_PROPERTY_TYPE_PREFIX
>> +                                "%s"
>> +                                LINK_PROPERTY_TYPE_SUFFIX, type);
>>
>>       object_property_add(obj, name, full_type,
>>                           object_get_link_property,
>> @@ -932,7 +1031,7 @@ gchar *object_get_canonical_path(Object *obj)
>>           g_assert(obj->parent != NULL);
>>
>>           QTAILQ_FOREACH(prop,&obj->parent->properties, node) {
>> -            if (!strstart(prop->type, "child<", NULL)) {
>> +            if (!object_property_is_child(prop)) {
>>                   continue;
>>               }
>>
>> @@ -980,15 +1079,7 @@ static Object *object_resolve_abs_path(Object
>> *parent,
>>           return NULL;
>>       }
>>
>> -    child = NULL;
>> -    if (strstart(prop->type, "link<", NULL)) {
>> -        Object **pchild = prop->opaque;
>> -        if (*pchild) {
>> -            child = *pchild;
>> -        }
>> -    } else if (strstart(prop->type, "child<", NULL)) {
>> -        child = prop->opaque;
>> -    }
>> +    child = object_property_extract_child(prop, NULL);
>>
>>       if (!child) {
>>           return NULL;
>> @@ -1010,7 +1101,7 @@ static Object *object_resolve_partial_path(Object
>> *parent,
>>       QTAILQ_FOREACH(prop,&parent->properties, node) {
>>           Object *found;
>>
>> -        if (!strstart(prop->type, "child<", NULL)) {
>> +        if (!object_property_is_child(prop)) {
>>               continue;
>>           }
>>
>>
>>
>>
> Paolo
Alex