[Qemu-devel] [PATCH] Revert "virtio_rng: replace custom backend API with UserCreatable.complete() callback"

[Qemu-devel] [PATCH] Revert "virtio_rng: replace custom backend API with UserCreatable.complete() callback"

[Amos Kong]

This reverts commit 57d3e1b3f52d07d215ed96df946ee01f8d9f9526.

The commit introduced a regression bug, the initialization order of virtio-rng
backend was changed.

 # x86_64-softmmu/qemu-system-x86_64 -monitor stdio -vnc :0 \
     -chardev socket,host=localhost,port=1024,id=chr0 \
     -object rng-egd,chardev=chr0,id=rng0

 qemu-system-x86_64: -object rng-egd,chardev=chr0,id=rng0: Device 'chr0' not found

Chardev 'chr0' isn't initialized when we try to open rng backend,

Signed-off-by: Amos Kong <akong@redhat.com>

Conflicts:
	hw/virtio/virtio-rng.c
---
 backends/rng.c         | 12 ++----------
 hw/virtio/virtio-rng.c | 15 ++++++---------
 include/sysemu/rng.h   | 11 +++++++++++
 3 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/backends/rng.c b/backends/rng.c
index 0f2fc11..26faf44 100644
--- a/backends/rng.c
+++ b/backends/rng.c
@@ -41,9 +41,9 @@ static bool rng_backend_prop_get_opened(Object *obj, Error **errp)
     return s->opened;
 }
 
-static void rng_backend_complete(UserCreatable *uc, Error **errp)
+void rng_backend_open(RngBackend *s, Error **errp)
 {
-    object_property_set_bool(OBJECT(uc), true, "opened", errp);
+    object_property_set_bool(OBJECT(s), true, "opened", errp);
 }
 
 static void rng_backend_prop_set_opened(Object *obj, bool value, Error **errp)
@@ -80,20 +80,12 @@ static void rng_backend_init(Object *obj)
                              NULL);
 }
 
-static void rng_backend_class_init(ObjectClass *oc, void *data)
-{
-    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
-
-    ucc->complete = rng_backend_complete;
-}
-
 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_USER_CREATABLE },
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 03fd04a..9323cbe 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -15,7 +15,6 @@
 #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)
 {
@@ -157,14 +156,6 @@ 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));
 
-        user_creatable_complete(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),
@@ -186,6 +177,12 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
 
     virtio_init(vdev, "virtio-rng", VIRTIO_ID_RNG, 0);
 
+    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);
     vrng->quota_remaining = vrng->conf.max_bytes;
 
diff --git a/include/sysemu/rng.h b/include/sysemu/rng.h
index 0a27c9b..7637fac 100644
--- a/include/sysemu/rng.h
+++ b/include/sysemu/rng.h
@@ -79,4 +79,15 @@ 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.9.3

Re: [Qemu-devel] [PATCH] Revert "virtio_rng: replace custom backend API with UserCreatable.complete() callback"

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

On Sat, Aug 16, 2014 at 12:23:52AM +0800, Amos Kong wrote:
> This reverts commit 57d3e1b3f52d07d215ed96df946ee01f8d9f9526.
> 
> The commit introduced a regression bug, the initialization order of virtio-rng
> backend was changed.
> 
>  # x86_64-softmmu/qemu-system-x86_64 -monitor stdio -vnc :0 \
>      -chardev socket,host=localhost,port=1024,id=chr0 \
>      -object rng-egd,chardev=chr0,id=rng0
> 
>  qemu-system-x86_64: -object rng-egd,chardev=chr0,id=rng0: Device 'chr0' not found
> 
> Chardev 'chr0' isn't initialized when we try to open rng backend,

More detail:
The problem is that vl.c:main() calls object_create() on -object before
-chardev options are processed.  Moving the object_create() call after
chardev is arbitrary and does not work if a chardev depends on an
-object.

It would have been nice to process command-line options in left-to-right
order instead of grouping them by option type.  I doubt we can change
this now since it would break the command-line.

> Conflicts:
> 	hw/virtio/virtio-rng.c

This shouldn't be here.

I agree with reverting commit 57d3e1b3f52d07d215ed96df946ee01f8d9f9526.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] Revert "virtio_rng: replace custom backend API with UserCreatable.complete() callback"

[Amos Kong]

[-- Attachment #1: Type: text/plain, Size: 1471 bytes --]

On Tue, Aug 19, 2014 at 03:47:20PM +0100, Stefan Hajnoczi wrote:
> On Sat, Aug 16, 2014 at 12:23:52AM +0800, Amos Kong wrote:
> > This reverts commit 57d3e1b3f52d07d215ed96df946ee01f8d9f9526.
> > 
> > The commit introduced a regression bug, the initialization order of virtio-rng
> > backend was changed.
> > 
> >  # x86_64-softmmu/qemu-system-x86_64 -monitor stdio -vnc :0 \
> >      -chardev socket,host=localhost,port=1024,id=chr0 \
> >      -object rng-egd,chardev=chr0,id=rng0


Paolo posted a new patch [1] to fix this bug, above commandline works.

> >  qemu-system-x86_64: -object rng-egd,chardev=chr0,id=rng0: Device 'chr0' not found
> > 
> > Chardev 'chr0' isn't initialized when we try to open rng backend,
> 
> More detail:
> The problem is that vl.c:main() calls object_create() on -object before
> -chardev options are processed.  Moving the object_create() call after
> chardev is arbitrary and does not work if a chardev depends on an
> -object.

Stefan, can you give an example of the complex relying? So Paolo's patch can't
solve this problem?
 
> It would have been nice to process command-line options in left-to-right
> order instead of grouping them by option type.  I doubt we can change
> this now since it would break the command-line.
> 
> > Conflicts:
> > 	hw/virtio/virtio-rng.c
> 
> This shouldn't be here.
> 
> I agree with reverting commit 57d3e1b3f52d07d215ed96df946ee01f8d9f9526.



-- 
			Amos.

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] Revert "virtio_rng: replace custom backend API with UserCreatable.complete() callback"

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Sat, Aug 16, 2014 at 12:23:52AM +0800, Amos Kong wrote:
>> This reverts commit 57d3e1b3f52d07d215ed96df946ee01f8d9f9526.
>> 
>> The commit introduced a regression bug, the initialization order of virtio-rng
>> backend was changed.
>> 
>>  # x86_64-softmmu/qemu-system-x86_64 -monitor stdio -vnc :0 \
>>      -chardev socket,host=localhost,port=1024,id=chr0 \
>>      -object rng-egd,chardev=chr0,id=rng0
>> 
>>  qemu-system-x86_64: -object rng-egd,chardev=chr0,id=rng0: Device 'chr0' not found
>> 
>> Chardev 'chr0' isn't initialized when we try to open rng backend,
>
> More detail:
> The problem is that vl.c:main() calls object_create() on -object before
> -chardev options are processed.  Moving the object_create() call after
> chardev is arbitrary and does not work if a chardev depends on an
> -object.
>
> It would have been nice to process command-line options in left-to-right
> order instead of grouping them by option type.  I doubt we can change
> this now since it would break the command-line.

In my private opinion, our command line could really use a thorough
breaking.

[...]