[Qemu-devel] [PATCH v4 0/4] test virtio-blk hotplug

[Qemu-devel] [PATCH v4 0/4] test virtio-blk hotplug

[Amos Kong]

It's worth to add a hotplug test to qtest, but without
cooperation of guest OS, new devices can't be initialized
by guest, and hot-unplug doesn't work.

However, the new test can cover some part of code of
hotplug/unplug.

I will write another subtest to test hotplug with pci support.

V2: move qmp_exec_hmp_cmd() to libqtest.c
    excape hmp cmd (stefanha)
    use qmp_exec_hmp_cmd() in blockdev-test
V3: use vp_list to format string, free escaped string
V4: free escaped string by g_free()

Amos Kong (4):
  virtio-blk-test.c: change pci_nop() to virtblk_init()
  qtest: introduce qmp_exec_hmp_cmd()
  virtio-blk-test.c: add hotplug subtest
  qtest: use qmp_exec_hmp_cmd() in blockdev-test

 tests/blockdev-test.c   | 23 ++---------------------
 tests/libqtest.c        | 26 ++++++++++++++++++++++++++
 tests/libqtest.h        |  9 +++++++++
 tests/virtio-blk-test.c | 44 +++++++++++++++++++++++++++++++++++++-------
 4 files changed, 74 insertions(+), 28 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v4 1/4] virtio-blk-test.c: change pci_nop() to virtblk_init()

[Amos Kong]

I want to add a new subtest in virtio-blk-test.c, it will start
guest without network. The original pci_init() did nothing, but
it's good to reserve a very simple initialization testing.

Signed-off-by: Amos Kong <akong@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/virtio-blk-test.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index d53f875..0fdec01 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -12,9 +12,12 @@
 #include "libqtest.h"
 #include "qemu/osdep.h"
 
-/* Tests only initialization so far. TODO: Replace with functional tests */
-static void pci_nop(void)
+/* Tests only initialization */
+static void virtblk_init(void)
 {
+    qtest_start("-drive id=drv0,if=none,file=/dev/null "
+                "-device virtio-blk-pci,drive=drv0");
+    qtest_end();
 }
 
 int main(int argc, char **argv)
@@ -22,13 +25,9 @@ int main(int argc, char **argv)
     int ret;
 
     g_test_init(&argc, &argv, NULL);
-    qtest_add_func("/virtio/blk/pci/nop", pci_nop);
+    qtest_add_func("/virtio/blk/pci/init", virtblk_init);
 
-    qtest_start("-drive id=drv0,if=none,file=/dev/null "
-                "-device virtio-blk-pci,drive=drv0");
     ret = g_test_run();
 
-    qtest_end();
-
     return ret;
 }
-- 
1.9.3

[Qemu-devel] [PATCH v4 2/4] qtest: introduce qmp_exec_hmp_cmd()

[Amos Kong]

This patch wraps a helper function to execute human command by
one QMP command (human-monitor-command). It also checks the return
string.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 tests/libqtest.c | 26 ++++++++++++++++++++++++++
 tests/libqtest.h |  9 +++++++++
 2 files changed, 35 insertions(+)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 71468ac..90babc9 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -646,3 +646,29 @@ void qmp_discard_response(const char *fmt, ...)
     qtest_qmpv_discard_response(global_qtest, fmt, ap);
     va_end(ap);
 }
