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

[Stefan Hajnoczi <stefanha@gmail.com>]

On Tue, May 27, 2014 at 03:14:41PM +0800, Amos Kong wrote:
> 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>
> ---
>  tests/virtio-blk-test.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
> index 0fdec01..54d1272 100644
> --- a/tests/virtio-blk-test.c
> +++ b/tests/virtio-blk-test.c
> @@ -7,11 +7,65 @@
>   * 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 exec_hmp_cmd(const char *cmd, const char *expected_ret)

This is a generic function, should it be moved to libqtest.c?

> +{
> +    QDict *response;
> +    const char *response_return;
> +
> +    response = qmp("{\"execute\": \"human-monitor-command\","
> +                   " \"arguments\": {"
> +                   "   \"command-line\": \"%s\""
> +                   "}}", cmd);

My only worry if we make this function generic is that cmd isn't escaped
so callers need to be careful.  Maybe something like g_strescape()
should be used:

https://developer.gnome.org/glib/2.37/glib-String-Utility-Functions.html#g-strescape

> +    g_assert(response);
> +    response_return = qdict_get_try_str(response, "return");
> +    g_assert(response_return);
> +    g_assert(strcmp(response_return, expected_ret) == 0);

Please use g_assert_cmpstr() so the error message will be pretty-printed
with the values of response_return and expected_ret:
https://developer.gnome.org/glib/stable/glib-Testing.html#g-assert-cmpstr

> +    QDECREF(response);
> +}

> +static void test_blk_hotplug(void)
> +{
> +    char addr[6];
> +    char cmd[100];
> +    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--) {
> +            sprintf(addr, "%x.%x", i, j);
> +            sprintf(cmd, "drive_add 0x%s if=none,file=/dev/null,id=drv-%s",
> +                    addr, addr);

Does the address matter with if=none?  I think you can just do
"drive_add 0 if=none,...".

> +            exec_hmp_cmd(cmd, "OK\r\n");
> +
> +            sprintf(cmd, "device_add virtio-blk-pci,id=dev-%s,drive=drv-%s,"
> +                         "addr=0x%s,multifunction=on", addr, addr, addr);
> +            exec_hmp_cmd(cmd, "");
> +        }
> +    }
> +
> +    /* hot-unplug doesn't work without cooperation of guest OS */

We could implement hotplug/hotunplug in libqos, PCI support already
exists: tests/libqos/pci.h.

But I guess this test is useful too, it checks the case where the guest
is not cooperating?