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

[Hendrik Schwartke]

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