Patchwork [04/16] qom: add QObject-based property get/set wrappers

login
register
mail settings
Submitter Paolo Bonzini
Date Feb. 2, 2012, 4:45 p.m.
Message ID <1328201142-26145-5-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/139186/
State New
Headers show

Comments

Paolo Bonzini - Feb. 2, 2012, 4:45 p.m.
Move the creation of QmpInputVisitor and QmpOutputVisitor from
qmp.c to qom/object.c, since it's the only practical way to access
object properties.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/object.h |   24 ++++++++++++++++++++++++
 qmp.c                 |   17 ++---------------
 qom/object.c          |   29 +++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 15 deletions(-)
Anthony Liguori - Feb. 2, 2012, 7:06 p.m.
On 02/02/2012 10:45 AM, Paolo Bonzini wrote:
> Move the creation of QmpInputVisitor and QmpOutputVisitor from
> qmp.c to qom/object.c, since it's the only practical way to access
> object properties.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   include/qemu/object.h |   24 ++++++++++++++++++++++++
>   qmp.c                 |   17 ++---------------
>   qom/object.c          |   29 +++++++++++++++++++++++++++++
>   3 files changed, 55 insertions(+), 15 deletions(-)

I don't want object.h to have a dependency on QObject.  We need to phase out 
QObject.

Couple things:

1) We shouldn't use generic interfaces to read/write properties from objects. 
We should use type-safe accessors provided by the types themselves.

2) If we want to get fancy, we can add property_set_int, etc. and then implement 
(1) via header files that just call these functions.

Regards,

Anthony Liguori

