[libvirt] [PATCH] Fix usb master startport parsing

2013-04-25 Thread Martin Kletzander
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 mklet...@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 000..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 000..895d932
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-no-companion.xml
@@ -0,0 +1,21 @@
+domain type='qemu'
+  nameQEMUGuest1/name
+  uuidc7a5fdbd-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='0x' bus='0x00' slot='0x04' function='0'/
+/controller
+memballoon model='virtio'
+  address type='pci' domain='0x' 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,
 

Re: [libvirt] [PATCH] Fix usb master startport parsing

2013-04-25 Thread Daniel P. Berrange
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 000..895d932
 --- /dev/null
 +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-no-companion.xml
 @@ -0,0 +1,21 @@
 +domain type='qemu'
 +  nameQEMUGuest1/name
 +  uuidc7a5fdbd-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='0x' 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 :|

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