Re: [pve-devel] [PATCH qemu-server 1/4] vmstatus: make boolean value explicit

[Fabian Ebner <f.ebner@proxmox.com>]

Am 04.03.21 um 09:04 schrieb Fabian Ebner:
> Am 03.03.21 um 17:53 schrieb Thomas Lamprecht:
>> On 03.03.21 12:01, Fabian Ebner wrote:
>>> as otherwise the empty string is printed with 'qm status <id> 
>>> --verbose' when
>>> it's not a template.
>>>
>>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>>> ---
>>>   PVE/QemuServer.pm | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>>> index a498444..43d7c6b 100644
>>> --- a/PVE/QemuServer.pm
>>> +++ b/PVE/QemuServer.pm
>>> @@ -2621,7 +2621,7 @@ sub vmstatus {
>>>       $d->{diskread} = 0;
>>>       $d->{diskwrite} = 0;
>>> -        $d->{template} = PVE::QemuConfig->is_template($conf);
>>> +        $d->{template} = PVE::QemuConfig->is_template($conf) ? 1 : 0;
>>
>>
>> Any reason to not do:
>>
>> $d->{template} = 1 if PVE::QemuConfig->is_template($conf);
>>
>> (no hard feelings, but this is relatively common pattern for such things,
>> especially if they can be normally more often omitted than not, templates
>> are normally rather outnumbered by non-templates)
>>
> 
> Not really a reason, just that the key was always present before the 
> change too. But it's not an API return key, so making this optional is 
> no problem at all (and we do so for other ones like the 'serial' below, 
> so it's also more consistent in a way). I'll send a v2.
> 

Well, turns out this is a reason. Namely the web UI currently relies on 
the property being there:

     rec = s.data.get('template');
     template = rec.data.value || false;

Should I still go for it?


Also, we say

     lock => {
         description => "The current config lock, if any.",
         type => 'string',
         optional => 1,
     },

but for containers, we do

     $d->{lock} = $conf->{lock} || '';

so it doesn't fully match the description (it's de-facto not optional). 
The reason is that there's a 'lock' column for 'pct list', see 
pve-container commit d02262048cbbe91ca8b12f98e3dc7bbab28e4c64. Is it 
worth changing the description or should the behavior rather be changed 
(while adapting the printing for 'pct list' of course)?

>>>       $d->{serial} = 1 if conf_has_serial($conf);
>>>       $d->{lock} = $conf->{lock} if $conf->{lock};
>>>
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>