[Qemu-devel] [PATCH 0/6] qdev and blockdev refcount leak fixes

[Qemu-devel] [PATCH 0/6] qdev and blockdev refcount leak fixes

[Stefan Hajnoczi]

It started with a bug report along these lines:

  (qemu) device_add virtio-blk-pci,drive=drive0,x-data-plane=on
  device is incompatible with x-data-plane, use scsi=off
  Device initialization failed.
  Device initialization failed.
  Device 'virtio-blk-pci' could not be initialized
  (qemu) drive_del drive0
  (qemu) drive_add 0 if=none,id=drive0
  Duplicate ID 'drive0' for drive

The drive_add command should succeed since the old "drive0" was deleted.

With the help of Andreas and Paolo we figured out that the problem is not
virtio-blk or dataplane.  There are actually two problems:

1. qdev_device_add() must release its DeviceState reference if
   qdev_init() failed.

2. blockdev_init() must release its QemuOpts on failure or early return when no
   file= option was specified.

This series fixes these problems and then qtest test cases for both bugs.  In
order to do this we need to add QMP response objects to the libqtest API, which
currently discards QMP responses.

Patches 1 & 2 fix the leaks.
Patches 2 & 3 add QMP response objects to libqtest.
Patches 5 & 6 add qtest test cases for the bugs.

Stefan Hajnoczi (6):
  blockdev: fix drive_init() opts and bs_opts leaks
  qdev: unref qdev when device_add fails
  libqtest: rename qmp() to qmp_discard_response()
  libqtest: add qmp(fmt, ...) -> QDict* function
  blockdev-test: add test case for drive_add duplicate IDs
  qdev-monitor-test: add device_add leak test cases

 blockdev.c                | 51 +++++++++++++++--------------
 qdev-monitor.c            |  4 ++-
 tests/Makefile            |  4 +++
 tests/blockdev-test.c     | 59 ++++++++++++++++++++++++++++++++++
 tests/boot-order-test.c   |  4 +--
 tests/fdc-test.c          | 15 +++++----
 tests/ide-test.c          | 10 +++---
 tests/libqtest.c          | 72 +++++++++++++++++++++++++++++++----------
 tests/libqtest.h          | 51 +++++++++++++++++++++++++----
 tests/qdev-monitor-test.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++
 10 files changed, 290 insertions(+), 61 deletions(-)
 create mode 100644 tests/blockdev-test.c
 create mode 100644 tests/qdev-monitor-test.c

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/6] blockdev: fix drive_init() opts and bs_opts leaks

[Stefan Hajnoczi]

These memory leaks also make drive_add if=none,id=drive0 without a file=
option leak the options list.  This keeps ID "drive0" around forever.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c | 51 +++++++++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 24 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index e70e16e..c053a58 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -360,7 +360,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
     if (error_is_set(&error)) {
         qerror_report_err(error);
         error_free(error);
-        return NULL;
+        goto early_err;
     }
 
     if (id) {
@@ -390,7 +390,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
             ;
         if (type == IF_COUNT) {
             error_report("unsupported bus type '%s'", buf);
-            return NULL;
+            goto early_err;
 	}
     } else {
         type = block_default_type;
@@ -401,15 +401,15 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
     if (cyls || heads || secs) {
         if (cyls < 1) {
             error_report("invalid physical cyls number");
-	    return NULL;
+            goto early_err;
 	}
         if (heads < 1) {
             error_report("invalid physical heads number");
-	    return NULL;
+            goto early_err;
 	}
         if (secs < 1) {
             error_report("invalid physical secs number");
-	    return NULL;
+            goto early_err;
 	}
     }
 
@@ -417,7 +417,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
         if (!cyls) {
             error_report("'%s' trans must be used with cyls, heads and secs",
                          buf);
-            return NULL;
+            goto early_err;
         }
         if (!strcmp(buf, "none"))
             translation = BIOS_ATA_TRANSLATION_NONE;
@@ -427,7 +427,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
             translation = BIOS_ATA_TRANSLATION_AUTO;
 	else {
             error_report("'%s' invalid translation type", buf);
-	    return NULL;
+            goto early_err;
 	}
     }
 
@@ -437,19 +437,19 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
 	} else if (!strcmp(buf, "cdrom")) {
             if (cyls || secs || heads) {
                 error_report("CHS can't be set with media=%s", buf);
-	        return NULL;
+                goto early_err;
             }
 	    media = MEDIA_CDROM;
 	} else {
 	    error_report("'%s' invalid media", buf);
-	    return NULL;
+	    goto early_err;
 	}
     }
 
     if ((buf = qemu_opt_get(opts, "discard")) != NULL) {
         if (bdrv_parse_discard_flags(buf, &bdrv_flags) != 0) {
             error_report("invalid discard option");
-            return NULL;
+            goto early_err;
         }
     }
 
