[libvirt] [PATCH 0/4] Xen fixes
Here are several patches relevant to Xen: The first two patches fix the test-suite when building as root in a Xen-dom0 domain. The other two patches fix problems when doing "virsh edit" on Xen domains. Philipp Hahn (4): tests: Add support for skipping tests tests: Skip Xen-HVM tests for root on dom0 xen: fix PyGrub boot device order xen: Return tap2 for tap2 disks src/xenxs/xen_sxpr.c | 33 ++++++++++++----- tests/testutils.c | 4 ++ tests/xencapstest.c | 95 ++++++++++++++++++++++++++++++++++---------------- 3 files changed, 93 insertions(+), 39 deletions(-)
AM_TESTS has support for skipping tests, while the C-implementation virtTestRun() does not support that feature. Print "_" or "SKIP" in verbose mode for tests returning EXIT_AM_SKIP=77. Signed-off-by: Philipp Hahn <hahn@univention.de> --- tests/testutils.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/tests/testutils.c b/tests/testutils.c index 08db732..c813637 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -160,6 +160,8 @@ virtTestRun(const char *title, int nloops, int (*body)(const void *data), const virtTestCountAverage(ts, nloops)); else if (ret == 0) fprintf(stderr, "OK\n"); + else if (ret == EXIT_AM_SKIP) + fprintf(stderr, "SKIP\n"); else fprintf(stderr, "FAILED\n"); } else { @@ -170,6 +172,8 @@ virtTestRun(const char *title, int nloops, int (*body)(const void *data), const } if (ret == 0) fprintf(stderr, "."); + else if (ret == EXIT_AM_SKIP) + fprintf(stderr, "_"); else fprintf(stderr, "!"); } -- 1.7.1
On 10/12/2011 02:08 AM, Philipp Hahn wrote:
AM_TESTS has support for skipping tests, while the C-implementation virtTestRun() does not support that feature.
Print "_" or "SKIP" in verbose mode for tests returning EXIT_AM_SKIP=77.
Signed-off-by: Philipp Hahn<hahn@univention.de> --- tests/testutils.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
ACK and pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Several tests fail when run as root on a Xen-dom0-system, since virInitialize() then succeeds to open /proc/xen/privcmd and returns the actual supported features instead of the faked one when calling xenHypervisorMakeCapabilitiesInternal(). Since Xen-4.1 supports additional features like "hap" and "viridian", the xencapstest fails. For now disable those 4 tests and return EXIT_AM_SKIP for them. A better fix would probably just check for the minimum required features instead of comparing the two XML documents for bit-equivalence. mergeTestRun() is implemented locally instead of globally in testutils.c, since the merge strategy for compound tests heavily depends on the specific test cases being merged. Signed-off-by: Philipp Hahn <hahn@univention.de> --- tests/xencapstest.c | 95 +++++++++++++++++++++++++++++++++++---------------- 1 files changed, 65 insertions(+), 30 deletions(-) diff --git a/tests/xencapstest.c b/tests/xencapstest.c index 9c1eba4..0bc830d 100644 --- a/tests/xencapstest.c +++ b/tests/xencapstest.c @@ -2,6 +2,7 @@ #include <stdio.h> #include <stdlib.h> +#include <stdbool.h> #include <string.h> #include <unistd.h> @@ -11,6 +12,31 @@ #include "xen/xen_hypervisor.h" #include "virfile.h" +/* When building as root in a Xen-4.1-dom0, the Hypervisor is accessible and + * reports additional features (hap, viridian), which breaks all HVM tests, + * since the XML-documents are compared for bit-equivalence. */ +static bool +skip_if_root(void) +{ + if (getuid()) + return false; + if (getenv("FAKEROOTKEY")) + return false; + return true; +} + +static int +mergeTestRun(int ret, int tr) +{ + switch (tr) { + default: + return -1; + case 0: + case EXIT_AM_SKIP: + return ret; + } +} + static int testCompareFiles(const char *hostmachine, const char *xml_rel, const char *cpuinfo_rel, const char *capabilities_rel) @@ -81,6 +107,8 @@ static int testXeni686PAE(const void *data ATTRIBUTE_UNUSED) { } static int testXeni686PAEHVM(const void *data ATTRIBUTE_UNUSED) { + if (skip_if_root()) + return EXIT_AM_SKIP; return testCompareFiles("i686", "xencapsdata/xen-i686-pae-hvm.xml", "xencapsdata/xen-i686-pae-hvm.cpuinfo", @@ -105,6 +133,8 @@ static int testXenx86_64(const void *data ATTRIBUTE_UNUSED) { "xencapsdata/xen-x86_64.caps"); } static int testXenx86_64HVM(const void *data ATTRIBUTE_UNUSED) { + if (skip_if_root()) + return EXIT_AM_SKIP; return testCompareFiles("x86_64", "xencapsdata/xen-x86_64-hvm.xml", "xencapsdata/xen-x86_64-hvm.cpuinfo", @@ -125,12 +155,16 @@ static int testXenia64BE(const void *data ATTRIBUTE_UNUSED) { } static int testXenia64HVM(const void *data ATTRIBUTE_UNUSED) { + if (skip_if_root()) + return EXIT_AM_SKIP; return testCompareFiles("ia64", "xencapsdata/xen-ia64-hvm.xml", "xencapsdata/xen-ia64-hvm.cpuinfo", "xencapsdata/xen-ia64-hvm.caps"); } static int testXenia64BEHVM(const void *data ATTRIBUTE_UNUSED) { + if (skip_if_root()) + return EXIT_AM_SKIP; return testCompareFiles("ia64", "xencapsdata/xen-ia64-be-hvm.xml", "xencapsdata/xen-ia64-be-hvm.cpuinfo", @@ -148,17 +182,18 @@ static int testXenppc64(const void *data ATTRIBUTE_UNUSED) { static int mymain(void) { + int tr; int ret = 0; virInitialize(); - if (virtTestRun("Capabilities for i686, no PAE, no HVM", - 1, testXeni686, NULL) != 0) - ret = -1; + tr = virtTestRun("Capabilities for i686, no PAE, no HVM", + 1, testXeni686, NULL); + ret = mergeTestRun(ret, tr); - if (virtTestRun("Capabilities for i686, PAE, no HVM", - 1, testXeni686PAE, NULL) != 0) - ret = -1; + tr = virtTestRun("Capabilities for i686, PAE, no HVM", + 1, testXeni686PAE, NULL); + ret = mergeTestRun(ret, tr); /* No PAE + HVM is non-sensical - all VMX capable CPUs have PAE */ @@ -167,37 +202,37 @@ mymain(void) ret = -1; */ - if (virtTestRun("Capabilities for i686, PAE, HVM", - 1, testXeni686PAEHVM, NULL) != 0) - ret = -1; + tr = virtTestRun("Capabilities for i686, PAE, HVM", + 1, testXeni686PAEHVM, NULL); + ret = mergeTestRun(ret, tr); - if (virtTestRun("Capabilities for x86_64, no HVM", - 1, testXenx86_64, NULL) != 0) - ret = -1; + tr = virtTestRun("Capabilities for x86_64, no HVM", + 1, testXenx86_64, NULL); + ret = mergeTestRun(ret, tr); - if (virtTestRun("Capabilities for x86_64, HVM", - 1, testXenx86_64HVM, NULL) != 0) - ret = -1; + tr = virtTestRun("Capabilities for x86_64, HVM", + 1, testXenx86_64HVM, NULL); + ret = mergeTestRun(ret, tr); - if (virtTestRun("Capabilities for ia64, no HVM, LE", - 1, testXenia64, NULL) != 0) - ret = -1; + tr = virtTestRun("Capabilities for ia64, no HVM, LE", + 1, testXenia64, NULL); + ret = mergeTestRun(ret, tr); - if (virtTestRun("Capabilities for ia64, HVM, LE", - 1, testXenia64HVM, NULL) != 0) - ret = -1; + tr = virtTestRun("Capabilities for ia64, HVM, LE", + 1, testXenia64HVM, NULL); + ret = mergeTestRun(ret, tr); - if (virtTestRun("Capabilities for ia64, no HVM, BE", - 1, testXenia64BE, NULL) != 0) - ret = -1; + tr = virtTestRun("Capabilities for ia64, no HVM, BE", + 1, testXenia64BE, NULL); + ret = mergeTestRun(ret, tr); - if (virtTestRun("Capabilities for ia64, HVM, BE", - 1, testXenia64BEHVM, NULL) != 0) - ret = -1; + tr = virtTestRun("Capabilities for ia64, HVM, BE", + 1, testXenia64BEHVM, NULL); + ret = mergeTestRun(ret, tr); - if (virtTestRun("Capabilities for ppc64", - 1, testXenppc64, NULL) != 0) - ret = -1; + tr = virtTestRun("Capabilities for ppc64", + 1, testXenppc64, NULL); + ret = mergeTestRun(ret, tr); return(ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); -- 1.7.1
On 10/12/2011 02:11 AM, Philipp Hahn wrote:
Several tests fail when run as root on a Xen-dom0-system, since virInitialize() then succeeds to open /proc/xen/privcmd and returns the actual supported features instead of the faked one when calling xenHypervisorMakeCapabilitiesInternal(). Since Xen-4.1 supports additional features like "hap" and "viridian", the xencapstest fails.
I'm not sure I like this. I'd much rather fix things to avoid parsing live /proc/xen/privcmd when run from the testsuite, perhaps by providing a hook function that only the testsuite overrides (we've done this elsewhere). It is a bug if a testsuite ever depends on live system information, and skipping the test just papers over the real bug.
For now disable those 4 tests and return EXIT_AM_SKIP for them.
A better fix would probably just check for the minimum required features instead of comparing the two XML documents for bit-equivalence.
Or better yet, fixing the test to parse hard-coded information captured in a tests/*data/... directory associated with the test rather than reading /proc/xen/privcmd in the first place. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
When PyGrub is used as the bootloader in Xen, it gets passed the first bootable disk. Xend supports a "bootable"-flag for this, which isn't explicitly supported by libvirt. When converting libvirt-xml to xen-sxpr the "bootable"-flag gets implicitly set by xen.xend.XenConfig.device_add() for the first disk (marked as "Compat hack -- mark first disk bootable"). When converting back xen-sxpr to libvirt-xml, the disks are returned in the internal order used by Xend ignoring the "bootable"-flag, which looses the original order. When the domain is then re-defined, the order of disks is changed, which breaks PyGrub, since a different disk gets passed. When converting xen-sxpt to libvirt-xml, use the "bootable"-flag to determine the first disk. This isn't perfect, since several disks can be marked as bootable using the Xend-API, but that is not supported by libvirt. In all known cases relevant to libvirt exactly one disk is marked as bootable. Signed-off-by: Philipp Hahn <hahn@univention.de> --- valgrind might complain about an uninitialized read access when copying disks[0] to disks[0], but that is overwritten again with the following assignment. <https://forge.univention.org/bugzilla/show_bug.cgi?id=21938> src/xenxs/xen_sxpr.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-) diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 72322a7..038c6bb 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -342,20 +342,24 @@ xenParseSxprDisks(virDomainDefPtr def, const char *src = NULL; const char *dst = NULL; const char *mode = NULL; + const char *bootable = NULL; /* Again dealing with (vbd...) vs (tap ...) differences */ if (sexpr_lookup(node, "device/vbd")) { src = sexpr_node(node, "device/vbd/uname"); dst = sexpr_node(node, "device/vbd/dev"); mode = sexpr_node(node, "device/vbd/mode"); + bootable = sexpr_node(node, "device/vbd/bootable"); } else if (sexpr_lookup(node, "device/tap2")) { src = sexpr_node(node, "device/tap2/uname"); dst = sexpr_node(node, "device/tap2/dev"); mode = sexpr_node(node, "device/tap2/mode"); + bootable = sexpr_node(node, "device/tap2/bootable"); } else { src = sexpr_node(node, "device/tap/uname"); dst = sexpr_node(node, "device/tap/dev"); mode = sexpr_node(node, "device/tap/mode"); + bootable = sexpr_node(node, "device/tap/bootable"); } if (VIR_ALLOC(disk) < 0) @@ -481,7 +485,13 @@ xenParseSxprDisks(virDomainDefPtr def, if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) goto no_memory; - def->disks[def->ndisks++] = disk; + /* re-order disks if there is a bootable device */ + if (STREQ_NULLABLE(bootable, "1")) { + def->disks[def->ndisks++] = def->disks[0]; + def->disks[0] = disk; + } else { + def->disks[def->ndisks++] = disk; + } disk = NULL; } } -- 1.7.1
On 10/12/2011 02:26 AM, Philipp Hahn wrote:
When PyGrub is used as the bootloader in Xen, it gets passed the first bootable disk. Xend supports a "bootable"-flag for this, which isn't explicitly supported by libvirt.
Hmm, the XML has been enhanced in the meantime; we can now mark various disks as bootable (and in fact, in which order they should be attempted at boot) when targeting qemu. Why can't the same xml changes be made to affect xen? But that can be done as a separate patch.
When converting libvirt-xml to xen-sxpr the "bootable"-flag gets implicitly set by xen.xend.XenConfig.device_add() for the first disk (marked as "Compat hack -- mark first disk bootable"). When converting back xen-sxpr to libvirt-xml, the disks are returned in the internal order used by Xend ignoring the "bootable"-flag, which looses the original order. When the domain is then re-defined, the order
s/looses/loses/
of disks is changed, which breaks PyGrub, since a different disk gets passed.
When converting xen-sxpt to libvirt-xml, use the "bootable"-flag to
s/sxpt/sxpr/
determine the first disk.
This isn't perfect, since several disks can be marked as bootable using the Xend-API, but that is not supported by libvirt. In all known cases relevant to libvirt exactly one disk is marked as bootable.
Signed-off-by: Philipp Hahn<hahn@univention.de> --- valgrind might complain about an uninitialized read access when copying disks[0] to disks[0], but that is overwritten again with the following assignment.
No problem from valgrind; VIR_ALLOC_N guarantees that disks starts life initialized at 0 (that is, calloc, not malloc).
@@ -481,7 +485,13 @@ xenParseSxprDisks(virDomainDefPtr def, if (VIR_REALLOC_N(def->disks, def->ndisks+1)< 0) goto no_memory;
- def->disks[def->ndisks++] = disk; + /* re-order disks if there is a bootable device */ + if (STREQ_NULLABLE(bootable, "1")) { + def->disks[def->ndisks++] = def->disks[0]; + def->disks[0] = disk; + } else { + def->disks[def->ndisks++] = disk; + }
Feels a bit hacky, compared to a memmove to slide all disks; but if, as you say, xend already returns disks in an arbitrary order, then it doesn't matter if we preserve order. ACK and pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
For some versions of Xen the difference between "tap" and "tap2" is important. When converting back from xen-sxpr to libvirt-xml, that information is lost, which breaks re-defining the domain using that data. Explicitly return "tap2" for disks defines as "device/tap2". Signed-off-by: Philipp Hahn <hahn@univention.de> --- <https://forge.univention.org/bugzilla/show_bug.cgi?id=22017> src/xenxs/xen_sxpr.c | 21 +++++++++++++-------- 1 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 038c6bb..4cfa9e8 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -391,14 +391,19 @@ xenParseSxprDisks(virDomainDefPtr def, goto error; } - if (VIR_ALLOC_N(disk->driverName, (offset-src)+1) < 0) - goto no_memory; - if (virStrncpy(disk->driverName, src, offset-src, - (offset-src)+1) == NULL) { - XENXS_ERROR(VIR_ERR_INTERNAL_ERROR, - _("Driver name %s too big for destination"), - src); - goto error; + if (sexpr_lookup(node, "device/tap2") && STRPREFIX(src, "tap:")) { + if (!(disk->driverName = strdup("tap2"))) + goto error; + } else { + if (VIR_ALLOC_N(disk->driverName, (offset-src)+1) < 0) + goto no_memory; + if (virStrncpy(disk->driverName, src, offset-src, + (offset-src)+1) == NULL) { + XENXS_ERROR(VIR_ERR_INTERNAL_ERROR, + _("Driver name %s too big for destination"), + src); + goto error; + } } src = offset + 1; -- 1.7.1
On 10/12/2011 02:55 AM, Philipp Hahn wrote:
For some versions of Xen the difference between "tap" and "tap2" is important. When converting back from xen-sxpr to libvirt-xml, that information is lost, which breaks re-defining the domain using that data.
Explicitly return "tap2" for disks defines as "device/tap2".
s/defines/defined/
Signed-off-by: Philipp Hahn<hahn@univention.de> --- <https://forge.univention.org/bugzilla/show_bug.cgi?id=22017>
src/xenxs/xen_sxpr.c | 21 +++++++++++++-------- 1 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 038c6bb..4cfa9e8 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -391,14 +391,19 @@ xenParseSxprDisks(virDomainDefPtr def, goto error; }
- if (VIR_ALLOC_N(disk->driverName, (offset-src)+1)< 0) - goto no_memory; - if (virStrncpy(disk->driverName, src, offset-src, - (offset-src)+1) == NULL) { - XENXS_ERROR(VIR_ERR_INTERNAL_ERROR, - _("Driver name %s too big for destination"), - src); - goto error; + if (sexpr_lookup(node, "device/tap2")&& STRPREFIX(src, "tap:")) { + if (!(disk->driverName = strdup("tap2"))) + goto error;
s/error/no_memory/
+ } else { + if (VIR_ALLOC_N(disk->driverName, (offset-src)+1)< 0)
Indentation is off. ACK with that fixed, so I pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Philipp Hahn