[Qemu-devel] [RFC 0/5] -object/object-add support custom location and 2nd stage initialization

[Qemu-devel] [RFC 0/5] -object/object-add support custom location and 2nd stage initialization

[Igor Mammedov]

Adds optional interfaces that objects could implement if
they need to:
  1. perform an additional initialization after object properties are set
  2. be placed not in '/objects' container

Series depends on 2 PULL requests in flight from Andreas & Luiz
with fixes for QOM interfaces and object-add monitor/QMP command.
Git tree for testing:
  https://github.com/imammedo/qemu/commits/extend-object-add

Igor Mammedov (5):
  object_add: consolidate error handling
  add optional 2nd stage initialization to
    -object/object-add/object_add commands
  virtio_rng: use object_realize interface instead of calling backend
    API
  vl.c: -object: handle duplicate 'id' properly
  -object/object-add: use custom default object location if provided

 backends/rng.c                  |   17 ++++++-
 hw/virtio/virtio-rng.c          |   15 ++++---
 include/qom/object_interfaces.h |   96 +++++++++++++++++++++++++++++++++++++++
 include/sysemu/rng.h            |   11 -----
 qmp.c                           |   30 +++++++++---
 qom/Makefile.objs               |    1 +
 qom/object_interfaces.c         |   57 +++++++++++++++++++++++
 vl.c                            |   21 ++++++++-
 8 files changed, 220 insertions(+), 28 deletions(-)
 create mode 100644 include/qom/object_interfaces.h
 create mode 100644 qom/object_interfaces.c

[Qemu-devel] [PATCH 1/5] object_add: consolidate error handling

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 qmp.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/qmp.c b/qmp.c
index 0f46171..a67e0c4 100644
--- a/qmp.c
+++ b/qmp.c
@@ -549,15 +549,17 @@ void object_add(const char *type, const char *id, const QDict *qdict,
         for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
             object_property_set(obj, v, e->key, &local_err);
             if (local_err) {
-                error_propagate(errp, local_err);
-                object_unref(obj);
-                return;
+                goto out;
             }
         }
     }
 
     object_property_add_child(container_get(object_get_root(), "/objects"),
-                              id, obj, errp);
+                              id, obj, &local_err);
+out:
+    if (local_err) {
+        error_propagate(errp, local_err);
+    }
     object_unref(obj);
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH 2/5] add optional 2nd stage initialization to -object/object-add/object_add commands

[Igor Mammedov]

Provides an ability to do an optional second stage initialization
of an object created with -object/object-add/object_add commands.

Patch adds interface that provides realize() callback, which is
called after the object properties were set upon completion of
-object/object-add/object_add command, if the type implements
OBJECT_REALIZE_INTERFACE.

It allows to:
 * generalize second stage backend initialization instead of adding
   custom APIs to perform it
 * early error detection of backend initialization at -object/
   object-add/object_add time rather than through a proxy DEVICE
   object that tries to use backend.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
Next patch will convert virtio_rng to a new interface as an example.
The same interface will be useful for memory backend.
---
 include/qom/object_interfaces.h |   53 +++++++++++++++++++++++++++++++++++++++
 qmp.c                           |    6 ++++
 qom/Makefile.objs               |    1 +
 qom/object_interfaces.c         |   33 ++++++++++++++++++++++++
 vl.c                            |   14 ++++++++++
 5 files changed, 107 insertions(+), 0 deletions(-)
 create mode 100644 include/qom/object_interfaces.h
 create mode 100644 qom/object_interfaces.c

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
new file mode 100644
index 0000000..8a42d46
--- /dev/null
+++ b/include/qom/object_interfaces.h
@@ -0,0 +1,53 @@
+#ifndef OBJECT_INTERFACES_H
+#define OBJECT_INTERFACES_H
+
+#include "qom/object.h"
+
+#define TYPE_OBJECT_REALIZE_INTERFACE "object-realize-interface"
+
+#define OBJECT_REALIZE_INTERFACE_CLASS(klass) \
+     OBJECT_CLASS_CHECK(ObjectRealizeInterfaceClass, (klass), \
+                        TYPE_OBJECT_REALIZE_INTERFACE)
+#define OBJECT_REALIZE_INTERFACE_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(ObjectRealizeInterfaceClass, (obj), \
+                      TYPE_OBJECT_REALIZE_INTERFACE)
+#define OBJECT_REALIZE_INTERFACE(obj) \
+     INTERFACE_CHECK(ObjectRealizeInterface, (obj), \
+                     TYPE_OBJECT_REALIZE_INTERFACE)
+
+
+typedef struct ObjectRealizeInterface {
+    /* <private> */
+    Object Parent;
+} ObjectRealizeInterface;
+
+/**
+ * ObjectRealizeInterfaceClass:
+ * @parent_class: the base class
+ * @realize: callback to be called after @obj's properties are set.
+ *
+ * Interface is designed to work with -object/object-add/object_add
+ * commands and provides an optional ability to do the second stage
+ * initialization of the object after its properties were set.
+ *
+ * For objects created without using -object/object-add/object_add,
+ * @call_object_realize_interface should be called manually if object's
+ * type implements OBJECT_REALIZE_INTERFACE.
+ */
+typedef struct ObjectRealizeInterfaceClass {
+    /* <private> */
+    InterfaceClass parent_class;
+
+    void (*realize)(ObjectRealizeInterface *obj, Error **errp);
+} ObjectRealizeInterfaceClass;
+
+/**
+ * call_object_realize_interface:
+ * @obj: the object whose realize() method is called
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Wrapper to call realize() method if one of obj's types
+ * implements OBJECT_REALIZE_INTERFACE, otherwise the call does nothing.
+ */
+void call_object_realize_interface(Object *obj, Error **errp);
+#endif
diff --git a/qmp.c b/qmp.c
index a67e0c4..3ed78cd 100644
--- a/qmp.c
+++ b/qmp.c
@@ -27,6 +27,7 @@
 #include "qapi/qmp/qobject.h"
 #include "qapi/qmp-input-visitor.h"
 #include "hw/boards.h"
