[libvirt] [PATCH] Fix vm define error with back compat console device
If you define a domain with serial devs > 0 && parallel devs >= serial devs, libvirtd segfaults when trying to set up the back compat console device. We were using a previous loop counter where we shouldn't. The attached patch fixes this. Thanks, Cole
Cole Robinson <crobinso@redhat.com> wrote:
If you define a domain with serial devs > 0 && parallel devs >= serial devs, libvirtd segfaults when trying to set up the back compat console device. We were using a previous loop counter where we shouldn't. The attached patch fixes this.
Thanks, Cole diff --git a/src/domain_conf.c b/src/domain_conf.c index 8bb51f7..890afe0 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -3320,8 +3320,9 @@ char *virDomainDefFormat(virConnectPtr conn, if (virDomainChrDefFormat(conn, &buf, def->console, "console") < 0) goto cleanup; } else if (def->nserials != 0) { - /* ..else for legacy compat duplicate the serial device as a console */ - if (virDomainChrDefFormat(conn, &buf, def->serials[n], "console") < 0) + /* ..else for legacy compat duplicate the first serial device as a + * console */ + if (virDomainChrDefFormat(conn, &buf, def->serials[0], "console") < 0) goto cleanup; }
Hi Cole, ACK! This looks like an obviously-right fix. Thanks for the sample input file you provided, I confirmed and used it to write a test case that demonstrates the problem. With the following test, running this command, make check -C tests TESTS=define-dev-segfault VERBOSE=yes would fail with output including this: + libvirtd + virsh --connect qemu:///session define devs.xml ./define-dev-segfault: line 84: 4882 Segmentation fault libvirtd > log 2>&1 With your patch applied, the test passes.
From b16f407a6369cf4c3a5770c4b49515b565f29b4f Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 15 Jan 2009 19:51:39 +0100 Subject: [PATCH] exercise a bug that could make libvirtd segfault
* tests/define-dev-segfault: New file. * tests/Makefile.am (test_scripts): Add define-dev-segfault. --- tests/Makefile.am | 1 + tests/define-dev-segfault | 92 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 0 deletions(-) create mode 100755 tests/define-dev-segfault diff --git a/tests/Makefile.am b/tests/Makefile.am index 0486ee3..7acb745 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -59,6 +59,7 @@ test_scripts = domainschematest if WITH_LIBVIRTD test_scripts += \ test_conf.sh \ + define-dev-segfault \ cpuset \ daemon-conf \ int-overflow \ diff --git a/tests/define-dev-segfault b/tests/define-dev-segfault new file mode 100755 index 0000000..903568f --- /dev/null +++ b/tests/define-dev-segfault @@ -0,0 +1,92 @@ +#!/bin/sh +# Exercise a bug whereby defining a valid domain could kill libvirtd. + +if test "$VERBOSE" = yes; then + set -x + libvirtd --version + virsh --version +fi + +test -z "$srcdir" && srcdir=$(pwd) +test -z "$abs_top_srcdir" && abs_top_srcdir=$(pwd)/.. +. "$srcdir/test-lib.sh" + +fail=0 + +# Domain definition from Cole Robinson. +cat <<\EOF > devs.xml || fail=1 +<domain type='kvm'> +<name>D</name> +<uuid>aaa3ae22-fed2-bfbd-ac02-3bea3bcfad82</uuid> +<memory>262144</memory> +<currentMemory>262144</currentMemory> +<vcpu>1</vcpu> +<os> +<type arch='i686' machine='pc'>hvm</type> +<boot dev='cdrom'/> +</os> +<features> +<acpi/> +</features> +<clock offset='utc'/> +<on_poweroff>destroy</on_poweroff> +<on_reboot>restart</on_reboot> +<on_crash>destroy</on_crash> +<devices> +<emulator>/usr/bin/qemu-kvm</emulator> +<disk type='file' device='cdrom'> + <target dev='hdc' bus='ide'/> + <readonly/> +</disk> +<disk type='file' device='floppy'> + <target dev='fdb' bus='fdc'/> +</disk> +<disk type='file' device='cdrom'> +<target dev='sda' bus='scsi'/> +<readonly/> +</disk> +<interface type='network'> +<mac address='54:52:00:6c:a0:ca'/> +<source network='aaaaaa'/> +</interface> +<interface type='network'> +<mac address='54:52:00:6c:bb:ca'/> +<source network='default'/> +</interface> +<serial type='pty'/> +<serial type='pty'/> +<serial type='pty'/> +<parallel type='pty'/> +<parallel type='pty'/> +<parallel type='pty'/> +<input type='mouse' bus='ps2'/> +<sound model='pcspk'/> +<sound model='es1370'/> +<hostdev mode='subsystem' type='usb'> +<source> +<address bus='3' device='3'/> +</source> +</hostdev> +<hostdev mode='subsystem' type='usb'> +<source> +<vendor id='0x0483'/> +<product id='0x2016'/> +</source> +</hostdev> +</devices> +</domain> +EOF + +libvirtd > log 2>&1 & pid=$! +sleep 1 + +virsh --connect qemu:///session define devs.xml > out 2>&1 \ + || fail=1 + +kill $pid + +printf 'Domain D defined from devs.xml\n\n' > exp || fail=1 + +compare exp out || fail=1 +compare /dev/null log || fail=1 +exit $fail -- 1.6.1.233.g5b4a8
On Thu, Jan 15, 2009 at 07:55:52PM +0100, Jim Meyering wrote:
Cole Robinson <crobinso@redhat.com> wrote:
If you define a domain with serial devs > 0 && parallel devs >= serial devs, libvirtd segfaults when trying to set up the back compat console device. We were using a previous loop counter where we shouldn't. The attached patch fixes this.
Thanks, Cole diff --git a/src/domain_conf.c b/src/domain_conf.c index 8bb51f7..890afe0 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -3320,8 +3320,9 @@ char *virDomainDefFormat(virConnectPtr conn, if (virDomainChrDefFormat(conn, &buf, def->console, "console") < 0) goto cleanup; } else if (def->nserials != 0) { - /* ..else for legacy compat duplicate the serial device as a console */ - if (virDomainChrDefFormat(conn, &buf, def->serials[n], "console") < 0) + /* ..else for legacy compat duplicate the first serial device as a + * console */ + if (virDomainChrDefFormat(conn, &buf, def->serials[0], "console") < 0) goto cleanup; }
Hi Cole,
ACK! This looks like an obviously-right fix. Thanks for the sample input file you provided,
I confirmed and used it to write a test case that demonstrates the problem.
With the following test, running this command,
make check -C tests TESTS=define-dev-segfault VERBOSE=yes
would fail with output including this:
+ libvirtd + virsh --connect qemu:///session define devs.xml
Shouldn't use qemu:///session for test cases like this - this is what the test:///default driver is for, avoiding the fragility & danger of using the daemon & live hypervisor drivers.
+# Domain definition from Cole Robinson. +cat <<\EOF > devs.xml || fail=1 +<domain type='kvm'> +<name>D</name> +<uuid>aaa3ae22-fed2-bfbd-ac02-3bea3bcfad82</uuid> +<memory>262144</memory> +<currentMemory>262144</currentMemory> +<vcpu>1</vcpu> +<os> +<type arch='i686' machine='pc'>hvm</type> +<boot dev='cdrom'/> +</os> +<features> +<acpi/> +</features> +<clock offset='utc'/> +<on_poweroff>destroy</on_poweroff> +<on_reboot>restart</on_reboot> +<on_crash>destroy</on_crash> +<devices> +<emulator>/usr/bin/qemu-kvm</emulator> +<disk type='file' device='cdrom'> + <target dev='hdc' bus='ide'/> + <readonly/> +</disk> +<disk type='file' device='floppy'> + <target dev='fdb' bus='fdc'/> +</disk> +<disk type='file' device='cdrom'> +<target dev='sda' bus='scsi'/> +<readonly/> +</disk> +<interface type='network'> +<mac address='54:52:00:6c:a0:ca'/> +<source network='aaaaaa'/> +</interface> +<interface type='network'> +<mac address='54:52:00:6c:bb:ca'/> +<source network='default'/> +</interface> +<serial type='pty'/> +<serial type='pty'/> +<serial type='pty'/> +<parallel type='pty'/> +<parallel type='pty'/> +<parallel type='pty'/> +<input type='mouse' bus='ps2'/> +<sound model='pcspk'/> +<sound model='es1370'/> +<hostdev mode='subsystem' type='usb'> +<source> +<address bus='3' device='3'/> +</source> +</hostdev> +<hostdev mode='subsystem' type='usb'> +<source> +<vendor id='0x0483'/> +<product id='0x2016'/> +</source> +</hostdev> +</devices> +</domain> +EOF
Can you pass that XML through xmllint -format so is has more readable indentation - good to remove all the disk/interface/hostdev devices too, since they're not relevant to the test in question.
+libvirtd > log 2>&1 & pid=$! +sleep 1
No need for the libvirtd daemon if using the test driver. 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 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
+ virsh --connect qemu:///session define devs.xml
Shouldn't use qemu:///session for test cases like this - this is what the test:///default driver is for, avoiding the fragility & danger of using the daemon & live hypervisor drivers.
There's no failure with test:///default. But I'll adjust it to use the new unix_sock_dir config option I proposed. However, when run by root, it'll still leave logs in the wrong place, which is currently hard-coded, so I'll have to do more. Either add a config. directive to change the root (log/sockdir,etc) directory, or yet another config setting, just for the log. Preference: log_dir or log_file?
Can you pass that XML through xmllint -format so is has more
Good idea. Done.
readable indentation - good to remove all the disk/interface/hostdev devices too, since they're not relevant to the test in question.
too much hubris ;-) Done, too.
+libvirtd > log 2>&1 & pid=$! +sleep 1
No need for the libvirtd daemon if using the test driver.
On Thu, Jan 15, 2009 at 10:17:56PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
+ virsh --connect qemu:///session define devs.xml
Shouldn't use qemu:///session for test cases like this - this is what the test:///default driver is for, avoiding the fragility & danger of using the daemon & live hypervisor drivers.
There's no failure with test:///default.
I'm rather surprised at that - both drivers use identical XML formating routines here, so given the same config, both should fail just as badly. Is this perhaps a case where we need to run it under valgrind to make it reliably fail. 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 Fri, Jan 16, 2009 at 12:09:33AM +0000, Daniel P. Berrange wrote:
On Thu, Jan 15, 2009 at 10:17:56PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
+ virsh --connect qemu:///session define devs.xml
Shouldn't use qemu:///session for test cases like this - this is what the test:///default driver is for, avoiding the fragility & danger of using the daemon & live hypervisor drivers.
There's no failure with test:///default.
I'm rather surprised at that - both drivers use identical XML formating routines here, so given the same config, both should fail just as badly. Is this perhaps a case where we need to run it under valgrind to make it reliably fail.
Maybe that can be debugged separately, I would like to see the patch fixed, maybe we can isolate the problem in the test driver (if any) but it should not block the initial patch from being commited, thanks, 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/
Daniel Veillard <veillard@redhat.com> wrote:
On Fri, Jan 16, 2009 at 12:09:33AM +0000, Daniel P. Berrange wrote:
On Thu, Jan 15, 2009 at 10:17:56PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
+ virsh --connect qemu:///session define devs.xml
Shouldn't use qemu:///session for test cases like this - this is what the test:///default driver is for, avoiding the fragility & danger of using the daemon & live hypervisor drivers.
There's no failure with test:///default.
I'm rather surprised at that - both drivers use identical XML formating routines here, so given the same config, both should fail just as badly. Is this perhaps a case where we need to run it under valgrind to make it reliably fail.
Maybe that can be debugged separately, I would like to see the patch fixed, maybe we can isolate the problem in the test driver (if any) but it should not block the initial patch from being commited,
Cole, can you commit your fix? As soon as that's in, I'll commit the following simple test to exercise it: It's similar to a previous patch, but with the following changes - uses test:///default driver rather than the qemu: one, per Daniel Berrange's suggestion - adjusts its input XML so that it's identical to dumpxml output I'm sure Dan and I will be talking about bigger, long-term testing framework changes in the next week or so. Jim
From bc7d653b1c7039da3edb63528a83bdc9609c7b6f Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 15 Jan 2009 19:51:39 +0100 Subject: [PATCH] exercise a bug that could make libvirtd segfault
* tests/define-dev-segfault: New file. * tests/Makefile.am (test_scripts): Add define-dev-segfault. --- tests/Makefile.am | 1 + tests/define-dev-segfault | 76 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 0 deletions(-) create mode 100755 tests/define-dev-segfault diff --git a/tests/Makefile.am b/tests/Makefile.am index c735d62..39b5727 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -60,6 +60,7 @@ if WITH_LIBVIRTD test_scripts += \ cpuset \ daemon-conf \ + define-dev-segfault \ int-overflow \ libvirtd-fail \ libvirtd-pool \ diff --git a/tests/define-dev-segfault b/tests/define-dev-segfault new file mode 100755 index 0000000..4ae286f --- /dev/null +++ b/tests/define-dev-segfault @@ -0,0 +1,76 @@ +#!/bin/sh +# Exercise a bug whereby defining a valid domain could kill libvirtd. +# The bug can also be exercised with a simple define/dumpxml pair to virsh. + +if test "$VERBOSE" = yes; then + set -x + virsh --version +fi + +test -z "$srcdir" && srcdir=$(pwd) +test -z "$abs_top_srcdir" && abs_top_srcdir=$(pwd)/.. +. "$srcdir/test-lib.sh" + +fail=0 + +# Domain definition from Cole Robinson. +cat <<\EOF > D.xml || fail=1 +<domain type='kvm'> + <name>D</name> + <uuid>aaa3ae22-fed2-bfbd-ac02-3bea3bcfad82</uuid> + <memory>262144</memory> + <currentMemory>262144</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='cdrom'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-kvm</emulator> + <serial type='pty'> + <target port='0'/> + </serial> + <serial type='pty'> + <target port='1'/> + </serial> + <serial type='pty'> + <target port='2'/> + </serial> + <parallel type='pty'> + <target port='0'/> + </parallel> + <parallel type='pty'> + <target port='1'/> + </parallel> + <parallel type='pty'> + <target port='2'/> + </parallel> + <console type='pty'> + <target port='0'/> + </console> + <sound model='pcspk'/> + <sound model='es1370'/> + </devices> +</domain> +EOF + +url=test:///default +virsh --connect "$url" 'define D.xml; dumpxml D' > out 2>&1 || fail=1 + +cat > exp <<EOF || fail=1 +Domain D defined from D.xml + +$(cat D.xml) + +EOF + +compare exp out || fail=1 + +exit $fail -- 1.6.1.258.g7ff14
Jim Meyering wrote:
Daniel Veillard <veillard@redhat.com> wrote:
On Thu, Jan 15, 2009 at 10:17:56PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
+ virsh --connect qemu:///session define devs.xml Shouldn't use qemu:///session for test cases like this - this is what the test:///default driver is for, avoiding the fragility & danger of using the daemon & live hypervisor drivers. There's no failure with test:///default. I'm rather surprised at that - both drivers use identical XML formating routines here, so given the same config, both should fail just as badly. Is this perhaps a case where we need to run it under valgrind to make it reliably fail. Maybe that can be debugged separately, I would like to see the
On Fri, Jan 16, 2009 at 12:09:33AM +0000, Daniel P. Berrange wrote: patch fixed, maybe we can isolate the problem in the test driver (if any) but it should not block the initial patch from being commited,
Cole, can you commit your fix?
Pushed now. Thanks, Cole
Cole Robinson <crobinso@redhat.com> wrote:
Jim Meyering wrote:
Daniel Veillard <veillard@redhat.com> wrote:
On Thu, Jan 15, 2009 at 10:17:56PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
> + virsh --connect qemu:///session define devs.xml Shouldn't use qemu:///session for test cases like this - this is what the test:///default driver is for, avoiding the fragility & danger of using the daemon & live hypervisor drivers. There's no failure with test:///default. I'm rather surprised at that - both drivers use identical XML formating routines here, so given the same config, both should fail just as badly. Is this perhaps a case where we need to run it under valgrind to make it reliably fail. Maybe that can be debugged separately, I would like to see the
On Fri, Jan 16, 2009 at 12:09:33AM +0000, Daniel P. Berrange wrote: patch fixed, maybe we can isolate the problem in the test driver (if any) but it should not block the initial patch from being commited,
Cole, can you commit your fix?
Pushed now.
Thanks! I've pushed the following two changes and rebased the git branch:
From 1c8099e9e4bfc4c1c3ca2f003822040d72c9cfcf Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 16 Jan 2009 18:06:33 +0000 Subject: [PATCH 1/2] tests: exercise a bug that could make virsh and libvirtd segfault
* tests/define-dev-segfault: New file. * tests/Makefile.am (test_scripts): Add define-dev-segfault. --- ChangeLog | 6 +++ tests/Makefile.am | 1 + tests/define-dev-segfault | 76 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 0 deletions(-) create mode 100755 tests/define-dev-segfault diff --git a/ChangeLog b/ChangeLog index 7655836..536c563 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +Fri Jan 16 18:41:40 +0100 2009 Jim Meyering <meyering@redhat.com> + + tests: exercise a bug that could make virsh and libvirtd segfault + * tests/define-dev-segfault: New file. + * tests/Makefile.am (test_scripts): Add define-dev-segfault. + Fri Jan 16 11:48:41 EST 2009 Cole Robinson <crobinso@redhat.com> * src/domain_conf.c: Fix segfault with console device back compat. diff --git a/tests/Makefile.am b/tests/Makefile.am index 0486ee3..98739c5 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -61,6 +61,7 @@ test_scripts += \ test_conf.sh \ cpuset \ daemon-conf \ + define-dev-segfault \ int-overflow \ read-bufsiz \ read-non-seekable \ diff --git a/tests/define-dev-segfault b/tests/define-dev-segfault new file mode 100755 index 0000000..4ae286f --- /dev/null +++ b/tests/define-dev-segfault @@ -0,0 +1,76 @@ +#!/bin/sh +# Exercise a bug whereby defining a valid domain could kill libvirtd. +# The bug can also be exercised with a simple define/dumpxml pair to virsh. + +if test "$VERBOSE" = yes; then + set -x + virsh --version +fi + +test -z "$srcdir" && srcdir=$(pwd) +test -z "$abs_top_srcdir" && abs_top_srcdir=$(pwd)/.. +. "$srcdir/test-lib.sh" + +fail=0 + +# Domain definition from Cole Robinson. +cat <<\EOF > D.xml || fail=1 +<domain type='kvm'> + <name>D</name> + <uuid>aaa3ae22-fed2-bfbd-ac02-3bea3bcfad82</uuid> + <memory>262144</memory> + <currentMemory>262144</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='cdrom'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-kvm</emulator> + <serial type='pty'> + <target port='0'/> + </serial> + <serial type='pty'> + <target port='1'/> + </serial> + <serial type='pty'> + <target port='2'/> + </serial> + <parallel type='pty'> + <target port='0'/> + </parallel> + <parallel type='pty'> + <target port='1'/> + </parallel> + <parallel type='pty'> + <target port='2'/> + </parallel> + <console type='pty'> + <target port='0'/> + </console> + <sound model='pcspk'/> + <sound model='es1370'/> + </devices> +</domain> +EOF + +url=test:///default +virsh --connect "$url" 'define D.xml; dumpxml D' > out 2>&1 || fail=1 + +cat > exp <<EOF || fail=1 +Domain D defined from D.xml + +$(cat D.xml) + +EOF + +compare exp out || fail=1 + +exit $fail -- 1.6.1.258.g7ff14
From 0337ba238e69322750d3036f6794798ff1ed80e5 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 16 Jan 2009 18:07:24 +0000 Subject: [PATCH 2/2] tests: virsh-all and virsh-synopsis were not being run
* tests/Makefile.am (test_scripts): Add two missing backslashes. --- ChangeLog | 5 ++++- tests/Makefile.am | 24 ++++++++++++------------ 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/ChangeLog b/ChangeLog index 536c563..689ec9e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,4 +1,7 @@ -Fri Jan 16 18:41:40 +0100 2009 Jim Meyering <meyering@redhat.com> +Fri Jan 16 18:44:08 +0100 2009 Jim Meyering <meyering@redhat.com> + + tests: virsh-all and virsh-synopsis were not being run + * tests/Makefile.am (test_scripts): Add two missing backslashes. tests: exercise a bug that could make virsh and libvirtd segfault * tests/define-dev-segfault: New file. diff --git a/tests/Makefile.am b/tests/Makefile.am index 98739c5..a2e5c0f 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -57,18 +57,18 @@ endif test_scripts = domainschematest if WITH_LIBVIRTD -test_scripts += \ - test_conf.sh \ - cpuset \ - daemon-conf \ - define-dev-segfault \ - int-overflow \ - read-bufsiz \ - read-non-seekable \ - start \ - undefine \ - vcpupin - virsh-all +test_scripts += \ + test_conf.sh \ + cpuset \ + daemon-conf \ + define-dev-segfault \ + int-overflow \ + read-bufsiz \ + read-non-seekable \ + start \ + undefine \ + vcpupin \ + virsh-all \ virsh-synopsis endif -- 1.6.1.258.g7ff14
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Thu, Jan 15, 2009 at 10:17:56PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
+ virsh --connect qemu:///session define devs.xml
Shouldn't use qemu:///session for test cases like this - this is what the test:///default driver is for, avoiding the fragility & danger of using the daemon & live hypervisor drivers.
There's no failure with test:///default.
I'm rather surprised at that - both drivers use identical XML formating routines here, so given the same config, both should fail just as badly. Is this perhaps a case where we need to run it under valgrind to make it reliably fail.
Note that virsh itself doesn't fail, in either case. It sends info to libvirtd that causes *it* to segfault, so in order to trigger the bug, I have to run libvirtd.
On Fri, Jan 16, 2009 at 09:24:33AM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Thu, Jan 15, 2009 at 10:17:56PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
+ virsh --connect qemu:///session define devs.xml
Shouldn't use qemu:///session for test cases like this - this is what the test:///default driver is for, avoiding the fragility & danger of using the daemon & live hypervisor drivers.
There's no failure with test:///default.
I'm rather surprised at that - both drivers use identical XML formating routines here, so given the same config, both should fail just as badly. Is this perhaps a case where we need to run it under valgrind to make it reliably fail.
Note that virsh itself doesn't fail, in either case. It sends info to libvirtd that causes *it* to segfault, so in order to trigger the bug, I have to run libvirtd.
virsh SEGV's nicely enough for me $ ./src/virsh --connect test:///default Welcome to lt-virsh, the virtualization interactive terminal. Type: 'help' for help with commands 'quit' to quit virsh # define a.xml Domain D defined from a.xml virsh # dumpxml D Segmentation fault In fact running virsh at all is redundant - this fails nicely enough if it is just added as a new datafile to the qemuxml2xmltest test case. Index: tests/qemuxml2xmltest.c =================================================================== RCS file: /data/cvs/libvirt/tests/qemuxml2xmltest.c,v retrieving revision 1.25 diff -u -p -u -p -r1.25 qemuxml2xmltest.c --- tests/qemuxml2xmltest.c 12 Jan 2009 15:09:19 -0000 1.25 +++ tests/qemuxml2xmltest.c 16 Jan 2009 10:04:07 -0000 @@ -120,6 +120,7 @@ mymain(int argc, char **argv) DO_TEST("serial-many"); DO_TEST("parallel-tcp"); DO_TEST("console-compat"); + DO_TEST("console-overflow"); DO_TEST("hostdev-usb-product"); DO_TEST("hostdev-usb-address"); Index: tests/qemuxml2argvdata/qemuxml2argv-console-overflow.xml =================================================================== RCS file: tests/qemuxml2argvdata/qemuxml2argv-console-overflow.xml diff -N tests/qemuxml2argvdata/qemuxml2argv-console-overflow.xml --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/qemuxml2argvdata/qemuxml2argv-console-overflow.xml 16 Jan 2009 10:04:07 -0000 @@ -0,0 +1,61 @@ +<?xml version="1.0"?> +<domain type="kvm"> + <name>D</name> + <uuid>aaa3ae22-fed2-bfbd-ac02-3bea3bcfad82</uuid> + <memory>262144</memory> + <currentMemory>262144</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch="i686" machine="pc">hvm</type> + <boot dev="cdrom"/> + </os> + <features> + <acpi/> + </features> + <clock offset="utc"/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-kvm</emulator> + <disk type="file" device="cdrom"> + <target dev="hdc" bus="ide"/> + <readonly/> + </disk> + <disk type="file" device="floppy"> + <target dev="fdb" bus="fdc"/> + </disk> + <disk type="file" device="cdrom"> + <target dev="sda" bus="scsi"/> + <readonly/> + </disk> + <interface type="network"> + <mac address="54:52:00:6c:a0:ca"/> + <source network="aaaaaa"/> + </interface> + <interface type="network"> + <mac address="54:52:00:6c:bb:ca"/> + <source network="default"/> + </interface> + <serial type="pty"/> + <serial type="pty"/> + <serial type="pty"/> + <parallel type="pty"/> + <parallel type="pty"/> + <parallel type="pty"/> + <input type="mouse" bus="ps2"/> + <sound model="pcspk"/> + <sound model="es1370"/> + <hostdev mode="subsystem" type="usb"> + <source> + <address bus="3" device="3"/> + </source> + </hostdev> + <hostdev mode="subsystem" type="usb"> + <source> + <vendor id="0x0483"/> + <product id="0x2016"/> + </source> + </hostdev> + </devices> +</domain> 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 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Fri, Jan 16, 2009 at 09:24:33AM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Thu, Jan 15, 2009 at 10:17:56PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
+ virsh --connect qemu:///session define devs.xml
Shouldn't use qemu:///session for test cases like this - this is what the test:///default driver is for, avoiding the fragility & danger of using the daemon & live hypervisor drivers.
There's no failure with test:///default.
I'm rather surprised at that - both drivers use identical XML formating routines here, so given the same config, both should fail just as badly. Is this perhaps a case where we need to run it under valgrind to make it reliably fail.
Note that virsh itself doesn't fail, in either case. It sends info to libvirtd that causes *it* to segfault, so in order to trigger the bug, I have to run libvirtd.
virsh SEGV's nicely enough for me
$ ./src/virsh --connect test:///default Welcome to lt-virsh, the virtualization interactive terminal.
Type: 'help' for help with commands 'quit' to quit
virsh # define a.xml Domain D defined from a.xml
virsh # dumpxml D Segmentation fault
Ah. I'd only run the define, which is enough to make libvirtd fail when using qemu:///session.
In fact running virsh at all is redundant - this fails nicely enough if it is just added as a new datafile to the qemuxml2xmltest test case.
Putting that test closer to the target code is good, so ACK, assuming it passes "make distcheck". However, part of my goal in running the qemu:///session server is to get more coverage on the server side. "make check" does very little testing of libvirtd, currently. So if you don't mind, I'll also add a comparable test using qemu:///session and unix_sock_dir, so it doesn't interfere with existing state. Maybe you want to keep the extra disk and interface sections after all? (the ones that you suggested I remove) Better coverage?
On Fri, Jan 16, 2009 at 12:42:35PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Fri, Jan 16, 2009 at 09:24:33AM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Thu, Jan 15, 2009 at 10:17:56PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
> + virsh --connect qemu:///session define devs.xml
Shouldn't use qemu:///session for test cases like this - this is what the test:///default driver is for, avoiding the fragility & danger of using the daemon & live hypervisor drivers.
There's no failure with test:///default.
I'm rather surprised at that - both drivers use identical XML formating routines here, so given the same config, both should fail just as badly. Is this perhaps a case where we need to run it under valgrind to make it reliably fail.
Note that virsh itself doesn't fail, in either case. It sends info to libvirtd that causes *it* to segfault, so in order to trigger the bug, I have to run libvirtd.
virsh SEGV's nicely enough for me
$ ./src/virsh --connect test:///default Welcome to lt-virsh, the virtualization interactive terminal.
Type: 'help' for help with commands 'quit' to quit
virsh # define a.xml Domain D defined from a.xml
virsh # dumpxml D Segmentation fault
Ah. I'd only run the define, which is enough to make libvirtd fail when using qemu:///session.
Oh, that'll be because when you run 'define' in QEMU, it parses the XML and then runs dumpxml internally to save it out to disk
In fact running virsh at all is redundant - this fails nicely enough if it is just added as a new datafile to the qemuxml2xmltest test case.
Putting that test closer to the target code is good, so ACK, assuming it passes "make distcheck".
However, part of my goal in running the qemu:///session server is to get more coverage on the server side. "make check" does very little testing of libvirtd, currently. So if you don't mind, I'll also add a comparable test using qemu:///session and unix_sock_dir, so it doesn't interfere with existing state.
I think this is the wrong way to go about testing the real live hypervisor drivers. For those we should have something that is like the libvirt-CIM test suite. eg a test system which has code to run all the libvirt APIs. This single test system is then run in a controlled environment once for each hypervisor URI that we support. This will mean we exercise all the core codepaths for the real drivers, and also validate that they are all responding in the same way with consistent semantics / underling operations. Adding ad-hoc test cases to leverage qemu:///session is wasted effort because its creating a functional/integration test suite which is neccessarily tied to QEMU driver, as opposed to being a generic solution. The 'make check' testsuite should be focused on unit testing either by calling specific APIs relating to the hypervisor specific code, or be running commands with the 'test' driver. The test driver has sufficient functionality to let us functionally test core operation of the libvirtd daemon too. So, this is the distinction between calling 'qemudBuildCommandLine' to test QEMU driver command line arg processing, and running the 'start' operation on the QEMU driver which indirectly calls the "qemudBuildCommandLine" method.
Maybe you want to keep the extra disk and interface sections after all? (the ones that you suggested I remove) Better coverage?
There's already a tonne of data files in qemuxml2argvdata that exercise the disk/nic/hostdev bits of the XML parser. The XML parsing is the one area where we have generally good test coverage for all common paths and just need to look at the code coverage reports to find edge cases like this scenario we're fixing here. 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 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Fri, Jan 16, 2009 at 12:42:35PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Fri, Jan 16, 2009 at 09:24:33AM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Thu, Jan 15, 2009 at 10:17:56PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ... >> + virsh --connect qemu:///session define devs.xml > > Shouldn't use qemu:///session for test cases like this - this is what > the test:///default driver is for, avoiding the fragility & danger of > using the daemon & live hypervisor drivers.
There's no failure with test:///default.
I'm rather surprised at that - both drivers use identical XML formating routines here, so given the same config, both should fail just as badly. Is this perhaps a case where we need to run it under valgrind to make it reliably fail.
Note that virsh itself doesn't fail, in either case. It sends info to libvirtd that causes *it* to segfault, so in order to trigger the bug, I have to run libvirtd.
virsh SEGV's nicely enough for me
$ ./src/virsh --connect test:///default Welcome to lt-virsh, the virtualization interactive terminal.
Type: 'help' for help with commands 'quit' to quit
virsh # define a.xml Domain D defined from a.xml
virsh # dumpxml D Segmentation fault
Ah. I'd only run the define, which is enough to make libvirtd fail when using qemu:///session.
Oh, that'll be because when you run 'define' in QEMU, it parses the XML and then runs dumpxml internally to save it out to disk
Exactly. It exercises a different code path that also terminates in the code fixed by Cole's patch.
In fact running virsh at all is redundant - this fails nicely enough if it is just added as a new datafile to the qemuxml2xmltest test case.
Putting that test closer to the target code is good, so ACK, assuming it passes "make distcheck".
However, part of my goal in running the qemu:///session server is to get more coverage on the server side. "make check" does very little testing of libvirtd, currently. So if you don't mind, I'll also add a comparable test using qemu:///session and unix_sock_dir, so it doesn't interfere with existing state.
I think this is the wrong way to go about testing the real live hypervisor drivers. For those we should have something that is like the libvirt-CIM test suite. eg a test system which has code to run all the libvirt APIs. This single test system is then run
?? I too would like to have it all now. I'm proposing what seems to me to be an easy way to incrementally add test cases like the one to exercise Cole's bug. We need to do much more of this (adding tests with each bug fix), not less.
in a controlled environment once for each hypervisor URI that we support. This will mean we exercise all the core codepaths for the real drivers, and also validate that they are all responding in the same way with consistent semantics / underling operations. Adding ad-hoc test cases to leverage qemu:///session is wasted effort because its creating a functional/integration test suite which is neccessarily tied to QEMU driver, as opposed to being a generic solution.
IMHO it's not wasted at all. This is a test that can be run by non-root users, and getting *some* coverage without requiring sudo or root is essential. However, the test can obviously be improved to cover more drivers. I propose to add the test now (new version below), and eventually put a loop around things so that, it does something like this for each $driver: * if running as root and compiled with $driver support, then run the test using $driver
The 'make check' testsuite should be focused on unit testing either by calling specific APIs relating to the hypervisor specific code,
If you don't want any more integration tests like this to be run via "make check", I can put them on a different target. Having a good mix of tests can only help. IME, both types of tests are necessary.
or be running commands with the 'test' driver. The test driver has sufficient functionality to let us functionally test core operation of the libvirtd daemon too.
Some, but obviously not all.
So, this is the distinction between calling 'qemudBuildCommandLine' to test QEMU driver command line arg processing, and running the 'start' operation on the QEMU driver which indirectly calls the "qemudBuildCommandLine" method.
Maybe you want to keep the extra disk and interface sections after all? (the ones that you suggested I remove) Better coverage?
There's already a tonne of data files in qemuxml2argvdata that exercise the disk/nic/hostdev bits of the XML parser. The XML parsing is the one area where we have generally good test coverage for all common paths and just need to look at the code coverage reports to find edge cases like this scenario we're fixing here.
Either way is fine with me. Just wanted to make sure you intended to add the parts of that XML that you'd suggested I remove. FYI, here's a new version of this latest test. Now it uses the new unix_sock_dir setting to avoid risk of interfering with any existing qemu-based settings. (this patch depends on the unix_sock_dir-adding patch that's still waiting for an ACK. Also, I have two or three other test-adding patches that depend on unix_sock_dir.
From 0558cb2d432e44213ecd5b2b95dee8eac1f31276 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 15 Jan 2009 19:51:39 +0100 Subject: [PATCH] exercise a bug that could make libvirtd segfault
* tests/define-dev-segfault: New file. * tests/Makefile.am (test_scripts): Add define-dev-segfault. --- tests/Makefile.am | 1 + tests/define-dev-segfault | 69 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 0 deletions(-) create mode 100755 tests/define-dev-segfault diff --git a/tests/Makefile.am b/tests/Makefile.am index c735d62..39b5727 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -60,6 +60,7 @@ if WITH_LIBVIRTD test_scripts += \ cpuset \ daemon-conf \ + define-dev-segfault \ int-overflow \ libvirtd-fail \ libvirtd-pool \ diff --git a/tests/define-dev-segfault b/tests/define-dev-segfault new file mode 100755 index 0000000..5d690bf --- /dev/null +++ b/tests/define-dev-segfault @@ -0,0 +1,69 @@ +#!/bin/sh +# Exercise a bug whereby defining a valid domain could kill libvirtd. + +if test "$VERBOSE" = yes; then + set -x + libvirtd --version + virsh --version +fi + +test -z "$srcdir" && srcdir=$(pwd) +test -z "$abs_top_srcdir" && abs_top_srcdir=$(pwd)/.. +. "$srcdir/test-lib.sh" + +fail=0 + +pwd=$(pwd) || fail=1 +sock_dir="$pwd" +cat > conf <<EOF || fail=1 +unix_sock_dir = "$sock_dir" +EOF + +# Domain definition from Cole Robinson. +cat <<\EOF > devs.xml || fail=1 +<domain type="kvm"> + <name>D</name> + <uuid>aaa3ae22-fed2-bfbd-ac02-3bea3bcfad82</uuid> + <memory>262144</memory> + <currentMemory>262144</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch="i686" machine="pc">hvm</type> + <boot dev="cdrom"/> + </os> + <features> + <acpi/> + </features> + <clock offset="utc"/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-kvm</emulator> + <serial type="pty"/> + <serial type="pty"/> + <serial type="pty"/> + <parallel type="pty"/> + <parallel type="pty"/> + <parallel type="pty"/> + <input type="mouse" bus="ps2"/> + <sound model="pcspk"/> + <sound model="es1370"/> + </devices> +</domain> +EOF + +libvirtd --config=conf > libvirtd-log 2>&1 & pid=$! +sleep 1 + +url="qemu:///session?socket=@$sock_dir/libvirt-sock" +virsh --connect "$url" define devs.xml > out 2>&1 || fail=1 + +kill $pid + +printf 'Domain D defined from devs.xml\n\n' > exp || fail=1 + +compare exp out || fail=1 +compare /dev/null libvirtd-log || fail=1 + +exit $fail -- 1.6.1.258.g7ff14
Jim Meyering <jim@meyering.net> wrote: ...
FYI, here's a new version of this latest test. Now it uses the new unix_sock_dir setting to avoid risk of interfering with any existing qemu-based settings.
(this patch depends on the unix_sock_dir-adding patch that's still waiting for an ACK. Also, I have two or three other test-adding patches that depend on unix_sock_dir.
From 0558cb2d432e44213ecd5b2b95dee8eac1f31276 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 15 Jan 2009 19:51:39 +0100 Subject: [PATCH] exercise a bug that could make libvirtd segfault
* tests/define-dev-segfault: New file. * tests/Makefile.am (test_scripts): Add define-dev-segfault. --- tests/Makefile.am | 1 + tests/define-dev-segfault | 69 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 0 deletions(-) create mode 100755 tests/define-dev-segfault
diff --git a/tests/Makefile.am b/tests/Makefile.am index c735d62..39b5727 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -60,6 +60,7 @@ if WITH_LIBVIRTD test_scripts += \ cpuset \ daemon-conf \ + define-dev-segfault \ int-overflow \ libvirtd-fail \ libvirtd-pool \
[in case you try to apply this patch, ... ] Note that this hunk will not apply for you, since it depends on an earlier still-pending one that aligned the backslashes. The point is just to add the new test name to the list.
On Fri, Jan 16, 2009 at 02:37:04PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Fri, Jan 16, 2009 at 12:42:35PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
Ah. I'd only run the define, which is enough to make libvirtd fail when using qemu:///session.
Oh, that'll be because when you run 'define' in QEMU, it parses the XML and then runs dumpxml internally to save it out to disk
Exactly. It exercises a different code path that also terminates in the code fixed by Cole's patch.
This bug can easily be covered by the unit test addition I proposed. It is pointless adding multiple tests for the same bug - it just slows down the test suite more, which is why i suggested anything that runs with the real hypervisor drivers should be built into a separate functional regression suite.
I think this is the wrong way to go about testing the real live hypervisor drivers. For those we should have something that is like the libvirt-CIM test suite. eg a test system which has code to run all the libvirt APIs. This single test system is then run
?? I too would like to have it all now. I'm proposing what seems to me to be an easy way to incrementally add test cases like the one to exercise Cole's bug. We need to do much more of this (adding tests with each bug fix), not less.
We don't have to create a functional test suite with 100% coverage right from the start. We can incrementally build it up in just the same way, starting with testing of the 'define' operation as your patch does. A very small amount of boilerplate could would let us start a useful functional test suite which can be run both for dev trees, and real world installations by admins - A 'tests/regression' directory - Build a command / provide a shell script 'libvirt-test' that gets installed into /usr/bin, accepting a HV uri as its arg, and then running all functional tests for that HV driver. - Data files (such as test XML files) which get installed into /usr/share/libvirt/test to be used by test cases - A sub-RPM that contains these files/scripts 'libvirt-test' Thus after install any admin can validate their deployment by running one of # libvirt-test qemu:///session # libvirt-test qemu:///system # libvirt-test xen:/// as per the hypervisor they actually want to test. This will help identify a wide range of problems, particularly where a new upstream release of Xen / QEMU / KVM comes out which we as libvirt developers haven't tested. We've had a long history of being broken by Xen releases, but never had any way for users to test if their install of libvirt actually works with the Xen install. Setting up these functional test cases as things the users can run, instead of just restircting them to use in make check is far more useful, and does not need to be significantly more if we build it up incrementally. The example data file you have could be the first test case for this functional test suite.
in a controlled environment once for each hypervisor URI that we support. This will mean we exercise all the core codepaths for the real drivers, and also validate that they are all responding in the same way with consistent semantics / underling operations. Adding ad-hoc test cases to leverage qemu:///session is wasted effort because its creating a functional/integration test suite which is neccessarily tied to QEMU driver, as opposed to being a generic solution.
IMHO it's not wasted at all. This is a test that can be run by non-root users, and getting *some* coverage without requiring sudo or root is essential. However, the test can obviously be improved to cover more drivers.
My point is that with very little extra effort we can make something that is far more broadly useful, both for our own dev usage, and for end user validation.
or be running commands with the 'test' driver. The test driver has sufficient functionality to let us functionally test core operation of the libvirtd daemon too.
Some, but obviously not all.
Assuming the test driver has 100% coverage of the libvirt public API, it should allow complete coverage of the libvirtd daemon code itself, since the daemon code is not driver specific in any way. The only missing part in the test driver currently is the device montor APIs. 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 :|
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering