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

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

[Stefan Hajnoczi]

v3:
 * I lost track of this patch, now I'm pushing it again
 * Rebase onto qemu.git/master
 * Add Patch 7 to do s/qdev_free()/object_unparent()/ [afaerber]

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.
Patch 7 replaces the confusing qdev_free() function with object_unparent()

Stefan Hajnoczi (7):
  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
  qdev: drop misleading qdev_free() function

 blockdev.c                | 27 +++++++++-------
 hw/acpi/piix4.c           |  2 +-
 hw/core/qdev.c            | 12 ++-----
 hw/pci/pci-hotplug-old.c  |  2 +-
 hw/pci/pci_bridge.c       |  2 +-
 hw/pci/pcie.c             |  2 +-
 hw/pci/shpc.c             |  2 +-
 hw/s390x/virtio-ccw.c     |  2 +-
 hw/scsi/scsi-bus.c        |  6 ++--
 hw/usb/bus.c              |  7 ++--
 hw/usb/dev-storage.c      |  2 +-
 hw/usb/host-legacy.c      |  2 +-
 hw/virtio/virtio-bus.c    |  4 +--
 hw/xen/xen_platform.c     |  2 +-
 include/hw/qdev-core.h    |  1 -
 qdev-monitor.c            |  6 ++--
 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 +++++++++++++++++++++++++++++++++++++++++++++++
 24 files changed, 299 insertions(+), 78 deletions(-)
 create mode 100644 tests/blockdev-test.c
 create mode 100644 tests/qdev-monitor-test.c

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/7] 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 | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b260477..86e6bff 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -341,7 +341,7 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
     qemu_opts_absorb_qdict(opts, bs_opts, &error);
     if (error_is_set(&error)) {
         error_propagate(errp, error);
-        return NULL;
+        goto early_err;
     }
 
     if (id) {
@@ -361,7 +361,7 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
     if ((buf = qemu_opt_get(opts, "discard")) != NULL) {
         if (bdrv_parse_discard_flags(buf, &bdrv_flags) != 0) {
             error_setg(errp, "invalid discard option");
-            return NULL;
+            goto early_err;
         }
     }
 
@@ -383,7 +383,7 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
             /* this is the default */
         } else {
            error_setg(errp, "invalid aio option");
-           return NULL;
+           goto early_err;
         }
     }
 #endif
@@ -393,13 +393,13 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
             error_printf("Supported formats:");
             bdrv_iterate_format(bdrv_format_print, NULL);
             error_printf("\n");
-            return NULL;
+            goto early_err;
         }
 
         drv = bdrv_find_format(buf);
         if (!drv) {
             error_setg(errp, "'%s' invalid format", buf);
-            return NULL;
+            goto early_err;
         }
     }
 
@@ -435,20 +435,20 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
 
     if (!check_throttle_config(&cfg, &error)) {
         error_propagate(errp, error);
-        return NULL;
+        goto early_err;
     }
 
     on_write_error = BLOCKDEV_ON_ERROR_ENOSPC;
     if ((buf = qemu_opt_get(opts, "werror")) != NULL) {
         if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && type != IF_NONE) {
             error_setg(errp, "werror is not supported by this bus type");
-            return NULL;
+            goto early_err;
         }
 
         on_write_error = parse_block_error_action(buf, 0, &error);
         if (error_is_set(&error)) {
             error_propagate(errp, error);
-            return NULL;
+            goto early_err;
         }
     }
 
@@ -456,13 +456,13 @@ static DriveInfo *blockdev_init(QDict *bs_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, &error);
         if (error_is_set(&error)) {
             error_propagate(errp, error);
-            return NULL;
+            goto early_err;
         }
     }
 
@@ -491,6 +491,8 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
         if (has_driver_specific_opts) {
             file = NULL;
         } else {
+            QDECREF(bs_opts);
+            qemu_opts_del(opts);
             return dinfo;
         }
     }
@@ -529,12 +531,13 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
     return dinfo;
 
 err:
