[libvirt] [PATCH] Fix usb master startport parsing
When all usb controllers connected to the same bus have <master startport='x'/> specified, none of them have 'id=usb' assigned and thus qemu fails due to invalid masterport specification (we use 'usb' for that purpose). Adding a check that at least one of the controllers is specified without <master startport='x'/> and in case this happens, don't error out, just emit a warning and fix it within the first such controller found. This makes UX better by not forcing them to fix controller definition after removing the only non-master usb controller. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 17 ++++++++++++++--- .../qemuxml2argv-usb-ich9-no-companion.args | 6 ++++++ .../qemuxml2argv-usb-ich9-no-companion.xml | 21 +++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 +++ 4 files changed, 44 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-no-companion.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-no-companion.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 958b77b..a948cfc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9912,7 +9912,8 @@ virDomainDefParseXML(xmlDocPtr xml, virHashTablePtr bootHash = NULL; xmlNodePtr cur; bool usb_none = false; - bool usb_other = false; + virDomainControllerDefPtr usb_first = NULL; + bool usb_master = false; bool primaryVideo = false; if (VIR_ALLOC(def) < 0) { @@ -10855,7 +10856,7 @@ virDomainDefParseXML(xmlDocPtr xml, /* sanitize handling of "none" usb controller */ if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) { if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { - if (usb_other || usb_none) { + if (usb_first || usb_none) { virDomainControllerDefFree(controller); virReportError(VIR_ERR_XML_DETAIL, "%s", _("Can't add another USB controller: " @@ -10871,14 +10872,24 @@ virDomainDefParseXML(xmlDocPtr xml, "USB is disabled for this domain")); goto error; } - usb_other = true; + usb_first = controller; } + + if (controller->info.mastertype == VIR_DOMAIN_CONTROLLER_MASTER_NONE) + usb_master = true; } virDomainControllerInsertPreAlloced(def, controller); } VIR_FREE(nodes); + if (usb_first && !usb_master) { + VIR_WARN("No usb controller specified without master startport, " + "omitting startport in first USB controller"); + usb_first->info.master.usb.startport = 0; + usb_first->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_NONE; + } + if (def->virtType == VIR_DOMAIN_VIRT_QEMU || def->virtType == VIR_DOMAIN_VIRT_KQEMU || def->virtType == VIR_DOMAIN_VIRT_KVM) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-no-companion.args b/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-no-companion.args new file mode 100644 index 0000000..c97a6d3 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-no-companion.args @@ -0,0 +1,6 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc \ +-m 214 -smp 1 -nographic -nodefconfig -nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c \ +-device ich9-usb-uhci2,id=usb,bus=pci.0,addr=0x4 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-no-companion.xml b/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-no-companion.xml new file mode 100644 index 0000000..895d932 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-no-companion.xml @@ -0,0 +1,21 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0'/> + </controller> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index f40d002..e485a70 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -768,6 +768,9 @@ mymain(void) DO_TEST("usb-ich9-companion", QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1); + DO_TEST("usb-ich9-no-companion", + QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1); DO_TEST("usb-hub", QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_USB_HUB, QEMU_CAPS_NODEFCONFIG); -- 1.8.2.1
On Thu, Apr 25, 2013 at 01:05:49PM +0200, Martin Kletzander wrote:
When all usb controllers connected to the same bus have <master startport='x'/> specified, none of them have 'id=usb' assigned and thus qemu fails due to invalid masterport specification (we use 'usb' for that purpose). Adding a check that at least one of the controllers is specified without <master startport='x'/> and in case this happens, don't error out, just emit a warning and fix it within the first such controller found. This makes UX better by not forcing them to fix controller definition after removing the only non-master usb controller.
No, using VIR_WARN in the parser is a really bad. This is a broken configuration and should be reported as such. It is not the responsibility of libvirt to workaround apps generating broken configs. if the user removes the master controller, then the app should be removing any non-master controllers that depend on it.
new file mode 100644 index 0000000..895d932 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-no-companion.xml @@ -0,0 +1,21 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0'/> + </controller>
This is just a broken configuration since there is no master controller to assocated with the companion with. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
When all usb controllers connected to the same bus have <master startport='x'/> specified, none of them have 'id=usb' assigned and thus qemu fails due to invalid masterport specification (we use 'usb' for that purpose). Adding a check that at least one of the controllers is specified without <master startport='x'/> and in case this happens, error out due to invalid configuration. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2: - Don't fix user/app errors, just error out. --- src/conf/domain_conf.c | 10 ++++++++++ .../qemuxml2argv-usb-ich9-no-companion.xml | 21 +++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 +++ 3 files changed, 34 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-no-companion.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 958b77b..99d3232 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9913,6 +9913,7 @@ virDomainDefParseXML(xmlDocPtr xml, xmlNodePtr cur; bool usb_none = false; bool usb_other = false; + bool usb_master = false; bool primaryVideo = false; if (VIR_ALLOC(def) < 0) { @@ -10873,12 +10874,21 @@ virDomainDefParseXML(xmlDocPtr xml, } usb_other = true; } + + if (controller->info.mastertype == VIR_DOMAIN_CONTROLLER_MASTER_NONE) + usb_master = true; } virDomainControllerInsertPreAlloced(def, controller); } VIR_FREE(nodes); + if (usb_first && !usb_master) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("No master USB controller specified")); + goto error; + } + if (def->virtType == VIR_DOMAIN_VIRT_QEMU || def->virtType == VIR_DOMAIN_VIRT_KQEMU || def->virtType == VIR_DOMAIN_VIRT_KVM) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-no-companion.xml b/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-no-companion.xml new file mode 100644 index 0000000..895d932 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-no-companion.xml @@ -0,0 +1,21 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0'/> + </controller> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index f40d002..abb3b27 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -768,6 +768,9 @@ mymain(void) DO_TEST("usb-ich9-companion", QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1); + DO_TEST_FAILURE("usb-ich9-no-companion", + QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1); DO_TEST("usb-hub", QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_USB_HUB, QEMU_CAPS_NODEFCONFIG); -- 1.8.2.1
On Thu, Apr 25, 2013 at 01:59:04PM +0200, Martin Kletzander wrote:
When all usb controllers connected to the same bus have <master startport='x'/> specified, none of them have 'id=usb' assigned and thus qemu fails due to invalid masterport specification (we use 'usb' for that purpose). Adding a check that at least one of the controllers is specified without <master startport='x'/> and in case this happens, error out due to invalid configuration.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2: - Don't fix user/app errors, just error out. --- src/conf/domain_conf.c | 10 ++++++++++ .../qemuxml2argv-usb-ich9-no-companion.xml | 21 +++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 +++ 3 files changed, 34 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-no-companion.xml
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 04/25/2013 02:45 PM, Daniel P. Berrange wrote:
On Thu, Apr 25, 2013 at 01:59:04PM +0200, Martin Kletzander wrote:
When all usb controllers connected to the same bus have <master startport='x'/> specified, none of them have 'id=usb' assigned and thus qemu fails due to invalid masterport specification (we use 'usb' for that purpose). Adding a check that at least one of the controllers is specified without <master startport='x'/> and in case this happens, error out due to invalid configuration.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2: - Don't fix user/app errors, just error out. --- src/conf/domain_conf.c | 10 ++++++++++ .../qemuxml2argv-usb-ich9-no-companion.xml | 21 +++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 +++ 3 files changed, 34 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-no-companion.xml
ACK
Daniel
Thanks, I've fixed up just a nit to make it build && check properly and pushed. Martin
participants (2)
-
Daniel P. Berrange -
Martin Kletzander