[libvirt] [PATCH] Fix qemu command flags fetching
New function qemudParseHelpStr was being called with arguments in the wrong order, so command flags == kvm_version :/ Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu_conf.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 22f315c..a669c11 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -597,7 +597,7 @@ int qemudExtractVersionInfo(const char *qemu, goto cleanup2; } - if (qemudParseHelpStr(help, &version, &kvm_version, &is_kvm, &flags) == -1) + if (qemudParseHelpStr(help, &flags, &version, &is_kvm, &kvm_version) == -1) goto cleanup2; if (retversion) -- 1.6.0.6
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/.gitignore | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/tests/.gitignore b/tests/.gitignore index f71d708..7a4d44f 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -12,6 +12,7 @@ xencapstest qemuxml2xmltest qemuxml2argvtest qemuargv2xmltest +qemuhelptest nodedevxml2xmltest nodeinfotest statstest -- 1.6.0.6
On Mon, Jun 15, 2009 at 06:11:04PM -0400, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/.gitignore | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
ACK, but don't modify .gitignore files yourself. Change the .cvsignore file and then run 'make sync-vcs-ignore-files' otherwise your changes will get wiped out in the future. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 06/16/2009 05:33 AM, Daniel P. Berrange wrote:
On Mon, Jun 15, 2009 at 06:11:04PM -0400, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/.gitignore | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
ACK, but don't modify .gitignore files yourself. Change the .cvsignore file and then run 'make sync-vcs-ignore-files' otherwise your changes will get wiped out in the future.
Daniel
Oops, I forgot about that. I'll make the proper change when I commit. Thanks, Cole
Qemu < 0.10.0 did not support it, and virt-* tools now try to add this by default, so it's extra important we ensure the option exists. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu_conf.c | 5 ++++- src/qemu_conf.h | 1 + tests/qemuhelptest.c | 8 ++++---- tests/qemuxml2argvtest.c | 20 ++++++++++++-------- 4 files changed, 21 insertions(+), 13 deletions(-) diff --git a/src/qemu_conf.c b/src/qemu_conf.c index a669c11..0c6221d 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -435,6 +435,8 @@ static unsigned int qemudComputeCmdFlags(const char *help, flags |= QEMUD_CMD_FLAG_DRIVE; if (strstr(help, "cache=writethrough|writeback|none")) flags |= QEMUD_CMD_FLAG_DRIVE_CACHE_V2; + if (strstr(help, "format=")) + flags |= QEMUD_CMD_FLAG_DRIVE_FORMAT; } if (strstr(help, "boot=on")) flags |= QEMUD_CMD_FLAG_DRIVE_BOOT; @@ -1231,7 +1233,8 @@ int qemudBuildCommandLine(virConnectPtr conn, if (bootable && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) virBufferAddLit(&opt, ",boot=on"); - if (disk->driverType) + if (disk->driverType && + qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE_FORMAT) virBufferVSprintf(&opt, ",format=%s", disk->driverType); if (disk->cachemode) { diff --git a/src/qemu_conf.h b/src/qemu_conf.h index 280ad25..c0ed46d 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -56,6 +56,7 @@ enum qemud_cmd_flags { QEMUD_CMD_FLAG_MIGRATE_QEMU_EXEC = (1 << 11), /* New migration syntax after merge to QEMU with EXEC transport */ QEMUD_CMD_FLAG_DRIVE_CACHE_V2 = (1 << 12), /* Is the cache= flag wanting new v2 values */ QEMUD_CMD_FLAG_KVM = (1 << 13), /* Whether KVM is compiled in */ + QEMUD_CMD_FLAG_DRIVE_FORMAT = (1 << 14), /* Is -drive format= avail */ }; /* Main driver state */ diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c index 836010f..bf8a293 100644 --- a/tests/qemuhelptest.c +++ b/tests/qemuhelptest.c @@ -90,10 +90,10 @@ mymain(int argc, char **argv) } while (0) DO_TEST("qemu-0.9.1", 0x002f, 9001, 0, 0); - DO_TEST("kvm-74", 0x233e, 9001, 1, 74); - DO_TEST("qemu-0.10.5", 0x1c6f, 10005, 0, 0); - DO_TEST("qemu-kvm-0.10.5", 0x3d7e, 10005, 1, 0); - DO_TEST("kvm-86", 0x3d7e, 10050, 1, 0); + DO_TEST("kvm-74", 0x633e, 9001, 1, 74); + DO_TEST("qemu-0.10.5", 0x5c6f, 10005, 0, 0); + DO_TEST("qemu-kvm-0.10.5", 0x7d7e, 10005, 1, 0); + DO_TEST("kvm-86", 0x7d7e, 10050, 1, 0); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 5ba6db3..28609eb 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -196,17 +196,21 @@ mymain(int argc, char **argv) DO_TEST("disk-drive-boot-cdrom", QEMUD_CMD_FLAG_DRIVE | QEMUD_CMD_FLAG_DRIVE_BOOT); DO_TEST("disk-drive-fmt-qcow", QEMUD_CMD_FLAG_DRIVE | - QEMUD_CMD_FLAG_DRIVE_BOOT); - DO_TEST("disk-drive-shared", QEMUD_CMD_FLAG_DRIVE); - DO_TEST("disk-drive-cache-v1-wt", QEMUD_CMD_FLAG_DRIVE); - DO_TEST("disk-drive-cache-v1-wb", QEMUD_CMD_FLAG_DRIVE); - DO_TEST("disk-drive-cache-v1-none", QEMUD_CMD_FLAG_DRIVE); + QEMUD_CMD_FLAG_DRIVE_BOOT | QEMUD_CMD_FLAG_DRIVE_FORMAT); + DO_TEST("disk-drive-shared", QEMUD_CMD_FLAG_DRIVE | + QEMUD_CMD_FLAG_DRIVE_FORMAT); + DO_TEST("disk-drive-cache-v1-wt", QEMUD_CMD_FLAG_DRIVE | + QEMUD_CMD_FLAG_DRIVE_FORMAT); + DO_TEST("disk-drive-cache-v1-wb", QEMUD_CMD_FLAG_DRIVE | + QEMUD_CMD_FLAG_DRIVE_FORMAT); + DO_TEST("disk-drive-cache-v1-none", QEMUD_CMD_FLAG_DRIVE | + QEMUD_CMD_FLAG_DRIVE_FORMAT); DO_TEST("disk-drive-cache-v2-wt", QEMUD_CMD_FLAG_DRIVE | - QEMUD_CMD_FLAG_DRIVE_CACHE_V2); + QEMUD_CMD_FLAG_DRIVE_CACHE_V2 | QEMUD_CMD_FLAG_DRIVE_FORMAT); DO_TEST("disk-drive-cache-v2-wb", QEMUD_CMD_FLAG_DRIVE | - QEMUD_CMD_FLAG_DRIVE_CACHE_V2); + QEMUD_CMD_FLAG_DRIVE_CACHE_V2 | QEMUD_CMD_FLAG_DRIVE_FORMAT); DO_TEST("disk-drive-cache-v2-none", QEMUD_CMD_FLAG_DRIVE | - QEMUD_CMD_FLAG_DRIVE_CACHE_V2); + QEMUD_CMD_FLAG_DRIVE_CACHE_V2 | QEMUD_CMD_FLAG_DRIVE_FORMAT); DO_TEST("disk-usb", 0); DO_TEST("graphics-vnc", 0); -- 1.6.0.6
On Mon, Jun 15, 2009 at 06:11:05PM -0400, Cole Robinson wrote:
Qemu < 0.10.0 did not support it, and virt-* tools now try to add this by default, so it's extra important we ensure the option exists.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu_conf.c | 5 ++++- src/qemu_conf.h | 1 + tests/qemuhelptest.c | 8 ++++---- tests/qemuxml2argvtest.c | 20 ++++++++++++-------- 4 files changed, 21 insertions(+), 13 deletions(-)
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Certain drivers always need an 'emulator' specified in the XML (qemu and lxc at least). Store this info in capabilities. We will eventually use this to move default emulator handling out of drivers and into domain_conf.* Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/capabilities.c | 10 ++++++++++ src/capabilities.h | 7 +++++++ src/libvirt_private.syms | 2 ++ src/lxc_conf.c | 4 ++++ src/qemu_conf.c | 3 +++ 5 files changed, 26 insertions(+), 0 deletions(-) diff --git a/src/capabilities.c b/src/capabilities.c index d6e3478..00a4407 100644 --- a/src/capabilities.c +++ b/src/capabilities.c @@ -707,3 +707,13 @@ virCapabilitiesGenerateMac(virCapsPtr caps, { virGenerateMacAddr(caps->macPrefix, mac); } + +extern void +virCapabilitiesSetEmulatorRequired(virCapsPtr caps) { + caps->emulatorRequired = 1; +} + +extern unsigned int +virCapabilitiesIsEmulatorRequired(virCapsPtr caps) { + return caps->emulatorRequired; +} diff --git a/src/capabilities.h b/src/capabilities.h index 5b0bbab..0d476d1 100644 --- a/src/capabilities.h +++ b/src/capabilities.h @@ -106,6 +106,7 @@ struct _virCaps { int nguests; virCapsGuestPtr *guests; unsigned char macPrefix[VIR_MAC_PREFIX_BUFLEN]; + unsigned int emulatorRequired : 1; }; @@ -125,6 +126,12 @@ extern void virCapabilitiesGenerateMac(virCapsPtr caps, unsigned char *mac); +extern void +virCapabilitiesSetEmulatorRequired(virCapsPtr caps); + +extern unsigned int +virCapabilitiesIsEmulatorRequired(virCapsPtr caps); + extern int virCapabilitiesAddHostFeature(virCapsPtr caps, const char *name); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6af8432..5ac904f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -27,6 +27,8 @@ virCapabilitiesFree; virCapabilitiesNew; virCapabilitiesSetMacPrefix; virCapabilitiesGenerateMac; +virCapabilitiesSetEmulatorRequired; +virCapabilitiesIsEmulatorRequired; # conf.h diff --git a/src/lxc_conf.c b/src/lxc_conf.c index 8a29c25..d06a024 100644 --- a/src/lxc_conf.c +++ b/src/lxc_conf.c @@ -69,6 +69,10 @@ virCapsPtr lxcCapsInit(void) 0, NULL) == NULL) goto no_memory; + + /* LXC Requires an emulator in the XML */ + virCapabilitiesSetEmulatorRequired(caps); + return caps; no_memory: diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 0c6221d..8c657be 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -405,6 +405,9 @@ virCapsPtr qemudCapsInit(void) { } } + /* QEMU Requires an emulator in the XML */ + virCapabilitiesSetEmulatorRequired(caps); + return caps; no_memory: -- 1.6.0.6
On Mon, Jun 15, 2009 at 06:11:06PM -0400, Cole Robinson wrote:
Certain drivers always need an 'emulator' specified in the XML (qemu and lxc at least). Store this info in capabilities.
We will eventually use this to move default emulator handling out of drivers and into domain_conf.*
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/capabilities.c | 10 ++++++++++ src/capabilities.h | 7 +++++++ src/libvirt_private.syms | 2 ++ src/lxc_conf.c | 4 ++++ src/qemu_conf.c | 3 +++ 5 files changed, 26 insertions(+), 0 deletions(-)
ACK Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Jun 15, 2009 at 06:11:06PM -0400, Cole Robinson wrote:
Certain drivers always need an 'emulator' specified in the XML (qemu and lxc at least). Store this info in capabilities.
yes this makes sense !
We will eventually use this to move default emulator handling out of drivers and into domain_conf.*
ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Rather than numerous instances of: emulator = vm->def->emulator; if (!emulator) emulator = virDomainDefDefaultEmulator(conn, vm->def, driver->caps); if (!emulator) return -1; Set this value at XML parse time in the domain config, so we can depend on it for all future emulator accesses. There were unchecked accesses in the qemu driver that were tripping up on this if no emulator was specified in the XML, see: http://www.redhat.com/archives/libvir-list/2008-October/msg00602.html Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/domain_conf.c | 66 ++++++++++++++++++++++++++------------------- src/domain_conf.h | 4 --- src/libvirt_private.syms | 1 - src/lxc_driver.c | 5 --- src/qemu_conf.c | 5 --- src/qemu_driver.c | 8 ----- 6 files changed, 38 insertions(+), 51 deletions(-) diff --git a/src/domain_conf.c b/src/domain_conf.c index e83f776..c695820 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -2109,6 +2109,39 @@ int virDomainDiskQSort(const void *a, const void *b) } #ifndef PROXY +static char *virDomainDefDefaultEmulator(virConnectPtr conn, + virDomainDefPtr def, + virCapsPtr caps) { + const char *type; + const char *emulator; + char *retemu; + + type = virDomainVirtTypeToString(def->virtType); + if (!type) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("unknown virt type")); + return NULL; + } + + emulator = virCapabilitiesDefaultGuestEmulator(caps, + def->os.type, + def->os.arch, + type); + + if (!emulator) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("no emulator for domain %s os type %s on architecture %s"), + type, def->os.type, def->os.arch); + return NULL; + } + + retemu = strdup(emulator); + if (!retemu) + virReportOOMError(conn); + + return retemu; +} + static virDomainDefPtr virDomainDefParseXML(virConnectPtr conn, virCapsPtr caps, xmlXPathContextPtr ctxt, int flags) @@ -2358,6 +2391,11 @@ static virDomainDefPtr virDomainDefParseXML(virConnectPtr conn, } def->emulator = virXPathString(conn, "string(./devices/emulator[1])", ctxt); + if (!def->emulator && virCapabilitiesIsEmulatorRequired(caps)) { + def->emulator = virDomainDefDefaultEmulator(conn, def, caps); + if (!def->emulator) + goto error; + } /* analysis of the disk devices */ if ((n = virXPathNodeSet(conn, "./devices/disk", ctxt, &nodes)) < 0) { @@ -4230,34 +4268,6 @@ int virDiskNameToBusDeviceIndex(const virDomainDiskDefPtr disk, return 0; } -const char *virDomainDefDefaultEmulator(virConnectPtr conn, - virDomainDefPtr def, - virCapsPtr caps) { - const char *type; - const char *emulator; - - type = virDomainVirtTypeToString(def->virtType); - if (!type) { - virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, - "%s", _("unknown virt type")); - return NULL; - } - - emulator = virCapabilitiesDefaultGuestEmulator(caps, - def->os.type, - def->os.arch, - type); - - if (!emulator) { - virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("no emulator for domain %s os type %s on architecture %s"), - type, def->os.type, def->os.arch); - return NULL; - } - - return emulator; -} - virDomainFSDefPtr virDomainGetRootFilesystem(virDomainDefPtr def) { int i; diff --git a/src/domain_conf.h b/src/domain_conf.h index aef1ec4..51310c1 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -665,10 +665,6 @@ int virDiskNameToBusDeviceIndex(virDomainDiskDefPtr disk, int *busIdx, int *devIdx); -const char *virDomainDefDefaultEmulator(virConnectPtr conn, - virDomainDefPtr def, - virCapsPtr caps); - virDomainFSDefPtr virDomainGetRootFilesystem(virDomainDefPtr def); void virDomainObjLock(virDomainObjPtr obj); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5ac904f..f63fa05 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -65,7 +65,6 @@ virDomainCpuSetParse; virDomainChrDefFree; virDomainChrTypeFromString; virDomainChrTypeToString; -virDomainDefDefaultEmulator; virDomainDefFormat; virDomainDefFree; virDomainDefParseFile; diff --git a/src/lxc_driver.c b/src/lxc_driver.c index e798c79..3503573 100644 --- a/src/lxc_driver.c +++ b/src/lxc_driver.c @@ -758,7 +758,6 @@ static int lxcControllerStart(virConnectPtr conn, fd_set keepfd; char appPtyStr[30]; const char *emulator; - lxc_driver_t *driver = conn->privateData; FD_ZERO(&keepfd); @@ -787,10 +786,6 @@ static int lxcControllerStart(virConnectPtr conn, snprintf(appPtyStr, sizeof(appPtyStr), "%d", appPty); emulator = vm->def->emulator; - if (!emulator) - emulator = virDomainDefDefaultEmulator(conn, vm->def, driver->caps); - if (!emulator) - return -1; ADD_ARG_LIT(emulator); ADD_ARG_LIT("--name"); diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 8c657be..ff11d2d 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -920,11 +920,6 @@ int qemudBuildCommandLine(virConnectPtr conn, } emulator = def->emulator; - if (!emulator) - emulator = virDomainDefDefaultEmulator(conn, def, driver->caps); - if (!emulator) - return -1; - /* Need to explicitly disable KQEMU if * 1. Arch matches host arch diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 665bc8d..d3eb3ad 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1377,10 +1377,6 @@ static int qemudStartVMDaemon(virConnectPtr conn, goto cleanup; emulator = vm->def->emulator; - if (!emulator) - emulator = virDomainDefDefaultEmulator(conn, vm->def, driver->caps); - if (!emulator) - goto cleanup; /* Make sure the binary we are about to try exec'ing exists. * Technically we could catch the exec() failure, but that's @@ -3425,10 +3421,6 @@ static char *qemuDomainXMLToNative(virConnectPtr conn, def->graphics[i]->data.vnc.port = 5900; } emulator = def->emulator; - if (!emulator) - emulator = virDomainDefDefaultEmulator(conn, def, driver->caps); - if (!emulator) - goto cleanup; /* Make sure the binary we are about to try exec'ing exists. * Technically we could catch the exec() failure, but that's -- 1.6.0.6
On Mon, Jun 15, 2009 at 06:11:07PM -0400, Cole Robinson wrote:
Rather than numerous instances of:
emulator = vm->def->emulator; if (!emulator) emulator = virDomainDefDefaultEmulator(conn, vm->def, driver->caps); if (!emulator) return -1;
Set this value at XML parse time in the domain config, so we can depend on it for all future emulator accesses. There were unchecked accesses in the qemu driver that were tripping up on this if no emulator was specified in the XML, see:
http://www.redhat.com/archives/libvir-list/2008-October/msg00602.html
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/domain_conf.c | 66 ++++++++++++++++++++++++++------------------- src/domain_conf.h | 4 --- src/libvirt_private.syms | 1 - src/lxc_driver.c | 5 --- src/qemu_conf.c | 5 --- src/qemu_driver.c | 8 ----- 6 files changed, 38 insertions(+), 51 deletions(-)
ACK. We did originally have this in the domain conf.c file, but it caused other problems, in particular breaking Xen. I think that with the additional per-driver flag you added though this should be ok now. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Jun 15, 2009 at 06:11:07PM -0400, Cole Robinson wrote:
Rather than numerous instances of:
emulator = vm->def->emulator; if (!emulator) emulator = virDomainDefDefaultEmulator(conn, vm->def, driver->caps); if (!emulator) return -1;
Set this value at XML parse time in the domain config, so we can depend on it for all future emulator accesses. There were unchecked accesses in the qemu driver that were tripping up on this if no emulator was specified in the XML, see:
Okay, I had to convince myself that we were not leaking the emulator string in the end, but that looks fine. ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
This regressed when CreateXMLFrom was added: we should only perform the backing store comparison if an input volume was passed. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/storage_backend_fs.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index ac7c424..5b9830e 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -1251,8 +1251,9 @@ static int createQemuImg(virConnectPtr conn, * backing store, not really sure what use it serves though, and it * may cause issues with lvm. Untested essentially. */ - if (!inputBackingPath || - !STREQ(inputBackingPath, vol->backingStore.path)) { + if (inputvol && + (!inputBackingPath || + !STREQ(inputBackingPath, vol->backingStore.path))) { virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("a different backing store can not " "be specified.")); -- 1.6.0.6
On Mon, Jun 15, 2009 at 06:11:08PM -0400, Cole Robinson wrote:
This regressed when CreateXMLFrom was added: we should only perform the backing store comparison if an input volume was passed.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/storage_backend_fs.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index ac7c424..5b9830e 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -1251,8 +1251,9 @@ static int createQemuImg(virConnectPtr conn, * backing store, not really sure what use it serves though, and it * may cause issues with lvm. Untested essentially. */ - if (!inputBackingPath || - !STREQ(inputBackingPath, vol->backingStore.path)) { + if (inputvol && + (!inputBackingPath || + !STREQ(inputBackingPath, vol->backingStore.path))) { virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("a different backing store can not " "be specified."));
ACK, but can you change !STREQ to STRNEQ when committing. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Jun 15, 2009 at 06:11:08PM -0400, Cole Robinson wrote:
This regressed when CreateXMLFrom was added: we should only perform the backing store comparison if an input volume was passed.
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Product = 0 is a valid value based on this bug report: https://www.redhat.com/archives/libvir-list/2009-May/msg00368.html Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/domain_conf.c | 12 ++++++++---- .../qemuxml2argv-hostdev-usb-product.args | 2 +- .../qemuxml2argv-hostdev-usb-product.xml | 6 ++++++ 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/domain_conf.c b/src/domain_conf.c index c695820..94082a5 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -1660,8 +1660,13 @@ virDomainHostdevSubsysUsbDefParseXML(virConnectPtr conn, int flags ATTRIBUTE_UNUSED) { int ret = -1; + int got_product; xmlNodePtr cur; + /* Product can validly be 0, so we need some extra help to determine + * if it is uninitialized*/ + got_product = 0; + cur = node->children; while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE) { @@ -1686,6 +1691,7 @@ virDomainHostdevSubsysUsbDefParseXML(virConnectPtr conn, char* product = virXMLPropString(cur, "id"); if (product) { + got_product = 1; if (virStrToLong_ui(product, NULL, 0, &def->source.subsys.u.usb.product) < 0) { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, @@ -1745,14 +1751,12 @@ virDomainHostdevSubsysUsbDefParseXML(virConnectPtr conn, cur = cur->next; } - if (def->source.subsys.u.usb.vendor == 0 && - def->source.subsys.u.usb.product != 0) { + if (def->source.subsys.u.usb.vendor == 0 && got_product) { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("missing vendor")); goto out; } - if (def->source.subsys.u.usb.vendor != 0 && - def->source.subsys.u.usb.product == 0) { + if (def->source.subsys.u.usb.vendor != 0 && !got_product) { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("missing product")); goto out; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.args index e207871..f4c8f60 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -usbdevice host:0204:6025 +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -usbdevice host:0204:6025 -usbdevice host:1234:0000 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.xml index b861665..6cb1ba1 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.xml @@ -24,5 +24,11 @@ <product id='0x6025'/> </source> </hostdev> + <hostdev mode='subsystem' type='usb' managed='no'> + <source> + <vendor id='0x1234'/> + <product id='0x0000'/> + </source> + </hostdev> </devices> </domain> -- 1.6.0.6
On Mon, Jun 15, 2009 at 06:11:09PM -0400, Cole Robinson wrote:
Product = 0 is a valid value based on this bug report:
https://www.redhat.com/archives/libvir-list/2009-May/msg00368.html
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/domain_conf.c | 12 ++++++++---- .../qemuxml2argv-hostdev-usb-product.args | 2 +- .../qemuxml2argv-hostdev-usb-product.xml | 6 ++++++ 3 files changed, 15 insertions(+), 5 deletions(-)
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Jun 15, 2009 at 06:11:09PM -0400, Cole Robinson wrote:
Product = 0 is a valid value based on this bug report:
https://www.redhat.com/archives/libvir-list/2009-May/msg00368.html
Looks fine, ACK. I was wondering about the vendor ID but apparently nobody uses 0 http://www.linux-usb.org/usb.ids but maybe the same cleanup should be done for vendor ID though it looks far less likely unless there is some hardware defect. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 06/16/2009 05:54 AM, Daniel Veillard wrote:
On Mon, Jun 15, 2009 at 06:11:09PM -0400, Cole Robinson wrote:
Product = 0 is a valid value based on this bug report:
https://www.redhat.com/archives/libvir-list/2009-May/msg00368.html
Looks fine, ACK. I was wondering about the vendor ID but apparently nobody uses 0 http://www.linux-usb.org/usb.ids but maybe the same cleanup should be done for vendor ID though it looks far less likely unless there is some hardware defect.
Committed now, with a similar cleanup for 'vendor', but only to give a less ambiguous error if vendor = 0. Thanks, Cole
On Mon, Jun 15, 2009 at 06:11:03PM -0400, Cole Robinson wrote:
New function qemudParseHelpStr was being called with arguments in the wrong order, so command flags == kvm_version :/
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu_conf.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Jun 15, 2009 at 06:11:03PM -0400, Cole Robinson wrote:
New function qemudParseHelpStr was being called with arguments in the wrong order, so command flags == kvm_version :/
Urgh ! ACK ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 06/16/2009 05:38 AM, Daniel Veillard wrote:
On Mon, Jun 15, 2009 at 06:11:03PM -0400, Cole Robinson wrote:
New function qemudParseHelpStr was being called with arguments in the wrong order, so command flags == kvm_version :/
Urgh ! ACK !
Daniel
Thanks, I've pushed this, and the rest of my posted patches. - Cole
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard