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

Re: [libvirt] [PATCH] Added the attribute vendor_id to the cpu model



On 27.06.2012 18:29, Michal Privoznik wrote:
On 27.06.2012 18:09, Hendrik Schwartke wrote:
Thank you Michal and Eric for fast

On 27.06.2012 17:22, Eric Blake wrote:
On 06/27/2012 09:14 AM, Michal Privoznik wrote:
On 21.06.2012 16:58, Hendrik Schwartke wrote:
Introducing the attribute vendor_id to force the CPUID instruction
in a kvm guest to return the specified vendor.
---
+<optional>
+<attribute name="vendor_id">
+<data type="string">
+<param name='pattern'>.{12}</param>
Rather than using '.', use [a-zA-Z...] to limit the RNG to the same set
of characters as the C code (I'm not sure what the official set is,
though).  I can understand rejecting a too-long name, but do we really
also want to reject a too-short name?
Ok, that right. I will fix the pattern. The reason for rejecting
too-short names is that the CPUID instruction returns the vendor id in
EBX, ECX and EDX, thus the vendor id must be exactly 12 characters long.
So it would be necessary to pad short names in some way. I think the
best way to handle them is to reject them.

+        def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) {
+
+           if(virXPathBoolean("boolean(./model[1]/@fallback)",
ctxt)) {
Style: space after 'if'


+           }
+
+           if(virXPathBoolean("boolean(./model[1]/@vendor_id)",
ctxt)) {
+               char *vendor_id;
+
+               vendor_id =
virXPathString("string(./model[1]/@vendor_id)", ctxt);
+               if(!vendor_id ||
strlen(vendor_id)!=VIR_CPU_VENDOR_ID_LENGTH) {
two more instances.  Also, space on both sides of '!='


+               }
+               for(i=0; i<strlen(vendor_id); i++) {
space after 'for'

+                   if(!isprint(vendor_id[i]) ||
isspace(vendor_id[i])) {
and 'if'


+#define VIR_CPU_VENDOR_ID_LENGTH 12
+
Insert one space between '#' and define so this is indented properly.
'make syntax-check' with 'cppi' installed will flag this for you.
Thanks a lot, that's a very good hint!
@@ -3924,6 +3926,8 @@ qemuBuildCpuArgStr(const struct qemud_driver
*driver,
                   goto cleanup;

               virBufferAdd(&buf, guest->model, -1);
+            if(guest->vendor_id)
space after 'if'

Otherwise looking good. Can somebody chime in and provide more light on
allowed characters in vendor id?
Well, the Intel instruction manual doesn't make any statement about the
allowed characters.
There are two reasons why I check for printable characters and spaces.
The first is that I think the CPUID instruction is intended that way.
The second is that I want to prevent problems calling qemu. It's for
example not possible to pass spaces in the vendor id to qemu.

Sorry, I can't help there.  But I think we've pointed out enough issues
to warrant a v2 submission.

I will rework the patch and post it tomorrow.

According to Wikipedia, it's possible to have spaces in vendor ID:

     http://en.wikipedia.org/wiki/CPUID#EAX.3D0:_Get_vendor_ID

e.g. "VIA VIA VIA ". In that case we need to find a way of telling this
to qemu. What about simple argument closing into quotation marks?

Michal
Yes, you're right. I tested again. The reason for my problem with spaces was the shell. I had a look at the qemu code and the only character which is not usable is ','.
I will post the patch shortly.

Hendrik


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