[pve-devel] [PATCH-SERIES] small improvements for parsing and printing

[pve-devel] [PATCH-SERIES] small improvements for parsing and printing

[Fabian Ebner]

Explicitly show 'template: 0' instead of 'template: ' for the verbose
status calls, and handle empty '-list' values for LXC configs.


qemu-server:

Fabian Ebner (2):
  vmstatus: make boolean value explicit
  config: parse: also warn about invalid lines

 PVE/QemuServer.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


pve-container:

Fabian Ebner (2):
  vmstatus: make boolean value explicit
  config: parse: also allow empty values

 src/PVE/LXC.pm        | 2 +-
 src/PVE/LXC/Config.pm | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.20.1

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

[Fabian Ebner]

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;
 
 	$d->{serial} = 1 if conf_has_serial($conf);
 	$d->{lock} = $conf->{lock} if $conf->{lock};
-- 
2.20.1

[pve-devel] [PATCH qemu-server 2/4] config: parse: also warn about invalid lines

[Fabian Ebner]

as we already do for containers.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 43d7c6b..4a17a37 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2215,6 +2215,8 @@ sub parse_vm_config {
 
 		$conf->{$key} = $value;
 	    }
+	} else {
+	    warn "vm $vmid - unable to parse config: $line\n";
 	}
     }
 
-- 
2.20.1

[pve-devel] [PATCH container 3/4] vmstatus: make boolean value explicit

[Fabian Ebner]

as otherwise the empty string is printed with 'pct status <id> --verbose' when
it's not a template.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PVE/LXC.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index bae236b..23a7b06 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -235,7 +235,7 @@ sub vmstatus {
 	$d->{diskread} = 0;
 	$d->{diskwrite} = 0;
 
-	$d->{template} = PVE::LXC::Config->is_template($conf);
+	$d->{template} = PVE::LXC::Config->is_template($conf) ? 1 : 0;
 	$d->{lock} = $conf->{lock} if $conf->{lock};
     }
 
-- 
2.20.1

[pve-devel] [PATCH container 4/4] config: parse: also allow empty values

[Fabian Ebner]

because they are valid for '-list' formats and it makes the behavior match with
what we do for VM configs. The new pattern is the same that is used for VM
configs. Because it is a non-greedy pattern, trailing whitespaces will not be
included in the value anymore. This /should/ cause no problems and the '\s*$'
at the end suggests that that is how it was intended in the first place.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PVE/LXC/Config.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 5d223b2..de7e924 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -933,7 +933,7 @@ sub parse_pct_config {
 	    } else {
 		warn "vm $vmid - property 'delete' is only allowed in [pve:pending]\n";
 	    }
-	} elsif ($line =~ m/^([a-z][a-z_]*\d*):\s*(\S.*)\s*$/) {
+	} elsif ($line =~ m/^([a-z][a-z_]*\d*):\s*(.+?)\s*$/) {
 	    my $key = $1;
 	    my $value = $2;
 	    eval { $value = PVE::LXC::Config->check_type($key, $value); };
-- 
2.20.1

[pve-devel] applied: [PATCH qemu-server 2/4] config: parse: also warn about invalid lines

[Thomas Lamprecht]

On 03.03.21 12:01, Fabian Ebner wrote:
> as we already do for containers.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuServer.pm | 2 ++
>  1 file changed, 2 insertions(+)
> 
>

applied, thanks!

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

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

>  
>  	$d->{serial} = 1 if conf_has_serial($conf);
>  	$d->{lock} = $conf->{lock} if $conf->{lock};
> 

Re: [pve-devel] [PATCH container 3/4] vmstatus: make boolean value explicit

[Thomas Lamprecht]

On 03.03.21 12:01, Fabian Ebner wrote:
> as otherwise the empty string is printed with 'pct status <id> --verbose' when
> it's not a template.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/LXC.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index bae236b..23a7b06 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -235,7 +235,7 @@ sub vmstatus {
>  	$d->{diskread} = 0;
>  	$d->{diskwrite} = 0;
>  
> -	$d->{template} = PVE::LXC::Config->is_template($conf);
> +	$d->{template} = PVE::LXC::Config->is_template($conf) ? 1 : 0;

same comment here, but again can be just fine they way you changed it.

>  	$d->{lock} = $conf->{lock} if $conf->{lock};
>      }
>  
> 

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

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

>>   
>>   	$d->{serial} = 1 if conf_has_serial($conf);
>>   	$d->{lock} = $conf->{lock} if $conf->{lock};
>>
> 

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

[Fabian Ebner]

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