+
+void qmp_exec_hmp_cmd(const char *expected_ret, const char *fmt, ...)
+{
+    va_list ap;
+    char cmd[1024];
+    char *escaped_cmd;
+    QDict *response;
+    const char *response_return;
+
+    va_start(ap, fmt);
+    vsprintf(cmd, fmt, ap);
+    va_end(ap);
+
+    escaped_cmd = g_strescape(cmd, NULL);
+    response = qmp("{\"execute\": \"human-monitor-command\","
+                   " \"arguments\": {"
+                   "   \"command-line\": \"%s\""
+                   "}}", escaped_cmd);
+    g_free(escaped_cmd);
+
+    g_assert(response);
+    response_return = qdict_get_try_str(response, "return");
+    g_assert(response_return);
+    g_assert_cmpstr(response_return, ==, expected_ret);
+    QDECREF(response);
+}
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 8f323c7..d2959d3 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -375,6 +375,15 @@ QDict *qmp(const char *fmt, ...);
 void qmp_discard_response(const char *fmt, ...);
 
 /**
+ * qmp_exec_hmp_cmd:
+ * @expected_ret: expected return string
+ * @fmt...: HMP command to execute
+ *
+ * Executes HMP command by 'human-monitor-command'.
+ */
+void qmp_exec_hmp_cmd(const char *expected_ret, const char *fmt, ...);
+
+/**
  * qmp_receive:
  *
  * Reads a QMP message from QEMU and returns the response.
-- 
1.9.3

[Qemu-devel] [PATCH v4 3/4] virtio-blk-test.c: add hotplug subtest

[Amos Kong]

This patch adds a new subtest, it hotplugs 29 * 8 = 232 virtio-blk
devices to guest, and try to hot-unplug them.

Note: the hot-unplug can't work without cooperation of guest OS.

Signed-off-by: Amos Kong <akong@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/virtio-blk-test.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 0fdec01..7358203 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -7,11 +7,41 @@
  * See the COPYING file in the top-level directory.
  */
 
+#include <stdio.h>
 #include <glib.h>
 #include <string.h>
 #include "libqtest.h"
 #include "qemu/osdep.h"
 
+static void test_blk_hotplug(void)
+{
+    int i, j;
+
+    /* start with no network/block device, slots 3~0x1f are free */
+    qtest_start("-net none");
+
+    for (i = 3; i <= 0x1f; i++) {
+        for (j = 7; j >= 0; j--) {
+            qmp_exec_hmp_cmd("OK\r\n",
+                             "drive_add 0 if=none,file=/dev/null,id=drv-%x.%x",
+                             i, j);
+            qmp_exec_hmp_cmd("",
+                       "device_add virtio-blk-pci,id=dev-%x.%x,drive=drv-%x.%x,"
+                       "addr=0x%x.%x,multifunction=on", i, j, i, j, i, j);
+        }
+    }
+
+    /* hot-unplug doesn't work without cooperation of guest OS */
+    for (i = 3; i <= 0x1f; i++) {
+        for (j = 7; j >= 0; j--) {
+            qmp_exec_hmp_cmd("", "drive_del drv-%x.%x", i, j);
+            qmp_exec_hmp_cmd("", "device_del dev-%x.%x", i, j);
+        }
+    }
+
+    qtest_end();
+}
+
 /* Tests only initialization */
 static void virtblk_init(void)
 {
@@ -26,6 +56,7 @@ int main(int argc, char **argv)
 
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/virtio/blk/pci/init", virtblk_init);
+    qtest_add_func("/virtio/blk/pci/hotplug", test_blk_hotplug);
 
     ret = g_test_run();
 
-- 
1.9.3

[Qemu-devel] [PATCH v4 4/4] qtest: use qmp_exec_hmp_cmd() in blockdev-test

[Amos Kong]

Signed-off-by: Amos Kong <akong@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/blockdev-test.c | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/tests/blockdev-test.c b/tests/blockdev-test.c
index c940e00..c9127c0 100644
--- a/tests/blockdev-test.c
+++ b/tests/blockdev-test.c
@@ -16,35 +16,16 @@
 
 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);