+#include "qom/object_interfaces.h"
 
 NameInfo *qmp_query_name(Error **errp)
 {
@@ -554,6 +555,11 @@ void object_add(const char *type, const char *id, const QDict *qdict,
         }
     }
 
+    call_object_realize_interface(obj, &local_err);
+    if (local_err) {
+        goto out;
+    }
+
     object_property_add_child(container_get(object_get_root(), "/objects"),
                               id, obj, &local_err);
 out:
diff --git a/qom/Makefile.objs b/qom/Makefile.objs
index 6a93ac7..985003b 100644
--- a/qom/Makefile.objs
+++ b/qom/Makefile.objs
@@ -1,2 +1,3 @@
 common-obj-y = object.o container.o qom-qobject.o
 common-obj-y += cpu.o
+common-obj-y += object_interfaces.o
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
new file mode 100644
index 0000000..d2aa722
--- /dev/null
+++ b/qom/object_interfaces.c
@@ -0,0 +1,33 @@
+#include "qom/object_interfaces.h"
+#include "qemu/module.h"
+
+void call_object_realize_interface(Object *obj, Error **errp)
+{
+    ObjectRealizeInterface *ori;
+    ObjectRealizeInterfaceClass *oric;
+
+    ori = (ObjectRealizeInterface *)
+        object_dynamic_cast(obj, TYPE_OBJECT_REALIZE_INTERFACE);
+    if (!ori) {
+        return;
+    }
+
+    oric = OBJECT_REALIZE_INTERFACE_GET_CLASS(ori);
+    if (oric->realize) {
+        oric->realize(ori, errp);
+    }
+}
+
+
+static void register_types(void)
+{
+    static const TypeInfo realize_interface_info = {
+        .name          = TYPE_OBJECT_REALIZE_INTERFACE,
+        .parent        = TYPE_INTERFACE,
+        .class_size = sizeof(ObjectRealizeInterfaceClass),
+    };
+
+    type_register_static(&realize_interface_info);
+}
+
+type_init(register_types)
diff --git a/vl.c b/vl.c
index 45f9177..1620393 100644
--- a/vl.c
+++ b/vl.c
@@ -170,6 +170,7 @@ int main(int argc, char **argv)
 
 #include "ui/qemu-spice.h"
 #include "qapi/string-input-visitor.h"
+#include "qom/object_interfaces.h"
 
 //#define DEBUG_NET
 //#define DEBUG_SLIRP
@@ -2798,6 +2799,7 @@ static int object_create(QemuOpts *opts, void *opaque)
 {
     const char *type = qemu_opt_get(opts, "qom-type");
     const char *id = qemu_opts_id(opts);
+    Error *local_err = NULL;
     Object *obj;
 
     g_assert(type != NULL);
@@ -2813,9 +2815,21 @@ static int object_create(QemuOpts *opts, void *opaque)
         return -1;
     }
 
+    call_object_realize_interface(obj, &local_err);
+    if (local_err) {
+        goto out;
+    }
+
     object_property_add_child(container_get(object_get_root(), "/objects"),
                               id, obj, NULL);
+
+out:
     object_unref(obj);
+    if (local_err) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        return -1;
+    }
     return 0;
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH 3/5] virtio_rng: use object_realize interface instead of calling backend API

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 backends/rng.c         |   17 +++++++++++++++--
 hw/virtio/virtio-rng.c |   15 +++++++++------
 include/sysemu/rng.h   |   11 -----------
 3 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/backends/rng.c b/backends/rng.c
index 85cb83f..a7a7b7f 100644
--- a/backends/rng.c
+++ b/backends/rng.c
@@ -12,6 +12,7 @@
 
 #include "sysemu/rng.h"
 #include "qapi/qmp/qerror.h"
+#include "qom/object_interfaces.h"
 
 void rng_backend_request_entropy(RngBackend *s, size_t size,
                                  EntropyReceiveFunc *receive_entropy,
@@ -40,9 +41,9 @@ static bool rng_backend_prop_get_opened(Object *obj, Error **errp)
     return s->opened;
 }
 
