[libvirt] [PATCH] Add domain type checking

[Daniel Veillard]

On Mon, Jul 11, 2011 at 06:16:11PM +0200, Matthias Bolte wrote:
> 2011/7/11 Daniel Veillard <veillard at redhat.com>:
> > On Sat, Jul 09, 2011 at 03:38:32PM +0200, Matthias Bolte wrote:
> >> 2011/7/8 Eric Blake <eblake at 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 at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/