+    qmp_exec_hmp_cmd("", "drive_del drive0");
 
     /* 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);
+    qmp_exec_hmp_cmd("OK\r\n", "drive_add 0 if=none,id=drive0");
 
     qtest_end();
 }
-- 
1.9.3

Re: [Qemu-devel] [PATCH v4 0/4] test virtio-blk hotplug

[Stefan Hajnoczi]

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

On Fri, Jun 06, 2014 at 10:33:49PM +0800, Amos Kong wrote:
> It's worth to add a hotplug test to qtest, but without
> cooperation of guest OS, new devices can't be initialized
> by guest, and hot-unplug doesn't work.
> 
> However, the new test can cover some part of code of
> hotplug/unplug.
> 
> I will write another subtest to test hotplug with pci support.
> 
> V2: move qmp_exec_hmp_cmd() to libqtest.c
>     excape hmp cmd (stefanha)
>     use qmp_exec_hmp_cmd() in blockdev-test
> V3: use vp_list to format string, free escaped string
> V4: free escaped string by g_free()
> 
> Amos Kong (4):
>   virtio-blk-test.c: change pci_nop() to virtblk_init()
>   qtest: introduce qmp_exec_hmp_cmd()
>   virtio-blk-test.c: add hotplug subtest
>   qtest: use qmp_exec_hmp_cmd() in blockdev-test
> 
>  tests/blockdev-test.c   | 23 ++---------------------
>  tests/libqtest.c        | 26 ++++++++++++++++++++++++++
>  tests/libqtest.h        |  9 +++++++++
>  tests/virtio-blk-test.c | 44 +++++++++++++++++++++++++++++++++++++-------
>  4 files changed, 74 insertions(+), 28 deletions(-)
> 
> -- 
> 1.9.3
> 

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

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

Re: [Qemu-devel] [PATCH v4 0/4] test virtio-blk hotplug

[Amos Kong]

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

On Mon, Jun 09, 2014 at 03:22:51PM +0200, Stefan Hajnoczi wrote:
> On Fri, Jun 06, 2014 at 10:33:49PM +0800, Amos Kong wrote:
> > It's worth to add a hotplug test to qtest, but without
> > cooperation of guest OS, new devices can't be initialized
> > by guest, and hot-unplug doesn't work.
> > 
> > However, the new test can cover some part of code of
> > hotplug/unplug.
> > 
> > I will write another subtest to test hotplug with pci support.
> > 
> > V2: move qmp_exec_hmp_cmd() to libqtest.c
> >     excape hmp cmd (stefanha)
> >     use qmp_exec_hmp_cmd() in blockdev-test
> > V3: use vp_list to format string, free escaped string
> > V4: free escaped string by g_free()

Hi Andreas,

Can you apply this patchset to your tree?

> > Amos Kong (4):
> >   virtio-blk-test.c: change pci_nop() to virtblk_init()
> >   qtest: introduce qmp_exec_hmp_cmd()
> >   virtio-blk-test.c: add hotplug subtest
> >   qtest: use qmp_exec_hmp_cmd() in blockdev-test
> > 
> >  tests/blockdev-test.c   | 23 ++---------------------
> >  tests/libqtest.c        | 26 ++++++++++++++++++++++++++
> >  tests/libqtest.h        |  9 +++++++++
> >  tests/virtio-blk-test.c | 44 +++++++++++++++++++++++++++++++++++++-------
> >  4 files changed, 74 insertions(+), 28 deletions(-)
> > 
> > -- 
> > 1.9.3
> > 
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

-- 
			Amos.

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

Re: [Qemu-devel] [PATCH v4 2/4] qtest: introduce qmp_exec_hmp_cmd()

[Andreas Färber]

Am 06.06.2014 16:33, schrieb Amos Kong:
> This patch wraps a helper function to execute human command by
> one QMP command (human-monitor-command). It also checks the return
> string.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>

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

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 v4 3/4] virtio-blk-test.c: add hotplug subtest

[Andreas Färber]

Am 06.06.2014 16:33, schrieb Amos Kong:
> This patch adds a new subtest, it hotplugs 29 * 8 = 232 virtio-blk
> devices to guest, and try to hot-unplug them.
> 
> Note: the hot-unplug can't work without cooperation of guest OS.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/virtio-blk-test.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
> index 0fdec01..7358203 100644
> --- a/tests/virtio-blk-test.c
> +++ b/tests/virtio-blk-test.c
> @@ -7,11 +7,41 @@
>   * See the COPYING file in the top-level directory.
>   */
>  
> +#include <stdio.h>
>  #include <glib.h>
>  #include <string.h>
>  #include "libqtest.h"
>  #include "qemu/osdep.h"
>  
> +static void test_blk_hotplug(void)
> +{
> +    int i, j;
> +
> +    /* start with no network/block device, slots 3~0x1f are free */

"3-0x1f" or "3 to 0x1f"?

> +    qtest_start("-net none");
> +
> +    for (i = 3; i <= 0x1f; i++) {
> +        for (j = 7; j >= 0; j--) {
> +            qmp_exec_hmp_cmd("OK\r\n",
> +                             "drive_add 0 if=none,file=/dev/null,id=drv-%x.%x",
> +                             i, j);
> +            qmp_exec_hmp_cmd("",
> +                       "device_add virtio-blk-pci,id=dev-%x.%x,drive=drv-%x.%x,"
> +                       "addr=0x%x.%x,multifunction=on", i, j, i, j, i, j);

Why are you using HMP-via-QMP here and not QMP directly?

> +        }
> +    }
> +
> +    /* hot-unplug doesn't work without cooperation of guest OS */
> +    for (i = 3; i <= 0x1f; i++) {
> +        for (j = 7; j >= 0; j--) {

While the function is still small, using a define or static const would
be a small improvement. :) Could be a follow-up of course.

Test looks good, thanks for your effort.

Regards,
Andreas

> +            qmp_exec_hmp_cmd("", "drive_del drv-%x.%x", i, j);
> +            qmp_exec_hmp_cmd("", "device_del dev-%x.%x", i, j);
> +        }
> +    }
> +
> +    qtest_end();
> +}
> +
>  /* Tests only initialization */
>  static void virtblk_init(void)
>  {
> @@ -26,6 +56,7 @@ int main(int argc, char **argv)
>  
>      g_test_init(&argc, &argv, NULL);
>      qtest_add_func("/virtio/blk/pci/init", virtblk_init);
> +    qtest_add_func("/virtio/blk/pci/hotplug", test_blk_hotplug);
>  
>      ret = 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 v4 4/4] qtest: use qmp_exec_hmp_cmd() in blockdev-test

