[pve-devel] [PATCH v2 qemu-server 04/12] Adapt CPUConfig to handle custom models

[Thomas Lamprecht]

On 10/2/19 10:24 AM, Stefan Reiter wrote:
> Thanks for the review, I'll prepare a v3 :)
> 
> Just some clarifications inline for this patch.
> 
> On 10/2/19 7:49 AM, Thomas Lamprecht wrote:
>> On 9/30/19 12:58 PM, Stefan Reiter wrote:
>>> Turn CPUConfig into a SectionConfig with parsing/writing support for
>>> custom CPU models. IO is handled using cfs.
>>>
>>> The "custom" parameter provides differentiation between custom and
>>> default types, even if the name is the same ('namespacing').
>>>
>>> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
>>> ---
>>>   PVE/QemuServer/CPUConfig.pm | 59 ++++++++++++++++++++++++++++++++++---
>>>   1 file changed, 55 insertions(+), 4 deletions(-)
>>>

>>> +    'reported-model' => {
>>> +    description => "CPU model and vendor to report to the guest. Must be a QEMU/KVM supported model."
>>> +             . " Only valid for custom CPU model definitions, default models will always report themselves to the guest OS.",
>>> +    type => 'string',
>>> +    enum => [ sort { "\L$a" cmp "\L$b" } keys %$cpu_vendor_list ],
>>
>> we do not use \L at all in our code, AFAICT, just use the a bit more
>> expressive, or at least already used all over the place, lc()
>>
> 
> TBH I just moved that part from the existing "cputype" enum, but I can change it in v3 - it sure looks clearer with lc().

argh, sorry for the wrong "accusations" then, didn't bother to look
it up but had no \L use in mind and that normally means that if there
are any they are outsiders.. ^^

> 
>>> +    default => 'kvm64',
>>> +    custom_only => 1,
>> .         ^^^^^^^^^^^
>> That's a fake JSONSchema attribute, we do not do those normally, either
>> some mechanism gets added to JSONSchema to really represent this in general
>> or just omit it.
>> >> +    optional => 1,
>>>       },
>>>       hidden => {
>>>       description => "Do not identify as a KVM virtual machine.",
>>> @@ -102,14 +129,34 @@ our $cpu_fmt = {
>>>       flags => {
>>>       description => "List of additional CPU flags separated by ';'."
>>>                . " Use '+FLAG' to enable, '-FLAG' to disable a flag."
>>> -             . " Currently supported flags: @{[join(', ', @supported_cpu_flags)]}.",
>>> +             . " Custom CPU models can specify any flag supported by"
>>> +             . " QEMU/KVM, VM-specific flags must be from the following"
>>> +             . " set for security reasons: @{[join(', ', @supported_cpu_flags)]}.",
>>
>> is the last part still true if the pattern restriction to $cpu_flag are lifted
>> below?
>>
> 
> Yes, because it is checked in verify_vm_cpu_conf from patch 06/12. I admit it's not entirely clear from just looking at this property definition, but it was the only way I could think of to somewhat cleanly handle the re-use of $cpu_fmt for VM-specific settings and custom models - maybe I'll add a comment.


so it "breaks" temporarily between here and 06/12? Can be overlooked but
in general not OK, series should not introduce temporary breakage of
things, if possible, if only to make review a bit easier.