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

["Andreas Färber" <afaerber@suse.de>]

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