@@ -471,7 +471,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
             /* this is the default */
         } else {
            error_report("invalid aio option");
-           return NULL;
+           goto early_err;
         }
     }
 #endif
@@ -481,7 +481,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
             error_printf("Supported formats:");
             bdrv_iterate_format(bdrv_format_print, NULL);
             error_printf("\n");
-            return NULL;
+            goto early_err;
         }
 
         drv = bdrv_find_whitelisted_format(buf, ro);
@@ -491,7 +491,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
             } else {
                 error_report("'%s' invalid format", buf);
             }
-            return NULL;
+            goto early_err;
         }
     }
 
@@ -512,7 +512,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
     if (!do_check_io_limits(&io_limits, &error)) {
         error_report("%s", error_get_pretty(error));
         error_free(error);
-        return NULL;
+        goto early_err;
     }
 
     if (qemu_opt_get(opts, "boot") != NULL) {
@@ -525,12 +525,12 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
     if ((buf = qemu_opt_get(opts, "werror")) != NULL) {
         if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && type != IF_NONE) {
             error_report("werror is not supported by this bus type");
-            return NULL;
+            goto early_err;
         }
 
         on_write_error = parse_block_error_action(buf, 0);
         if (on_write_error < 0) {
-            return NULL;
+            goto early_err;
         }
     }
 
@@ -538,19 +538,19 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
     if ((buf = qemu_opt_get(opts, "rerror")) != NULL) {
         if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && type != IF_NONE) {
             error_report("rerror is not supported by this bus type");
-            return NULL;
+            goto early_err;
         }
 
         on_read_error = parse_block_error_action(buf, 1);
         if (on_read_error < 0) {
-            return NULL;
+            goto early_err;
         }
     }
 
     if ((devaddr = qemu_opt_get(opts, "addr")) != NULL) {
         if (type != IF_VIRTIO) {
             error_report("addr is not supported by this bus type");
-            return NULL;
+            goto early_err;
         }
     }
 
@@ -559,7 +559,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
     if (index != -1) {
         if (bus_id != 0 || unit_id != -1) {
             error_report("index cannot be used with bus and unit");
-            return NULL;
+            goto early_err;
         }
         bus_id = drive_index_to_bus_id(type, index);
         unit_id = drive_index_to_unit_id(type, index);
@@ -585,7 +585,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
     if (max_devs && unit_id >= max_devs) {
         error_report("unit %d too big (max is %d)",
                      unit_id, max_devs - 1);
-        return NULL;
+        goto early_err;
     }
 
     /*
@@ -595,7 +595,7 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
     if (drive_get(type, bus_id, unit_id) != NULL) {
         error_report("drive with bus=%d, unit=%d (index=%d) exists",
                      bus_id, unit_id, index);
-        return NULL;
+        goto early_err;
     }
 
     /* init */
@@ -672,6 +672,8 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
         if (has_driver_specific_opts) {
             file = NULL;
         } else {
+            QDECREF(bs_opts);
+            qemu_opts_del(opts);
             return dinfo;
         }
     }
@@ -730,12 +732,13 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
     return dinfo;
 
 err:
-    qemu_opts_del(opts);
-    QDECREF(bs_opts);
     bdrv_delete(dinfo->bdrv);
     g_free(dinfo->id);
     QTAILQ_REMOVE(&drives, dinfo, next);
     g_free(dinfo);
+early_err:
+    QDECREF(bs_opts);
+    qemu_opts_del(opts);
     return NULL;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/6] qdev: unref qdev when device_add fails

[Stefan Hajnoczi]

qdev_device_add() leaks the created qdev upon failure.  I suspect this
problem crept in because qdev_free() unparents the qdev but does not
drop a reference - confusing name.

Also drop trailing whitespace after curly bracket.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qdev-monitor.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index 410cdcb..5657cdc 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -512,6 +512,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     }
     if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
         qdev_free(qdev);
+        object_unref(OBJECT(qdev));
         return NULL;
     }
     if (qdev->id) {
@@ -523,8 +524,9 @@ DeviceState *qdev_device_add(QemuOpts *opts)
         object_property_add_child(qdev_get_peripheral_anon(), name,
                                   OBJECT(qdev), NULL);
         g_free(name);
-    }        
+    }
     if (qdev_init(qdev) < 0) {
+        object_unref(OBJECT(qdev));
         qerror_report(QERR_DEVICE_INIT_FAILED, driver);
         return NULL;
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/6] libqtest: rename qmp() to qmp_discard_response()