-    qemu_opts_del(opts);
-    QDECREF(bs_opts);
     bdrv_unref(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 v3 2/7] 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 a02c925..0cb53a5 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -521,6 +521,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) {
@@ -532,8 +533,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 v3 3/7] 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 v3 4/7] 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 v3 5/7] 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 fa4c9f0..7863123 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -67,6 +67,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))
@@ -174,6 +175,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)
 tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o
 
 # 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 v3 6/7] 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 7863123..2771f92 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -68,6 +68,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))
@@ -176,6 +177,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)
 tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o
 
 # 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

[Qemu-devel] [PATCH v3 7/7] qdev: drop misleading qdev_free() function

[Stefan Hajnoczi]

The qdev_free() function name is misleading since all the function does
is unlink the device from its parent.  The device is not necessarily
freed.

The device will be freed when its QObject refcount reaches zero.  It is
usual for the parent (bus) to hold the final reference but there are
cases where something else holds a reference so "free" is a misleading
name.

Call object_unparent(obj) directly instead of having a qdev wrapper
function.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/acpi/piix4.c          |  2 +-
 hw/core/qdev.c           | 12 +++---------
 hw/pci/pci-hotplug-old.c |  2 +-
 hw/pci/pci_bridge.c      |  2 +-
 hw/pci/pcie.c            |  2 +-
 hw/pci/shpc.c            |  2 +-
 hw/s390x/virtio-ccw.c    |  2 +-
 hw/scsi/scsi-bus.c       |  6 +++---
 hw/usb/bus.c             |  7 ++++---
 hw/usb/dev-storage.c     |  2 +-
 hw/usb/host-legacy.c     |  2 +-
 hw/virtio/virtio-bus.c   |  4 +---
 hw/xen/xen_platform.c    |  2 +-
 include/hw/qdev-core.h   |  1 -
 qdev-monitor.c           |  2 +-
 15 files changed, 21 insertions(+), 29 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index b46bd5e..7f5ab24 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -326,7 +326,7 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots)
             if (pc->no_hotplug) {
                 slot_free = false;
             } else {
-                qdev_free(qdev);
+                object_unparent(OBJECT(qdev));
             }
         }
     }
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 533f6dd..e374a93 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -164,7 +164,7 @@ int qdev_init(DeviceState *dev)
     if (local_err != NULL) {
         qerror_report_err(local_err);
         error_free(local_err);
-        qdev_free(dev);
+        object_unparent(OBJECT(dev));
         return -1;
     }
     return 0;
@@ -258,7 +258,7 @@ void qbus_reset_all_fn(void *opaque)
 int qdev_simple_unplug_cb(DeviceState *dev)
 {
     /* just zap it */
-    qdev_free(dev);
+    object_unparent(OBJECT(dev));
     return 0;
 }
 
@@ -280,12 +280,6 @@ void qdev_init_nofail(DeviceState *dev)
     }
 }
 
