[Qemu-devel] [PATCH v2 0/4] rng-random: implement request queue

[Qemu-devel] [PATCH v2 0/4] rng-random: implement request queue

[Ladi Prosek]

As suggested by Paolo, I have moved the RngRequest implementation
up to the RngBackend parent class and made both child classes use
it. Apart from the refactoring, the only functional change
compared to v1 is the use of heap instead of stack allocation for
the read buffer in rng-random.

The parent class takes care of creating new requests and adding
them to the queue, as well as removing them from the queue and
deleting them. Child classes have access to the raw GSList * to 
do whatever else they need to do (walk the queue, peek the head
of the queue, ..)

[PATCH v2 1/4] rng: remove the unused request cancellation code
[PATCH v2 2/4] rng: move request queue from RngEgd to RngBackend
[PATCH v2 3/4] rng: move request queue cleanup from RngEgd to
[PATCH v2 4/4] rng: add request queue support to rng-random

[Qemu-devel] [PATCH v2 1/4] rng: remove the unused request cancellation code

[Ladi Prosek]

rng_backend_cancel_requests has no callers and none of the code
deleted in this commit ever runs.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 backends/rng-egd.c   | 12 ------------
 backends/rng.c       |  9 ---------
 include/sysemu/rng.h | 11 -----------
 3 files changed, 32 deletions(-)

diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index 2de5cd5..0b2976a 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -125,17 +125,6 @@ static void rng_egd_free_requests(RngEgd *s)
     s->requests = NULL;
 }
 
-static void rng_egd_cancel_requests(RngBackend *b)
-{
-    RngEgd *s = RNG_EGD(b);
-
-    /* We simply delete the list of pending requests.  If there is data in the 
-     * queue waiting to be read, this is okay, because there will always be
-     * more data than we requested originally
-     */
-    rng_egd_free_requests(s);
-}
-
 static void rng_egd_opened(RngBackend *b, Error **errp)
 {
     RngEgd *s = RNG_EGD(b);
@@ -213,7 +202,6 @@ static void rng_egd_class_init(ObjectClass *klass, void *data)
     RngBackendClass *rbc = RNG_BACKEND_CLASS(klass);
 
     rbc->request_entropy = rng_egd_request_entropy;
-    rbc->cancel_requests = rng_egd_cancel_requests;
     rbc->opened = rng_egd_opened;
 }
 
diff --git a/backends/rng.c b/backends/rng.c
index b7820ef..2f2f3ee 100644
--- a/backends/rng.c
+++ b/backends/rng.c
@@ -26,15 +26,6 @@ void rng_backend_request_entropy(RngBackend *s, size_t size,
     }
 }
 
-void rng_backend_cancel_requests(RngBackend *s)
-{
-    RngBackendClass *k = RNG_BACKEND_GET_CLASS(s);
-
-    if (k->cancel_requests) {
-        k->cancel_requests(s);
-    }
-}
-
 static bool rng_backend_prop_get_opened(Object *obj, Error **errp)
 {
     RngBackend *s = RNG_BACKEND(obj);
diff --git a/include/sysemu/rng.h b/include/sysemu/rng.h
index 0a27c9b..c7da17d 100644
--- a/include/sysemu/rng.h
+++ b/include/sysemu/rng.h
@@ -38,7 +38,6 @@ struct RngBackendClass
 
     void (*request_entropy)(RngBackend *s, size_t size,
                             EntropyReceiveFunc *receive_entropy, void *opaque);
-    void (*cancel_requests)(RngBackend *s);
 
     void (*opened)(RngBackend *s, Error **errp);
 };
@@ -69,14 +68,4 @@ struct RngBackend
 void rng_backend_request_entropy(RngBackend *s, size_t size,
                                  EntropyReceiveFunc *receive_entropy,
                                  void *opaque);
-
-/**
- * rng_backend_cancel_requests:
- * @s: the backend to cancel all pending requests in
- *
- * Cancels all pending requests submitted by @rng_backend_request_entropy.  This
- * should be used by a device during reset or in preparation for live migration
- * to stop tracking any request.
- */
-void rng_backend_cancel_requests(RngBackend *s);
 #endif