[Andreas Färber]

Am 06.06.2014 16:33, schrieb Amos Kong:
> Signed-off-by: Amos Kong <akong@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Surely makes sense,

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

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 v4 0/4] test virtio-blk hotplug

[Andreas Färber]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Amos,

Am 17.06.2014 14:54, schrieb Amos Kong:
> On Mon, Jun 09, 2014 at 03:22:51PM +0200, Stefan Hajnoczi wrote:
>> On Fri, Jun 06, 2014 at 10:33:49PM +0800, Amos Kong wrote:
>>> It's worth to add a hotplug test to qtest, but without 
>>> cooperation of guest OS, new devices can't be initialized by
>>> guest, and hot-unplug doesn't work.
>>> 
>>> However, the new test can cover some part of code of 
>>> hotplug/unplug.
>>> 
>>> I will write another subtest to test hotplug with pci support.
>>> 
>>> V2: move qmp_exec_hmp_cmd() to libqtest.c excape hmp cmd
>>> (stefanha) use qmp_exec_hmp_cmd() in blockdev-test V3: use
>>> vp_list to format string, free escaped string V4: free escaped
>>> string by g_free()
> 
> Hi Andreas,
> 
> Can you apply this patchset to your tree?

Sorry for the late reply, I was on vacation and am still fighting a
mail backlog...

I had a question about the new HMP helper function - I see in 4/4 that
drive_add was already being done via HMP, so I guess that's the
culprit, but still I wonder whether we can do device-add via QMP.

That said, I've reviewed the generic qtest part OK, and would assume
virtio-blk-test (you can drop the .c in the topic please) to go
through the same tree virtio-blk does, so via Stefan/Kevin.

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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJToESEAAoJEPou0S0+fgE/fbAP/Ainb27oOKTtkQwslqWeFWrq
p/HS+IJ/mw1FJl7THu0lH9nUtSUhxf86sWa0zU1q7EYmc0Pl/ZqxZnK28R9ebsVg
5Fkij4Ze6nPwngbu8vOJ1DiI6yjxQfEteZpt3vS3ThtE8A1lC1iM5htiNdFsfCgj
08C7NR4+jQUGKZlSXtRYfdAR/LHyS7vtFscxCx4k4Erx3+R5hXbdFovkXz31PbJI
rn439DeQTnkpBNmfr/S6S74WSKAwhZASOKN+d0jJ+RIjAqU/bFZEhE6LPsZ/MnVg
GD2MZl+diLk44nr66BCK3Q/qiNS8rkSZ9UgvCo1SuwgSeo48f4MAtrEDfP0QS3Kz
zo+nHkHtJlfWvbGC2US2xaHAaeQU/JpNa+dr5PBcM6Bwj5d1xQ2TVJmid7UzhZEC
eB3KL5Za8gYIkzRBZqfH4cpiaABC8SFBu90ACmFMxRAydOqlK7fOXK761gGcaaif
1JJ0qk4InWG3PBR0N/YWX14nik5SdCsAcnjoulLuxSZ2HtJ3Rz3e4yII0qdjQsmB
NMp9vLIVY+YlOwD2qzCS+4JOxkKlEF9fplsNwxelOKv28Ge2WCwcgiOySiVghUqn
cVNpPrP7jgALFHMlnbQomyvT7Cu9GoF0ScJO3IpAXManSQ5j8BaDxkt3ZbwiT5kT
ixvewd2AeNN9o/q1mZWq
=9qZC
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH v4 2/4] qtest: introduce qmp_exec_hmp_cmd()