-/* Unlink device from bus and free the structure.  */
-void qdev_free(DeviceState *dev)
-{
-    object_unparent(OBJECT(dev));
-}
-
 void qdev_machine_creation_done(void)
 {
     /*
@@ -458,7 +452,7 @@ static void bus_unparent(Object *obj)
 
     while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
         DeviceState *dev = kid->child;
-        qdev_free(dev);
+        object_unparent(OBJECT(dev));
     }
     if (bus->parent) {
         QLIST_REMOVE(bus, sibling);
diff --git a/hw/pci/pci-hotplug-old.c b/hw/pci/pci-hotplug-old.c
index 619fe47..8dbc3c1 100644
--- a/hw/pci/pci-hotplug-old.c
+++ b/hw/pci/pci-hotplug-old.c
@@ -248,7 +248,7 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
         }
         dev = pci_create(bus, devfn, "virtio-blk-pci");
         if (qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv) < 0) {
-            qdev_free(&dev->qdev);
+            object_unparent(OBJECT(dev));
             dev = NULL;
             break;
         }
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index e6b22b8..290abab 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -391,7 +391,7 @@ void pci_bridge_exitfn(PCIDevice *pci_dev)
     pci_bridge_region_cleanup(s, s->windows);
     memory_region_destroy(&s->address_space_mem);
     memory_region_destroy(&s->address_space_io);
-    /* qbus_free() is called automatically by qdev_free() */
+    /* qbus_free() is called automatically during device deletion */
 }
 
 /*
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 50af3c1..a27acf3 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -251,7 +251,7 @@ static int pcie_cap_slot_hotplug(DeviceState *qdev,
                                    PCI_EXP_SLTSTA_PDS);
         pcie_cap_slot_event(d, PCI_EXP_HP_EV_PDC);
     } else {
-        qdev_free(&pci_dev->qdev);
+        object_unparent(OBJECT(pci_dev));
         pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
                                      PCI_EXP_SLTSTA_PDS);
         pcie_cap_slot_event(d, PCI_EXP_HP_EV_PDC);
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index eb092fd..29a8c36 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -254,7 +254,7 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot)
          ++devfn) {
         PCIDevice *affected_dev = shpc->sec_bus->devices[devfn];
         if (affected_dev) {
-            qdev_free(&affected_dev->qdev);
+            object_unparent(OBJECT(affected_dev));
         }
     }
 }
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index cd67db5..f93a81c 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1239,7 +1239,7 @@ static int virtio_ccw_busdev_unplug(DeviceState *dev)
 
     css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid, 1, 0);
 
-    qdev_free(dev);
+    object_unparent(OBJECT(dev));
     return 0;
 }
 
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 24ec52f..ea916d1 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -178,7 +178,7 @@ static int scsi_qdev_init(DeviceState *qdev)
         d = scsi_device_find(bus, dev->channel, dev->id, dev->lun);
         assert(d);
         if (d->lun == dev->lun && dev != d) {
-            qdev_free(&d->qdev);
+            object_unparent(OBJECT(d));
         }
     }
 
@@ -231,13 +231,13 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv,
     }
     if (qdev_prop_set_drive(dev, "drive", bdrv) < 0) {
         error_setg(errp, "Setting drive property failed");
-        qdev_free(dev);
+        object_unparent(OBJECT(dev));
         return NULL;
     }
     object_property_set_bool(OBJECT(dev), true, "realized", &err);
     if (err != NULL) {
         error_propagate(errp, err);
-        qdev_free(dev);
+        object_unparent(OBJECT(dev));
         return NULL;
     }
     return SCSI_DEVICE(dev);
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 72d5b92..ca329be 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -356,8 +356,9 @@ void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr)
 
 void usb_unregister_port(USBBus *bus, USBPort *port)
 {
-    if (port->dev)
-        qdev_free(&port->dev->qdev);
+    if (port->dev) {
+        object_unparent(OBJECT(port->dev));
+    }
     QTAILQ_REMOVE(&bus->free, port, next);
     bus->nfree--;
 }
@@ -505,7 +506,7 @@ int usb_device_delete_addr(int busnr, int addr)
         return -1;
     dev = port->dev;
 
-    qdev_free(&dev->qdev);
+    object_unparent(OBJECT(dev));
     return 0;
 }
 
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 1d81ac2..c434c56 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -703,7 +703,7 @@ static USBDevice *usb_msd_init(USBBus *bus, const char *filename)
         return NULL;
     }
     if (qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv) < 0) {
-        qdev_free(&dev->qdev);
+        object_unparent(OBJECT(dev));
         return NULL;
     }
     if (qdev_init(&dev->qdev) < 0)
diff --git a/hw/usb/host-legacy.c b/hw/usb/host-legacy.c
index 3a5f705..3cc9c42 100644
--- a/hw/usb/host-legacy.c
+++ b/hw/usb/host-legacy.c
@@ -132,7 +132,7 @@ USBDevice *usb_host_device_open(USBBus *bus, const char *devname)
     return dev;
 
 fail:
-    qdev_free(&dev->qdev);
+    object_unparent(OBJECT(dev));
     return NULL;
 }
 
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 6849a01..e6b103c 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -67,7 +67,6 @@ void virtio_bus_reset(VirtioBusState *bus)
 /* Destroy the VirtIODevice */
 void virtio_bus_destroy_device(VirtioBusState *bus)
 {
-    DeviceState *qdev;
     BusState *qbus = BUS(bus);
     VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus);
     DPRINTF("%s: remove device.\n", qbus->name);
