[Qemu-devel] [PATCH 0/6] Improve drive_del test coverage

[Qemu-devel] [PATCH 0/6] Improve drive_del test coverage

[Markus Armbruster]

The meat is in the last patch, the others are just cleanup.

Markus Armbruster (6):
  drive_del-test: Merge of qdev-monitor-test, blockdev-test
  blockdev-test: Use single rather than double quotes in QMP
  blockdev-test: Clean up bogus drive_add argument
  blockdev-test: Simplify by using g_assert_cmpstr()
  blockdev-test: Factor out some common code into helpers
  blockdev-test: Test device_del after drive_del

 tests/Makefile            |   5 +-
 tests/blockdev-test.c     |  59 --------------------
 tests/drive_del-test.c    | 137 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qdev-monitor-test.c |  77 --------------------------
 4 files changed, 139 insertions(+), 139 deletions(-)
 delete mode 100644 tests/blockdev-test.c
 create mode 100644 tests/drive_del-test.c
 delete mode 100644 tests/qdev-monitor-test.c

-- 
1.9.3

[Qemu-devel] [PATCH 1/6] drive_del-test: Merge of qdev-monitor-test, blockdev-test

[Markus Armbruster]

Each of qdev-monitor-test and blockdev-test has just one test case,
and both are about drive_del.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/Makefile                                  |  5 +--
 tests/blockdev-test.c                           | 59 -------------------------
 tests/{qdev-monitor-test.c => drive_del-test.c} | 57 +++++++++++++++++++-----
 3 files changed, 47 insertions(+), 74 deletions(-)
 delete mode 100644 tests/blockdev-test.c
 rename tests/{qdev-monitor-test.c => drive_del-test.c} (56%)

diff --git a/tests/Makefile b/tests/Makefile
index 834279c..ffa8312 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -140,8 +140,7 @@ check-qtest-i386-y += tests/bios-tables-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-i386-y += tests/qdev-monitor-test$(EXESUF)
+check-qtest-i386-y += tests/drive_del-test$(EXESUF)
 check-qtest-i386-y += tests/wdt_ib700-test$(EXESUF)
 gcov-files-i386-y += hw/watchdog/watchdog.c hw/watchdog/wdt_ib700.c
 check-qtest-i386-y += $(check-qtest-pci-y)
@@ -335,7 +334,7 @@ tests/tpci200-test$(EXESUF): tests/tpci200-test.o
 tests/display-vga-test$(EXESUF): tests/display-vga-test.o
 tests/ipoctal232-test$(EXESUF): tests/ipoctal232-test.o
 tests/qom-test$(EXESUF): tests/qom-test.o
-tests/blockdev-test$(EXESUF): tests/blockdev-test.o $(libqos-pc-obj-y)
+tests/drive_del-test$(EXESUF): tests/drive_del-test.o $(libqos-pc-obj-y)
 tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o $(libqos-pc-obj-y)
 tests/nvme-test$(EXESUF): tests/nvme-test.o
 tests/pvpanic-test$(EXESUF): tests/pvpanic-test.o
diff --git a/tests/blockdev-test.c b/tests/blockdev-test.c
deleted file mode 100644
index c940e00..0000000
--- a/tests/blockdev-test.c
+++ /dev/null
@@ -1,59 +0,0 @@
-/*
- * 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("/qmp/drive_add_empty", test_drive_add_empty);
-
-    return g_test_run();
-}
diff --git a/tests/qdev-monitor-test.c b/tests/drive_del-test.c
similarity index 56%
rename from tests/qdev-monitor-test.c
rename to tests/drive_del-test.c
index e20ffd6..ba3ee12 100644
--- a/tests/qdev-monitor-test.c
+++ b/tests/drive_del-test.c
@@ -1,5 +1,5 @@
 /*
- * qdev-monitor.c test cases
+ * blockdev.c test cases
  *
  * Copyright (C) 2013 Red Hat Inc.
  *
@@ -10,12 +10,46 @@
  * See the COPYING.LIB file in the top-level directory.
  */
 
-#include <string.h>
 #include <glib.h>
+#include <string.h>
 #include "libqtest.h"
-#include "qapi/qmp/qjson.h"
 
-static void test_device_add(void)
+static void test_drive_without_dev(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();
+}
+
+static void test_after_failed_device_add(void)
 {
     QDict *response;
     QDict *error;
@@ -62,16 +96,15 @@ 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("/qmp/device_add", test_device_add);
+    qtest_add_func("/drive_del/without-dev", test_drive_without_dev);
+
+    /* TODO I guess any arch with PCI would do */
+    if (!strcmp(arch, "i386") || !strcmp(arch, "x86_64")) {
+        qtest_add_func("/drive_del/after_failed_device_add",
+                       test_after_failed_device_add);
+    }
 
     return g_test_run();
 }