[Paolo Bonzini]

Il 06/06/2014 16:33, Amos Kong ha scritto:
> +    va_end(ap);
> +
> +    escaped_cmd = g_strescape(cmd, NULL);
> +    response = qmp("{\"execute\": \"human-monitor-command\","
> +                   " \"arguments\": {"
> +                   "   \"command-line\": \"%s\""
> +                   "}}", escaped_cmd);
> +    g_free(escaped_cmd);

Instead of adding g_strescape everywhere, we should use json-parser's 
own interpolation support.  See this patch: 
http://article.gmane.org/gmane.comp.emulators.qemu/279836 which also 
fixes a leak as a bonus.

Also, you can use ' instead of " if you fix another long-standing bug: 
http://article.gmane.org/gmane.comp.emulators.qemu/279835

Paolo

Re: [Qemu-devel] [PATCH v4 0/4] test virtio-blk hotplug

[Amos Kong]

On Tue, Jun 17, 2014 at 03:37:08PM +0200, Andreas Färber wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Hi Amos,
> 
> Am 17.06.2014 14:54, schrieb Amos Kong:
> > On Mon, Jun 09, 2014 at 03:22:51PM +0200, Stefan Hajnoczi wrote:
> >> On Fri, Jun 06, 2014 at 10:33:49PM +0800, Amos Kong wrote:
> >>> It's worth to add a hotplug test to qtest, but without 
> >>> cooperation of guest OS, new devices can't be initialized by
> >>> guest, and hot-unplug doesn't work.
> >>> 
> >>> However, the new test can cover some part of code of 
> >>> hotplug/unplug.
> >>> 
> >>> I will write another subtest to test hotplug with pci support.
> >>> 
> >>> V2: move qmp_exec_hmp_cmd() to libqtest.c excape hmp cmd
> >>> (stefanha) use qmp_exec_hmp_cmd() in blockdev-test V3: use
> >>> vp_list to format string, free escaped string V4: free escaped
> >>> string by g_free()
> > 
> > Hi Andreas,
> > 
> > Can you apply this patchset to your tree?
> 
> Sorry for the late reply, I was on vacation and am still fighting a
> mail backlog...

That's OK :)
 
> I had a question about the new HMP helper function - I see in 4/4 that
> drive_add was already being done via HMP, so I guess that's the
> culprit, but still I wonder whether we can do device-add via QMP.

I didn't find device_add/del definition in qapi-schema.json, so I
used HMP instead. Actually those two QMP commands exist and work.

{ "execute": "blockdev-add", "arguments": { "options": { "driver":
  "file", "filename": "/dev/null", "id": "id1" } } }

{ "execute": "device_add", "arguments": { "driver": "virtio-blk-pci",
  "drive": "id1", "driver": "virtio-blk-pci", "id": "id2" } }

I will convert virtio-blk-test to use QMP, send a V5.
 
> That said, I've reviewed the generic qtest part OK, and would assume
> virtio-blk-test (you can drop the .c in the topic please) to go
> through the same tree virtio-blk does, so via Stefan/Kevin.

Stefan said (offline) it belongs to you, and I found the first commit of
this file was merged by you.
 
Thanks.

