Re: [libvirt] [PATCH v2 4/4] qemu: Allow use of iothreads for disk definitions

On 08/27/2014 03:00 AM, Martin Kletzander wrote: > On Tue, Aug 26, 2014 at 06:15:49PM -0400, John Ferlan wrote: >> For virtio-blk-pci disks with the disk iothread attribute that are >> running the correct emulator, add the "iothread=iothread#" to the >> -device command line in order to enable iothreads for the disk. >> >> This code will check both the "start" and "hotplug" paths for the capability, >> whether the iothreadsmap has been defined, and whether there's an available >> iothread to be used for the disk. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/qemu/qemu_command.c | 51 ++++++++++++++++++++++ >> src/qemu/qemu_hotplug.c | 6 +++ >> .../qemuxml2argv-iothreads-disk.args | 17 ++++++++ >> .../qemuxml2argv-iothreads-disk.xml | 40 +++++++++++++++++ >> tests/qemuxml2argvtest.c | 2 + >> tests/qemuxml2xmltest.c | 1 + >> 6 files changed, 117 insertions(+) >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.args >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.xml >> >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >> index ba0b977..763bb74 100644 >> --- a/src/qemu/qemu_command.c >> +++ b/src/qemu/qemu_command.c >> @@ -3673,6 +3673,49 @@ qemuBuildDriveStr(virConnectPtr conn, >> return NULL; >> } >> >> + >> +static bool >> +qemuCheckIothreads(virDomainDefPtr def, >> + virQEMUCapsPtr qemuCaps, >> + virDomainDiskDefPtr disk) >> +{ >> + bool inuse; >> + >> + /* Have capability */ >> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("IOThreads not supported for this QEMU")); >> + return false; >> + } >> + >> + /* Right "type" of disk" */ >> + if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO && >> + disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("IOThreads not available for this disk")); > > Sorry to say that, but this is like another extreme, it feels too > generic. If there are multiple disks, the user might not be sure > which one you mean. Listing the allowed ones is fine, I'd say > something like "IOThreads only available for virtio pci disk" is > enough. > > ACK with that changed. > > OK - I left it at "virtio pci", but can change it to "virtio-pci" or "virtio-blk-pci" based on the answer to the following question...

Is this safe enough to be pushed with the freeze now in effect. I know the review started before and all that, but I figured I'd ask before pushing in any case!