-- 
1.9.3

[Qemu-devel] [PATCH 2/6] blockdev-test: Use single rather than double quotes in QMP

[Markus Armbruster]

QMP accepts both single and double quotes.  This is the only test
using double quotes.  They need to be quoted in C strings.  Replace
them by single quotes.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/drive_del-test.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
index ba3ee12..786f026 100644
--- a/tests/drive_del-test.c
+++ b/tests/drive_del-test.c
@@ -23,9 +23,9 @@ static void test_drive_without_dev(void)
     qtest_start("-drive if=none,id=drive0");
 
     /* Delete the drive */
-    response = qmp("{\"execute\": \"human-monitor-command\","
-                   " \"arguments\": {"
-                   "   \"command-line\": \"drive_del drive0\""
+    response = qmp("{'execute': 'human-monitor-command',"
+                   " 'arguments': {"
+                   "   'command-line': 'drive_del drive0'"
                    "}}");
     g_assert(response);
     response_return = qdict_get_try_str(response, "return");
@@ -36,9 +36,9 @@ static void test_drive_without_dev(void)
     /* 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\""
+    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");
@@ -59,10 +59,10 @@ static void test_after_failed_device_add(void)
     /* 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\""
+    response = qmp("{'execute': 'device_add',"
+                   " 'arguments': {"
+                   "   'driver': 'virtio-blk-pci',"
+                   "   'drive': 'drive0'"
                    "}}");
     g_assert(response);
     error = qdict_get_qdict(response, "error");
@@ -70,9 +70,9 @@ static void test_after_failed_device_add(void)
     QDECREF(response);
 
     /* Delete the drive */
-    response = qmp("{\"execute\": \"human-monitor-command\","
-                   " \"arguments\": {"
-                   "   \"command-line\": \"drive_del drive0\""
+    response = qmp("{'execute': 'human-monitor-command',"
+                   " 'arguments': {"
+                   "   'command-line': 'drive_del drive0'"
                    "}}");
     g_assert(response);
     g_assert_cmpstr(qdict_get_try_str(response, "return"), ==, "");
@@ -81,9 +81,9 @@ static void test_after_failed_device_add(void)
     /* 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\""
+    response = qmp("{'execute': 'human-monitor-command',"
+                   " 'arguments': {"
+                   "   'command-line': 'drive_add pci-addr=auto if=none,id=drive0'"
                    "}}");
     g_assert(response);
     g_assert_cmpstr(qdict_get_try_str(response, "return"), ==, "OK\r\n");
-- 
1.9.3

[Qemu-devel] [PATCH 3/6] blockdev-test: Clean up bogus drive_add argument

[Markus Armbruster]

The first argument should be a PCI address, which pci-addr=auto isn't.
Doesn't really matter, as drive_add ignores its first argument when
its second argument has if=none.  Clean it up anyway.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/drive_del-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
index 786f026..67309fd 100644
--- a/tests/drive_del-test.c
+++ b/tests/drive_del-test.c
@@ -83,7 +83,7 @@ static void test_after_failed_device_add(void)
      */
     response = qmp("{'execute': 'human-monitor-command',"
                    " 'arguments': {"
-                   "   'command-line': 'drive_add pci-addr=auto if=none,id=drive0'"
+                   "   'command-line': 'drive_add 0 if=none,id=drive0'"
                    "}}");
     g_assert(response);
     g_assert_cmpstr(qdict_get_try_str(response, "return"), ==, "OK\r\n");
-- 
1.9.3

[Qemu-devel] [PATCH 4/6] blockdev-test: Simplify by using g_assert_cmpstr()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/drive_del-test.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
index 67309fd..2baf0b7 100644
--- a/tests/drive_del-test.c
+++ b/tests/drive_del-test.c
@@ -17,7 +17,6 @@
 static void test_drive_without_dev(void)
 {
     QDict *response;
-    const char *response_return;
 
     /* Start with an empty drive */
     qtest_start("-drive if=none,id=drive0");
@@ -28,9 +27,7 @@ static void test_drive_without_dev(void)
                    "   '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);
+    g_assert_cmpstr(qdict_get_try_str(response, "return"), ==, "");
     QDECREF(response);
 
     /* Ensure re-adding the drive works - there should be no duplicate ID error
@@ -41,9 +38,7 @@ static void test_drive_without_dev(void)
                    "   '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);
+    g_assert_cmpstr(qdict_get_try_str(response, "return"), ==, "OK\r\n");
     QDECREF(response);
 
     qtest_end();
-- 
1.9.3

[Qemu-devel] [PATCH 5/6] blockdev-test: Factor out some common code into helpers

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/drive_del-test.c | 60 +++++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
index 2baf0b7..463479d 100644
--- a/tests/drive_del-test.c
+++ b/tests/drive_del-test.c
@@ -14,32 +14,44 @@
 #include <string.h>
 #include "libqtest.h"
 
+static void drive_add(void)
+{
+    QDict *response;
+
+    response = qmp("{'execute': 'human-monitor-command',"
+                   " 'arguments': {"
+                   "   'command-line': 'drive_add 0 if=none,id=drive0'"
+                   "}}");
+    g_assert(response);
+    g_assert_cmpstr(qdict_get_try_str(response, "return"), ==, "OK\r\n");
+    QDECREF(response);
+}
+
+static void drive_del(void)
+{
+    QDict *response;
+
+    response = qmp("{'execute': 'human-monitor-command',"
+                   " 'arguments': {"
+                   "   'command-line': 'drive_del drive0'"
+                   "}}");
+    g_assert(response);
+    g_assert_cmpstr(qdict_get_try_str(response, "return"), ==, "");
+    QDECREF(response);
+}
+
 static void test_drive_without_dev(void)
 {
-    QDict *response;
-
     /* 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);
-    g_assert_cmpstr(qdict_get_try_str(response, "return"), ==, "");
-    QDECREF(response);
+    drive_del();
 
     /* 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);
-    g_assert_cmpstr(qdict_get_try_str(response, "return"), ==, "OK\r\n");
-    QDECREF(response);
+    drive_add();
 
     qtest_end();
 }
@@ -65,24 +77,12 @@ static void test_after_failed_device_add(void)
     QDECREF(response);
 
     /* Delete the drive */
-    response = qmp("{'execute': 'human-monitor-command',"
-                   " 'arguments': {"
-                   "   'command-line': 'drive_del drive0'"
-                   "}}");
-    g_assert(response);
-    g_assert_cmpstr(qdict_get_try_str(response, "return"), ==, "");
-    QDECREF(response);
+    drive_del();
 
     /* 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 0 if=none,id=drive0'"
-                   "}}");
-    g_assert(response);
-    g_assert_cmpstr(qdict_get_try_str(response, "return"), ==, "OK\r\n");
-    QDECREF(response);
+    drive_add();
 
     qtest_end();
 }
-- 
1.9.3

[Qemu-devel] [PATCH 6/6] blockdev-test: Test device_del after drive_del

[Markus Armbruster]

Executed in this order, drive_del and device_del's automatic drive
deletion take notoriously tricky special paths.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/drive_del-test.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
index 463479d..9ac3d48 100644
--- a/tests/drive_del-test.c
+++ b/tests/drive_del-test.c
@@ -40,6 +40,19 @@ static void drive_del(void)
     QDECREF(response);
 }
 
+static void device_del(void)
+{
+    QDict *response;
+
+    /* Complication: ignore DEVICE_DELETED event */
+    qmp_discard_response("{'execute': 'device_del',"
+                         " 'arguments': { 'id': 'dev0' } }");
+    response = qmp_receive();
+    g_assert(response);
+    g_assert(qdict_haskey(response, "return"));
+    QDECREF(response);
+}
+
 static void test_drive_without_dev(void)
 {
     /* Start with an empty drive */
@@ -87,6 +100,23 @@ static void test_after_failed_device_add(void)
     qtest_end();
 }
 
+static void test_drive_del_device_del(void)
+{
+    /* Start with a drive used by an device that unplugs instantaneously */
+    qtest_start("-drive if=none,id=drive0,file=/dev/null"
+                " -device virtio-scsi-pci"
+                " -device scsi-hd,drive=drive0,id=dev0");
+
+    /*
+     * Delete the drive, and then the device
+     * Doing it in this order takes notoriously tricky special paths
+     */
+    drive_del();
+    device_del();
+
+    qtest_end();
+}
+
 int main(int argc, char **argv)
 {
     const char *arch = qtest_get_arch();
@@ -99,6 +129,8 @@ int main(int argc, char **argv)
     if (!strcmp(arch, "i386") || !strcmp(arch, "x86_64")) {
         qtest_add_func("/drive_del/after_failed_device_add",
                        test_after_failed_device_add);
+        qtest_add_func("/blockdev/drive_del_device_del",
+                       test_drive_del_device_del);
     }
 
     return g_test_run();
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/6] blockdev-test: Use single rather than double quotes in QMP

[Eric Blake]

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

On 10/02/2014 08:51 AM, Markus Armbruster wrote:
> QMP accepts both single and double quotes.  This is the only test
> using double quotes.  They need to be quoted in C strings.  Replace
> them by single quotes.

Ooh, the use of single quotes on input is undocumented, and a violation
of JSON. We always output double quotes, and I've been relying on
.json/.hx files using single quotes for QAPI vs. double quotes for QMP
examples.  What would break if we tightened up our input parser to
forbid non-JSON inputs and mandate that QMP use double quotes, bringing
us into compliance with our docs (docs/qmp/README)?

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/drive_del-test.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)

If we WANT to keep undocumented support for single quotes (and/or
document our extension to JSON), then this is fine; otherwise, I'd
prefer that we use double quotes in QMP.

But if we decide single quotes are tolerable,

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: 539 bytes --]

