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

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.