Re: [PATCH v2 1/4] domain_conf: Introduce iothreads XML

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Aug 26, 2014 at 06:15:46PM -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@xxxxxxxxxx>
---
docs/formatdomain.html.in     | 26 ++++++++++++++++++++++++++
docs/schemas/domaincommon.rng |  6 ++++++
src/conf/domain_conf.c        | 20 ++++++++++++++++++++
src/conf/domain_conf.h        |  2 ++
4 files changed, 54 insertions(+)


I forgot two things in the review.

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index dd512ca..81a3fdf 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11957,6 +11957,15 @@ virDomainDefParseXML(xmlDocPtr xml,
        }
    }

+    /* Optional - iothreads */
+    tmp = virXPathString("string(./iothreads[1])", ctxt);
+    if (tmp && virStrToLong_uip(tmp, NULL, 0, &def->iothreads) < 0) {

One of the things is that you use '0' here (as I suggested, sorry for
that), which would not that big of a deal, it would even be awesome,
but unfortunately we use '10' everywhere in the XML parsing code :(
Even when parsing the iothread="" in 3/4 you are using '10', so this
should be changed to '10'.

The second thing is that there might be a qemuxml2xmltest case for the
qemuxml2argv-iothreads.xml (which can be added in this patch), but
that's just a bike-shedding.

The ACK still stands, of course.

Martin

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]