[Stefan Hajnoczi]

Existing qmp() callers do not expect a response object.  In order to
implement real QMP test cases it will be necessary to inspect the
response object.

Rename qmp() to qmp_discard_response().  Later patches will introduce a
qmp() function that returns the response object and tests that use it.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/boot-order-test.c |  4 ++--
 tests/fdc-test.c        | 15 +++++++++------
 tests/ide-test.c        | 10 ++++++----
 tests/libqtest.c        |  8 ++++----
 tests/libqtest.h        | 20 ++++++++++----------
 5 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index 4b233d0..da158c3 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -41,12 +41,12 @@ static void test_a_boot_order(const char *machine,
     qtest_start(args);
     actual = read_boot_order();
     g_assert_cmphex(actual, ==, expected_boot);
-    qmp("{ 'execute': 'system_reset' }");
+    qmp_discard_response("{ 'execute': 'system_reset' }");
     /*
      * system_reset only requests reset.  We get a RESET event after
      * the actual reset completes.  Need to wait for that.
      */
-    qmp("");                    /* HACK: wait for event */
+    qmp_discard_response("");   /* HACK: wait for event */
     actual = read_boot_order();
     g_assert_cmphex(actual, ==, expected_reboot);
     qtest_quit(global_qtest);
diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index fd198dc..38b5b17 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -290,10 +290,12 @@ static void test_media_insert(void)
 
     /* Insert media in drive. DSKCHK should not be reset until a step pulse
      * is sent. */
-    qmp("{'execute':'change', 'arguments':{ 'device':'floppy0', "
-        "'target': '%s' }}", test_image);
-    qmp(""); /* ignore event (FIXME open -> open transition?!) */
-    qmp(""); /* ignore event */
+    qmp_discard_response("{'execute':'change', 'arguments':{"
+                         " 'device':'floppy0', 'target': '%s' }}",
+                         test_image);
+    qmp_discard_response(""); /* ignore event
+                                 (FIXME open -> open transition?!) */
+    qmp_discard_response(""); /* ignore event */
 
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
@@ -322,8 +324,9 @@ static void test_media_change(void)
 
     /* Eject the floppy and check that DSKCHG is set. Reading it out doesn't
      * reset the bit. */
-    qmp("{'execute':'eject', 'arguments':{ 'device':'floppy0' }}");
-    qmp(""); /* ignore event */
+    qmp_discard_response("{'execute':'eject', 'arguments':{"
+                         " 'device':'floppy0' }}");
+    qmp_discard_response(""); /* ignore event */
 
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
diff --git a/tests/ide-test.c b/tests/ide-test.c
index 7307f1d..0e6fa5d 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -435,8 +435,9 @@ static void test_flush(void)
         tmp_path);
 
     /* Delay the completion of the flush request until we explicitly do it */
-    qmp("{'execute':'human-monitor-command', 'arguments': { "
-        "'command-line': 'qemu-io ide0-hd0 \"break flush_to_os A\"'} }");
+    qmp_discard_response("{'execute':'human-monitor-command', 'arguments': {"
+                         " 'command-line':"
+                         " 'qemu-io ide0-hd0 \"break flush_to_os A\"'} }");
 
     /* FLUSH CACHE command on device 0*/
     outb(IDE_BASE + reg_device, 0);
@@ -448,8 +449,9 @@ static void test_flush(void)
     assert_bit_clear(data, DF | ERR | DRQ);
 
     /* Complete the command */
-    qmp("{'execute':'human-monitor-command', 'arguments': { "
-        "'command-line': 'qemu-io ide0-hd0 \"resume A\"'} }");
+    qmp_discard_response("{'execute':'human-monitor-command', 'arguments': {"
+                         " 'command-line':"
+                         " 'qemu-io ide0-hd0 \"resume A\"'} }");
 
     /* Check registers */
     data = inb(IDE_BASE + reg_device);
diff --git a/tests/libqtest.c b/tests/libqtest.c
index bb82069..d46ad02 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -151,8 +151,8 @@ QTestState *qtest_init(const char *extra_args)
     }
 
     /* Read the QMP greeting and then do the handshake */