-- 
2.5.0

[Qemu-devel] [PATCH v2 2/4] rng: move request queue from RngEgd to RngBackend

[Ladi Prosek]

The 'requests' field now lives in the RngBackend parent class.
There are no functional changes in this commit.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 backends/rng-egd.c   | 28 +++++++++-------------------
 include/sysemu/rng.h | 11 +++++++++++
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index 0b2976a..b061362 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -25,19 +25,8 @@ typedef struct RngEgd
 
     CharDriverState *chr;
     char *chr_name;
-
-    GSList *requests;
 } RngEgd;
 
-typedef struct RngRequest
-{
-    EntropyReceiveFunc *receive_entropy;
-    uint8_t *data;
-    void *opaque;
-    size_t offset;
-    size_t size;
-} RngRequest;
-
 static void rng_egd_request_entropy(RngBackend *b, size_t size,
                                     EntropyReceiveFunc *receive_entropy,
                                     void *opaque)
@@ -66,7 +55,7 @@ static void rng_egd_request_entropy(RngBackend *b, size_t size,
         size -= len;
     }
 
-    s->requests = g_slist_append(s->requests, req);
+    s->parent.requests = g_slist_append(s->parent.requests, req);
 }
 
 static void rng_egd_free_request(RngRequest *req)
@@ -81,7 +70,7 @@ static int rng_egd_chr_can_read(void *opaque)
     GSList *i;
     int size = 0;
 