@@ -76,8 +75,7 @@ void virtio_bus_destroy_device(VirtioBusState *bus)
         if (klass->device_unplug != NULL) {
             klass->device_unplug(qbus->parent);
         }
-        qdev = DEVICE(bus->vdev);
-        qdev_free(qdev);
+        object_unparent(OBJECT(bus->vdev));
         bus->vdev = NULL;
     }
 }
diff --git a/hw/xen/xen_platform.c b/hw/xen/xen_platform.c
index 79bf0b3..70875e4 100644
--- a/hw/xen/xen_platform.c
+++ b/hw/xen/xen_platform.c
@@ -95,7 +95,7 @@ static void unplug_nic(PCIBus *b, PCIDevice *d, void *o)
     if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
             PCI_CLASS_NETWORK_ETHERNET
             && strcmp(d->name, "xen-pci-passthrough") != 0) {
-        qdev_free(DEVICE(d));
+        object_unparent(OBJECT(d));
     }
 }
 
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index e191ca0..f2043a6 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -221,7 +221,6 @@ void qdev_init_nofail(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
 void qdev_unplug(DeviceState *dev, Error **errp);
-void qdev_free(DeviceState *dev);
 int qdev_simple_unplug_cb(DeviceState *dev);
 void qdev_machine_creation_done(void);
 bool qdev_machine_modified(void);
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 0cb53a5..4f558c4 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -520,7 +520,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
         qdev->id = id;
     }
     if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
-        qdev_free(qdev);
+        object_unparent(OBJECT(qdev));
         object_unref(OBJECT(qdev));
         return NULL;
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 0/7] qdev and blockdev refcount leak fixes

[Andreas Färber]

Am 30.10.2013 14:54, schrieb Stefan Hajnoczi:
> v3:
>  * I lost track of this patch, now I'm pushing it again

Part of this series is in a pending qom-next pull of mine (on a
different base), which Anthony didn't merge due to some questions or
problems, and during Hackathon he disappeared and so far hasn't followed
up further. :(

I can surely send a rebased PULL v2 now that the block fix I think is
in, but not sure if that is all to do...

Regards,
Andreas

>  * Rebase onto qemu.git/master
>  * Add Patch 7 to do s/qdev_free()/object_unparent()/ [afaerber]
> 
> 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.
> Patch 7 replaces the confusing qdev_free() function with object_unparent()
> 
> Stefan Hajnoczi (7):
>   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
>   qdev: drop misleading qdev_free() function
> 
>  blockdev.c                | 27 +++++++++-------
>  hw/acpi/piix4.c           |  2 +-
>  hw/core/qdev.c            | 12 ++-----
>  hw/pci/pci-hotplug-old.c  |  2 +-
>  hw/pci/pci_bridge.c       |  2 +-
>  hw/pci/pcie.c             |  2 +-
>  hw/pci/shpc.c             |  2 +-
>  hw/s390x/virtio-ccw.c     |  2 +-
>  hw/scsi/scsi-bus.c        |  6 ++--
>  hw/usb/bus.c              |  7 ++--
>  hw/usb/dev-storage.c      |  2 +-
>  hw/usb/host-legacy.c      |  2 +-
>  hw/virtio/virtio-bus.c    |  4 +--
>  hw/xen/xen_platform.c     |  2 +-
>  include/hw/qdev-core.h    |  1 -
>  qdev-monitor.c            |  6 ++--
>  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 +++++++++++++++++++++++++++++++++++++++++++++++
>  24 files changed, 299 insertions(+), 78 deletions(-)
>  create mode 100644 tests/blockdev-test.c
>  create mode 100644 tests/qdev-monitor-test.c
> 


-- 
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 v3 0/7] qdev and blockdev refcount leak fixes