>
> diff --git a/include/qemu/object.h b/include/qemu/object.h
> index 947cf29..71090f2 100644
> --- a/include/qemu/object.h
> +++ b/include/qemu/object.h
> @@ -542,6 +542,18 @@ void object_property_get(Object *obj, struct Visitor *v, const char *name,
>                            struct Error **errp);
>
>   /**
> + * object_property_get_qobject:
> + * @obj: the object
> + * @name: the name of the property
> + * @errp: returns an error if this function fails
> + *
> + * Returns: the value of the property, converted to QObject, or NULL if
> + * an error occurs.
> + */
> +struct QObject *object_property_get_qobject(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
> @@ -556,6 +568,18 @@ void object_property_set(Object *obj, struct Visitor *v, const char *name,
>                            struct Error **errp);
>
>   /**
> + * object_property_set_qobject:
> + * @obj: the object
> + * @ret: The value that will be written to the property.
> + * @name: the name of the property
> + * @errp: returns an error if this function fails
> + *
> + * Writes a property to a object.
> + */
> +void object_property_set_qobject(Object *obj, struct QObject *qobj,
> +                                 const char *name, struct Error **errp);
> +
> +/**
>    * @object_property_get_type:
>    * @obj: the object
>    * @name: the name of the property
> diff --git a/qmp.c b/qmp.c
> index 45052cc..c7a81cc 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -21,8 +21,6 @@
>   #include "kvm.h"
>   #include "arch_init.h"
>   #include "hw/qdev.h"
> -#include "qapi/qmp-input-visitor.h"
> -#include "qapi/qmp-output-visitor.h"
>   #include "blockdev.h"
>
>   NameInfo *qmp_query_name(Error **errp)
> @@ -198,7 +196,6 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret)
>       const char *property = qdict_get_str(qdict, "property");
>       QObject *value = qdict_get(qdict, "value");
>       Error *local_err = NULL;
> -    QmpInputVisitor *mi;
>       Object *obj;
>
>       obj = object_resolve_path(path, NULL);
> @@ -207,10 +204,7 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret)
>           goto out;
>       }
>
> -    mi = qmp_input_visitor_new(value);
> -    object_property_set(obj, qmp_input_get_visitor(mi), property,&local_err);
> -
> -    qmp_input_visitor_cleanup(mi);
> +    object_property_set_qobject(obj, value, property,&local_err);
>
>   out:
>       if (local_err) {
> @@ -227,7 +221,6 @@ int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret)
>       const char *path = qdict_get_str(qdict, "path");
>       const char *property = qdict_get_str(qdict, "property");
>       Error *local_err = NULL;
> -    QmpOutputVisitor *mo;
>       Object *obj;
>
>       obj = object_resolve_path(path, NULL);
> @@ -236,13 +229,7 @@ int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret)
>           goto out;
>       }
>
> -    mo = qmp_output_visitor_new();
> -    object_property_get(obj, qmp_output_get_visitor(mo), property,&local_err);
> -    if (!local_err) {
> -        *ret = qmp_output_get_qobject(mo);
> -    }
> -
> -    qmp_output_visitor_cleanup(mo);
> +    *ret = object_property_get_qobject(obj, property,&local_err);
>
>   out:
>       if (local_err) {
> diff --git a/qom/object.c b/qom/object.c
> index 299e146..13c8bec 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -13,6 +13,8 @@
>   #include "qemu/object.h"
>   #include "qemu-common.h"
>   #include "qapi/qapi-visit-core.h"
> +#include "qapi/qmp-input-visitor.h"
> +#include "qapi/qmp-output-visitor.h"
>
>   #define MAX_INTERFACES 32
>
> @@ -646,6 +648,33 @@ void object_property_set(Object *obj, Visitor *v, const char *name,
>       }
>   }
>
> +void object_property_set_qobject(Object *obj, QObject *value,
> +                                 const char *name, Error **errp)
> +{
> +    QmpInputVisitor *mi;
> +    mi = qmp_input_visitor_new(value);
> +    object_property_set(obj, qmp_input_get_visitor(mi), name, errp);
> +
> +    qmp_input_visitor_cleanup(mi);
> +}
> +
> +QObject *object_property_get_qobject(Object *obj, const char *name,
> +                                     Error **errp)
> +{
> +    QObject *ret = NULL;
> +    Error *local_err = NULL;
> +    QmpOutputVisitor *mo;
> +
> +    mo = qmp_output_visitor_new();
> +    object_property_get(obj, qmp_output_get_visitor(mo), name,&local_err);
> +    if (!local_err) {
> +        ret = qmp_output_get_qobject(mo);
> +    }
> +    error_propagate(errp, local_err);
> +    qmp_output_visitor_cleanup(mo);
> +    return ret;
> +}
> +
>   const char *object_property_get_type(Object *obj, const char *name, Error **errp)
>   {
>       ObjectProperty *prop = object_property_find(obj, name);
Andreas Färber - Feb. 2, 2012, 7:21 p.m.
Am 02.02.2012 20:06, schrieb Anthony Liguori:
> On 02/02/2012 10:45 AM, Paolo Bonzini wrote:
>> Move the creation of QmpInputVisitor and QmpOutputVisitor from
>> qmp.c to qom/object.c, since it's the only practical way to access
>> object properties.
>>
>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>> ---
>>   include/qemu/object.h |   24 ++++++++++++++++++++++++
>>   qmp.c                 |   17 ++---------------
>>   qom/object.c          |   29 +++++++++++++++++++++++++++++
>>   3 files changed, 55 insertions(+), 15 deletions(-)
> 
> I don't want object.h to have a dependency on QObject.  We need to phase
> out QObject.

We did get that dependency though by your move of the property code to
object.c. As you will see shortly, we now need qobject-obj-y and
qapi-obj-y plus some stubs to make the user emulators compile with QOM.

Andreas
Paolo Bonzini - Feb. 2, 2012, 7:24 p.m.
On 02/02/2012 08:06 PM, Anthony Liguori wrote:
> I don't want object.h to have a dependency on QObject.  We need to phase
> out QObject.

The header doesn't.

> Couple things:
>
> 1) We shouldn't use generic interfaces to read/write properties from
> objects. We should use type-safe accessors provided by the types
> themselves.
>
> 2) If we want to get fancy, we can add property_set_int, etc. and then
> implement (1) via header files that just call these functions.

That's what patch 5 does.  But writing visitors in C is a royal PITA. 
The only sane way to do so is via QObject.

Paolo
Paolo Bonzini - Feb. 2, 2012, 7:29 p.m.
On 02/02/2012 08:24 PM, Paolo Bonzini wrote:
>>
>> 1) We shouldn't use generic interfaces to read/write properties from
>> objects. We should use type-safe accessors provided by the types
>> themselves.

That doesn't change the fact that we need simple wrappers using C types 
(at various levels: object_property_set_qobject, object_property_set, 
qdev_set_*) to implement these type-safe accessors on top of dynamic 
properties.

>> 2) If we want to get fancy, we can add property_set_int, etc. and then
>> implement (1) via header files that just call these functions.
>
> That's what patch 5 does.  But writing visitors in C is a royal PITA.
> The only sane way to do so is via QObject.

BTW, I don't really think it's possible to proceed on this except by 
accepting compromises.  We need to be the #1 QOM client, _now_ or it 
will remain buggy & bitrot.

Paolo
Anthony Liguori - Feb. 2, 2012, 7:36 p.m.
On 02/02/2012 01:24 PM, Paolo Bonzini wrote:
> On 02/02/2012 08:06 PM, Anthony Liguori wrote:
>> I don't want object.h to have a dependency on QObject. We need to phase
>> out QObject.
>
> The header doesn't.
>
>> Couple things:
>>
>> 1) We shouldn't use generic interfaces to read/write properties from
>> objects. We should use type-safe accessors provided by the types
>> themselves.
>>
>> 2) If we want to get fancy, we can add property_set_int, etc. and then
>> implement (1) via header files that just call these functions.
>
> That's what patch 5 does. But writing visitors in C is a royal PITA. The only
> sane way to do so is via QObject.

You just need a variant visitor.  It's pretty simple to do, essentially:

typedef struct VariantVisitor
{
     Visitor parent;
     enum { VV_INT, VV_STR } kind;
     union { int64_t v_int; char *v_str };
} VariantVisitor;

/* input */
static void visit_int(...)
{
    v->kind = TYPE_INT;
    v->v_int = *value;
}

/* output */
static void visit_int(...)
{
    assert(v->kind == TYPE_INT);
    *value = v->v_int;
}

void variant_visitor_set_int(VariantVisitor *v, int64_t value)
{
    v->kind = TYPE_INT;
    v->v_int = value;
}

The only types that matter are int and string so the variant visitor is pretty 
simple.

Regards,

Anthony Liguori

>
> Paolo
>
>
Anthony Liguori - Feb. 2, 2012, 8:01 p.m.
On 02/02/2012 01:29 PM, Paolo Bonzini wrote:
> On 02/02/2012 08:24 PM, Paolo Bonzini wrote:
>>>
>>> 1) We shouldn't use generic interfaces to read/write properties from
>>> objects. We should use type-safe accessors provided by the types
>>> themselves.
>
> That doesn't change the fact that we need simple wrappers using C types (at
> various levels: object_property_set_qobject, object_property_set, qdev_set_*) to
> implement these type-safe accessors on top of dynamic properties.
>
>>> 2) If we want to get fancy, we can add property_set_int, etc. and then
>>> implement (1) via header files that just call these functions.
>>
>> That's what patch 5 does. But writing visitors in C is a royal PITA.
>> The only sane way to do so is via QObject.
>
> BTW, I don't really think it's possible to proceed on this except by accepting
> compromises. We need to be the #1 QOM client, _now_ or it will remain buggy &
> bitrot.

