Re: [libvirt] [PATCH] Add domain type checking

2011/7/11 Daniel Veillard <veillard(a)redhat.com&gt;: > On Sat, Jul 09, 2011 at 03:38:32PM +0200, Matthias Bolte wrote: >> 2011/7/8 Eric Blake <eblake(a)redhat.com&gt;: >> > 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; }