4:40 a.m.
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.
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.
Comments and opinions are welcome.
Pavel Fedin (3):
Implement virQEMUCapsCache mockup
Use mockup cache
Removed unneeded check
src/qemu/qemu_capabilities.c | 10 +---------
src/qemu/qemu_capspriv.h | 36 +++++++++++++++++++++++++++++++++++
src/qemu/qemu_domain.c | 5 +----
tests/qemuagenttest.c | 9 ++++++++-
tests/qemuargv2xmltest.c | 5 +++++
tests/qemuhotplugtest.c | 23 ++++++++++++++--------
tests/qemuxml2argvtest.c | 5 +++++
tests/qemuxml2xmltest.c | 6 ++++++
tests/qemuxmlnstest.c | 5 +++++
tests/testutilsqemu.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
tests/testutilsqemu.h | 3 +++
11 files changed, 130 insertions(+), 22 deletions(-)
create mode 100644 src/qemu/qemu_capspriv.h
mode change 100644 => 100755 tests/qemuagenttest.c
mode change 100644 => 100755 tests/qemuhotplugtest.c
mode change 100644 => 100755 tests/qemuxml2xmltest.c
mode change 100644 => 100755 tests/testutilsqemu.c
--
2.1.4
4:40 a.m.
New subject: [libvirt] [RFC PATCH 1/3] Implement virQEMUCapsCache mockup
This patch introduces qemuTestMakeCapsCache() function, which creates
capability cache containing predefined set of capabilities associated
with predefined binary.
For simplicity of integration binary name is deduced from test name,
which follows a simple scheme of being prefixed with architecture
name, with some exceptions.
Signed-off-by: Pavel Fedin <p.fedin(a)samsung.com>
---
src/qemu/qemu_capabilities.c | 10 +---------
src/qemu/qemu_capspriv.h | 36 +++++++++++++++++++++++++++++++++++
tests/testutilsqemu.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
tests/testutilsqemu.h | 3 +++
4 files changed, 85 insertions(+), 9 deletions(-)
create mode 100644 src/qemu/qemu_capspriv.h
mode change 100644 => 100755 tests/testutilsqemu.c
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 43d11af..62b1aea 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -42,6 +42,7 @@
#include "virstring.h"
#include "qemu_hostdev.h"
#include "qemu_domain.h"
+#include "qemu_capspriv.h"
#include <fcntl.h>
#include <sys/stat.h>
@@ -327,15 +328,6 @@ struct _virQEMUCaps {
unsigned int *machineMaxCpus;
};
-struct _virQEMUCapsCache {
- virMutex lock;
- virHashTablePtr binaries;
- char *libDir;
- char *cacheDir;
- uid_t runUid;
- gid_t runGid;
-};
-
struct virQEMUCapsSearchData {
virArch arch;
};
diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h
new file mode 100644
index 0000000..f915ea9
--- /dev/null
+++ b/src/qemu/qemu_capspriv.h
@@ -0,0 +1,36 @@
+/*
+ * qemu_capspriv.h: private declarations for QEMU capabilities generation
+ *
+ * Copyright (C) 2015 Samsung Electronics Co. Ltd
+ * Copyright (C) 2015 Pavel Fedin
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>;.
+ *
+ * Author: Pavel Fedin <p.fedin(a)samsung.com>
+ */
+
+#ifndef __QEMU_CAPSPRIV_H__
+#define __QEMU_CAPSPRIV_H__
+
+struct _virQEMUCapsCache {
+ virMutex lock;
+ virHashTablePtr binaries;
+ char *libDir;
+ char *cacheDir;
+ uid_t runUid;
+ gid_t runGid;
+};
+
+#endif
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
old mode 100644
new mode 100755
index a2f4299..3dc50f2
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -8,6 +8,7 @@
# include "cpu_conf.h"
# include "qemu/qemu_driver.h"
# include "qemu/qemu_domain.h"
+# include "qemu/qemu_capspriv.h"
# include "virstring.h"
# define VIR_FROM_THIS VIR_FROM_QEMU
@@ -526,4 +527,48 @@ qemuTestParseCapabilities(const char *capsFile)
xmlXPathFreeContext(ctxt);
return NULL;
}
+
+virQEMUCapsCachePtr
+qemuTestMakeCapsCache(const char *test, virQEMUCapsPtr caps)
+{
+ virQEMUCapsCachePtr cache;
+ const char *binary;
+
+ cache = virQEMUCapsCacheNew("/dev/null", "/dev/null", 0, 0);
+ if (!cache)
+ return NULL;
+
+ if (caps) {
+ /* Our caps were created artificially, so we don't want
+ * virQEMUCapsCacheFree() to attempt to deallocate them */
+ virObjectRef(caps);
+ } else {
+ caps = virQEMUCapsNew();
+ if (!caps) {
+ virQEMUCapsCacheFree(cache);
+ return NULL;
+ }
+ }
+
+ if (STRPREFIX(test, "aarch64-"))
+ binary = "/usr/bin/qemu-system-aarch64";
+ else if (STRPREFIX(test, "arm-"))
+ binary = "/usr/bin/qemu-system-arm";
+ else if (STRPREFIX(test, "pseries-"))
+ binary = "/usr/bin/qemu-system-ppc64";
+ else if (STRPREFIX(test, "s390-"))
+ binary = "/usr/bin/qemu-system-s390x";
+ else if (strstr(test, "keywrap-")) /* May be rename these ? */
+ binary = "/usr/bin/qemu-system-s390x";
+ else
+ binary = "/usr/bin/qemu";
+
+ if (virHashAddEntry(cache->binaries, binary, caps) < 0) {
+ virQEMUCapsCacheFree(cache);
+ virObjectUnref(caps);
+ return NULL;
+ }
+
+ return cache;
+}
#endif
diff --git a/tests/testutilsqemu.h b/tests/testutilsqemu.h
index 0ec5dad..6fe923b 100644
--- a/tests/testutilsqemu.h
+++ b/tests/testutilsqemu.h
@@ -16,4 +16,7 @@ extern virCPUDefPtr cpuHaswell;
void testQemuCapsSetCPU(virCapsPtr caps,
virCPUDefPtr hostCPU);
+virQEMUCapsCachePtr qemuTestMakeCapsCache(const char *test,
+ virQEMUCapsPtr caps);
+
#endif
--
2.1.4
4:40 a.m.
New subject: [libvirt] [RFC PATCH 2/3] Use mockup cache
Create capabilities cache using neq qemuTestMakeCapsCache() before
parsing XML files
Signed-off-by: Pavel Fedin <p.fedin(a)samsung.com>
---
tests/qemuagenttest.c | 9 ++++++++-
tests/qemuargv2xmltest.c | 5 +++++
tests/qemuhotplugtest.c | 23 +++++++++++++++--------
tests/qemuxml2argvtest.c | 5 +++++
tests/qemuxml2xmltest.c | 6 ++++++
tests/qemuxmlnstest.c | 5 +++++
6 files changed, 44 insertions(+), 9 deletions(-)
mode change 100644 => 100755 tests/qemuagenttest.c
mode change 100644 => 100755 tests/qemuhotplugtest.c
mode change 100644 => 100755 tests/qemuxml2xmltest.c
diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c
old mode 100644
new mode 100755
index 52cc834..729b82b
--- a/tests/qemuagenttest.c
+++ b/tests/qemuagenttest.c
@@ -31,6 +31,8 @@
#define VIR_FROM_THIS VIR_FROM_NONE
+static virQEMUDriver driver;
+
static int
testQemuAgentFSFreeze(const void *data)
{
@@ -181,6 +183,10 @@ testQemuAgentGetFSInfo(const void *data)
abs_srcdir) < 0)
goto cleanup;
+ driver.qemuCapsCache = qemuTestMakeCapsCache("", NULL);
+ if (!driver.qemuCapsCache)
+ goto cleanup;
+
if (!(def = virDomainDefParseFile(domain_filename, caps, xmlopt,
VIR_DOMAIN_DEF_PARSE_INACTIVE)))
goto cleanup;
@@ -293,6 +299,7 @@ testQemuAgentGetFSInfo(const void *data)
virDomainFSInfoFree(info[i]);
VIR_FREE(info);
VIR_FREE(domain_filename);
+ virQEMUCapsCacheFree(driver.qemuCapsCache);
virObjectUnref(caps);
virDomainDefFree(def);
qemuMonitorTestFree(test);
@@ -917,7 +924,7 @@ mymain(void)
#endif
if (virThreadInitialize() < 0 ||
- !(xmlopt = virQEMUDriverCreateXMLConf(NULL)))
+ !(xmlopt = virQEMUDriverCreateXMLConf(&driver)))
return EXIT_FAILURE;
virEventRegisterDefaultImpl();
diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c
index ea85913..348f2dc 100644
--- a/tests/qemuargv2xmltest.c
+++ b/tests/qemuargv2xmltest.c
@@ -130,9 +130,14 @@ testCompareXMLToArgvHelper(const void *data)
abs_srcdir, info->name) < 0)
goto cleanup;
+ driver.qemuCapsCache = qemuTestMakeCapsCache(info->name, NULL);
+ if (!driver.qemuCapsCache)
+ goto cleanup;
+
result = testCompareXMLToArgvFiles(xml, args, info->flags);
cleanup:
+ virQEMUCapsCacheFree(driver.qemuCapsCache);
VIR_FREE(xml);
VIR_FREE(args);
return result;
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
old mode 100644
new mode 100755
index 368a5e7..0f9932d
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -57,7 +57,7 @@ static int
qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
virDomainObjPtr *vm,
const char *domxml,
- bool event)
+ bool event, const char *testname)
{
int ret = -1;
qemuDomainObjPrivatePtr priv = NULL;
@@ -65,12 +65,6 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
if (!(*vm = virDomainObjNew(xmlopt)))
goto cleanup;
- if (!((*vm)->def = virDomainDefParseString(domxml,
- driver.caps,
- driver.xmlopt,
- VIR_DOMAIN_DEF_PARSE_INACTIVE)))
- goto cleanup;
-
priv = (*vm)->privateData;
if (!(priv->qemuCaps = virQEMUCapsNew()))
@@ -85,6 +79,16 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
if (event)
virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT);
+ driver.qemuCapsCache = qemuTestMakeCapsCache(testname, priv->qemuCaps);
+ if (!driver.qemuCapsCache)
+ goto cleanup;
+
+ if (!((*vm)->def = virDomainDefParseString(domxml,
+ driver.caps,
+ driver.xmlopt,
+ VIR_DOMAIN_DEF_PARSE_INACTIVE)))
+ goto cleanup;
+
if (qemuDomainAssignAddresses((*vm)->def, priv->qemuCaps, *vm) < 0)
goto cleanup;
@@ -243,7 +247,8 @@ testQemuHotplug(const void *data)
vm = test->vm;
} else {
if (qemuHotplugCreateObjects(driver.xmlopt, &vm, domain_xml,
- test->deviceDeletedEvent) < 0)
+ test->deviceDeletedEvent,
+ test->domain_filename) < 0)
goto cleanup;
}
@@ -318,10 +323,12 @@ testQemuHotplug(const void *data)
} else {
virObjectUnref(vm);
test->vm = NULL;
+ virQEMUCapsCacheFree(driver.qemuCapsCache);
}
virDomainDeviceDefFree(dev);
virObjectUnref(caps);
qemuMonitorTestFree(test_mon);
+
return ((ret < 0 && fail) || (!ret && !fail)) ? 0 : -1;
}
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index c2482e6..428204d 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -423,11 +423,16 @@ testCompareXMLToArgvHelper(const void *data)
if (virQEMUCapsGet(info->extraFlags, QEMU_CAPS_ENABLE_FIPS))
flags |= FLAG_FIPS;
+ driver.qemuCapsCache = qemuTestMakeCapsCache(info->name, info->extraFlags);
+ if (!driver.qemuCapsCache)
+ goto cleanup;
+
result = testCompareXMLToArgvFiles(xml, args, info->extraFlags,
info->migrateFrom, info->migrateFd,
flags);
cleanup:
+ virQEMUCapsCacheFree(driver.qemuCapsCache);
VIR_FREE(xml);
VIR_FREE(args);
return result;
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
old mode 100644
new mode 100755
index 5c1c2e9..aa8218c
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -216,6 +216,8 @@ testCompareStatusXMLToXMLFiles(const void *opaque)
static void
testInfoFree(struct testInfo *info)
{
+ virQEMUCapsCacheFree(driver.qemuCapsCache);
+
VIR_FREE(info->inName);
VIR_FREE(info->inFile);
@@ -240,6 +242,10 @@ testInfoSet(struct testInfo *info,
if (virtTestLoadFile(info->inName, &info->inFile) < 0)
goto error;
+ driver.qemuCapsCache = qemuTestMakeCapsCache(name, NULL);
+ if (!driver.qemuCapsCache)
+ goto error;
+
if (when & WHEN_INACTIVE) {
if (different) {
if (virAsprintf(&info->outInactiveName,
diff --git a/tests/qemuxmlnstest.c b/tests/qemuxmlnstest.c
index 8eaab8a..8265d17 100644
--- a/tests/qemuxmlnstest.c
+++ b/tests/qemuxmlnstest.c
@@ -175,11 +175,16 @@ testCompareXMLToArgvHelper(const void *data)
abs_srcdir, info->name) < 0)
goto cleanup;
+ driver.qemuCapsCache = qemuTestMakeCapsCache(info->name, info->extraFlags);
+ if (!driver.qemuCapsCache)
+ goto cleanup;
+
result = testCompareXMLToArgvFiles(xml, args, info->extraFlags,
info->migrateFrom, info->migrateFd,
info->json, info->expectError);
cleanup:
+ virQEMUCapsCacheFree(driver.qemuCapsCache);
VIR_FREE(xml);
VIR_FREE(args);
return result;
--
2.1.4
4:40 a.m.
New subject: [libvirt] [RFC PATCH 3/3] Removed unneeded check
Since test suite now correctly creates capabilities cache, the hack is not
needed any more.
Signed-off-by: Pavel Fedin <p.fedin(a)samsung.com>
---
src/qemu/qemu_domain.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 84e5fa5..11b28cb 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1039,10 +1039,7 @@ qemuDomainDefPostParse(virDomainDefPtr def,
return ret;
- /* This condition is actually a (temporary) hack for test suite which
- * does not create capabilities cache */
- if (driver && driver->qemuCapsCache)
- qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator);
+ qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator);
/* Add implicit PCI root controller if the machine has one */
switch (def->os.arch) {
--
2.1.4
4:19 p.m.
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.
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.
- Cole
2:30 a.m.
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
1:51 a.m.
New subject: [libvirt] [RFC PATCH 0/3] Implement mockup capabilities cache in QEMU tests
Hello!
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
We don't need to add this because we already have domain post-parse callback. And
this is what i
actually disliked.
There is a very simple way to patch this callback. Currently in our tests we call
virQEMUDriverCreateXMLConf(), which returns a structure, containing our callbacks.
Actually we could
make a wrapper around this function which would replace domainPostParseCallback with own
pointer,
remembering the original one. Then our function would take emulator name from the parsed
XML, insert
capabilities cache, and proceed to original function.
We should be really careful about not depending on the host in
tests,
so any assert like this would be much appreciated.
To tell the truth i'm not a big fan of test-only code in library body. But, i agree
with this, and
i see no other way round. I think we could add some global Boolean variable like bool
testMode,
which would be set to true by test suite.
Actually, this variable could even modify capsCache behavior to ignore binary name at
all, and
return, let's say, the first entry. But we should be really careful about it because i
believe we
want to exercise the actual library code, and not some test-only code.
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
8:34 a.m.
On Thu, Sep 03, 2015 at 09:51:11AM +0300, Pavel Fedin wrote:
Hello!
> 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
We don't need to add this because we already have domain post-parse callback. And
this is what i
actually disliked.
There is a very simple way to patch this callback. Currently in our tests we call
virQEMUDriverCreateXMLConf(), which returns a structure, containing our callbacks.
Actually we could
make a wrapper around this function which would replace domainPostParseCallback with own
pointer,
remembering the original one. Then our function would take emulator name from the parsed
XML, insert
capabilities cache, and proceed to original function.
It's just that we would have to replace and patch both domain callback
and domaindevice callback (domaindevice callback can be called without
domain callback) and I thought it would be nicer to have it in one
place. But I think both works for me.
> We should be really careful about not depending on the host in
tests,
> so any assert like this would be much appreciated.
To tell the truth i'm not a big fan of test-only code in library body. But, i agree
with this, and
i see no other way round. I think we could add some global Boolean variable like bool
testMode,
which would be set to true by test suite.
Actually, this variable could even modify capsCache behavior to ignore binary name at
all, and
return, let's say, the first entry. But we should be really careful about it because i
believe we
want to exercise the actual library code, and not some test-only code.
It's just an assert. We have those for various reasons and extra
carefulness cannot hurt. I don't think it needs differentiating
between whether the code is ran in tests or not. That defeats the
purpose of our tests. It'd be better to resort to mocking some simple
function if needed.
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
1:47 a.m.
Hello!
It's just an assert. We have those for various reasons and
extra
carefulness cannot hurt. I don't think it needs differentiating
between whether the code is ran in tests or not.
It would need differentiating because in tests we should not talk to the real filesystem,
while in
real environment it's OK. So we would need some "extern bool qemuTestMode".
Well, anyway, take a look at my new implementation, i sent it yesterday. It uses a
similar global
variable, with only a 2-line change in the library code. The basic idea is to override
'binary' name
in virQEMUCapsCacheLookup(), so that we can force it to return particular capability set
no matter
what def->emulator is. This very simple change actually allows us to do lots of nice
things, and
improve our test suite even further by removing hardcoded capabilities at all.
I cc'ed to you, but just in case:
http://www.redhat.com/archives/libvir-list/2015-September/msg00084.html
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
3367
days inactive
3384
days old
8 comments
3 participants
participants (3)
-
Cole Robinson -
Martin Kletzander -
Pavel Fedin