[libvirt] [PATCH 1/4] domain_conf: Introduce iothreads XML

[John Ferlan]

On 08/26/2014 01:03 AM, Martin Kletzander wrote:
> On Mon, Aug 25, 2014 at 08:38:06PM -0400, John Ferlan wrote:
>> Introduce XML to allowing adding iothreads to the domain. These can be
>> used by virtio-blk-pci devices in order to assign a specific thread to
>> handle the workload for the device.  The iothreads are the official
>> implementation of the virtio-blk Data Plane that's been in tech preview
>> for QEMU.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>> docs/formatdomain.html.in     | 26 ++++++++++++++++++++++++++
>> docs/schemas/domaincommon.rng | 12 ++++++++++++
>> src/conf/domain_conf.c        | 28 ++++++++++++++++++++++++++++
>> src/conf/domain_conf.h        |  2 ++
>> 4 files changed, 68 insertions(+)
>>
> [...]
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index 9a89dd8..b4ac483 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -632,6 +632,12 @@
>>       </optional>
>>
>>       <optional>
>> +        <element name="iothreads">
>> +          <ref name="countIOThreads"/>
> 
> What's the difference between this and using ref name="unsignedInt"
> directly?
> 

I (more or less) modeled (eg, cut, copy, paste) after the vcpu element
which as you'll note is "countCPU" for both current and maxvcpus.
Unlike the vcpu, I went with an 'int' instead of a 'short' although
technically I cannot imagine more than a short number of threads or
disks assigned to any domain!

Also unlike the vcpu argument a "0" is a perfectly valid value.

>> +        </element>
>> +      </optional>
>> +
>> +      <optional>
>>         <ref name="blkiotune"/>
>>       </optional>
>>
>> @@ -4747,6 +4753,12 @@
>>       <param name="minInclusive">1</param>
>>     </data>
>>   </define>
>> +  <define name="countIOThreads">
>> +    <data type="unsignedInt">
>> +      <param name="pattern">[0-9]+</param>
>> +      <param name="minInclusive">0</param>
>> +    </data>
>> +  </define>
>>   <define name="vcpuid">
>>     <data type="unsignedShort">
>>       <param name="pattern">[0-9]+</param>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 22a7f7e..671c41c 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -11953,6 +11953,23 @@ virDomainDefParseXML(xmlDocPtr xml,
>>         }
>>     }
>>
>> +    /* Optional - iothreads */
>> +    n = virXPathULong("string(./iothreads[1])", ctxt, &count);
>> +    if (n == -2) {
>> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                       _("iothreads count must be an integer"));
>> +        goto error;
>> +    } else if (n < 0) {
>> +        def->iothreads = 0;
>> +    } else {
>> +        if ((unsigned int) count != count) {
> 
> Instead of this machinery, it would be more straightforward to just do
> (example written by hand, not tested):
> 
>  tmp = virXPathString("string(./iothreads[1])", ctxt);
>  if (tmp && virStrToLong_uip(tmp, NULL, 0, &def->iothreads) < 0)
>      virReportError(VIR_ERR_XML_ERROR, _("invalid iothreads count '%s'"), tmp);
> 

See my above explanation and then look about 10-20 lines before
iothreads and see vcpu/current and maxvcpus.  The power of the visual
suggestion of what code around area I was changing was doing was far
greater than thinking hmm.. should this code use the virStrToLong calls.
 Unlike of course when I added the iothread to the disk property which
does use the function.

Maybe in a different patch it'd be worth working through the various
other calls to virXPathU* and seeing if they too could use the
virStrToLong* interfaces.

I'll adjust both - no problem.

John