-    for (i = s->requests; i; i = i->next) {
+    for (i = s->parent.requests; i; i = i->next) {
         RngRequest *req = i->data;
         size += req->size - req->offset;
     }
@@ -94,8 +83,8 @@ static void rng_egd_chr_read(void *opaque, const uint8_t *buf, int size)
     RngEgd *s = RNG_EGD(opaque);
     size_t buf_offset = 0;
 
-    while (size > 0 && s->requests) {
-        RngRequest *req = s->requests->data;
+    while (size > 0 && s->parent.requests) {
+        RngRequest *req = s->parent.requests->data;
         int len = MIN(size, req->size - req->offset);
 
         memcpy(req->data + req->offset, buf + buf_offset, len);
@@ -104,7 +93,8 @@ static void rng_egd_chr_read(void *opaque, const uint8_t *buf, int size)
         size -= len;
 
         if (req->offset == req->size) {
-            s->requests = g_slist_remove_link(s->requests, s->requests);
+            s->parent.requests = g_slist_remove_link(s->parent.requests,
+                                                     s->parent.requests);
 
             req->receive_entropy(req->opaque, req->data, req->size);
 
@@ -117,12 +107,12 @@ static void rng_egd_free_requests(RngEgd *s)
 {
     GSList *i;
 
-    for (i = s->requests; i; i = i->next) {
+    for (i = s->parent.requests; i; i = i->next) {
         rng_egd_free_request(i->data);
     }
 
-    g_slist_free(s->requests);
-    s->requests = NULL;
+    g_slist_free(s->parent.requests);
+    s->parent.requests = NULL;
 }
 
 static void rng_egd_opened(RngBackend *b, Error **errp)
diff --git a/include/sysemu/rng.h b/include/sysemu/rng.h
index c7da17d..084164c 100644
--- a/include/sysemu/rng.h
+++ b/include/sysemu/rng.h
@@ -25,6 +25,7 @@
 #define RNG_BACKEND_CLASS(klass) \
     OBJECT_CLASS_CHECK(RngBackendClass, (klass), TYPE_RNG_BACKEND)
 
+typedef struct RngRequest RngRequest;
 typedef struct RngBackendClass RngBackendClass;
 typedef struct RngBackend RngBackend;
 
@@ -32,6 +33,15 @@ typedef void (EntropyReceiveFunc)(void *opaque,
                                   const void *data,
                                   size_t size);
 
+struct RngRequest
+{
+    EntropyReceiveFunc *receive_entropy;
+    uint8_t *data;
+    void *opaque;
+    size_t offset;
+    size_t size;
+};
+
 struct RngBackendClass
 {
     ObjectClass parent_class;
@@ -48,6 +58,7 @@ struct RngBackend
 
     /*< protected >*/
     bool opened;
+    GSList *requests;
 };
 
 /**
-- 
2.5.0

[Qemu-devel] [PATCH v2 3/4] rng: move request queue cleanup from RngEgd to RngBackend

[Ladi Prosek]

RngBackend is now in charge of cleaning up the linked list on
instance finalization. It also exposes a function to finalize
individual RngRequest instances, called by its child classes.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 backends/rng-egd.c   | 25 +------------------------
 backends/rng.c       | 32 ++++++++++++++++++++++++++++++++
 include/sysemu/rng.h | 12 ++++++++++++
 3 files changed, 45 insertions(+), 24 deletions(-)

diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index b061362..8f2bd16 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -58,12 +58,6 @@ static void rng_egd_request_entropy(RngBackend *b, size_t size,
     s->parent.requests = g_slist_append(s->parent.requests, req);
 }
 
-static void rng_egd_free_request(RngRequest *req)
-{
-    g_free(req->data);
-    g_free(req);
-}
-
 static int rng_egd_chr_can_read(void *opaque)
 {
     RngEgd *s = RNG_EGD(opaque);
@@ -93,28 +87,13 @@ static void rng_egd_chr_read(void *opaque, const uint8_t *buf, int size)
         size -= len;
 
         if (req->offset == req->size) {
-            s->parent.requests = g_slist_remove_link(s->parent.requests,
-                                                     s->parent.requests);
-
             req->receive_entropy(req->opaque, req->data, req->size);
 
-            rng_egd_free_request(req);
+            rng_backend_finalize_request(&s->parent, req);
         }
     }
 }
 
-static void rng_egd_free_requests(RngEgd *s)
-{
-    GSList *i;
-
-    for (i = s->parent.requests; i; i = i->next) {
-        rng_egd_free_request(i->data);
-    }
-
-    g_slist_free(s->parent.requests);
-    s->parent.requests = NULL;
-}
-
 static void rng_egd_opened(RngBackend *b, Error **errp)
 {
     RngEgd *s = RNG_EGD(b);
@@ -183,8 +162,6 @@ static void rng_egd_finalize(Object *obj)
     }
 
     g_free(s->chr_name);
-
-    rng_egd_free_requests(s);
 }
 
 static void rng_egd_class_init(ObjectClass *klass, void *data)
diff --git a/backends/rng.c b/backends/rng.c
index 2f2f3ee..014cb9d 100644
--- a/backends/rng.c
+++ b/backends/rng.c
@@ -64,6 +64,30 @@ static void rng_backend_prop_set_opened(Object *obj, bool value, Error **errp)
     s->opened = true;
 }
 
+static void rng_backend_free_request(RngRequest *req)
+{
+    g_free(req->data);
+    g_free(req);
+}
+
+static void rng_backend_free_requests(RngBackend *s)
+{
+    GSList *i;
+
+    for (i = s->requests; i; i = i->next) {
+        rng_backend_free_request(i->data);
+    }
+
+    g_slist_free(s->requests);
+    s->requests = NULL;
+}
+
+void rng_backend_finalize_request(RngBackend *s, RngRequest *req)
+{
+    s->requests = g_slist_remove(s->requests, req);
+    rng_backend_free_request(req);
+}
+
 static void rng_backend_init(Object *obj)
 {
     object_property_add_bool(obj, "opened",
@@ -72,6 +96,13 @@ static void rng_backend_init(Object *obj)
                              NULL);
 }
 
+static void rng_backend_finalize(Object *obj)
+{
+    RngBackend *s = RNG_BACKEND(obj);
+
+    rng_backend_free_requests(s);
+}
+
 static void rng_backend_class_init(ObjectClass *oc, void *data)
 {
     UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
@@ -84,6 +115,7 @@ static const TypeInfo rng_backend_info = {
     .parent = TYPE_OBJECT,
     .instance_size = sizeof(RngBackend),
     .instance_init = rng_backend_init,
+    .instance_finalize = rng_backend_finalize,
     .class_size = sizeof(RngBackendClass),
     .class_init = rng_backend_class_init,
     .abstract = true,
diff --git a/include/sysemu/rng.h b/include/sysemu/rng.h
index 084164c..c2c9035 100644
--- a/include/sysemu/rng.h
+++ b/include/sysemu/rng.h
@@ -61,6 +61,7 @@ struct RngBackend
     GSList *requests;
 };
 
+
 /**
  * rng_backend_request_entropy:
  * @s: the backend to request entropy from
@@ -79,4 +80,15 @@ struct RngBackend
 void rng_backend_request_entropy(RngBackend *s, size_t size,
                                  EntropyReceiveFunc *receive_entropy,
                                  void *opaque);
+
+/**
+ * rng_backend_free_request:
+ * @s: the backend that created the request
+ * @req: the request to finalize
+ *
+ * Used by child rng backend classes to finalize requests once they've been
+ * processed. The request is removed from the list of active requests and
+ * deleted.
+ */
+void rng_backend_finalize_request(RngBackend *s, RngRequest *req);
 #endif
-- 
2.5.0

[Qemu-devel] [PATCH v2 4/4] rng: add request queue support to rng-random

[Ladi Prosek]

Requests are now created in the RngBackend parent class and the
code path is shared by both rng-egd and rng-random.

This commit fixes the rng-random implementation which currently
processes only one request at a time and simply discards all
but the most recent one. In the guest this manifests as delayed
completion of reads from virtio-rng, i.e. a read is completed
only after another read is issued.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 backends/rng-egd.c    | 16 ++--------------
 backends/rng-random.c | 43 +++++++++++++++++++------------------------
 backends/rng.c        | 13 ++++++++++++-
 include/sysemu/rng.h  |  3 +--
 4 files changed, 34 insertions(+), 41 deletions(-)

diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index 8f2bd16..30332ed 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -27,20 +27,10 @@ typedef struct RngEgd
     char *chr_name;
 } RngEgd;
 
-static void rng_egd_request_entropy(RngBackend *b, size_t size,
-                                    EntropyReceiveFunc *receive_entropy,
-                                    void *opaque)
+static void rng_egd_request_entropy(RngBackend *b, RngRequest *req)
 {
     RngEgd *s = RNG_EGD(b);
-    RngRequest *req;
-
-    req = g_malloc(sizeof(*req));
-
-    req->offset = 0;
-    req->size = size;
-    req->receive_entropy = receive_entropy;
-    req->opaque = opaque;
-    req->data = g_malloc(req->size);
+    size_t size = req->size;
 
     while (size > 0) {
         uint8_t header[2];
@@ -54,8 +44,6 @@ static void rng_egd_request_entropy(RngBackend *b, size_t size,
 
         size -= len;
     }
-
-    s->parent.requests = g_slist_append(s->parent.requests, req);
 }
 
 static int rng_egd_chr_can_read(void *opaque)
diff --git a/backends/rng-random.c b/backends/rng-random.c
index 8cdad6a..a6cb385 100644
--- a/backends/rng-random.c
+++ b/backends/rng-random.c
@@ -22,10 +22,6 @@ struct RndRandom
 
     int fd;
     char *filename;
-
-    EntropyReceiveFunc *receive_func;
-    void *opaque;
-    size_t size;
 };
 
 /**
@@ -38,36 +34,35 @@ struct RndRandom
 static void entropy_available(void *opaque)
 {
     RndRandom *s = RNG_RANDOM(opaque);
-    uint8_t buffer[s->size];
-    ssize_t len;
 
-    len = read(s->fd, buffer, s->size);
-    if (len < 0 && errno == EAGAIN) {
-        return;
+    while (s->parent.requests != NULL) {
+        RngRequest *req = s->parent.requests->data;
+        ssize_t len;
+
+        len = read(s->fd, req->data, req->size);
+        if (len < 0 && errno == EAGAIN) {
+            return;
+        }
+        g_assert(len != -1);
+
+        req->receive_entropy(req->opaque, req->data, len);
+
+        rng_backend_finalize_request(&s->parent, req);
     }
-    g_assert(len != -1);
-
-    s->receive_func(s->opaque, buffer, len);
-    s->receive_func = NULL;
 
+    /* We've drained all requests, the fd handler can be reset. */
     qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
 }
 
-static void rng_random_request_entropy(RngBackend *b, size_t size,
-                                        EntropyReceiveFunc *receive_entropy,
-                                        void *opaque)
+static void rng_random_request_entropy(RngBackend *b, RngRequest *req)
 {
     RndRandom *s = RNG_RANDOM(b);
 
-    if (s->receive_func) {
-        s->receive_func(s->opaque, NULL, 0);
+    if (s->parent.requests == NULL) {
+        /* If there are no pending requests yet, we need to
+         * install our fd handler. */
+        qemu_set_fd_handler(s->fd, entropy_available, NULL, s);
     }
-
-    s->receive_func = receive_entropy;
-    s->opaque = opaque;
-    s->size = size;
-
-    qemu_set_fd_handler(s->fd, entropy_available, NULL, s);
 }
 
 static void rng_random_opened(RngBackend *b, Error **errp)
diff --git a/backends/rng.c b/backends/rng.c
index 014cb9d..277a41b 100644
--- a/backends/rng.c
+++ b/backends/rng.c
@@ -20,9 +20,20 @@ void rng_backend_request_entropy(RngBackend *s, size_t size,
                                  void *opaque)
 {
     RngBackendClass *k = RNG_BACKEND_GET_CLASS(s);
+    RngRequest *req;
 
     if (k->request_entropy) {
-        k->request_entropy(s, size, receive_entropy, opaque);
+        req = g_malloc(sizeof(*req));
+
+        req->offset = 0;
+        req->size = size;
+        req->receive_entropy = receive_entropy;
+        req->opaque = opaque;
+        req->data = g_malloc(req->size);
+
+        k->request_entropy(s, req);
+
+        s->requests = g_slist_append(s->requests, req);
     }
 }
 
diff --git a/include/sysemu/rng.h b/include/sysemu/rng.h
index c2c9035..a7ed580 100644
--- a/include/sysemu/rng.h
+++ b/include/sysemu/rng.h
@@ -46,8 +46,7 @@ struct RngBackendClass
 {
     ObjectClass parent_class;
 
-    void (*request_entropy)(RngBackend *s, size_t size,
-                            EntropyReceiveFunc *receive_entropy, void *opaque);
+    void (*request_entropy)(RngBackend *s, RngRequest *req);
 
     void (*opened)(RngBackend *s, Error **errp);
 };
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 4/4] rng: add request queue support to rng-random

[Paolo Bonzini]

On 10/02/2016 16:53, Ladi Prosek wrote:
> +        req->size = size;
> +        req->receive_entropy = receive_entropy;
> +        req->opaque = opaque;
> +        req->data = g_malloc(req->size);
> +
> +        k->request_entropy(s, req);
> +
> +        s->requests = g_slist_append(s->requests, req);
>      }

g_slist_append has to traverse the entire list to find the place to add
the node.  You probably are better off using QSIMPLEQ (which is an
intrusive list unlike GSList).

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v2 4/4] rng: add request queue support to rng-random

[Ladi Prosek]

> 
> 
> On 10/02/2016 16:53, Ladi Prosek wrote:
> > +        req->size = size;
> > +        req->receive_entropy = receive_entropy;
> > +        req->opaque = opaque;
> > +        req->data = g_malloc(req->size);
> > +
> > +        k->request_entropy(s, req);
> > +
> > +        s->requests = g_slist_append(s->requests, req);
> >      }
> 
> g_slist_append has to traverse the entire list to find the place to add
> the node.  You probably are better off using QSIMPLEQ (which is an
> intrusive list unlike GSList).