-    qtest_qmp(s, "");
-    qtest_qmp(s, "{ 'execute': 'qmp_capabilities' }");
+    qtest_qmp_discard_response(s, "");
+    qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
 
     if (getenv("QTEST_STOP")) {
         kill(qtest_qemu_pid(s), SIGSTOP);
@@ -291,7 +291,7 @@ redo:
     return words;
 }
 
-void qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
+void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap)
 {
     bool has_reply = false;
     int nesting = 0;
@@ -326,7 +326,7 @@ void qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
     }
 }
 
-void qtest_qmp(QTestState *s, const char *fmt, ...)
+void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...)
 {
     va_list ap;
 
diff --git a/tests/libqtest.h b/tests/libqtest.h
index a6e99bd..4f1b060 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -44,23 +44,23 @@ QTestState *qtest_init(const char *extra_args);
 void qtest_quit(QTestState *s);
 
 /**
- * qtest_qmp:
+ * qtest_qmp_discard_response:
  * @s: #QTestState instance to operate on.
  * @fmt...: QMP message to send to qemu
  *
- * Sends a QMP message to QEMU
+ * Sends a QMP message to QEMU and consumes the response.
  */
-void qtest_qmp(QTestState *s, const char *fmt, ...);
+void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
 
 /**
- * qtest_qmpv:
+ * qtest_qmpv_discard_response:
  * @s: #QTestState instance to operate on.
  * @fmt: QMP message to send to QEMU
  * @ap: QMP message arguments
  *
- * Sends a QMP message to QEMU.
+ * Sends a QMP message to QEMU and consumes the response.
  */
-void qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
+void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap);
 
 /**
  * qtest_get_irq:
@@ -331,17 +331,17 @@ static inline void qtest_end(void)
 }
 
 /**
- * qmp:
+ * qmp_discard_response:
  * @fmt...: QMP message to send to qemu
  *
- * Sends a QMP message to QEMU
+ * Sends a QMP message to QEMU and consumes the response.
  */
-static inline void qmp(const char *fmt, ...)
+static inline void qmp_discard_response(const char *fmt, ...)
 {
     va_list ap;
 
     va_start(ap, fmt);
-    qtest_qmpv(global_qtest, fmt, ap);
+    qtest_qmpv_discard_response(global_qtest, fmt, ap);
     va_end(ap);
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/6] libqtest: add qmp(fmt, ...) -> QDict* function

[Stefan Hajnoczi]

Add a qtest qmp() function that returns the response object.  This
allows test cases to verify the result or to check for error responses.
It also allows waiting for QMP events.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqtest.c | 66 ++++++++++++++++++++++++++++++++++++++++++++------------
 tests/libqtest.h | 37 +++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+), 14 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index d46ad02..83424c3 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -30,6 +30,8 @@
 
 #include "qemu/compiler.h"
 #include "qemu/osdep.h"
+#include "qapi/qmp/json-streamer.h"
+#include "qapi/qmp/json-parser.h"
 
 #define MAX_IRQ 256
 
@@ -291,16 +293,38 @@ redo:
     return words;
 }
 
-void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap)
+typedef struct {
+    JSONMessageParser parser;
+    QDict *response;
+} QMPResponseParser;
+
+static void qmp_response(JSONMessageParser *parser, QList *tokens)
 {
-    bool has_reply = false;
-    int nesting = 0;
+    QMPResponseParser *qmp = container_of(parser, QMPResponseParser, parser);
+    QObject *obj;
+
+    obj = json_parser_parse(tokens, NULL);
+    if (!obj) {
+        fprintf(stderr, "QMP JSON response parsing failed\n");
+        exit(1);
+    }
+
+    g_assert(qobject_type(obj) == QTYPE_QDICT);
+    g_assert(!qmp->response);
+    qmp->response = (QDict *)obj;
+}
+
+QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
+{
+    QMPResponseParser qmp;
 
     /* Send QMP request */
     socket_sendf(s->qmp_fd, fmt, ap);
 
     /* Receive reply */
-    while (!has_reply || nesting > 0) {
+    qmp.response = NULL;
+    json_message_parser_init(&qmp.parser, qmp_response);
+    while (!qmp.response) {
         ssize_t len;
         char c;
 
@@ -314,25 +338,39 @@ void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap)
             exit(1);
         }
 
-        switch (c) {
-        case '{':
-            nesting++;
-            has_reply = true;
-            break;
-        case '}':
-            nesting--;
-            break;
-        }
+        json_message_parser_feed(&qmp.parser, &c, 1);
     }