> Regards,
> Andreas

-- 
			Amos.

Re: [Qemu-devel] [PATCH v4 0/4] test virtio-blk hotplug

[Stefan Hajnoczi]

On Wed, Jun 18, 2014 at 10:58 AM, Amos Kong <akong@redhat.com> wrote:
> On Tue, Jun 17, 2014 at 03:37:08PM +0200, Andreas Färber wrote:
>> That said, I've reviewed the generic qtest part OK, and would assume
>> virtio-blk-test (you can drop the .c in the topic please) to go
>> through the same tree virtio-blk does, so via Stefan/Kevin.
>
> Stefan said (offline) it belongs to you, and I found the first commit of
> this file was merged by you.

The important thing is that both Andreas and I/Kevin review it.  I
don't mind merging it once Andreas has given his Reviewed-by.

Stefan

Re: [Qemu-devel] [PATCH v4 0/4] test virtio-blk hotplug

[Amos Kong]

On Wed, Jun 18, 2014 at 10:58:52AM +0800, Amos Kong wrote:
> On Tue, Jun 17, 2014 at 03:37:08PM +0200, Andreas Färber wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> > 
> > Hi Amos,
> > 
> > Am 17.06.2014 14:54, schrieb Amos Kong:
> > > On Mon, Jun 09, 2014 at 03:22:51PM +0200, Stefan Hajnoczi wrote:
> > >> On Fri, Jun 06, 2014 at 10:33:49PM +0800, Amos Kong wrote:
> > >>> It's worth to add a hotplug test to qtest, but without 
> > >>> cooperation of guest OS, new devices can't be initialized by
> > >>> guest, and hot-unplug doesn't work.
> > >>> 
> > >>> However, the new test can cover some part of code of 
> > >>> hotplug/unplug.
> > >>> 
> > >>> I will write another subtest to test hotplug with pci support.
> > >>> 
> > >>> V2: move qmp_exec_hmp_cmd() to libqtest.c excape hmp cmd
> > >>> (stefanha) use qmp_exec_hmp_cmd() in blockdev-test V3: use
> > >>> vp_list to format string, free escaped string V4: free escaped
> > >>> string by g_free()
> > > 
> > > Hi Andreas,
> > > 
> > > Can you apply this patchset to your tree?
> > 
> > Sorry for the late reply, I was on vacation and am still fighting a
> > mail backlog...
> 
> That's OK :)
>  
> > I had a question about the new HMP helper function - I see in 4/4 that
> > drive_add was already being done via HMP, so I guess that's the
> > culprit, but still I wonder whether we can do device-add via QMP.
> 
> I didn't find device_add/del definition in qapi-schema.json, so I
> used HMP instead. Actually those two QMP commands exist and work.
> 
> { "execute": "blockdev-add", "arguments": { "options": { "driver":
>   "file", "filename": "/dev/null", "id": "id1" } } }
> 
> { "execute": "device_add", "arguments": { "driver": "virtio-blk-pci",
>   "drive": "id1", "driver": "virtio-blk-pci", "id": "id2" } }
> 
> I will convert virtio-blk-test to use QMP, send a V5.

We have QMP command to hot-unplug device:

{ "execute": "device_del", "arguments": { "id": "id2" } }

But there is not QMP command to hot-unplug drive/blockdev

drive_del / blockdev-del doesn't exist, so I will use
human-monitor-command to hot-unplug blockdev.
  
> > That said, I've reviewed the generic qtest part OK, and would assume
> > virtio-blk-test (you can drop the .c in the topic please) to go
> > through the same tree virtio-blk does, so via Stefan/Kevin.
> 
> Stefan said (offline) it belongs to you, and I found the first commit of
> this file was merged by you.
>  
> Thanks.
> 
> > Regards,
> > Andreas
> 
> -- 
> 			Amos.

-- 
			Amos.

Re: [Qemu-devel] [PATCH v4 2/4] qtest: introduce qmp_exec_hmp_cmd()

[Amos Kong]