This is what rng-egd does today and I would argue that since the expected
length of the list is very small - it's going to be longer than 1 only
very rarely - a simple lightweight data structure is a better choice than
trying to be O(1) in the worst case.

I'll be happy to switch to QSIMPLEQ if you want though. Your call.

Thanks!
Ladi
 
> Thanks,
> 
> Paolo
> 
> 

Re: [Qemu-devel] [PATCH v2 4/4] rng: add request queue support to rng-random

[Paolo Bonzini]

On 10/02/2016 17:40, Ladi Prosek wrote:
>>
>>
>> On 10/02/2016 16:53, Ladi Prosek wrote:
>>> +        req->size = size;
>>> +        req->receive_entropy = receive_entropy;
>>> +        req->opaque = opaque;
>>> +        req->data = g_malloc(req->size);
>>> +
>>> +        k->request_entropy(s, req);
>>> +
>>> +        s->requests = g_slist_append(s->requests, req);
>>>      }
>>
>> g_slist_append has to traverse the entire list to find the place to add
>> the node.  You probably are better off using QSIMPLEQ (which is an
>> intrusive list unlike GSList).
> 
> This is what rng-egd does today and I would argue that since the expected
> length of the list is very small - it's going to be longer than 1 only
> very rarely - a simple lightweight data structure is a better choice than
> trying to be O(1) in the worst case.
> 
> I'll be happy to switch to QSIMPLEQ if you want though. Your call.

