[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: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


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