Not disagreeing at all with the goal, just the implementation :-)

We can pretty easily avoid a QObject dependency.  I can throw together that 
patch if you'd like.

Regards,

Anthony Liguori

>
> Paolo
>
Paolo Bonzini - Feb. 2, 2012, 8:08 p.m.
On 02/02/2012 08:36 PM, Anthony Liguori wrote:
> The only types that matter are int and string so the variant visitor is
> pretty simple.

Sure, only ~150 lines of code.  I also do not disagree with the goals 
(mine and yours), just with the priorities. :)

Paolo
Anthony Liguori - Feb. 2, 2012, 8:58 p.m.
On 02/02/2012 01:21 PM, Andreas Färber wrote:
> Am 02.02.2012 20:06, schrieb Anthony Liguori:
>> On 02/02/2012 10:45 AM, Paolo Bonzini wrote:
>>> Move the creation of QmpInputVisitor and QmpOutputVisitor from
>>> qmp.c to qom/object.c, since it's the only practical way to access
>>> object properties.
>>>
>>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>>> ---
>>>    include/qemu/object.h |   24 ++++++++++++++++++++++++
>>>    qmp.c                 |   17 ++---------------
>>>    qom/object.c          |   29 +++++++++++++++++++++++++++++
>>>    3 files changed, 55 insertions(+), 15 deletions(-)
>>
>> I don't want object.h to have a dependency on QObject.  We need to phase
>> out QObject.
>
> We did get that dependency though by your move of the property code to
> object.c. As you will see shortly, we now need qobject-obj-y and
> qapi-obj-y plus some stubs to make the user emulators compile with QOM.
>

That's an implementation detail of Error, that's not because QObject is used 
anywhere in QOM.

Regards,

Anthony Liguori

> Andreas
>
Anthony Liguori - Feb. 2, 2012, 8:59 p.m.
On 02/02/2012 02:08 PM, Paolo Bonzini wrote:
> On 02/02/2012 08:36 PM, Anthony Liguori wrote:
>> The only types that matter are int and string so the variant visitor is
>> pretty simple.
>
> Sure, only ~150 lines of code. I also do not disagree with the goals (mine and
> yours), just with the priorities. :)

That's fine, it's a priority for me, so I'm happy to send a patch to your series.

I think it's important to maintain strict modularity at the core layer of QOM.

Regards,

Anthony Liguori

>
> Paolo
>

Patch