Ok, it can be done on top I guess.  I'll let others review the patches
more closely!

Paolo

Re: [Qemu-devel] [PATCH v2 0/4] rng-random: implement request queue

[Pankaj Gupta]

> 
> As suggested by Paolo, I have moved the RngRequest implementation
> up to the RngBackend parent class and made both child classes use
> it. Apart from the refactoring, the only functional change
> compared to v1 is the use of heap instead of stack allocation for
> the read buffer in rng-random.
> 
> The parent class takes care of creating new requests and adding
> them to the queue, as well as removing them from the queue and
> deleting them. Child classes have access to the raw GSList * to
> do whatever else they need to do (walk the queue, peek the head
> of the queue, ..)

Hi Ladi,

Overall code changes looks good to me.
 
> 
> [PATCH v2 1/4] rng: remove the unused request cancellation code
> [PATCH v2 2/4] rng: move request queue from RngEgd to RngBackend
> [PATCH v2 3/4] rng: move request queue cleanup from RngEgd to
> [PATCH v2 4/4] rng: add request queue support to rng-random
> 
> 

Re: [Qemu-devel] [PATCH v2 3/4] rng: move request queue cleanup from RngEgd to RngBackend

[Amit Shah]

On (Wed) 10 Feb 2016 [16:53:24], Ladi Prosek wrote:
> RngBackend is now in charge of cleaning up the linked list on
> instance finalization. It also exposes a function to finalize
> individual RngRequest instances, called by its child classes.
> 
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>

