On Mon, Jul 11, 2011 at 06:16:11PM +0200, Matthias Bolte wrote:
2011/7/11 Daniel Veillard <veillard@redhat.com>:
On Sat, Jul 09, 2011 at 03:38:32PM +0200, Matthias Bolte wrote:
2011/7/8 Eric Blake <eblake@redhat.com>:
On 07/08/2011 02:13 AM, Matthias Bolte wrote:
The drivers were accepting domain configs without checking if those were actually meant for them. For example the LXC driver happily accepts configs with type QEMU.
For convenience add an optional check for the domain type for the virDomainDefParse* functions. It's optional because in some places virDomainDefParse* is used to parse a config without caring about it's type. Also the QEMU driver has to accept 4 different types and does this checking own it's own.
Can we factor the 4 QEMU types back into the common method? Do this by treating expectedType as a bitmask: [...] @@ -5836,6 +5842,13 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, } VIR_FREE(tmp);
+ if (((1 << def->virtType) & expectedVirtTypes) == 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected domain type %s"), + virDomainVirtTypeToString(def->virtType)); + goto error; + } +
Looks fine, ACK
My only regret here is that we can't really suggest the value expected because QEmu accepts more than one, but for other drivers we should be able to provide what type is expected.
Yes, we can do that even for QEMU. See attached diff between v2 and v3 for easier review.
Anyway the main error here is when people use qemu instead of kvm and end up with a non-accelerated guest and there is nothing we can do there :-\
Yes, because the user might do this on purpose and not by accident.
-- Matthias Bolte http://photron.blogspot.com
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a784f4d..39ed317 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -29,6 +29,7 @@ #include <fcntl.h> #include <dirent.h> #include <sys/time.h> +#include <math.h>
#include "virterror_internal.h" #include "datatypes.h" @@ -48,6 +49,7 @@ #include "files.h" #include "bitmap.h" #include "verify.h" +#include "count-one-bits.h"
#define VIR_FROM_THIS VIR_FROM_DOMAIN
@@ -5846,10 +5848,42 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, } VIR_FREE(tmp);
- if (((1 << def->virtType) & expectedVirtTypes) == 0) { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected domain type %s"), - virDomainVirtTypeToString(def->virtType)); + if ((expectedVirtTypes & (1 << def->virtType)) == 0) { + if (count_one_bits(expectedVirtTypes) == 1) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected domain type %s, expecting %s"), + virDomainVirtTypeToString(def->virtType), + virDomainVirtTypeToString(log2(expectedVirtTypes))); + } else { + virBuffer buffer = VIR_BUFFER_INITIALIZER; + char *string; + + for (i = 0; i < VIR_DOMAIN_VIRT_LAST; ++i) { + if ((expectedVirtTypes & (1 << i)) != 0) { + if (virBufferUse(&buffer) > 0) + virBufferAddLit(&buffer, ", "); + + virBufferAdd(&buffer, virDomainVirtTypeToString(i), -1); + } + } + + if (virBufferError(&buffer)) { + virReportOOMError(); + virBufferFreeAndReset(&buffer); + goto error; + } + + string = virBufferContentAndReset(&buffer); + + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected domain type %s, " + "expecting one of these: %s"), + virDomainVirtTypeToString(def->virtType), + string); + + VIR_FREE(string); + } + goto error; }
Ah, right, that's quite abit of code for an error report but I think it's worth it as this can be quite challenging for a newbie, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/