[Stefan Hajnoczi]

On Wed, Oct 30, 2013 at 04:44:18PM +0100, Andreas Färber wrote:
> Am 30.10.2013 14:54, schrieb Stefan Hajnoczi:
> > v3:
> >  * I lost track of this patch, now I'm pushing it again
> 
> Part of this series is in a pending qom-next pull of mine (on a
> different base), which Anthony didn't merge due to some questions or
> problems, and during Hackathon he disappeared and so far hasn't followed
> up further. :(
> 
> I can surely send a rebased PULL v2 now that the block fix I think is
> in, but not sure if that is all to do...

Ah, that explains what happened.

Anyway this v3 series should be useful to you because it resolves
conflicts with qemu.git/master.

Stefan

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

[Eric Blake]

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

On 10/30/2013 07:54 AM, Stefan Hajnoczi wrote:
> 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 | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Eric Blake]

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

On 10/30/2013 07:54 AM, Stefan Hajnoczi wrote:
> 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.

Is it worth renaming in a future patch?

> 
> 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(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Eric Blake]

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

On 10/30/2013 07:54 AM, Stefan Hajnoczi wrote:
> 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(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Eric Blake]

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

On 10/30/2013 07:54 AM, Stefan Hajnoczi wrote:
> 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(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> +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);

I prefer EXIT_FAILURE, but you're not the first person to use 1 instead.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Eric Blake]

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

On 10/30/2013 07:54 AM, Stefan Hajnoczi wrote:
> 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

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Eric Blake]

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

On 10/30/2013 07:54 AM, Stefan Hajnoczi wrote:
> 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

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v3 7/7] qdev: drop misleading qdev_free() function

[Eric Blake]

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

On 10/30/2013 07:54 AM, Stefan Hajnoczi wrote:
> The qdev_free() function name is misleading since all the function does
> is unlink the device from its parent.  The device is not necessarily
> freed.

Aha - you anticipated my comment on 2/7 :)

> 
> The device will be freed when its QObject refcount reaches zero.  It is
> usual for the parent (bus) to hold the final reference but there are
> cases where something else holds a reference so "free" is a misleading
> name.
> 
> Call object_unparent(obj) directly instead of having a qdev wrapper
> function.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---

>      while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
>          DeviceState *dev = kid->child;
> -        qdev_free(dev);
> +        object_unparent(OBJECT(dev));

In most cases, you are just expanding the old call inline...

>      }
>      if (bus->parent) {
>          QLIST_REMOVE(bus, sibling);
> diff --git a/hw/pci/pci-hotplug-old.c b/hw/pci/pci-hotplug-old.c
> index 619fe47..8dbc3c1 100644
> --- a/hw/pci/pci-hotplug-old.c
> +++ b/hw/pci/pci-hotplug-old.c
> @@ -248,7 +248,7 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
>          }
>          dev = pci_create(bus, devfn, "virtio-blk-pci");
>          if (qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv) < 0) {
> -            qdev_free(&dev->qdev);
> +            object_unparent(OBJECT(dev));

...but in this case, you are unparenting OBJECT(dev) in the new code
while the old code unparented OBJECT(&dev->qdev).  Ah, I see - qdev is
the first member of dev, so it is the same pointer address.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Stefan Hajnoczi]

On Tue, Nov 05, 2013 at 04:50:30AM -0700, Eric Blake wrote:
> On 10/30/2013 07:54 AM, Stefan Hajnoczi wrote:
> > 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.
> 
> Is it worth renaming in a future patch?

Yep, the last patch does that.

Re: [Qemu-devel] [PATCH v3 0/7] qdev and blockdev refcount leak fixes

[Stefan Hajnoczi]

