Re: [Qemu-devel] [PATCH 4/6] tests: Introduce generic device hot-plug/hot-unplug functions

[David Hildenbrand <david@redhat.com>]

On 17.08.2017 12:57, Thomas Huth wrote:
> On 17.08.2017 11:53, David Hildenbrand wrote:
>> On 17.08.2017 08:25, Thomas Huth wrote:
>>> A lot of tests provide code for adding and removing a device via the
>>> device_add and device_del QMP commands. Maintaining this code in so
>>> many places is cumbersome and error-prone (some of the code parts
>>> check the responses in an incorrect way, for example), so let's
>>> provide some proper generic qtest functions for adding and removing a
>>> device instead.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
> [...]
>>> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
>>> index 2dcdead..aada753 100644
>>> --- a/tests/libqos/pci.c
>>> +++ b/tests/libqos/pci.c
>>> @@ -394,21 +394,6 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
>>>  void qpci_plug_device_test(const char *driver, const char *id,
>>>                             uint8_t slot, const char *opts)
>>>  {
>>> -    QDict *response;
>>> -    char *cmd;
>>> -
>>> -    cmd = g_strdup_printf("{'execute': 'device_add',"
>>> -                          " 'arguments': {"
>>> -                          "   'driver': '%s',"
>>> -                          "   'addr': '%d',"
>>> -                          "   %s%s"
>>> -                          "   'id': '%s'"
>>> -                          "}}", driver, slot,
>>> -                          opts ? opts : "", opts ? "," : "",
>>> -                          id);
>>> -    response = qmp(cmd);
>>> -    g_free(cmd);
>>> -    g_assert(response);
>>> -    g_assert(!qdict_haskey(response, "error"));
>>> -    QDECREF(response);
>>> +    qtest_hot_plug_device(driver, id, "'addr': '%d'%s%s", slot,
>>> +                          opts ? ", " : "", opts ? opts : "");
> 
> While PCI uses 'addr' as additional parameter here...

so addr would be needed in addition.

> 
> [...]
>>> +    qtest_hot_plug_device("usb-tablet", id, "'port': '%d', 'bus': '%s.0'",
>>> +                          port, hcd_id);
> 
> ... USB uses 'port' and 'bus' ...
> 
> [...]
>>
>> Not sure if it would be better to avoid the fmt and provide more
>> parameters instead. Specifying a parameter as NULL will ifgnore it.
>>
>> void qtest_hot_plug_device(const char *driver, const char *id,
>>                            const char *drive, const char *port,
>>                            const char *bus)
>>
>> that should cover all cases and callers don't have to build strings in a
>> special format.
> 
> ... so I'm afraid, but no, this does not work. The additional parameters
> are specific to the device type, so there is no way to get around the
> format string if we want to stay flexible here.

It will work, but yes it is not flexible. Just an idea because I don't
like having to specify json strings on a function.

> 
>  Thomas
> 


-- 

Thanks,

David