[Qemu-devel] [PATCH v2 0/2] cpu: Add /machine/cpus[<index>] links

[Qemu-devel] [PATCH v2 0/2] cpu: Add /machine/cpus[<index>] links

[Eduardo Habkost]

Changes v1 -> v2:
* Implement a object_property_add_const_link() function
* Use "/machine/cpus[<index>]" instead of "/machine/cpus/<index>"
* Grab reference to object before adding link

Eduardo Habkost (2):
  qom: Implement object_property_add_const_link()
  cpu: Register QOM links at /machine/cpus[<index>]

 exec.c               | 15 +++++++++++++++
 include/qom/object.h | 23 +++++++++++++++++++++++
 qom/object.c         | 15 +++++++++++++++
 3 files changed, 53 insertions(+)

-- 
2.1.0

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

[Eduardo Habkost]

It can be used in simpler cases where a read-only property is needed.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 include/qom/object.h | 23 +++++++++++++++++++++++
 qom/object.c         | 15 +++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index d2d7748..78bb941 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1123,6 +1123,8 @@ void object_property_add_child(Object *obj, const char *name,
 typedef enum {
     /* Unref the link pointer when the property is deleted */
     OBJ_PROP_LINK_UNREF_ON_RELEASE = 0x1,
+    /* Free the child pointer passed to object_property_add_link() */
+    OBJ_PROP_LINK_FREE_CHILD_POINTER = 0x2,
 } ObjectPropertyLinkFlags;
 
 /**
@@ -1162,6 +1164,8 @@ void object_property_allow_set_link(Object *, const char *,
  * property is deleted with object_property_del().  If the
  * <code>@flags</code> <code>OBJ_PROP_LINK_UNREF_ON_RELEASE</code> bit is set,
  * the reference count is decremented when the property is deleted.
+ * If the <code>OBJ_PROP_LINK_FREE_CHILD_POINTER</code> bit is set,
+ * @child is freed when the property is deleted.
  */
 void object_property_add_link(Object *obj, const char *name,
                               const char *type, Object **child,
@@ -1170,6 +1174,25 @@ void object_property_add_link(Object *obj, const char *name,
                               ObjectPropertyLinkFlags flags,
                               Error **errp);
 
+
+/**
+ * object_property_add_const_link:
+ * @obj: the object to add a property to
+ * @name: the name of the property
+ * @type: the qobj type of the link
+ * @child: pointer to the linked object
+ * @flags: additional options for the link
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Like object_property_add_link(), but adds a link property that will
+ * always point to the same object.
+ */
+void object_property_add_const_link(Object *obj, const char *name,
+                                       const char *type, Object *child,
+                                       ObjectPropertyLinkFlags flags,
+                                       Error **errp);
+
+
 /**
  * object_property_add_str:
  * @obj: the object to add a property to
diff --git a/qom/object.c b/qom/object.c
index b8dff43..7d4a46d 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1227,6 +1227,9 @@ static void object_release_link_property(Object *obj, const char *name,
     if ((prop->flags & OBJ_PROP_LINK_UNREF_ON_RELEASE) && *prop->child) {
         object_unref(*prop->child);
     }
+    if (prop->flags & OBJ_PROP_LINK_FREE_CHILD_POINTER) {
+        g_free(prop->child);
+    }
     g_free(prop);
 }
 
@@ -1266,6 +1269,18 @@ out:
     g_free(full_type);
 }
 
+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);
+}
+
 gchar *object_get_canonical_path_component(Object *obj)
 {
     ObjectProperty *prop = NULL;
-- 
2.1.0

[Qemu-devel] [PATCH v2 2/2] cpu: Register QOM links at /machine/cpus[<index>]

[Eduardo Habkost]

This will provide a predictable path for the CPU objects, and a more
powerful alternative for the query-cpus QMP command, as now every QOM
property on CPU objects can be easily queried.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* Use object_property_add_const_link()
* Use "/machine/cpus[<index>]" instead of "/machine/cpus/<index>"
* Grab reference to object before adding link

Note that this doesn't replace any future topology enumeration
mechanisms we may choose to implement. It just replaces the existing
topology-unaware VCPU enumeration mechanism that is query-cpus.

References to previous discussions:

  Date: Thu, 23 Apr 2015 10:17:36 -0300
  From: Eduardo Habkost <ehabkost@redhat.com>
  Subject: Re: [Qemu-devel] cpu modelling and hotplug (was: [PATCH RFC 0/4] target-i386: PC socket/core/thread modeling, part 1)
  Message-ID: <20150423131736.GA17796@thinpad.lan.raisama.net>

  Date: Tue, 31 Mar 2015 10:16:23 -0300
  From: Eduardo Habkost <ehabkost@redhat.com>
  Subject: Re: [Qemu-devel] [PATCH v4 11/15] target-s390x: New QMP command query-cpu-model
  Message-ID: <20150331131623.GG7031@thinpad.lan.raisama.net>

  Date: Fri, 16 May 2014 12:16:41 -0300
  From: Eduardo Habkost <ehabkost@redhat.com>
  Subject: Re: [Qemu-devel] [RFC 0/5] Allow object-add on X86CPU subclasses, for CPU model probing
  Message-ID: <20140516151641.GY3302@otherpad.lan.raisama.net>
---
 exec.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/exec.c b/exec.c
index ae37b98..a68f0af 100644
--- a/exec.c
+++ b/exec.c
@@ -519,6 +519,19 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
 }
 #endif
 
+static void cpu_add_qom_link(CPUState *cpu)
+{
+#if !defined(CONFIG_USER_ONLY)
+    Object *obj = OBJECT(cpu);
+    char *path = g_strdup_printf("cpus[%d]", cpu->cpu_index);
+
+    object_ref(obj);
+    object_property_add_const_link(OBJECT(current_machine), path, TYPE_CPU,
+                                      obj, 0, &error_abort);
+    g_free(path);
+#endif
+}
+
 void cpu_exec_init(CPUArchState *env)
 {
     CPUState *cpu = ENV_GET_CPU(env);
@@ -558,6 +571,8 @@ void cpu_exec_init(CPUArchState *env)
     if (cc->vmsd != NULL) {
         vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
     }
+
+    cpu_add_qom_link(cpu);
 }
 
 #if defined(CONFIG_USER_ONLY)
-- 
2.1.0

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

[Paolo Bonzini]

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?

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.

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

-------------------- 8< ---------------
>From 653ca80e93fcaa94c7fa172edbaef75457d4952d Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 31 Mar 2015 13:56:24 +0200
Subject: [PATCH] qom: add . and .. properties

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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

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 <pbonzini@redhat.com>
> Date: Tue, 31 Mar 2015 13:56:24 +0200
> Subject: [PATCH] qom: add . and .. properties
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  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

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

[Paolo Bonzini]

On 05/05/2015 18:06, Eduardo Habkost wrote:
> 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.
> 

You don't need this function anymore though, right?  So I'll just adjust
my series to use object_property_add_const_link().

Paolo