[Qemu-devel] [PATCH 0/2] virtio-rng: Fix memory leaks in virtio_rng_device_realize

[Qemu-devel] [PATCH 0/2] virtio-rng: Fix memory leaks in virtio_rng_device_realize

[John Snow]

My previous commit, 713e8a10, did not address the fact that
the error checking pathways do not attempt to call
virtio_cleanup and thus can leak memory in hotplug scenarios.

This patchset shuffles around the error checking so it does not
need to perform said cleanup, and changes the error functions
from error_set to error_setg.

John Snow (2):
  virtio-rng: Move error-checking forward to prevent memory leak
  virtio-rng: replace error_set calls with error_setg

 hw/virtio/virtio-rng.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/2] virtio-rng: Move error-checking forward to prevent memory leak

[John Snow]

This patch pushes the error-checking forward and the virtio
initialization backward in the device realization function
in order to prevent memory leaks for hot plug scenarios.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/virtio/virtio-rng.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 7c5a675..ff58917 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -147,6 +147,14 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    /* Workaround: Property parsing does not enforce unsigned integers,
+     * So this is a hack to reject such numbers. */
+    if (vrng->conf.max_bytes > INT64_MAX) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "max-bytes",
+                  "a non-negative integer below 2^63");
+        return;
+    }
+
     if (vrng->conf.rng == NULL) {
         vrng->conf.default_backend = RNG_RANDOM(object_new(TYPE_RNG_RANDOM));
 
@@ -171,23 +179,15 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
                                  "rng", NULL);
     }
 
-    virtio_init(vdev, "virtio-rng", VIRTIO_ID_RNG, 0);
-
     vrng->rng = vrng->conf.rng;
     if (vrng->rng == NULL) {
         error_set(errp, QERR_INVALID_PARAMETER_VALUE, "rng", "a valid object");
         return;
     }
 
-    vrng->vq = virtio_add_queue(vdev, 8, handle_input);
+    virtio_init(vdev, "virtio-rng", VIRTIO_ID_RNG, 0);
 
-    /* Workaround: Property parsing does not enforce unsigned integers,
-     * So this is a hack to reject such numbers. */
-    if (vrng->conf.max_bytes > INT64_MAX) {
-        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "max-bytes",
-                  "a non-negative integer below 2^63");
-        return;
-    }
+    vrng->vq = virtio_add_queue(vdev, 8, handle_input);
     vrng->quota_remaining = vrng->conf.max_bytes;
 
     vrng->rate_limit_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
-- 
1.9.3

[Qemu-devel] [PATCH 2/2] virtio-rng: replace error_set calls with error_setg

[John Snow]

Under recommendation from Luiz Capitulino, we are changing
the error_set calls to error_setg while we are fixing up
the error handling pathways of virtio-rng.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/virtio/virtio-rng.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index ff58917..03fd04a 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -142,16 +142,15 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
     Error *local_err = NULL;
 
     if (!vrng->conf.period_ms > 0) {
-        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "period",
-                  "a positive number");
+        error_setg(errp, "'period' parameter expects a positive integer");
         return;
     }
 
     /* Workaround: Property parsing does not enforce unsigned integers,
      * So this is a hack to reject such numbers. */
     if (vrng->conf.max_bytes > INT64_MAX) {
-        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "max-bytes",
-                  "a non-negative integer below 2^63");
+        error_setg(errp, "'max-bytes' parameter must be non-negative, "
+                   "and less than 2^63");
         return;
     }
 
@@ -181,7 +180,7 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
 
     vrng->rng = vrng->conf.rng;
     if (vrng->rng == NULL) {
-        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "rng", "a valid object");
+        error_setg(errp, "'rng' parameter expects a valid object");
         return;
     }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH 0/2] virtio-rng: Fix memory leaks in virtio_rng_device_realize

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> My previous commit, 713e8a10, did not address the fact that
> the error checking pathways do not attempt to call
> virtio_cleanup and thus can leak memory in hotplug scenarios.
>
> This patchset shuffles around the error checking so it does not
> need to perform said cleanup, and changes the error functions
> from error_set to error_setg.
>
> John Snow (2):
>   virtio-rng: Move error-checking forward to prevent memory leak
>   virtio-rng: replace error_set calls with error_setg
>
>  hw/virtio/virtio-rng.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 0/2] virtio-rng: Fix memory leaks in virtio_rng_device_realize

[Stefan Hajnoczi]

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

On Tue, Jul 29, 2014 at 07:28:56PM -0400, John Snow wrote:
> My previous commit, 713e8a10, did not address the fact that
> the error checking pathways do not attempt to call
> virtio_cleanup and thus can leak memory in hotplug scenarios.
> 
> This patchset shuffles around the error checking so it does not
> need to perform said cleanup, and changes the error functions
> from error_set to error_setg.
> 
> John Snow (2):
>   virtio-rng: Move error-checking forward to prevent memory leak
>   virtio-rng: replace error_set calls with error_setg
> 
>  hw/virtio/virtio-rng.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Re: [Qemu-devel] [PATCH 0/2] virtio-rng: Fix memory leaks in virtio_rng_device_realize

[Amit Shah]

On (Tue) 29 Jul 2014 [19:28:56], John Snow wrote:
> My previous commit, 713e8a10, did not address the fact that
> the error checking pathways do not attempt to call
> virtio_cleanup and thus can leak memory in hotplug scenarios.
> 
> This patchset shuffles around the error checking so it does not
> need to perform said cleanup, and changes the error functions
> from error_set to error_setg.

Thanks, queued up for 2.2.


		Amit