> @@ -183,8 +162,6 @@ static void rng_egd_finalize(Object *obj)
>      }
>  
>      g_free(s->chr_name);
> -
> -    rng_egd_free_requests(s);

Missing call to rng_backend_free_requests()?

> diff --git a/include/sysemu/rng.h b/include/sysemu/rng.h
> index 084164c..c2c9035 100644
> --- a/include/sysemu/rng.h
> +++ b/include/sysemu/rng.h
> @@ -61,6 +61,7 @@ struct RngBackend
>      GSList *requests;
>  };
>  
> +
>  /**

Extra whitespace?


		Amit

Re: [Qemu-devel] [PATCH v2 3/4] rng: move request queue cleanup from RngEgd to RngBackend

[Ladi Prosek]

On Wed, Mar 2, 2016 at 8:15 AM, Amit Shah <amit.shah@redhat.com> wrote:
> On (Wed) 10 Feb 2016 [16:53:24], Ladi Prosek wrote:
>> RngBackend is now in charge of cleaning up the linked list on
>> instance finalization. It also exposes a function to finalize
>> individual RngRequest instances, called by its child classes.
>>
>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>
>> @@ -183,8 +162,6 @@ static void rng_egd_finalize(Object *obj)
>>      }
>>
>>      g_free(s->chr_name);
>> -
>> -    rng_egd_free_requests(s);
>
> Missing call to rng_backend_free_requests()?

Not needed here, this is handled by the parent class. Its
instance_finalize callback calls rng_backend_free_requests.

>> diff --git a/include/sysemu/rng.h b/include/sysemu/rng.h
>> index 084164c..c2c9035 100644
>> --- a/include/sysemu/rng.h
>> +++ b/include/sysemu/rng.h
>> @@ -61,6 +61,7 @@ struct RngBackend
>>      GSList *requests;
>>  };
>>
>> +
>>  /**
>
> Extra whitespace?

This was intentional to separate internal declarations and public
entry points. Same thing exists for example in tpm_backend.h in the
same directory. I can certainly delete the empty line if you'd prefer
that.

Thanks,
Ladi

>
>                 Amit

Re: [Qemu-devel] [PATCH v2 4/4] rng: add request queue support to rng-random

[Amit Shah]

On (Wed) 10 Feb 2016 [16:53:25], Ladi Prosek wrote:
> Requests are now created in the RngBackend parent class and the
> code path is shared by both rng-egd and rng-random.
> 
> This commit fixes the rng-random implementation which currently
> processes only one request at a time and simply discards all
> but the most recent one. In the guest this manifests as delayed
> completion of reads from virtio-rng, i.e. a read is completed
> only after another read is issued.

Nice commit message, but one convention I like is: when you're fixing
something, refer to the bug in past tense, so it's not confusing with
existing behaviour (i.e. since you're already fixing that bug in this
patch, it's better to write in the past tense about this bug).

Also, since you mentioned the move from stack-based allocation to
heap-based in the cover letter, and since this patch actually does
that, please include a one-liner here for it too.

Otherwise, the series is fine.

		Amit

Re: [Qemu-devel] [PATCH v2 3/4] rng: move request queue cleanup from RngEgd to RngBackend

[Amit Shah]

On (Wed) 02 Mar 2016 [09:32:48], Ladi Prosek wrote:
> On Wed, Mar 2, 2016 at 8:15 AM, Amit Shah <amit.shah@redhat.com> wrote:
> > On (Wed) 10 Feb 2016 [16:53:24], Ladi Prosek wrote:
> >> RngBackend is now in charge of cleaning up the linked list on
> >> instance finalization. It also exposes a function to finalize
> >> individual RngRequest instances, called by its child classes.
> >>
> >> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> >
> >> @@ -183,8 +162,6 @@ static void rng_egd_finalize(Object *obj)
> >>      }
> >>
> >>      g_free(s->chr_name);
> >> -
> >> -    rng_egd_free_requests(s);
> >
> > Missing call to rng_backend_free_requests()?
> 
> Not needed here, this is handled by the parent class. Its
> instance_finalize callback calls rng_backend_free_requests.

OK.

> >> diff --git a/include/sysemu/rng.h b/include/sysemu/rng.h
> >> index 084164c..c2c9035 100644
> >> --- a/include/sysemu/rng.h
> >> +++ b/include/sysemu/rng.h
> >> @@ -61,6 +61,7 @@ struct RngBackend
> >>      GSList *requests;
> >>  };
> >>
> >> +
> >>  /**
> >
> > Extra whitespace?
> 
> This was intentional to separate internal declarations and public
> entry points. Same thing exists for example in tpm_backend.h in the
> same directory. I can certainly delete the empty line if you'd prefer
> that.

No, this is fine, and I agree with the separation.

		Amit

Re: [Qemu-devel] [PATCH v2 4/4] rng: add request queue support to rng-random

[Ladi Prosek]

On Wed, Mar 2, 2016 at 10:56 AM, Amit Shah <amit.shah@redhat.com> wrote:
> On (Wed) 10 Feb 2016 [16:53:25], Ladi Prosek wrote:
>> Requests are now created in the RngBackend parent class and the
>> code path is shared by both rng-egd and rng-random.
>>
>> This commit fixes the rng-random implementation which currently
>> processes only one request at a time and simply discards all
>> but the most recent one. In the guest this manifests as delayed
>> completion of reads from virtio-rng, i.e. a read is completed
>> only after another read is issued.
>
> Nice commit message, but one convention I like is: when you're fixing
> something, refer to the bug in past tense, so it's not confusing with
> existing behaviour (i.e. since you're already fixing that bug in this
> patch, it's better to write in the past tense about this bug).
>
> Also, since you mentioned the move from stack-based allocation to
> heap-based in the cover letter, and since this patch actually does
> that, please include a one-liner here for it too.

Thanks for the review. I'll update the commit messages and send v3.

> Otherwise, the series is fine.
>
>                 Amit