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

[Markus Armbruster]

Amos Kong <address@hidden> writes:

> On Thu, Jun 05, 2014 at 01:12:46PM +0800, Amos Kong wrote:
>> On Wed, Jun 04, 2014 at 05:35:06PM +0200, Stefan Hajnoczi wrote:
>> > 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 <address@hidden>
>> > > ---
>> > >  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?
>> 
>> OK
>> 
>> > 
>> > > +{
>> > > +    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
>>  
>> OK
>> 
>> > > +    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,...".
>> 
>> drive_add [[<domain>:]<bus>:]<slot>
>> 
>> multiple function is enabled, slot parameter is needed.
>
> pci addr is assigned in device_add command, and it contains drive's id.
> I tried to just use "drive_add 0 if=none,...", it also works.

So does

    (qemu) drive_add "" if=none,file=tmp.qcow2
    OK

or even

    (qemu) drive_add "I'm a little teapot" if=none,file=tmp.qcow2
    OK

because the first argument is ignored.

Fortunately we're getting close to replacing this badly designed
command.