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

[Alexander Barabash]

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