[libvirt] [PATCH] domain_conf: fix graphics parsing
Commit dc98a5bc refactored the code a lot and forget about checking if listen attribute is specified. This ensures that listen attribute and first listen element are compared only if both exist. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 3 +- .../qemuxml2argv-graphics-vnc-no-listen-attr.args | 22 ++++++++++++ .../qemuxml2argv-graphics-vnc-no-listen-attr.xml | 36 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + .../qemuxml2xmlout-graphics-vnc-no-listen-attr.xml | 41 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-no-listen-attr.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-no-listen-attr.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-vnc-no-listen-attr.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9e547b4..d2cf8d5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10741,7 +10741,8 @@ virDomainGraphicsListensParseXML(virDomainGraphicsDefPtr def, if (STREQ_NULLABLE(listenAddr, "")) VIR_FREE(listenAddr); - if (address && STRNEQ_NULLABLE(address->address, listenAddr)) { + if (address && listenAddr && + STRNEQ_NULLABLE(address->address, listenAddr)) { virReportError(VIR_ERR_XML_ERROR, _("graphics listen attribute %s must match address " "attribute of first listen element (found %s)"), diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-no-listen-attr.args b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-no-listen-attr.args new file mode 100644 index 0000000..f374cff --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-no-listen-attr.args @@ -0,0 +1,22 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-vnc '[2001:1:2:3:4:5:1234:1234]:3' \ +-vga cirrus diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-no-listen-attr.xml b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-no-listen-attr.xml new file mode 100644 index 0000000..f80a010 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-no-listen-attr.xml @@ -0,0 +1,36 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' port='5903' autoport='no'> + <listen type='address' address='2001:1:2:3:4:5:1234:1234'/> + </graphics> + <video> + <model type='cirrus' vram='16384' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index f1b2a8d..975e358 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -891,6 +891,7 @@ mymain(void) DO_TEST("graphics-vnc-socket", QEMU_CAPS_VNC); DO_TEST("graphics-vnc-websocket", QEMU_CAPS_VNC, QEMU_CAPS_VNC_WEBSOCKET); DO_TEST("graphics-vnc-policy", QEMU_CAPS_VNC, QEMU_CAPS_VNC_SHARE_POLICY); + DO_TEST("graphics-vnc-no-listen-attr", QEMU_CAPS_VNC); driver.config->vncSASL = 1; VIR_FREE(driver.config->vncSASLdir); diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-vnc-no-listen-attr.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-vnc-no-listen-attr.xml new file mode 100644 index 0000000..0627bbd --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-vnc-no-listen-attr.xml @@ -0,0 +1,41 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' port='5903' autoport='no' listen='2001:1:2:3:4:5:1234:1234'> + <listen type='address' address='2001:1:2:3:4:5:1234:1234'/> + </graphics> + <video> + <model type='cirrus' vram='16384' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 0735677..dddc775 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -430,6 +430,7 @@ mymain(void) DO_TEST("graphics-vnc-websocket"); DO_TEST("graphics-vnc-sasl"); DO_TEST("graphics-vnc-tls"); + DO_TEST("graphics-vnc-no-listen-attr"); DO_TEST("graphics-sdl"); DO_TEST("graphics-sdl-fullscreen"); DO_TEST("graphics-spice"); -- 2.8.1
On Mon, Apr 11, 2016 at 13:37:16 +0200, Pavel Hrdina wrote:
Commit dc98a5bc refactored the code a lot and forget about checking if listen attribute is specified. This ensures that listen attribute and first listen element are compared only if both exist.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 3 +- .../qemuxml2argv-graphics-vnc-no-listen-attr.args | 22 ++++++++++++ .../qemuxml2argv-graphics-vnc-no-listen-attr.xml | 36 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + .../qemuxml2xmlout-graphics-vnc-no-listen-attr.xml | 41 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-no-listen-attr.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-no-listen-attr.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-vnc-no-listen-attr.xml
ACK
On 04/11/2016 07:37 AM, Pavel Hrdina wrote:
Commit dc98a5bc refactored the code a lot and forget about checking if listen attribute is specified. This ensures that listen attribute and first listen element are compared only if both exist.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 3 +- .../qemuxml2argv-graphics-vnc-no-listen-attr.args | 22 ++++++++++++ .../qemuxml2argv-graphics-vnc-no-listen-attr.xml | 36 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + .../qemuxml2xmlout-graphics-vnc-no-listen-attr.xml | 41 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-no-listen-attr.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-no-listen-attr.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-vnc-no-listen-attr.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9e547b4..d2cf8d5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10741,7 +10741,8 @@ virDomainGraphicsListensParseXML(virDomainGraphicsDefPtr def, if (STREQ_NULLABLE(listenAddr, "")) VIR_FREE(listenAddr);
- if (address && STRNEQ_NULLABLE(address->address, listenAddr)) { + if (address && listenAddr && + STRNEQ_NULLABLE(address->address, listenAddr)) { virReportError(VIR_ERR_XML_ERROR, _("graphics listen attribute %s must match address " "attribute of first listen element (found %s)"), diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-no-listen-attr.args b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-no-listen-attr.args new file mode 100644 index 0000000..f374cff --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-no-listen-attr.args @@ -0,0 +1,22 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-vnc '[2001:1:2:3:4:5:1234:1234]:3' \ +-vga cirrus diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-no-listen-attr.xml b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-no-listen-attr.xml new file mode 100644 index 0000000..f80a010 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-no-listen-attr.xml @@ -0,0 +1,36 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk>
When adding new tests I think we should shoot for dropping as many redundant devices as possible... it's just extra time spent in the test suite. Not a blocker, just a general comment
+ <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' port='5903' autoport='no'> + <listen type='address' address='2001:1:2:3:4:5:1234:1234'/> + </graphics> + <video> + <model type='cirrus' vram='16384' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index f1b2a8d..975e358 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -891,6 +891,7 @@ mymain(void) DO_TEST("graphics-vnc-socket", QEMU_CAPS_VNC); DO_TEST("graphics-vnc-websocket", QEMU_CAPS_VNC, QEMU_CAPS_VNC_WEBSOCKET); DO_TEST("graphics-vnc-policy", QEMU_CAPS_VNC, QEMU_CAPS_VNC_SHARE_POLICY); + DO_TEST("graphics-vnc-no-listen-attr", QEMU_CAPS_VNC);
driver.config->vncSASL = 1; VIR_FREE(driver.config->vncSASLdir); diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-vnc-no-listen-attr.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-vnc-no-listen-attr.xml new file mode 100644 index 0000000..0627bbd --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-vnc-no-listen-attr.xml @@ -0,0 +1,41 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' port='5903' autoport='no' listen='2001:1:2:3:4:5:1234:1234'> + <listen type='address' address='2001:1:2:3:4:5:1234:1234'/> + </graphics> + <video> + <model type='cirrus' vram='16384' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 0735677..dddc775 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -430,6 +430,7 @@ mymain(void) DO_TEST("graphics-vnc-websocket"); DO_TEST("graphics-vnc-sasl"); DO_TEST("graphics-vnc-tls"); + DO_TEST("graphics-vnc-no-listen-attr"); DO_TEST("graphics-sdl"); DO_TEST("graphics-sdl-fullscreen"); DO_TEST("graphics-spice");
Adding a qemu test here is overkill IMO, this is just checking a function of the generic XML parser so can be more easily tested with genericxml2xml. In fact I already have patches testing the compat from the other direction: http://www.redhat.com/archives/libvir-list/2016-April/msg00395.html We could extend that test case with another <graphics> block that tests the listen compat in the other direction. - Cole
On Mon, Apr 11, 2016 at 09:44:17AM -0400, Cole Robinson wrote:
On 04/11/2016 07:37 AM, Pavel Hrdina wrote:
Commit dc98a5bc refactored the code a lot and forget about checking if listen attribute is specified. This ensures that listen attribute and first listen element are compared only if both exist.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> ---
[...]
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 0735677..dddc775 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -430,6 +430,7 @@ mymain(void) DO_TEST("graphics-vnc-websocket"); DO_TEST("graphics-vnc-sasl"); DO_TEST("graphics-vnc-tls"); + DO_TEST("graphics-vnc-no-listen-attr"); DO_TEST("graphics-sdl"); DO_TEST("graphics-sdl-fullscreen"); DO_TEST("graphics-spice");
Adding a qemu test here is overkill IMO, this is just checking a function of the generic XML parser so can be more easily tested with genericxml2xml. In fact I already have patches testing the compat from the other direction:
http://www.redhat.com/archives/libvir-list/2016-April/msg00395.html
We could extend that test case with another <graphics> block that tests the listen compat in the other direction.
That's a new information that we have genericxml2xml, I've somehow missed. In that case, it would be better to use this one only. Pavel
On 04/11/2016 09:44 AM, Cole Robinson wrote:
On 04/11/2016 07:37 AM, Pavel Hrdina wrote:
+ <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> When adding new tests I think we should shoot for dropping as many redundant devices as possible... it's just extra time spent in the test suite. Not a blocker, just a general comment
I was thinking the same thing the last time I was messing with the tests. A bit of redundancy can be a good thing, but we're testing the same paths of the same code hundreds of times in many cases, which not only takes extra runtime, but also makes for much larger patches when there is an expected/correct change, and can lead to failures in seemingly unrelated tests when there is an unintentional change. The problem (well, not really a problem in itself, but it's what leads to all the redundancy) is that we all make new tests by copying an old one in order to start out with something that is by definition correct - that's quicker than starting from scratch. In the most recent set of tests I added (for pxb) I tried removing as much obviously non-essential stuff as possible. Maybe having a "minimal" case for each test that could be pointed at in documentation as a template would be a help? (Or maybe they would be just more extra redundancy that would be ignored, not sure)
On 04/11/2016 11:59 AM, Laine Stump wrote:
On 04/11/2016 09:44 AM, Cole Robinson wrote:
On 04/11/2016 07:37 AM, Pavel Hrdina wrote:
+ <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> When adding new tests I think we should shoot for dropping as many redundant devices as possible... it's just extra time spent in the test suite. Not a blocker, just a general comment
I was thinking the same thing the last time I was messing with the tests. A bit of redundancy can be a good thing, but we're testing the same paths of the same code hundreds of times in many cases, which not only takes extra runtime, but also makes for much larger patches when there is an expected/correct change, and can lead to failures in seemingly unrelated tests when there is an unintentional change.
The problem (well, not really a problem in itself, but it's what leads to all the redundancy) is that we all make new tests by copying an old one in order to start out with something that is by definition correct - that's quicker than starting from scratch. In the most recent set of tests I added (for pxb) I tried removing as much obviously non-essential stuff as possible. Maybe having a "minimal" case for each test that could be pointed at in documentation as a template would be a help? (Or maybe they would be just more extra redundancy that would be ignored, not sure)
a minimal test/template would be nice. but also we have too many test cases anyways... many of the tests for qemu argv generation could be consolidated, like testing multiple disk configs in a single XML file for example. But part of that is checking gcov output to make sure we don't regress our coverage. I plan to come up with a few sample commits for things like that and then link them on BiteSizedTasks wiki page as a new contributor idea - Cole
participants (4)
-
Cole Robinson -
Laine Stump -
Pavel Hrdina -
Peter Krempa