Re: [Qemu-devel] [PATCH v2 1/2] qom: Implement object_property_add_const_link()

[Eduardo Habkost]

On Fri, May 01, 2015 at 08:15:24PM +0200, Paolo Bonzini wrote:
> On 01/05/2015 16:09, Eduardo Habkost wrote:
> > +void object_property_add_const_link(Object *obj, const char *name,
> > +                                       const char *type, Object *child,
> > +                                       ObjectPropertyLinkFlags flags,
> > +                                       Error **errp)
> > +{
> > +    Object **childp = g_new0(Object*, 1);
> > +
> > +    *childp = child;
> > +    object_property_add_link(obj, name, type, childp, NULL,
> > +                             flags | OBJ_PROP_LINK_FREE_CHILD_POINTER, 
> > errp);
> > +}
> > +
> 
> This works, but is the extra functionality needed, compared to
> an alias?  Namely, when is flags going to be != 0?

Flags is going to be != 0 if the caller grabs a reference to the target
object (to ensure it won't disappear) and wants it to be automatically
dropped when the property is removed. It is not strictly necessary, but
I thought it could be useful.

But to be honest, I don't love the flags argument in
object_property_add_link(), either. I mean: why do we need flags in
object_property_add_link() and not in object_property_add_alias()?

> 
> FWIW, here is my ./.. patch.  I'm all for adding a helper like
> object_property_add_const_link on top if we go for it.

Looks good to me.

> 
> Another possibility is to not introduce any of our patches and reuse
> the child<> getter and resolve functions in
> object_property_add_const_link.

Using the "." property would allow object_property_add_const_link() to
be a one-liner, so it sounds better to me.

> 
> -------------------- 8< ---------------
> From 653ca80e93fcaa94c7fa172edbaef75457d4952d Mon Sep 17 00:00:00 2001
> From: Paolo Bonzini <address@hidden>
> Date: Tue, 31 Mar 2015 13:56:24 +0200
> Subject: [PATCH] qom: add . and .. properties
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  qom/object.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/qom/object.c b/qom/object.c
> index b8dff43..3dd87b7 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1783,9 +1783,51 @@ void object_property_set_description(Object *obj, 
> const char *name,
>      op->description = g_strdup(description);
>  }
>  
> +static void property_get_dot(Object *obj, struct Visitor *v, void *opaque,
> +                             const char *name, Error **errp)
> +{
> +    gchar *path = object_get_canonical_path(obj);
> +    visit_type_str(v, &path, name, errp);
> +    g_free(path);
> +}
> +
> +static Object *property_resolve_dot(Object *obj, void *opaque,
> +                                      const gchar *part)
> +{
> +    return obj;
> +}
> +
> +static void property_get_dotdot(Object *obj, struct Visitor *v, void *opaque,
> +                                const char *name, Error **errp)
> +{
> +    if (obj->parent) {
> +        gchar *path = object_get_canonical_path(obj);
> +        visit_type_str(v, &path, name, errp);
> +        g_free(path);
> +    } else {
> +        gchar *path = (gchar *)"";
> +        visit_type_str(v, &path, name, errp);
> +    }
> +}
> +
> +static Object *property_resolve_dotdot(Object *obj, void *opaque,
> +                                      const gchar *part)
> +{
> +    return obj->parent;
> +}
> +
>  static void object_instance_init(Object *obj)
>  {
> +    ObjectProperty *op;
>      object_property_add_str(obj, "type", qdev_get_type, NULL, NULL);
> +
> +    op = object_property_add(obj, ".", "link<Object>", property_get_dot,
> +                             NULL, NULL, NULL, NULL);
> +    op->resolve = property_resolve_dot;
> +
> +    op = object_property_add(obj, "..", "link<Object>", property_get_dotdot,
> +                             NULL, NULL, NULL, NULL);
> +    op->resolve = property_resolve_dotdot;
>  }
>  
>  static void register_types(void)
> -- 
> 2.3.5
> 

-- 
Eduardo