diff --git a/include/qemu/object.h b/include/qemu/object.h
index 947cf29..71090f2 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -542,6 +542,18 @@  void object_property_get(Object *obj, struct Visitor *v, const char *name,
                          struct Error **errp);
 
 /**
+ * object_property_get_qobject:
+ * @obj: the object
+ * @name: the name of the property
+ * @errp: returns an error if this function fails
+ *
+ * Returns: the value of the property, converted to QObject, or NULL if
+ * an error occurs.
+ */
+struct QObject *object_property_get_qobject(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
@@ -556,6 +568,18 @@  void object_property_set(Object *obj, struct Visitor *v, const char *name,
                          struct Error **errp);
 
 /**
+ * object_property_set_qobject:
+ * @obj: the object
+ * @ret: The value that will be written to the property.
+ * @name: the name of the property
+ * @errp: returns an error if this function fails
+ *
+ * Writes a property to a object.
+ */
+void object_property_set_qobject(Object *obj, struct QObject *qobj,
+                                 const char *name, struct Error **errp);
+
+/**
  * @object_property_get_type:
  * @obj: the object
  * @name: the name of the property
diff --git a/qmp.c b/qmp.c
index 45052cc..c7a81cc 100644
--- a/qmp.c
+++ b/qmp.c
@@ -21,8 +21,6 @@ 
 #include "kvm.h"
 #include "arch_init.h"
 #include "hw/qdev.h"
-#include "qapi/qmp-input-visitor.h"
-#include "qapi/qmp-output-visitor.h"
 #include "blockdev.h"
 
 NameInfo *qmp_query_name(Error **errp)
@@ -198,7 +196,6 @@  int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret)
     const char *property = qdict_get_str(qdict, "property");
     QObject *value = qdict_get(qdict, "value");
     Error *local_err = NULL;
-    QmpInputVisitor *mi;
     Object *obj;
 
     obj = object_resolve_path(path, NULL);
@@ -207,10 +204,7 @@  int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret)
         goto out;
     }
 
-    mi = qmp_input_visitor_new(value);
-    object_property_set(obj, qmp_input_get_visitor(mi), property, &local_err);
-
-    qmp_input_visitor_cleanup(mi);
+    object_property_set_qobject(obj, value, property, &local_err);
 
 out:
     if (local_err) {
@@ -227,7 +221,6 @@  int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret)
     const char *path = qdict_get_str(qdict, "path");
     const char *property = qdict_get_str(qdict, "property");
     Error *local_err = NULL;
-    QmpOutputVisitor *mo;
     Object *obj;
 
     obj = object_resolve_path(path, NULL);
@@ -236,13 +229,7 @@  int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret)
         goto out;
     }
 
-    mo = qmp_output_visitor_new();
-    object_property_get(obj, qmp_output_get_visitor(mo), property, &local_err);
-    if (!local_err) {
-        *ret = qmp_output_get_qobject(mo);
-    }
-
-    qmp_output_visitor_cleanup(mo);
+    *ret = object_property_get_qobject(obj, property, &local_err);
 
 out:
     if (local_err) {
diff --git a/qom/object.c b/qom/object.c
index 299e146..13c8bec 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -13,6 +13,8 @@ 
 #include "qemu/object.h"
 #include "qemu-common.h"
 #include "qapi/qapi-visit-core.h"
+#include "qapi/qmp-input-visitor.h"
+#include "qapi/qmp-output-visitor.h"
 
 #define MAX_INTERFACES 32
 
@@ -646,6 +648,33 @@  void object_property_set(Object *obj, Visitor *v, const char *name,
     }
 }
 
+void object_property_set_qobject(Object *obj, QObject *value,
+                                 const char *name, Error **errp)
+{
+    QmpInputVisitor *mi;
+    mi = qmp_input_visitor_new(value);
+    object_property_set(obj, qmp_input_get_visitor(mi), name, errp);
+
+    qmp_input_visitor_cleanup(mi);
+}
+
+QObject *object_property_get_qobject(Object *obj, const char *name,
+                                     Error **errp)
+{
+    QObject *ret = NULL;
+    Error *local_err = NULL;
+    QmpOutputVisitor *mo;
+
+    mo = qmp_output_visitor_new();
+    object_property_get(obj, qmp_output_get_visitor(mo), name, &local_err);
+    if (!local_err) {
+        ret = qmp_output_get_qobject(mo);
+    }
+    error_propagate(errp, local_err);
+    qmp_output_visitor_cleanup(mo);
+    return ret;
+}
+
 const char *object_property_get_type(Object *obj, const char *name, Error **errp)
 {
     ObjectProperty *prop = object_property_find(obj, name);