-void rng_backend_open(RngBackend *s, Error **errp)
+static void rng_backend_realize(ObjectRealizeInterface *objif, Error **errp)
 {
-    object_property_set_bool(OBJECT(s), true, "opened", errp);
+    object_property_set_bool(OBJECT(objif), true, "opened", errp);
 }
 
 static void rng_backend_prop_set_opened(Object *obj, bool value, Error **errp)
@@ -76,13 +77,25 @@ static void rng_backend_init(Object *obj)
                              NULL);
 }
 
+static void rng_backend_class_init(ObjectClass *oc, void *data)
+{
+    ObjectRealizeInterfaceClass *oric = OBJECT_REALIZE_INTERFACE_CLASS(oc);
+
+    oric->realize = rng_backend_realize;
+}
+
 static const TypeInfo rng_backend_info = {
     .name = TYPE_RNG_BACKEND,
     .parent = TYPE_OBJECT,
     .instance_size = sizeof(RngBackend),
     .instance_init = rng_backend_init,
     .class_size = sizeof(RngBackendClass),
+    .class_init = rng_backend_class_init,
     .abstract = true,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_OBJECT_REALIZE_INTERFACE },
+        { }
+    }
 };
 
 static void register_types(void)
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 755fdee..e06875a 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -15,6 +15,7 @@
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/virtio-rng.h"
 #include "sysemu/rng.h"