+    json_message_parser_destroy(&qmp.parser);
+
+    return qmp.response;
+}
+
+QDict *qtest_qmp(QTestState *s, const char *fmt, ...)
+{
+    va_list ap;
+    QDict *response;
+
+    va_start(ap, fmt);
+    response = qtest_qmpv(s, fmt, ap);
+    va_end(ap);
+    return response;
+}
+
+void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap)
+{
+    QDict *response = qtest_qmpv(s, fmt, ap);
+    QDECREF(response);
 }
 
 void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...)
 {
     va_list ap;
+    QDict *response;
 
     va_start(ap, fmt);
-    qtest_qmpv(s, fmt, ap);
+    response = qtest_qmpv(s, fmt, ap);
     va_end(ap);
+    QDECREF(response);
 }
 
 const char *qtest_get_arch(void)
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 4f1b060..9deebdc 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -22,6 +22,7 @@
 #include <stdbool.h>
 #include <stdarg.h>
 #include <sys/types.h>
+#include "qapi/qmp/qdict.h"
 
 typedef struct QTestState QTestState;
 
@@ -53,6 +54,15 @@ void qtest_quit(QTestState *s);
 void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
 
 /**
+ * qtest_qmp:
+ * @s: #QTestState instance to operate on.
+ * @fmt...: QMP message to send to qemu
+ *
+ * Sends a QMP message to QEMU and returns the response.
+ */
+QDict *qtest_qmp(QTestState *s, const char *fmt, ...);
+
+/**
  * qtest_qmpv_discard_response:
  * @s: #QTestState instance to operate on.
  * @fmt: QMP message to send to QEMU
@@ -63,6 +73,16 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
 void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap);
 
 /**
+ * qtest_qmpv:
+ * @s: #QTestState instance to operate on.
+ * @fmt: QMP message to send to QEMU
+ * @ap: QMP message arguments
+ *
+ * Sends a QMP message to QEMU and returns the response.
+ */
+QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
+
+/**
  * qtest_get_irq:
  * @s: #QTestState instance to operate on.
  * @num: Interrupt to observe.
@@ -331,6 +351,23 @@ static inline void qtest_end(void)
 }
 
 /**
+ * qmp:
+ * @fmt...: QMP message to send to qemu
+ *
+ * Sends a QMP message to QEMU and returns the response.
+ */
+static inline QDict *qmp(const char *fmt, ...)
+{
+    va_list ap;
+    QDict *response;
+
+    va_start(ap, fmt);
+    response = qtest_qmpv(global_qtest, fmt, ap);
+    va_end(ap);
+    return response;
+}
+
+/**
  * qmp_discard_response:
  * @fmt...: QMP message to send to qemu
  *
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/6] blockdev-test: add test case for drive_add duplicate IDs

[Stefan Hajnoczi]

The following should work:

  (qemu) drive_add if=none,id=drive0
  (qemu) drive_del drive0
  (qemu) drive_add if=none,id=drive0

Previous versions of QEMU produced a duplicate ID error because
drive_add leaked the options.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/Makefile        |  2 ++
 tests/blockdev-test.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)
 create mode 100644 tests/blockdev-test.c

diff --git a/tests/Makefile b/tests/Makefile
index baba9e9..fe71ec9 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -66,6 +66,7 @@ check-qtest-i386-y += tests/boot-order-test$(EXESUF)
 check-qtest-i386-y += tests/rtc-test$(EXESUF)
 check-qtest-i386-y += tests/i440fx-test$(EXESUF)
 check-qtest-i386-y += tests/fw_cfg-test$(EXESUF)
+check-qtest-i386-y += tests/blockdev-test$(EXESUF)
 check-qtest-x86_64-y = $(check-qtest-i386-y)
 gcov-files-i386-y += i386-softmmu/hw/mc146818rtc.c
 gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
@@ -172,6 +173,7 @@ tests/boot-order-test$(EXESUF): tests/boot-order-test.o $(libqos-obj-y)
 tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
 tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
 tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
+tests/blockdev-test$(EXESUF): tests/blockdev-test.o $(libqos-pc-obj-y)
 
 # QTest rules
 
diff --git a/tests/blockdev-test.c b/tests/blockdev-test.c
new file mode 100644
index 0000000..8b7371e
--- /dev/null
+++ b/tests/blockdev-test.c
@@ -0,0 +1,59 @@
+/*
+ * blockdev.c test cases
+ *
+ * Copyright (C) 2013 Red Hat Inc.
+ *
+ * Authors:
+ *  Stefan Hajnoczi <stefanha@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#include <glib.h>
+#include <string.h>
+#include "libqtest.h"
+
+static void test_drive_add_empty(void)
+{
+    QDict *response;
+    const char *response_return;
+
+    /* Start with an empty drive */
+    qtest_start("-drive if=none,id=drive0");
+
+    /* Delete the drive */
+    response = qmp("{\"execute\": \"human-monitor-command\","
+                   " \"arguments\": {"
+                   "   \"command-line\": \"drive_del drive0\""
+                   "}}");
+    g_assert(response);
+    response_return = qdict_get_try_str(response, "return");
+    g_assert(response_return);
+    g_assert(strcmp(response_return, "") == 0);
+    QDECREF(response);
+
+    /* Ensure re-adding the drive works - there should be no duplicate ID error
+     * because the old drive must be gone.
+     */
+    response = qmp("{\"execute\": \"human-monitor-command\","
+                   " \"arguments\": {"
+                   "   \"command-line\": \"drive_add 0 if=none,id=drive0\""
+                   "}}");
+    g_assert(response);
+    response_return = qdict_get_try_str(response, "return");
+    g_assert(response_return);
+    g_assert(strcmp(response_return, "OK\r\n") == 0);
+    QDECREF(response);
+
+    qtest_end();
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    qtest_add_func("/qdev/drive_add_empty", test_drive_add_empty);
+
+    return g_test_run();
+}
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/6] qdev-monitor-test: add device_add leak test cases

