[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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;
     }
 

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]