+#include "qom/object_interfaces.h"
 
 static bool is_guest_ready(VirtIORNG *vrng)
 {
@@ -148,6 +149,14 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
     if (vrng->conf.rng == NULL) {
         vrng->conf.default_backend = RNG_RANDOM(object_new(TYPE_RNG_RANDOM));
 
+        call_object_realize_interface(OBJECT(vrng->conf.default_backend),
+                                      &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            object_unref(OBJECT(vrng->conf.default_backend));
+            return;
+        }
+
         object_property_add_child(OBJECT(dev),
                                   "default-backend",
                                   OBJECT(vrng->conf.default_backend),
@@ -166,12 +175,6 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    rng_backend_open(vrng->rng, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
     vrng->vq = virtio_add_queue(vdev, 8, handle_input);
 
     assert(vrng->conf.max_bytes <= INT64_MAX);
diff --git a/include/sysemu/rng.h b/include/sysemu/rng.h
index 7637fac..0a27c9b 100644
--- a/include/sysemu/rng.h
+++ b/include/sysemu/rng.h
@@ -79,15 +79,4 @@ void rng_backend_request_entropy(RngBackend *s, size_t size,
  * to stop tracking any request.
  */
 void rng_backend_cancel_requests(RngBackend *s);
-
-/**
- * rng_backend_open:
- * @s: the backend to open
- * @errp: a pointer to return the #Error object if an error occurs.
- *
- * This function will open the backend if it is not already open.  Calling this
- * function on an already opened backend will not result in an error.
- */
-void rng_backend_open(RngBackend *s, Error **errp);
-
 #endif
-- 
1.7.1

[Qemu-devel] [PATCH 4/5] vl.c: -object: handle duplicate 'id' properly

[Igor Mammedov]

object_property_add_child() may fail if 'id' matches already
existing object. Which means incorrect command line,
so instead of silently ignoring error, report it and
go to error path (i.e. terminate QEMU).

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 vl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/vl.c b/vl.c
index 1620393..84cb7a8 100644
--- a/vl.c
+++ b/vl.c
@@ -2821,7 +2821,7 @@ static int object_create(QemuOpts *opts, void *opaque)
     }
 
     object_property_add_child(container_get(object_get_root(), "/objects"),
-                              id, obj, NULL);
+                              id, obj, &local_err);
 
 out:
     object_unref(obj);
-- 
1.7.1

[Qemu-devel] [PATCH 5/5] -object/object-add: use custom default object location if provided

[Igor Mammedov]

Add a optional OBJECT_PATH_INTERFACE, that will allow to set custom
container path if object's type implements this interface.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
it will be used for memdev backend to so that
 -object membackendX,id=memdevY
could create object with path /backends/memdev/memdevY,
grouping similar objects in one place and not clattering "/objects"
container.

it also could be used for iothread backend, which could use -object
instead of custom -iothread option.
---
 include/qom/object_interfaces.h |   43 +++++++++++++++++++++++++++++++++++++++
 qmp.c                           |   14 ++++++++++--
 qom/object_interfaces.c         |   24 +++++++++++++++++++++
 vl.c                            |    5 +++-
 4 files changed, 82 insertions(+), 4 deletions(-)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 8a42d46..9c4b4b3 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -50,4 +50,47 @@ typedef struct ObjectRealizeInterfaceClass {
  * implements OBJECT_REALIZE_INTERFACE, otherwise the call does nothing.
  */
 void call_object_realize_interface(Object *obj, Error **errp);
+
+
+#define TYPE_OBJECT_PATH_INTERFACE "object-path-interface"
+
+#define OBJECT_PATH_INTERFACE_CLASS(klass) \
+     OBJECT_CLASS_CHECK(ObjectPathInterfaceClass, (klass), \
+                        TYPE_OBJECT_PATH_INTERFACE)
+#define OBJECT_PATH_INTERFACE_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(ObjectPathInterfaceClass, (obj), \
+                      TYPE_OBJECT_PATH_INTERFACE)
+#define OBJECT_PATH_INTERFACE(obj) \
+     INTERFACE_CHECK(ObjectPathInterface, (obj), \
+                     TYPE_OBJECT_PATH_INTERFACE)
+
+typedef struct ObjectPathInterface {
+    /* <private> */
+    Object Parent;
+} ObjectPathInterface;
+
+/**
+ * ObjectPathInterfaceClass:
+ * @parent_class: the base class
+ * @default_parent: returns default parent path
+ *
+ * Interface is designed to work with -object/object-add/object_add
+ * commands and provides an optional ability for object to
+ * provide its default location in QOM tree.
+ */
+typedef struct ObjectPathInterfaceClass {
+    /* <private> */
+    InterfaceClass parent_class;
+
+    const char *(*default_parent)(ObjectPathInterface *obj);
+} ObjectPathInterfaceClass;
+
+/**
+ * object_default_parent_path:
+ * @obj: the object default_parent() method is called
+ *
+ * Wrapper to call default_parent() method if type implements
+ * OBJECT_PATH_INTERFACE, returns default parent path.
+ */
+const char *object_default_parent_path(Object *obj);
 #endif
diff --git a/qmp.c b/qmp.c
index 3ed78cd..53ee026 100644
--- a/qmp.c
+++ b/qmp.c
@@ -539,6 +539,7 @@ void object_add(const char *type, const char *id, const QDict *qdict,
     Object *obj;
     const QDictEntry *e;
     Error *local_err = NULL;
+    const char *default_path;
 
     if (!object_class_by_name(type)) {
         error_setg(errp, "invalid class name");
@@ -560,7 +561,9 @@ void object_add(const char *type, const char *id, const QDict *qdict,
         goto out;
     }
 
-    object_property_add_child(container_get(object_get_root(), "/objects"),
+    default_path = object_default_parent_path(obj);
+    default_path = default_path ? default_path : "/objects";
+    object_property_add_child(container_get(object_get_root(), default_path),
                               id, obj, &local_err);
 out:
     if (local_err) {
@@ -603,13 +606,18 @@ out:
 void qmp_object_del(const char *id, Error **errp)
 {
     Object *container;
+    bool ambiguous;
     Object *obj;
 
-    container = container_get(object_get_root(), "/objects");
-    obj = object_resolve_path_component(container, id);
+    obj = object_resolve_path(id, &ambiguous);
     if (!obj) {
         error_setg(errp, "object id not found");
         return;
     }
+    if (ambiguous) {
+        error_setg(errp, "object id is ambiguous, use absolute"
+                         " QOM path to specify the object");
+        return;
+    }
     object_unparent(obj);
 }
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index d2aa722..66eb052 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -18,6 +18,23 @@ void call_object_realize_interface(Object *obj, Error **errp)
     }
 }
 
+const char *object_default_parent_path(Object *obj)
+{
+    ObjectPathInterface *opi;
+    ObjectPathInterfaceClass *opic;
+
+    opi = (ObjectPathInterface *)
+        object_dynamic_cast(obj, TYPE_OBJECT_PATH_INTERFACE);
+    if (!opi) {
+        return NULL;
+    }
+
+    opic = OBJECT_PATH_INTERFACE_GET_CLASS(opi);
+    if (opic->default_parent) {
+        return opic->default_parent(opi);
+    }
+    return NULL;
+}
 
 static void register_types(void)
 {
@@ -27,7 +44,14 @@ static void register_types(void)
         .class_size = sizeof(ObjectRealizeInterfaceClass),
     };
 
+    static const TypeInfo path_interface_info = {
+        .name          = TYPE_OBJECT_PATH_INTERFACE,
+        .parent        = TYPE_INTERFACE,
+        .class_size = sizeof(ObjectPathInterfaceClass),
+    };
+
     type_register_static(&realize_interface_info);
+    type_register_static(&path_interface_info);
 }
 
 type_init(register_types)
diff --git a/vl.c b/vl.c
index 84cb7a8..fc2a2ca 100644
--- a/vl.c
+++ b/vl.c
@@ -2799,6 +2799,7 @@ static int object_create(QemuOpts *opts, void *opaque)
 {
     const char *type = qemu_opt_get(opts, "qom-type");
     const char *id = qemu_opts_id(opts);
+    const char *default_path;
     Error *local_err = NULL;
     Object *obj;
 
@@ -2820,7 +2821,9 @@ static int object_create(QemuOpts *opts, void *opaque)
         goto out;
     }
 
-    object_property_add_child(container_get(object_get_root(), "/objects"),
+    default_path = object_default_parent_path(obj);
+    default_path = default_path ? default_path : "/objects";
+    object_property_add_child(container_get(object_get_root(), default_path),
                               id, obj, &local_err);
 
 out:
-- 
1.7.1

Re: [Qemu-devel] [PATCH 5/5] -object/object-add: use custom default object location if provided

[Stefan Hajnoczi]

On Wed, Jan 08, 2014 at 05:09:42PM +0100, Igor Mammedov wrote:
> @@ -603,13 +606,18 @@ out:
>  void qmp_object_del(const char *id, Error **errp)
>  {
>      Object *container;

This local is now unused, please delete it to prevent a compiler
warning.

Re: [Qemu-devel] [PATCH 5/5] -object/object-add: use custom default object location if provided

[Igor Mammedov]

On Thu, 9 Jan 2014 12:35:12 +0800
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Wed, Jan 08, 2014 at 05:09:42PM +0100, Igor Mammedov wrote:
> > @@ -603,13 +606,18 @@ out:
> >  void qmp_object_del(const char *id, Error **errp)
> >  {
> >      Object *container;
> 
> This local is now unused, please delete it to prevent a compiler
> warning.
> 
sure

-- 
Regards,
  Igor

Re: [Qemu-devel] [RFC 0/5] -object/object-add support custom location and 2nd stage initialization

[Paolo Bonzini]

Il 08/01/2014 17:09, Igor Mammedov ha scritto:
> Adds optional interfaces that objects could implement if
> they need to:
>   1. perform an additional initialization after object properties are set
>   2. be placed not in '/objects' container
> 
> Series depends on 2 PULL requests in flight from Andreas & Luiz
> with fixes for QOM interfaces and object-add monitor/QMP command.
> Git tree for testing:
>   https://github.com/imammedo/qemu/commits/extend-object-add
> 
> Igor Mammedov (5):
>   object_add: consolidate error handling
>   add optional 2nd stage initialization to
>     -object/object-add/object_add commands
>   virtio_rng: use object_realize interface instead of calling backend
>     API
>   vl.c: -object: handle duplicate 'id' properly
>   -object/object-add: use custom default object location if provided
> 
>  backends/rng.c                  |   17 ++++++-
>  hw/virtio/virtio-rng.c          |   15 ++++---
>  include/qom/object_interfaces.h |   96 +++++++++++++++++++++++++++++++++++++++
>  include/sysemu/rng.h            |   11 -----
>  qmp.c                           |   30 +++++++++---
>  qom/Makefile.objs               |    1 +
>  qom/object_interfaces.c         |   57 +++++++++++++++++++++++
>  vl.c                            |   21 ++++++++-
>  8 files changed, 220 insertions(+), 28 deletions(-)
>  create mode 100644 include/qom/object_interfaces.h
>  create mode 100644 qom/object_interfaces.c
> 

Thanks Igor!  I like very much patches 1-4 (though I'm thinking that we
need some style conventions for interfaces).  I think patch 5 adds more
complexity than we need, but I'm open to discussion.

Paolo

Re: [Qemu-devel] [RFC 0/5] -object/object-add support custom location and 2nd stage initialization

[Andreas Färber]

Am 08.01.2014 17:24, schrieb Paolo Bonzini:
> Il 08/01/2014 17:09, Igor Mammedov ha scritto:
>> Adds optional interfaces that objects could implement if
>> they need to:
>>   1. perform an additional initialization after object properties are set
>>   2. be placed not in '/objects' container
>>
>> Series depends on 2 PULL requests in flight from Andreas & Luiz
>> with fixes for QOM interfaces and object-add monitor/QMP command.
>> Git tree for testing:
>>   https://github.com/imammedo/qemu/commits/extend-object-add
>>
>> Igor Mammedov (5):
>>   object_add: consolidate error handling
>>   add optional 2nd stage initialization to
>>     -object/object-add/object_add commands
>>   virtio_rng: use object_realize interface instead of calling backend
>>     API
>>   vl.c: -object: handle duplicate 'id' properly
>>   -object/object-add: use custom default object location if provided
>>
>>  backends/rng.c                  |   17 ++++++-
>>  hw/virtio/virtio-rng.c          |   15 ++++---
>>  include/qom/object_interfaces.h |   96 +++++++++++++++++++++++++++++++++++++++
>>  include/sysemu/rng.h            |   11 -----
>>  qmp.c                           |   30 +++++++++---
>>  qom/Makefile.objs               |    1 +
>>  qom/object_interfaces.c         |   57 +++++++++++++++++++++++
>>  vl.c                            |   21 ++++++++-
>>  8 files changed, 220 insertions(+), 28 deletions(-)
>>  create mode 100644 include/qom/object_interfaces.h
>>  create mode 100644 qom/object_interfaces.c
>>
> 
> Thanks Igor!  I like very much patches 1-4 (though I'm thinking that we
> need some style conventions for interfaces).  I think patch 5 adds more
> complexity than we need, but I'm open to discussion.

Hm, I have doubts about the use of "realize" here. So far that is only
implemented for devices, patches for bus still pending my review, and
for those we don't want that  to be handled by -object or object-add but
recursive realization as part of machine initialization, allowing
interaction via qom-set before. It that's different for backends, can we
maybe pick a name different from "realize"?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [RFC 0/5] -object/object-add support custom location and 2nd stage initialization

[Igor Mammedov]

On Wed, 08 Jan 2014 17:45:25 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 08.01.2014 17:24, schrieb Paolo Bonzini:
> > Il 08/01/2014 17:09, Igor Mammedov ha scritto:
> >> Adds optional interfaces that objects could implement if
> >> they need to:
> >>   1. perform an additional initialization after object properties are set
> >>   2. be placed not in '/objects' container
> >>
> >> Series depends on 2 PULL requests in flight from Andreas & Luiz
> >> with fixes for QOM interfaces and object-add monitor/QMP command.
> >> Git tree for testing:
> >>   https://github.com/imammedo/qemu/commits/extend-object-add
> >>
> >> Igor Mammedov (5):
> >>   object_add: consolidate error handling
> >>   add optional 2nd stage initialization to
> >>     -object/object-add/object_add commands
> >>   virtio_rng: use object_realize interface instead of calling backend
> >>     API
> >>   vl.c: -object: handle duplicate 'id' properly
> >>   -object/object-add: use custom default object location if provided
> >>
> >>  backends/rng.c                  |   17 ++++++-
> >>  hw/virtio/virtio-rng.c          |   15 ++++---
> >>  include/qom/object_interfaces.h |   96 +++++++++++++++++++++++++++++++++++++++
> >>  include/sysemu/rng.h            |   11 -----
> >>  qmp.c                           |   30 +++++++++---
> >>  qom/Makefile.objs               |    1 +
> >>  qom/object_interfaces.c         |   57 +++++++++++++++++++++++
> >>  vl.c                            |   21 ++++++++-
> >>  8 files changed, 220 insertions(+), 28 deletions(-)
> >>  create mode 100644 include/qom/object_interfaces.h
> >>  create mode 100644 qom/object_interfaces.c
> >>
> > 
> > Thanks Igor!  I like very much patches 1-4 (though I'm thinking that we
> > need some style conventions for interfaces).  I think patch 5 adds more
> > complexity than we need, but I'm open to discussion.
> 
> Hm, I have doubts about the use of "realize" here. So far that is only
> implemented for devices, patches for bus still pending my review, and
> for those we don't want that  to be handled by -object or object-add but
> recursive realization as part of machine initialization, allowing
> interaction via qom-set before. It that's different for backends, can we
> maybe pick a name different from "realize"?
Any suggestions?

> 
> Andreas
> 

Re: [Qemu-devel] [RFC 0/5] -object/object-add support custom location and 2nd stage initialization

[Igor Mammedov]

On Wed, 08 Jan 2014 17:24:31 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 08/01/2014 17:09, Igor Mammedov ha scritto:
> > Adds optional interfaces that objects could implement if
> > they need to:
> >   1. perform an additional initialization after object properties are set
> >   2. be placed not in '/objects' container
> > 
> > Series depends on 2 PULL requests in flight from Andreas & Luiz
> > with fixes for QOM interfaces and object-add monitor/QMP command.
> > Git tree for testing:
> >   https://github.com/imammedo/qemu/commits/extend-object-add
> > 
> > Igor Mammedov (5):
> >   object_add: consolidate error handling
> >   add optional 2nd stage initialization to
> >     -object/object-add/object_add commands
> >   virtio_rng: use object_realize interface instead of calling backend
> >     API
> >   vl.c: -object: handle duplicate 'id' properly
> >   -object/object-add: use custom default object location if provided
> > 
> >  backends/rng.c                  |   17 ++++++-
> >  hw/virtio/virtio-rng.c          |   15 ++++---
> >  include/qom/object_interfaces.h |   96 +++++++++++++++++++++++++++++++++++++++
> >  include/sysemu/rng.h            |   11 -----
> >  qmp.c                           |   30 +++++++++---
> >  qom/Makefile.objs               |    1 +
> >  qom/object_interfaces.c         |   57 +++++++++++++++++++++++
> >  vl.c                            |   21 ++++++++-
> >  8 files changed, 220 insertions(+), 28 deletions(-)
> >  create mode 100644 include/qom/object_interfaces.h
> >  create mode 100644 qom/object_interfaces.c
> > 
> 
> Thanks Igor!  I like very much patches 1-4 (though I'm thinking that we
> need some style conventions for interfaces).  I think patch 5 adds more
> complexity than we need, but I'm open to discussion.
I'm sorry that it took so long.
The reason for separate interfaces is that realize interface is more generic
and might be used outside of '-object'. While I don't see 'path' interface
ever used outside of -object.

Anyway any suggestions are welcome.

> 
> Paolo

Re: [Qemu-devel] [RFC 0/5] -object/object-add support custom location and 2nd stage initialization

[Paolo Bonzini]

Il 08/01/2014 17:51, Igor Mammedov ha scritto:
>> > 
>> > Thanks Igor!  I like very much patches 1-4 (though I'm thinking that we
>> > need some style conventions for interfaces).  I think patch 5 adds more
>> > complexity than we need, but I'm open to discussion.
> I'm sorry that it took so long.
> The reason for separate interfaces is that realize interface is more generic
> and might be used outside of '-object'. While I don't see 'path' interface
> ever used outside of -object.

Yeah, I think the two interfaces are a good idea.  The question is
whether we want the second interface at all.  I think it's fine to
delegate namespace conventions to management.

Regarding the "overloading" of the realize name, I was against it in
previous discussion and I still am (I was in favor of something like
UserCreatable and naming the method "complete" or "construct"), but I
didn't want to sound too negative. :)

Paolo

Re: [Qemu-devel] [RFC 0/5] -object/object-add support custom location and 2nd stage initialization

[Igor Mammedov]

On Wed, 08 Jan 2014 18:33:11 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 08/01/2014 17:51, Igor Mammedov ha scritto:
> >> > 
> >> > Thanks Igor!  I like very much patches 1-4 (though I'm thinking that we
> >> > need some style conventions for interfaces).  I think patch 5 adds more
> >> > complexity than we need, but I'm open to discussion.
> > I'm sorry that it took so long.
> > The reason for separate interfaces is that realize interface is more generic
> > and might be used outside of '-object'. While I don't see 'path' interface
> > ever used outside of -object.
> 
> Yeah, I think the two interfaces are a good idea.  The question is
> whether we want the second interface at all.  I think it's fine to
> delegate namespace conventions to management.
with dropping it, backends for sure can work without it, they will be just
placed directly under "/objects". For memdev backend it might be upto 256
objects, clattering "/objects" container.
Stefan had the similar idea about grouping iothread objects inside
"/backends/iothreads".


> 
> Regarding the "overloading" of the realize name, I was against it in
> previous discussion and I still am (I was in favor of something like
> UserCreatable and naming the method "complete" or "construct"), but I
> didn't want to sound too negative. :)
issue with naming interface as CommandLine or UserCreatable is that, it could
be used not only by CLI/user but also it could be used internally. For example
see "[PATCH 3/5] virtio_rng: use object_realize interface instead of calling
backend API", where default backend is created by frontend.

how about naming it for what it is: object-2nd-stage-init or neutral
object-complementary-interface that could be extended later with
another methods (we could event squash path interface in it in the last case)

> Paolo


-- 
Regards,
  Igor

Re: [Qemu-devel] [RFC 0/5] -object/object-add support custom location and 2nd stage initialization

[Paolo Bonzini]

Il 10/01/2014 12:28, Igor Mammedov ha scritto:
>> > Regarding the "overloading" of the realize name, I was against it in
>> > previous discussion and I still am (I was in favor of something like
>> > UserCreatable and naming the method "complete" or "construct"), but I
>> > didn't want to sound too negative. :)
> issue with naming interface as CommandLine or UserCreatable is that, it could
> be used not only by CLI/user but also it could be used internally. For example
> see "[PATCH 3/5] virtio_rng: use object_realize interface instead of calling
> backend API", where default backend is created by frontend.

I see.  Yes, with something like UserCreatable, you would not have that
patch.  Instead, UserCreatable's complete method would redirect to the
backend-specific API.

BTW, note that UserCreatable's complete method should take a
UserCreatable (or whatever the name is) as the first parameter, not an
Object.  This would affect that patch, too.

> how about naming it for what it is: object-2nd-stage-init

That would also work for me (TwoStageConstructable or something like that).

One advantage of UserCreatable is that -object/object_add could check
for it and reject creation of objects that are not meant for
command-line instantiation.  You could do the same for
TwoStageConstructable, but it would look weird to define an object as
two-stage constructable with an empty complete method.

With a name like UserCreatable, instead, it would be quite natural to do
this:

void user_creatable_complete(UserCreatable *uc, Error **errp)
{
    UserCreatableClass *ucc = USER_CREATABLE_GET_CLASS(uc);
    if (ucc->complete) {
        ucc->complete(uc, errp);
    }
}

> neutral object-complementary-interface that could be extended later with
> another methods

No, we don't want a hodge-podge interface that's basically UserCreatable
except in the name. :)

Paolo

Re: [Qemu-devel] [RFC 0/5] -object/object-add support custom location and 2nd stage initialization

[Igor Mammedov]

On Fri, 10 Jan 2014 12:38:57 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 10/01/2014 12:28, Igor Mammedov ha scritto:
> >> > Regarding the "overloading" of the realize name, I was against it in
> >> > previous discussion and I still am (I was in favor of something like
> >> > UserCreatable and naming the method "complete" or "construct"), but I
> >> > didn't want to sound too negative. :)
> > issue with naming interface as CommandLine or UserCreatable is that, it could
> > be used not only by CLI/user but also it could be used internally. For example
> > see "[PATCH 3/5] virtio_rng: use object_realize interface instead of calling
> > backend API", where default backend is created by frontend.
> 
> I see.  Yes, with something like UserCreatable, you would not have that
I'm not sure why I wouldn't have that path. It does exactly what you've
just written vvv,

> patch.  Instead, UserCreatable's complete method would redirect to the
> backend-specific API.
i.e. it calls  cast<UserCreatable>(default_rng).complete() which
redirects to backend specific API, where UserCreatable.complete()
is rng_backend_realize()

> 
> BTW, note that UserCreatable's complete method should take a
> UserCreatable (or whatever the name is) as the first parameter, not an
> Object.  This would affect that patch, too.
It does, 'void (*realize)(ObjectRealizeInterface *obj, Error **errp);'

call_object_realize_interface(Object *obj,...) is a wrapper
that reduces casting code duplication at call sites since it's used
at more then 1 place.

> 
> > how about naming it for what it is: object-2nd-stage-init
> 
> That would also work for me (TwoStageConstructable or something like that).
> 
> One advantage of UserCreatable is that -object/object_add could check
> for it and reject creation of objects that are not meant for
> command-line instantiation.  You could do the same for
> TwoStageConstructable, but it would look weird to define an object as
> two-stage constructable with an empty complete method.
> 
> With a name like UserCreatable, instead, it would be quite natural to do
> this:
> 
> void user_creatable_complete(UserCreatable *uc, Error **errp)
> {
>     UserCreatableClass *ucc = USER_CREATABLE_GET_CLASS(uc);
>     if (ucc->complete) {
>         ucc->complete(uc, errp);
>     }
> }
> 
> > neutral object-complementary-interface that could be extended later with
> > another methods
> 
> No, we don't want a hodge-podge interface that's basically UserCreatable
> except in the name. :)
I'm fine with UserCreatable, lets wait couple days if there is no objection
or another suggestions and I'll then respin series.

> 
> Paolo

Re: [Qemu-devel] [RFC 0/5] -object/object-add support custom location and 2nd stage initialization

[Paolo Bonzini]

Il 10/01/2014 15:44, Igor Mammedov ha scritto:
> On Fri, 10 Jan 2014 12:38:57 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Il 10/01/2014 12:28, Igor Mammedov ha scritto:
>>>>> Regarding the "overloading" of the realize name, I was against it in
>>>>> previous discussion and I still am (I was in favor of something like
>>>>> UserCreatable and naming the method "complete" or "construct"), but I
>>>>> didn't want to sound too negative. :)
>>> issue with naming interface as CommandLine or UserCreatable is that, it could
>>> be used not only by CLI/user but also it could be used internally. For example
>>> see "[PATCH 3/5] virtio_rng: use object_realize interface instead of calling
>>> backend API", where default backend is created by frontend.
>>
>> I see.  Yes, with something like UserCreatable, you would not have that
> I'm not sure why I wouldn't have that path. It does exactly what you've
> just written vvv,

You're right, I misremembered.

>> patch.  Instead, UserCreatable's complete method would redirect to the
>> backend-specific API.
> i.e. it calls  cast<UserCreatable>(default_rng).complete() which
> redirects to backend specific API, where UserCreatable.complete()
> is rng_backend_realize()
> 
>>
>> BTW, note that UserCreatable's complete method should take a
>> UserCreatable (or whatever the name is) as the first parameter, not an
>> Object.  This would affect that patch, too.
> It does, 'void (*realize)(ObjectRealizeInterface *obj, Error **errp);'
> 
> call_object_realize_interface(Object *obj,...) is a wrapper
> that reduces casting code duplication at call sites since it's used
> at more then 1 place.

This is needed only because you allow creating objects that do not have
ObjectRealizeInterface.  With a UserCreatable interface, the dynamic
cast would be in vl.c and qmp.c (to check whether the user is actually
allowed to use -object on that device) and there's no duplication at the
call sites of the 2nd-stage init method.

> I'm fine with UserCreatable, lets wait couple days if there is no objection
> or another suggestions and I'll then respin series.

Thanks!

Paolo

Re: [Qemu-devel] [RFC 0/5] -object/object-add support custom location and 2nd stage initialization

[Igor Mammedov]

On Fri, 10 Jan 2014 12:28:10 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> On Wed, 08 Jan 2014 18:33:11 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > Il 08/01/2014 17:51, Igor Mammedov ha scritto:
[...]
> > > The reason for separate interfaces is that realize interface is more generic
> > > and might be used outside of '-object'. While I don't see 'path' interface
> > > ever used outside of -object.
> > 
> > Yeah, I think the two interfaces are a good idea.  The question is
> > whether we want the second interface at all.  I think it's fine to
> > delegate namespace conventions to management.
> with dropping it, backends for sure can work without it, they will be just
> placed directly under "/objects". For memdev backend it might be upto 256
> objects, clattering "/objects" container.
> Stefan had the similar idea about grouping iothread objects inside
> "/backends/iothreads".
so what do we do with PATH interface?
 1. drop it and put all backends into '/objects' container
 2. keep it, allowing QEMU set default namespace
 3. as discussed in "autogenerate ID" thread let management to pass QOM path
    as ID and in case it starts with '/' use provided QOM path and default
    to '/object' for not path ID.

> > Paolo
> 
>