On Wed, Oct 30, 2013 at 02:54:29PM +0100, Stefan Hajnoczi wrote:
> v3:
>  * I lost track of this patch, now I'm pushing it again
>  * Rebase onto qemu.git/master
>  * Add Patch 7 to do s/qdev_free()/object_unparent()/ [afaerber]
> 
> 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.
> Patch 7 replaces the confusing qdev_free() function with object_unparent()
> 
> Stefan Hajnoczi (7):
>   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
>   qdev: drop misleading qdev_free() function
> 
>  blockdev.c                | 27 +++++++++-------
>  hw/acpi/piix4.c           |  2 +-
>  hw/core/qdev.c            | 12 ++-----
>  hw/pci/pci-hotplug-old.c  |  2 +-
>  hw/pci/pci_bridge.c       |  2 +-
>  hw/pci/pcie.c             |  2 +-
>  hw/pci/shpc.c             |  2 +-
>  hw/s390x/virtio-ccw.c     |  2 +-
>  hw/scsi/scsi-bus.c        |  6 ++--
>  hw/usb/bus.c              |  7 ++--
>  hw/usb/dev-storage.c      |  2 +-
>  hw/usb/host-legacy.c      |  2 +-
>  hw/virtio/virtio-bus.c    |  4 +--
>  hw/xen/xen_platform.c     |  2 +-
>  include/hw/qdev-core.h    |  1 -
>  qdev-monitor.c            |  6 ++--
>  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 +++++++++++++++++++++++++++++++++++++++++++++++
>  24 files changed, 299 insertions(+), 78 deletions(-)
>  create mode 100644 tests/blockdev-test.c
>  create mode 100644 tests/qdev-monitor-test.c
> 
> -- 
> 1.8.3.1

Applied the remaining patches to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

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

[Andreas Färber]

Am 30.10.2013 14:54, schrieb 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(-)

Reviewed-by: Andreas Färber <afaerber@suse.de>

I notice that at least in two places [qtest_]qmp() was being called with
literal "" argument. If we could get a separate qmp_wait_for_ready() or
so function, then we could actually enabling format string checking for
those functions. That'd a follow-up though.

Regards,
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 v3 4/7] libqtest: add qmp(fmt, ...) -> QDict* function

[Andreas Färber]

Am 30.10.2013 14:54, schrieb 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.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
>   *

I can't really judge the JSON implementation, but it looks sane reusing
existing code.

The new prototypes could get the GCC_FMT_ATTR(2,3) and
GCC_FMT_ATTR(1,2), but since that's an improvement over existing code it
can be done as follow-up, so

Reviewed-by: Andreas Färber <afaerber@suse.de>

and thanks a lot for following up on Jason's earlier attempt!

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 v3 5/7] blockdev-test: add test case for drive_add duplicate IDs

[Andreas Färber]

Am 30.10.2013 14:54, schrieb 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 fa4c9f0..7863123 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -67,6 +67,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

What's missing here is to add the source file(s) for gcov checking. But
can be done as follow-up, just don't forget. :)

>  gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
> @@ -174,6 +175,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)
>  tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o
>  
>  # 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
[...]
> +int main(int argc, char **argv)
> +{
> +    g_test_init(&argc, &argv, NULL);
> +
> +    qtest_add_func("/qdev/drive_add_empty", test_drive_add_empty);

I would prefer not to name that "qdev" if you can think of something
better... otherwise test looks fine.

Andreas

> +
> +    return g_test_run();
> +}
> 


-- 
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 v3 6/7] qdev-monitor-test: add device_add leak test cases

[Andreas Färber]

Am 30.10.2013 14:54, schrieb 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 7863123..2771f92 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -68,6 +68,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

Same comment here about adding to the list.

>  gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
> @@ -176,6 +177,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)
>  tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o
>  
>  # 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);

What about naming these tests /qmp/...?

Again, test itself looks great on a brief look.

Andreas

> +
> +    return g_test_run();
> +}
> 


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