[libvirt] [PATCH] Fix vm define error with back compat console device

[Daniel P. Berrange]

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