[Stefan Hajnoczi]

Ensure that the device_add error code path deletes device objects.
Failure to do so not only leaks the objects but can also keep other
objects (like drive or netdev) alive due to qdev properties holding
references.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/Makefile            |  2 ++
 tests/qdev-monitor-test.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+)
 create mode 100644 tests/qdev-monitor-test.c

diff --git a/tests/Makefile b/tests/Makefile
index fe71ec9..dc3b8f1 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -67,6 +67,7 @@ check-qtest-i386-y += tests/rtc-test$(EXESUF)
 check-qtest-i386-y += tests/i440fx-test$(EXESUF)
 check-qtest-i386-y += tests/fw_cfg-test$(EXESUF)
 check-qtest-i386-y += tests/blockdev-test$(EXESUF)
+check-qtest-i386-y += tests/qdev-monitor-test$(EXESUF)
 check-qtest-x86_64-y = $(check-qtest-i386-y)
 gcov-files-i386-y += i386-softmmu/hw/mc146818rtc.c
 gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
@@ -174,6 +175,7 @@ tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
 tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
 tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
 tests/blockdev-test$(EXESUF): tests/blockdev-test.o $(libqos-pc-obj-y)
+tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o $(libqos-pc-obj-y)
 
 # QTest rules
 
diff --git a/tests/qdev-monitor-test.c b/tests/qdev-monitor-test.c
new file mode 100644
index 0000000..d3d6d39
--- /dev/null
+++ b/tests/qdev-monitor-test.c
@@ -0,0 +1,81 @@
+/*
+ * qdev-monitor.c test cases
+ *
+ * Copyright (C) 2013 Red Hat Inc.
+ *
+ * Authors:
+ *  Stefan Hajnoczi <stefanha@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#include <string.h>
+#include <glib.h>
+#include "libqtest.h"
+#include "qapi/qmp/qjson.h"
+
+static void test_device_add(void)
+{
+    QDict *response;
+    QDict *error;
+
+    qtest_start("-drive if=none,id=drive0");
+
+    /* Make device_add fail.  If this leaks the virtio-blk-pci device then a
+     * reference to drive0 will also be held (via qdev properties).
+     */
+    response = qmp("{\"execute\": \"device_add\","
+                   " \"arguments\": {"
+                   "   \"driver\": \"virtio-blk-pci\","
+                   "   \"drive\": \"drive0\""
+                   "}}");
+    g_assert(response);
+    error = qdict_get_qdict(response, "error");
+    g_assert(!strcmp(qdict_get_try_str(error, "class") ?: "",
+                     "GenericError"));
+    g_assert(!strcmp(qdict_get_try_str(error, "desc") ?: "",
+                     "Device initialization failed."));
+    QDECREF(response);
+
+    /* Delete the drive */
+    response = qmp("{\"execute\": \"human-monitor-command\","
+                   " \"arguments\": {"
+                   "   \"command-line\": \"drive_del drive0\""
+                   "}}");
+    g_assert(response);
+    g_assert(!strcmp(qdict_get_try_str(response, "return") ?: "(null)", ""));
+    QDECREF(response);
+
+    /* Try to re-add the drive.  This fails with duplicate IDs if a leaked
+     * virtio-blk-pci exists that holds a reference to the old drive0.
+     */
+    response = qmp("{\"execute\": \"human-monitor-command\","
+                   " \"arguments\": {"
+                   "   \"command-line\": \"drive_add pci-addr=auto if=none,id=drive0\""
+                   "}}");
+    g_assert(response);
+    g_assert(!strcmp(qdict_get_try_str(response, "return") ?: "",
+                     "OK\r\n"));
+    QDECREF(response);
+
+    qtest_end();
+}
+
+int main(int argc, char **argv)
+{
+    const char *arch = qtest_get_arch();
+
+    /* Check architecture */
+    if (strcmp(arch, "i386") && strcmp(arch, "x86_64")) {
+        g_test_message("Skipping test for non-x86\n");
+        return 0;
+    }
+
+    /* Run the tests */
+    g_test_init(&argc, &argv, NULL);
+
+    qtest_add_func("/qdev/device_add", test_device_add);
+
+    return g_test_run();
+}
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/6] qdev: unref qdev when device_add fails

