7:37 a.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
On Fri, Jan 16, 2009 at 12:42:35PM +0100, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange(a)redhat.com> wrote:
>
> > On Fri, Jan 16, 2009 at 09:24:33AM +0100, Jim Meyering wrote:
> >> "Daniel P. Berrange" <berrange(a)redhat.com> wrote:
> >> > On Thu, Jan 15, 2009 at 10:17:56PM +0100, Jim Meyering wrote:
> >> >> "Daniel P. Berrange" <berrange(a)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(a)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