Re: [Qemu-devel] [PATCH 1/6] drive_del-test: Merge of qdev-monitor-test, blockdev-test

[Eric Blake]

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

On 10/02/2014 08:51 AM, Markus Armbruster wrote:
> Each of qdev-monitor-test and blockdev-test has just one test case,
> and both are about drive_del.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/Makefile                                  |  5 +--
>  tests/blockdev-test.c                           | 59 -------------------------
>  tests/{qdev-monitor-test.c => drive_del-test.c} | 57 +++++++++++++++++++-----
>  3 files changed, 47 insertions(+), 74 deletions(-)
>  delete mode 100644 tests/blockdev-test.c
>  rename tests/{qdev-monitor-test.c => drive_del-test.c} (56%)

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


> --- a/tests/qdev-monitor-test.c
> +++ b/tests/drive_del-test.c
> @@ -1,5 +1,5 @@
>  /*
> - * qdev-monitor.c test cases
> + * blockdev.c test cases
>   *
>   * Copyright (C) 2013 Red Hat Inc.

Worth bumping this to include 2014 while at it?

-- 
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: 539 bytes --]

Re: [Qemu-devel] [PATCH 3/6] blockdev-test: Clean up bogus drive_add argument

[Eric Blake]

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

On 10/02/2014 08:51 AM, Markus Armbruster wrote:
> The first argument should be a PCI address, which pci-addr=auto isn't.
> Doesn't really matter, as drive_add ignores its first argument when
> its second argument has if=none.  Clean it up anyway.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/drive_del-test.c | 2 +-
>  1 file changed, 1 insertion(+), 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: 539 bytes --]

Re: [Qemu-devel] [PATCH 4/6] blockdev-test: Simplify by using g_assert_cmpstr()

[Eric Blake]

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

On 10/02/2014 08:51 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/drive_del-test.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 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: 539 bytes --]

Re: [Qemu-devel] [PATCH 5/6] blockdev-test: Factor out some common code into helpers

[Eric Blake]

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

On 10/02/2014 08:51 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/drive_del-test.c | 60 +++++++++++++++++++++++++-------------------------
>  1 file changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
> index 2baf0b7..463479d 100644
> --- a/tests/drive_del-test.c
> +++ b/tests/drive_del-test.c
> @@ -14,32 +14,44 @@
>  #include <string.h>
>  #include "libqtest.h"
>  
> +static void drive_add(void)
> +{
> +    QDict *response;
> +
> +    response = qmp("{'execute': 'human-monitor-command',"
> +                   " 'arguments': {"
> +                   "   'command-line': 'drive_add 0 if=none,id=drive0'"
> +                   "}}");

I'm still not necessarily a fan of '' in QMP, but this follows existing
practice.

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: 539 bytes --]

Re: [Qemu-devel] [PATCH 6/6] blockdev-test: Test device_del after drive_del

[Eric Blake]

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

On 10/02/2014 08:51 AM, Markus Armbruster wrote:
> Executed in this order, drive_del and device_del's automatic drive
> deletion take notoriously tricky special paths.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/drive_del-test.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c

> +    /* Complication: ignore DEVICE_DELETED event */
> +    qmp_discard_response("{'execute': 'device_del',"

Still not a fan of single quotes in QMP, but it's existing style.


> +static void test_drive_del_device_del(void)
> +{
> +    /* Start with a drive used by an device that unplugs instantaneously */

s/an device/a device/

New tests are always good; with the typo fix:
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: 539 bytes --]

Re: [Qemu-devel] [PATCH 1/6] drive_del-test: Merge of qdev-monitor-test, blockdev-test

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 10/02/2014 08:51 AM, Markus Armbruster wrote:
>> Each of qdev-monitor-test and blockdev-test has just one test case,
>> and both are about drive_del.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/Makefile                                  |  5 +--
>>  tests/blockdev-test.c | 59 -------------------------
>>  tests/{qdev-monitor-test.c => drive_del-test.c} | 57 +++++++++++++++++++-----
>>  3 files changed, 47 insertions(+), 74 deletions(-)
>>  delete mode 100644 tests/blockdev-test.c
>>  rename tests/{qdev-monitor-test.c => drive_del-test.c} (56%)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>
>> --- a/tests/qdev-monitor-test.c
>> +++ b/tests/drive_del-test.c
>> @@ -1,5 +1,5 @@
>>  /*
>> - * qdev-monitor.c test cases
>> + * blockdev.c test cases
>>   *
>>   * Copyright (C) 2013 Red Hat Inc.
>
> Worth bumping this to include 2014 while at it?

Sure, if I have to respin anyway.

Thanks!

Re: [Qemu-devel] [PATCH 2/6] blockdev-test: Use single rather than double quotes in QMP

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 10/02/2014 08:51 AM, Markus Armbruster wrote:
>> QMP accepts both single and double quotes.  This is the only test
>> using double quotes.  They need to be quoted in C strings.  Replace
>> them by single quotes.
>
> Ooh, the use of single quotes on input is undocumented, and a violation
> of JSON. We always output double quotes, and I've been relying on
> .json/.hx files using single quotes for QAPI vs. double quotes for QMP
> examples.  What would break if we tightened up our input parser to
> forbid non-JSON inputs and mandate that QMP use double quotes, bringing
> us into compliance with our docs (docs/qmp/README)?

A whole bunch of tests.  Easily fixable.

No telling what out-of-tree stuff breaks.  Maybe even nothing.

Perhaps Anthony (cc'ed) can advise.

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/drive_del-test.c | 32 ++++++++++++++++----------------
>>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> If we WANT to keep undocumented support for single quotes (and/or
> document our extension to JSON), then this is fine; otherwise, I'd
> prefer that we use double quotes in QMP.
>
> But if we decide single quotes are tolerable,
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 6/6] blockdev-test: Test device_del after drive_del

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 10/02/2014 08:51 AM, Markus Armbruster wrote:
>> Executed in this order, drive_del and device_del's automatic drive
>> deletion take notoriously tricky special paths.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/drive_del-test.c | 32 ++++++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>> 
>> diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
>
>> +    /* Complication: ignore DEVICE_DELETED event */
>> +    qmp_discard_response("{'execute': 'device_del',"
>
> Still not a fan of single quotes in QMP, but it's existing style.
>
>
>> +static void test_drive_del_device_del(void)
>> +{
>> +    /* Start with a drive used by an device that unplugs instantaneously */
>
> s/an device/a device/
>
> New tests are always good; with the typo fix:
> Reviewed-by: Eric Blake <eblake@redhat.com>

I'll get that fixed, either on commit, or via respin.  Thanks!

Re: [Qemu-devel] [PATCH 0/6] Improve drive_del test coverage

[Stefan Hajnoczi]

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

On Thu, Oct 02, 2014 at 04:51:30PM +0200, Markus Armbruster wrote:
> The meat is in the last patch, the others are just cleanup.
> 
> Markus Armbruster (6):
>   drive_del-test: Merge of qdev-monitor-test, blockdev-test
>   blockdev-test: Use single rather than double quotes in QMP
>   blockdev-test: Clean up bogus drive_add argument
>   blockdev-test: Simplify by using g_assert_cmpstr()
>   blockdev-test: Factor out some common code into helpers
>   blockdev-test: Test device_del after drive_del
> 
>  tests/Makefile            |   5 +-
>  tests/blockdev-test.c     |  59 --------------------
>  tests/drive_del-test.c    | 137 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qdev-monitor-test.c |  77 --------------------------
>  4 files changed, 139 insertions(+), 139 deletions(-)
>  delete mode 100644 tests/blockdev-test.c
>  create mode 100644 tests/drive_del-test.c
>  delete mode 100644 tests/qdev-monitor-test.c

I made the copyright year and typo fixups when applying the patches.

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

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