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

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