Re: [RFC PATCH 0/3] Implement mockup capabilities cache in QEMU tests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Sep 01, 2015 at 05:19:39PM -0400, Cole Robinson wrote:
On 08/18/2015 05:40 AM, Pavel Fedin wrote:
Since commit e8d55172544c1fafe31a9e09346bdebca4f0d6f9 qemu driver checks
emulator capabilities during domain XML post-parse. However, test suite
does not initialize it, therefore a condition to skip all checks if there
is no cache supplied was added. This is actually a hack, whose sole
purpose is to make existing test suite working. Additionally, it prevents
from writing new tests for this particular functionality.

This series attempts to solve this problem by implementing proper cache
mockup in test suite. The main idea is to create a cache in standard way
and put there a pre-defined capabilities set (which tests already have).

The main problem here is to know emulator binary name, which is contained
in the source XML. However, we have to create our cache before reading the
XML. The simplest way to resolve this is to assume particular binary name
from test name. Currently tests which assume cross-architecture binary are
all prefixed with the architecture name (with one exception of "keywrap"
tests which all assume /usr/bin/qemu-system-s390x and do not have "s390-"
prefix in their name).

This scheme works fine, unless we use "native" emulator binary. Here we
have a mess. Most newer tests use /usr/bin/qemu, however there is a large
number of tests which use /usr/libexec/qemu-kvm or /usr/bin/kvm (i guess
these are leftovers from the epoch when qemu-kvm was a separate fork of
qemu). This is currently not handled in any way, and these tests may
report errors due to missing binaries (because virQEMUCapsCacheLookup()
attempts to populate the cache automatically by querying the binary if
not already known).

There are several possible ways to resolve this:
a) Add all possible names as aliases for /usr/bin/qemu
b) Forbid to use oldstyle names at all in these tests
c) Declare some prefix like "kvm-" for those tests who want to use
   /usr/libexec/qemu-kvm. Again, this would ban /usr/bin/kvm and
   /usr/bin/qemu-kvm (if not using aliases like in (b)
d) Hardcode (optional) emulator name per test. IMHO a bad idea because
   number of tests is huge.
e) Do some preparsing of the XML and extract binary name from it. Again,
   i disliked it for not being simple enough.


With the current approach I think e) is the way to go... yeah it's redundant
parsing but better than trying to maintain a whitelist or reformat a bunch of
test cases to match this. And for example these patches as is cause breakage
in other tests, like qemuargv2xml, and I think it's related to the incomplete
whitelist.

Using libvirt helpers it shouldn't be too hard to parse out the emulator path,
basically just virXMLParseStringCtxt and then

emulator = virXPathString("string(./devices/emulator[1])", ctxt);

Maybe other people have better ideas about how to mock this out though.

I also thought about an alternate implementation which would patch
postParseCallback and insert own function there which builds a cache. At
this point binary name is already known from the XML. However, such a
design looks like an ugly  hack by itself, so i stopped going in this
direction.


Yeah that sounds quite intense. Maybe there's some way to override a function
with our own definition from the test suite, but that's outside my realm of
knowledge.


The current xmlopt structure has two callbacks in parser
configuration, one for post-parsing of the whole domain XML and one
for device XML.  We could add new (optional) callback in this
structure that would be called before other post-parse callbacks, but
still after the xml is parsed.  This would not be utilized anywhere
else than in tests, currently, but I don't see it as ugly and it might
be beneficial e.g. for some drivers or in the future.  Another benefit
is that it does not add that much of a code as it would be called only
in virDomainDefPostParse() and virDomainDeviceDefParse(), plus the
time complexity can be kept at minimum if the function called is no-op
in case the cache is already built.

Actually populating the cache does exercise the most qemu code though, so
that's one benefit.

Another minor bit to consider is possibly adding an assert in
qemu_capabilities.c to ensure that we never try to actually probe the host FS
from the test suite, since we don't want the test suite to accidentally become
dependent on host state. Maybe there's a test suite environment variable we
can check for.


We should be really careful about not depending on the host in tests,
so any assert like this would be much appreciated.

- Cole

Attachment: signature.asc
Description: PGP signature

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]