[Paolo Bonzini]

Il 10/09/2013 18:21, Stefan Hajnoczi ha scritto:
> qdev_device_add() leaks the created qdev upon failure.  I suspect this
> problem crept in because qdev_free() unparents the qdev but does not
> drop a reference - confusing name.

Right, the name a leftover from pre-refcounting days.

BTW, not dropping a reference is the right thing to do because the
reference is dropped much earlier, typically as soon as qdev_device_add
returns. The QOM object tree then will still provide means to access
devices, until they are unparented.

In this case, however, qdev_device_add's caller does not have a
reference to free; doing that is the responsibility of qdev_device_add,
since it returns NULL.

> Also drop trailing whitespace after curly bracket.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  qdev-monitor.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 410cdcb..5657cdc 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -512,6 +512,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>      }
>      if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
>          qdev_free(qdev);
> +        object_unref(OBJECT(qdev));
>          return NULL;
>      }
>      if (qdev->id) {
> @@ -523,8 +524,9 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>          object_property_add_child(qdev_get_peripheral_anon(), name,
>                                    OBJECT(qdev), NULL);
>          g_free(name);
> -    }        
> +    }
>      if (qdev_init(qdev) < 0) {
> +        object_unref(OBJECT(qdev));
>          qerror_report(QERR_DEVICE_INIT_FAILED, driver);
>          return NULL;
>      }
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 2/6] qdev: unref qdev when device_add fails

[Andreas Färber]

Am 10.09.2013 18:49, schrieb Paolo Bonzini:
> Il 10/09/2013 18:21, Stefan Hajnoczi ha scritto:
>> qdev_device_add() leaks the created qdev upon failure.  I suspect this
>> problem crept in because qdev_free() unparents the qdev but does not
>> drop a reference - confusing name.
> 
> Right, the name a leftover from pre-refcounting days.
> 
> BTW, not dropping a reference is the right thing to do because the
> reference is dropped much earlier, typically as soon as qdev_device_add
> returns. The QOM object tree then will still provide means to access
> devices, until they are unparented.
> 
> In this case, however, qdev_device_add's caller does not have a
> reference to free; doing that is the responsibility of qdev_device_add,
> since it returns NULL.
> 
>> Also drop trailing whitespace after curly bracket.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  qdev-monitor.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 410cdcb..5657cdc 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -512,6 +512,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>      }
>>      if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
>>          qdev_free(qdev);
>> +        object_unref(OBJECT(qdev));
>>          return NULL;
>>      }
>>      if (qdev->id) {

Given that qdev_free() doesn't do what one might expect, I would suggest
to s/qdev_free/object_unparent/g above.

>> @@ -523,8 +524,9 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>          object_property_add_child(qdev_get_peripheral_anon(), name,
>>                                    OBJECT(qdev), NULL);
>>          g_free(name);
>> -    }        
>> +    }
>>      if (qdev_init(qdev) < 0) {
>> +        object_unref(OBJECT(qdev));
>>          qerror_report(QERR_DEVICE_INIT_FAILED, driver);
>>          return NULL;
>>      }
>>
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

I would like to take this through qom-next tree since I have pending
variable cleanups there ("qdev" being touched here). Not sure how to
handle that wrt block changes in this series?

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] [PATCH 2/6] qdev: unref qdev when device_add fails

[Paolo Bonzini]