On Tue, Jun 17, 2014 at 07:37:06PM +0200, Paolo Bonzini wrote:
> Il 06/06/2014 16:33, Amos Kong ha scritto:
> >+    va_end(ap);
> >+
> >+    escaped_cmd = g_strescape(cmd, NULL);
> >+    response = qmp("{\"execute\": \"human-monitor-command\","
> >+                   " \"arguments\": {"
> >+                   "   \"command-line\": \"%s\""
> >+                   "}}", escaped_cmd);
> >+    g_free(escaped_cmd);
> 
> Instead of adding g_strescape everywhere, we should use json-parser's own
> interpolation support.  See this patch:
> http://article.gmane.org/gmane.comp.emulators.qemu/279836 which also fixes a
> leak as a bonus.
> 
> Also, you can use ' instead of " if you fix another long-standing bug:
> http://article.gmane.org/gmane.comp.emulators.qemu/279835

I will use ' instead of ", and escape string in QMP command as http://article.gmane.org/gmane.comp.emulators.qemu/279836

Thanks.
 
> Paolo

-- 
			Amos.

Re: [Qemu-devel] [PATCH v4 3/4] virtio-blk-test.c: add hotplug subtest

[Amos Kong]

On Tue, Jun 17, 2014 at 03:25:34PM +0200, Andreas Färber wrote:
> Am 06.06.2014 16:33, schrieb Amos Kong:
> > This patch adds a new subtest, it hotplugs 29 * 8 = 232 virtio-blk
> > devices to guest, and try to hot-unplug them.
> > 
> > Note: the hot-unplug can't work without cooperation of guest OS.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  tests/virtio-blk-test.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > 
> > diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
> > index 0fdec01..7358203 100644
> > --- a/tests/virtio-blk-test.c
> > +++ b/tests/virtio-blk-test.c
> > @@ -7,11 +7,41 @@
> >   * See the COPYING file in the top-level directory.
> >   */
> >  
> > +#include <stdio.h>
> >  #include <glib.h>
> >  #include <string.h>
> >  #include "libqtest.h"
> >  #include "qemu/osdep.h"
> >  
> > +static void test_blk_hotplug(void)
> > +{
> > +    int i, j;
> > +
> > +    /* start with no network/block device, slots 3~0x1f are free */
> 
> "3-0x1f" or "3 to 0x1f"?
> 
> > +    qtest_start("-net none");
> > +
> > +    for (i = 3; i <= 0x1f; i++) {
> > +        for (j = 7; j >= 0; j--) {
> > +            qmp_exec_hmp_cmd("OK\r\n",
> > +                             "drive_add 0 if=none,file=/dev/null,id=drv-%x.%x",
> > +                             i, j);
> > +            qmp_exec_hmp_cmd("",
> > +                       "device_add virtio-blk-pci,id=dev-%x.%x,drive=drv-%x.%x,"
> > +                       "addr=0x%x.%x,multifunction=on", i, j, i, j, i, j);

Hi Andreas,

 
> Why are you using HMP-via-QMP here and not QMP directly?

I referenced existed test code.
 
> > +        }
> > +    }
> > +
> > +    /* hot-unplug doesn't work without cooperation of guest OS */
> > +    for (i = 3; i <= 0x1f; i++) {
> > +        for (j = 7; j >= 0; j--) {
> 
> While the function is still small, using a define or static const would
> be a small improvement. :) Could be a follow-up of course.

Sorry I don't get it. 

test_blk_hotplug() was already decorated by 'static'
and we can't decorate a function that returns nothing.

tests/virtio-blk-test.c:16:19: error: function definition has
qualified void return type [-Werror]
 static const void test_blk_hotplug(void)
 
> Test looks good, thanks for your effort.
> 
> Regards,
> Andreas


Thanks, Amos
 
> > +            qmp_exec_hmp_cmd("", "drive_del drv-%x.%x", i, j);
> > +            qmp_exec_hmp_cmd("", "device_del dev-%x.%x", i, j);
> > +        }
> > +    }
> > +
> > +    qtest_end();
> > +}
> > +
> >  /* Tests only initialization */
> >  static void virtblk_init(void)
> >  {
> > @@ -26,6 +56,7 @@ int main(int argc, char **argv)
> >  
> >      g_test_init(&argc, &argv, NULL);
> >      qtest_add_func("/virtio/blk/pci/init", virtblk_init);
> > +    qtest_add_func("/virtio/blk/pci/hotplug", test_blk_hotplug);
> >  
> >      ret = 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

-- 
			Amos.

Re: [Qemu-devel] [PATCH v4 2/4] qtest: introduce qmp_exec_hmp_cmd()

[Paolo Bonzini]

Il 18/06/2014 08:29, Amos Kong ha scritto:
>> > Instead of adding g_strescape everywhere, we should use json-parser's own
>> > interpolation support.  See this patch:
>> > http://article.gmane.org/gmane.comp.emulators.qemu/279836 which also fixes a
>> > leak as a bonus.
>> >
>> > Also, you can use ' instead of " if you fix another long-standing bug:
>> > http://article.gmane.org/gmane.comp.emulators.qemu/279835
> I will use ' instead of ", and escape string in QMP command as http://article.gmane.org/gmane.comp.emulators.qemu/279836

Thanks Amos!  This will avoid duplicating work as well as semantic 
conflicts.  I've added your Reviewed-by for the lexer patch.

Paolo

Re: [Qemu-devel] [PATCH v4 3/4] virtio-blk-test.c: add hotplug subtest

[Andreas Färber]

Am 18.06.2014 08:40, schrieb Amos Kong:
> On Tue, Jun 17, 2014 at 03:25:34PM +0200, Andreas Färber wrote:
>> Am 06.06.2014 16:33, schrieb Amos Kong:
>>> This patch adds a new subtest, it hotplugs 29 * 8 = 232 virtio-blk
>>> devices to guest, and try to hot-unplug them.
>>>
>>> Note: the hot-unplug can't work without cooperation of guest OS.
>>>
>>> Signed-off-by: Amos Kong <akong@redhat.com>
>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  tests/virtio-blk-test.c | 31 +++++++++++++++++++++++++++++++
>>>  1 file changed, 31 insertions(+)
>>>
>>> diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
>>> index 0fdec01..7358203 100644
>>> --- a/tests/virtio-blk-test.c
>>> +++ b/tests/virtio-blk-test.c
>>> @@ -7,11 +7,41 @@
>>>   * See the COPYING file in the top-level directory.
>>>   */
>>>  
>>> +#include <stdio.h>
>>>  #include <glib.h>
>>>  #include <string.h>
>>>  #include "libqtest.h"
>>>  #include "qemu/osdep.h"
>>>  
>>> +static void test_blk_hotplug(void)
>>> +{
>>> +    int i, j;
>>> +
>>> +    /* start with no network/block device, slots 3~0x1f are free */
>>
>> "3-0x1f" or "3 to 0x1f"?
>>
>>> +    qtest_start("-net none");
>>> +
>>> +    for (i = 3; i <= 0x1f; i++) {
>>> +        for (j = 7; j >= 0; j--) {
>>> +            qmp_exec_hmp_cmd("OK\r\n",
>>> +                             "drive_add 0 if=none,file=/dev/null,id=drv-%x.%x",
>>> +                             i, j);
>>> +            qmp_exec_hmp_cmd("",
>>> +                       "device_add virtio-blk-pci,id=dev-%x.%x,drive=drv-%x.%x,"
>>> +                       "addr=0x%x.%x,multifunction=on", i, j, i, j, i, j);
> 
> Hi Andreas,
> 
>  
>> Why are you using HMP-via-QMP here and not QMP directly?
> 
> I referenced existed test code.
>  
>>> +        }
>>> +    }
>>> +
>>> +    /* hot-unplug doesn't work without cooperation of guest OS */
>>> +    for (i = 3; i <= 0x1f; i++) {
>>> +        for (j = 7; j >= 0; j--) {
>>
>> While the function is still small, using a define or static const would
>> be a small improvement. :) Could be a follow-up of course.
> 
> Sorry I don't get it. 

You are adding two new loops that are supposed to match.

My suggestion is to avoid the four magic numbers by using
#define MIN_PCI_SLOT 3
or
static const int max_pci_slot = 0x1f;
etc. to enforce they always match and can more easily be tweaked once,
e.g., another slot is taken by default. Just a cosmetic cleanup.

Cheers,
Andreas

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