[libvirt] [PATCH 2/4] qemu: Add support for iothreads

[John Ferlan]

On 08/26/2014 01:03 AM, Martin Kletzander wrote:
> On Mon, Aug 25, 2014 at 08:38:07PM -0400, John Ferlan wrote:
>> Add a new capability to ensure the iothreads feature exists for the qemu
>> emulator being run - requires the "query-iothreads" QMP command. Using the
>> domain XML add correspoding command argument in order to generate the
>> threads. The iothreads will use a name space "libvirtIothread#" where, the
>> future patch to add support for using an iothread to a disk definition to
>> merely define which of the available threads to use.
>>
>> Add tests to ensure the xml/argv processing is correct.  Note that no
>> change was made to qemuargv2xmltest.c as processing the -object element
>> would require knowing more than just iothreads.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>> src/qemu/qemu_capabilities.c                       |  2 ++
>> src/qemu/qemu_capabilities.h                       |  1 +
>> src/qemu/qemu_command.c                            | 13 ++++++++++
>> tests/qemuxml2argvdata/qemuxml2argv-iothreads.args |  8 ++++++
>> tests/qemuxml2argvdata/qemuxml2argv-iothreads.xml  | 29 ++++++++++++++++++++++
>> tests/qemuxml2argvtest.c                           |  2 ++
>> tests/qemuxml2xmltest.c                            |  1 +
>> 7 files changed, 56 insertions(+)
>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads.args
>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads.xml
>>
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index 410086b..b331be7 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -268,6 +268,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>>               "rtc-reset-reinjection",
>>
>>               "splash-timeout", /* 175 */
>> +              "query-iothreads",
> 
> Why "query-" when the capability is _OBJECT_?  Or is this just a typo?
> </bikeshed>
> 

hmm... so how is this different then say perhaps
QEMU_CAPS_OBJECT_RNG_RANDOM, QEMU_CAPS_OBJECT_MEMORY_RAM or
QEMU_CAPS_OBJECT_MEMORY_FILE which each eventually uses "-object" to
build it's part of the command line

>>     );
>>
>>
>> @@ -1430,6 +1431,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = {
>>     { "nbd-server-start", QEMU_CAPS_NBD_SERVER },
>>     { "change-backing-file", QEMU_CAPS_CHANGE_BACKING_FILE },
>>     { "rtc-reset-reinjection", QEMU_CAPS_RTC_RESET_REINJECTION },
>> +    { "query-iothreads", QEMU_CAPS_OBJECT_IOTHREAD},
> 
> We have virQEMUCapsObjectTypes[] where you can just stick the name of
> the object you want to test, and not rely on a related command to
> probe for it.
> 

Hmm..  I wasn't completely sure where to put this - it's not overly
clear which of the various capabilities structures should be used for
what reason.  Although I guess since the above named flags are in
ObjectTypes I suppose I should have used it.  I'll move it.

>> };
>>
>> struct virQEMUCapsStringFlags virQEMUCapsEvents[] = {
>> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
>> index 48a4eaa..0980c00 100644
>> --- a/src/qemu/qemu_capabilities.h
>> +++ b/src/qemu/qemu_capabilities.h
>> @@ -215,6 +215,7 @@ typedef enum {
>>     QEMU_CAPS_OBJECT_USB_AUDIO   = 173, /* usb-audio device support */
>>     QEMU_CAPS_RTC_RESET_REINJECTION = 174, /* rtc-reset-reinjection monitor command */
>>     QEMU_CAPS_SPLASH_TIMEOUT     = 175, /* -boot splash-time */
>> +    QEMU_CAPS_OBJECT_IOTHREAD    = 176, /* -object iothread */
>>
>>     QEMU_CAPS_LAST,                   /* this must always be the last item */
>> } virQEMUCapsFlags;
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 6dac9d3..287a3f3 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -7510,6 +7510,19 @@ qemuBuildCommandLine(virConnectPtr conn,
>>     virCommandAddArg(cmd, smp);
>>     VIR_FREE(smp);
>>
>> +    if (def->iothreads > 0 &&
>> +        virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) {
>> +        /* Create named iothread objects starting with 1. These may be used
>> +         * by a disk definition which will associate to an iothread by
>> +         * supplying a value of 1 up to the number of iothreads available
>> +         * (since 0 would indicate to not use the feature).
>> +         */
>> +        for (i = 1; i <= def->iothreads; i++) {
>> +            virCommandAddArg(cmd, "-object");
>> +            virCommandAddArgFormat(cmd, "iothread,id=libvirtIothread%zu", i);
> 
> I don't see we would use 'libvirt*' naming for any other IDs,
> 'iothread%zu' would be enough, I guess (and the command-line wouldn't
> be so long as well).

Using 'libvirt' as the prefix for "iothread" was more of a convention to
make sure it was clear what created it.  I can go with the shorter name
and initially had done so.

John