Il 10/09/2013 18:59, Andreas Färber ha scritto:
> Am 10.09.2013 18:49, schrieb Paolo Bonzini:
>> Il 10/09/2013 18:21, Stefan Hajnoczi ha scritto:
>>> qdev_device_add() leaks the created qdev upon failure.  I suspect this
>>> problem crept in because qdev_free() unparents the qdev but does not
>>> drop a reference - confusing name.
>>
>> Right, the name a leftover from pre-refcounting days.
>>
>> BTW, not dropping a reference is the right thing to do because the
>> reference is dropped much earlier, typically as soon as qdev_device_add
>> returns. The QOM object tree then will still provide means to access
>> devices, until they are unparented.
>>
>> In this case, however, qdev_device_add's caller does not have a
>> reference to free; doing that is the responsibility of qdev_device_add,
>> since it returns NULL.
>>
>>> Also drop trailing whitespace after curly bracket.
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  qdev-monitor.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>>> index 410cdcb..5657cdc 100644
>>> --- a/qdev-monitor.c
>>> +++ b/qdev-monitor.c
>>> @@ -512,6 +512,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>>      }
>>>      if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
>>>          qdev_free(qdev);
>>> +        object_unref(OBJECT(qdev));
>>>          return NULL;
>>>      }
>>>      if (qdev->id) {
> 
> Given that qdev_free() doesn't do what one might expect, I would suggest
> to s/qdev_free/object_unparent/g above.

Then do it everywhere...

Paolo

>>> @@ -523,8 +524,9 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>>          object_property_add_child(qdev_get_peripheral_anon(), name,
>>>                                    OBJECT(qdev), NULL);
>>>          g_free(name);
>>> -    }        
>>> +    }
>>>      if (qdev_init(qdev) < 0) {
>>> +        object_unref(OBJECT(qdev));
>>>          qerror_report(QERR_DEVICE_INIT_FAILED, driver);
>>>          return NULL;
>>>      }
>>>
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> I would like to take this through qom-next tree since I have pending
> variable cleanups there ("qdev" being touched here). Not sure how to
> handle that wrt block changes in this series?
> 
> Andreas
> 

Re: [Qemu-devel] [PATCH 2/6] qdev: unref qdev when device_add fails

[Stefan Hajnoczi]

On Tue, Sep 10, 2013 at 6:59 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 10.09.2013 18:49, schrieb Paolo Bonzini:
>> Il 10/09/2013 18:21, Stefan Hajnoczi ha scritto:
>>> qdev_device_add() leaks the created qdev upon failure.  I suspect this
>>> problem crept in because qdev_free() unparents the qdev but does not
>>> drop a reference - confusing name.
>>
>> Right, the name a leftover from pre-refcounting days.
>>
>> BTW, not dropping a reference is the right thing to do because the
>> reference is dropped much earlier, typically as soon as qdev_device_add
>> returns. The QOM object tree then will still provide means to access
>> devices, until they are unparented.
>>
>> In this case, however, qdev_device_add's caller does not have a
>> reference to free; doing that is the responsibility of qdev_device_add,
>> since it returns NULL.
>>
>>> Also drop trailing whitespace after curly bracket.
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  qdev-monitor.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>>> index 410cdcb..5657cdc 100644
>>> --- a/qdev-monitor.c
>>> +++ b/qdev-monitor.c
>>> @@ -512,6 +512,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>>      }
>>>      if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
>>>          qdev_free(qdev);
>>> +        object_unref(OBJECT(qdev));
>>>          return NULL;
>>>      }
>>>      if (qdev->id) {
>
> Given that qdev_free() doesn't do what one might expect, I would suggest
> to s/qdev_free/object_unparent/g above.

I'll send a follow-up patch to do this across the tree.

>>> @@ -523,8 +524,9 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>>          object_property_add_child(qdev_get_peripheral_anon(), name,
>>>                                    OBJECT(qdev), NULL);
>>>          g_free(name);
>>> -    }
>>> +    }
>>>      if (qdev_init(qdev) < 0) {
>>> +        object_unref(OBJECT(qdev));
>>>          qerror_report(QERR_DEVICE_INIT_FAILED, driver);
>>>          return NULL;
>>>      }
>>>
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
> I would like to take this through qom-next tree since I have pending
> variable cleanups there ("qdev" being touched here). Not sure how to
> handle that wrt block changes in this series?

It would be simplest to take the whole series.  Splitting it is hard
because the qdev device_add test case requires the blockdev.c fix
(otherwise the drive ID is held due to the leaked options list).

Or you can cherry-pick just this patch and the rest of the series can
go via the block tree?

Stefan