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

[Thomas Huth]

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 <address@hidden>
>> ---
[...]
>> 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...

[...]
>> +    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.

 Thomas