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

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

[Fabian Ebner]

Patches #2 and #3 need versioned breaks for pve-manager!

Patches #4 and #5 can be applied independently.


Changes from v1:
    * don't return template property for non-templates as Thomas suggested
    * add a patch for the UI to make that work
    * add a patch to make the lock property optional again for LXC's vmstatus


manager:

Fabian Ebner (1):
  ui: avoid errors when 'template' property is not present in guest
    status

 www/manager6/lxc/Config.js  | 2 +-
 www/manager6/qemu/Config.js | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)


qemu:

Fabian Ebner (1):
  vmstatus: make template property optional

 PVE/QemuServer.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


container:

Fabian Ebner (3):
  vmstatus: make template property optional
  config: parse: also allow empty values
  vmstatus: make lock property optional again

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

-- 
2.20.1

[pve-devel] [PATCH v2 manager 1/5] ui: avoid errors when 'template' property is not present in guest status

[Fabian Ebner]

The API doesn't advertise the property as non-optional and it's safer and more
in line with the surrounding code.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

Needed for the next two patches to work.

 www/manager6/lxc/Config.js  | 2 +-
 www/manager6/qemu/Config.js | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/www/manager6/lxc/Config.js b/www/manager6/lxc/Config.js
index 89747a0a..73e34614 100644
--- a/www/manager6/lxc/Config.js
+++ b/www/manager6/lxc/Config.js
@@ -347,7 +347,7 @@ Ext.define('PVE.lxc.Config', {
 		var rec = s.data.get('status');
 		status = rec ? rec.data.value : 'unknown';
 		rec = s.data.get('template');
-		template = rec.data.value || false;
+		template = rec ? rec.data.value : false;
 		rec = s.data.get('lock');
 		lock = rec ? rec.data.value : undefined;
 	    }
diff --git a/www/manager6/qemu/Config.js b/www/manager6/qemu/Config.js
index c0c6552c..10bf10a4 100644
--- a/www/manager6/qemu/Config.js
+++ b/www/manager6/qemu/Config.js
@@ -385,7 +385,7 @@ Ext.define('PVE.qemu.Config', {
 		rec = s.data.get('qmpstatus');
 		qmpstatus = rec ? rec.data.value : 'unknown';
 		rec = s.data.get('template');
-		template = rec.data.value || false;
+		template = rec ? rec.data.value : false;
 		rec = s.data.get('lock');
 		lock = rec ? rec.data.value : undefined;
 
-- 
2.20.1

[pve-devel] applied: [PATCH v2 manager 1/5] ui: avoid errors when 'template' property is not present in guest status

[Thomas Lamprecht]

On 11.03.21 11:26, Fabian Ebner wrote:
> The API doesn't advertise the property as non-optional and it's safer and more
> in line with the surrounding code.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> New in v2.
> 
> Needed for the next two patches to work.
> 
>  www/manager6/lxc/Config.js  | 2 +-
>  www/manager6/qemu/Config.js | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
>

applied, thanks!

[pve-devel] [PATCH v2 qemu-server 2/5] vmstatus: make template property optional

[Fabian Ebner]

to avoid printing 'template: ' with 'qm status <id> --verbose' if it's false.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Breaks pve-manager without the previous patch! (e.g. guest's Start/Shutdown
buttons won't be updated anymore)

Changes from v1:
    * make property optional

 PVE/QemuServer.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 1410ecb..8326e66 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2633,7 +2633,7 @@ sub vmstatus {
 	$d->{diskread} = 0;
 	$d->{diskwrite} = 0;
 
-        $d->{template} = PVE::QemuConfig->is_template($conf);
+        $d->{template} = 1 if PVE::QemuConfig->is_template($conf);
 
 	$d->{serial} = 1 if conf_has_serial($conf);
 	$d->{lock} = $conf->{lock} if $conf->{lock};
-- 
2.20.1

Re: [pve-devel] [PATCH v2 qemu-server 2/5] vmstatus: make template property optional

[Thomas Lamprecht]

On 11.03.21 11:26, Fabian Ebner wrote:
> to avoid printing 'template: ' with 'qm status <id> --verbose' if it's false.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Breaks pve-manager without the previous patch! (e.g. guest's Start/Shutdown
> buttons won't be updated anymore)
> 

we need to hold that one off for a 7.x release, ideally 7.0

> Changes from v1:
>     * make property optional
> 
>  PVE/QemuServer.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 1410ecb..8326e66 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -2633,7 +2633,7 @@ sub vmstatus {
>  	$d->{diskread} = 0;
>  	$d->{diskwrite} = 0;
>  
> -        $d->{template} = PVE::QemuConfig->is_template($conf);
> +        $d->{template} = 1 if PVE::QemuConfig->is_template($conf);
>  
>  	$d->{serial} = 1 if conf_has_serial($conf);
>  	$d->{lock} = $conf->{lock} if $conf->{lock};
> 

Re: [pve-devel] [PATCH v2 qemu-server 2/5] vmstatus: make template property optional

[Fabian Ebner]

Am 15.03.21 um 14:00 schrieb Thomas Lamprecht:
> On 11.03.21 11:26, Fabian Ebner wrote:
>> to avoid printing 'template: ' with 'qm status <id> --verbose' if it's false.
>>
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>
>> Breaks pve-manager without the previous patch! (e.g. guest's Start/Shutdown
>> buttons won't be updated anymore)
>>
> 
> we need to hold that one off for a 7.x release, ideally 7.0
> 

Ping for this one and patch #3 from the same series, now that the 
versions are bumped.

>> Changes from v1:
>>      * make property optional
>>
>>   PVE/QemuServer.pm | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 1410ecb..8326e66 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -2633,7 +2633,7 @@ sub vmstatus {
>>   	$d->{diskread} = 0;
>>   	$d->{diskwrite} = 0;
>>   
>> -        $d->{template} = PVE::QemuConfig->is_template($conf);
>> +        $d->{template} = 1 if PVE::QemuConfig->is_template($conf);
>>   
>>   	$d->{serial} = 1 if conf_has_serial($conf);
>>   	$d->{lock} = $conf->{lock} if $conf->{lock};
>>
> 

[pve-devel] applied: [PATCH v2 qemu-server 2/5] vmstatus: make template property optional

[Thomas Lamprecht]

On 11.03.21 11:26, Fabian Ebner wrote:
> to avoid printing 'template: ' with 'qm status <id> --verbose' if it's false.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Breaks pve-manager without the previous patch! (e.g. guest's Start/Shutdown
> buttons won't be updated anymore)
> 
> Changes from v1:
>     * make property optional
> 
>  PVE/QemuServer.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied, thanks!

[pve-devel] [PATCH v2 container 3/5] vmstatus: make template property optional

[Fabian Ebner]

to avoid printing 'template: ' with 'pct status <id> --verbose' if it's false.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Breaks pve-manager without the first patch! (e.g. guest's Start/Shutdown
buttons won't be updated anymore)

Changes from v1:
    * make property optional

 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..082cab8 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} = 1 if PVE::LXC::Config->is_template($conf);
 	$d->{lock} = $conf->{lock} if $conf->{lock};
     }
 
-- 
2.20.1

Re: [pve-devel] [PATCH v2 container 3/5] vmstatus: make template property optional

[Thomas Lamprecht]

On 11.03.21 11:26, Fabian Ebner wrote:
> to avoid printing 'template: ' with 'pct status <id> --verbose' if it's false.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Breaks pve-manager without the first patch! (e.g. guest's Start/Shutdown
> buttons won't be updated anymore)
> 

same here, looks OK, but we can only apply it to master once we branched off
the stable-6 release

> Changes from v1:
>     * make property optional
> 
>  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..082cab8 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} = 1 if PVE::LXC::Config->is_template($conf);
>  	$d->{lock} = $conf->{lock} if $conf->{lock};
>      }
>  
> 

Re: [pve-devel] [PATCH v2 container 3/5] vmstatus: make template property optional

[Fabian Ebner]

Am 15.03.21 um 14:01 schrieb Thomas Lamprecht:
> On 11.03.21 11:26, Fabian Ebner wrote:
>> to avoid printing 'template: ' with 'pct status <id> --verbose' if it's false.
>>
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>
>> Breaks pve-manager without the first patch! (e.g. guest's Start/Shutdown
>> buttons won't be updated anymore)
>>
> 
> same here, looks OK, but we can only apply it to master once we branched off
> the stable-6 release
> 

Ok, I'll try to remember and send a ping/rebased version then.

>> Changes from v1:
>>      * make property optional
>>
>>   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..082cab8 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} = 1 if PVE::LXC::Config->is_template($conf);
>>   	$d->{lock} = $conf->{lock} if $conf->{lock};
>>       }
>>   
>>
> 

[pve-devel] applied: [PATCH v2 container 3/5] vmstatus: make template property optional

[Thomas Lamprecht]

On 11.03.21 11:26, Fabian Ebner wrote:
> to avoid printing 'template: ' with 'pct status <id> --verbose' if it's false.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Breaks pve-manager without the first patch! (e.g. guest's Start/Shutdown
> buttons won't be updated anymore)
> 
> Changes from v1:
>     * make property optional
> 
>  src/PVE/LXC.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied, thanks!

[pve-devel] [PATCH v2 container 4/5] 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>
---

No changes from v1.

 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 v2 container 4/5] config: parse: also allow empty values

[Thomas Lamprecht]

On 11.03.21 11:26, Fabian Ebner wrote:
> 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>
> ---
> 
> No changes from v1.
> 
>  src/PVE/LXC/Config.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied, thanks!

[pve-devel] [RFC/PATCH v2 container 5/5] vmstatus: make lock property optional again

[Fabian Ebner]

Commit d02262048cbbe91ca8b12f98e3dc7bbab28e4c64 made the property de-facto
non-optional. Partially revert this and instead adapt the printing, making the
behavior match the API description again. The conditional assignment is
already there further down the vmstatus function.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

Alternatively, the property description should be updated, but this patch makes
it in line with what we do for VMs and is preferable IMHO.

 src/PVE/CLI/pct.pm | 3 ++-
 src/PVE/LXC.pm     | 1 -
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
index 856d5a5..69faf8d 100755
--- a/src/PVE/CLI/pct.pm
+++ b/src/PVE/CLI/pct.pm
@@ -810,7 +810,8 @@ our $cmddef = {
 	my $format = "%-10s %-10s %-12s %-20s\n";
 	printf($format, 'VMID', 'Status', 'Lock', 'Name');
 	foreach my $d (sort {$a->{vmid} <=> $b->{vmid} } @$res) {
-	    printf($format, $d->{vmid}, $d->{status}, $d->{lock}, $d->{name});
+	    my $lock = $d->{lock} || '';
+	    printf($format, $d->{vmid}, $d->{status}, $lock, $d->{name});
 	}
     }],
     config => [ "PVE::API2::LXC::Config", 'vm_config', ['vmid'], 
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 082cab8..7a52196 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -203,7 +203,6 @@ sub vmstatus {
 	$d->{cpus} = $conf->{cores} || $conf->{cpulimit};
 	$d->{cpus} = $cpucount if !$d->{cpus};
 
-	$d->{lock} = $conf->{lock} || '';
 	$d->{tags} = $conf->{tags} if defined($conf->{tags});
 
 	if ($d->{pid}) {
-- 
2.20.1

[pve-devel] applied: [RFC/PATCH v2 container 5/5] vmstatus: make lock property optional again

[Thomas Lamprecht]

On 11.03.21 11:26, Fabian Ebner wrote:
> Commit d02262048cbbe91ca8b12f98e3dc7bbab28e4c64 made the property de-facto
> non-optional. Partially revert this and instead adapt the printing, making the
> behavior match the API description again. The conditional assignment is
> already there further down the vmstatus function.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> New in v2.
> 
> Alternatively, the property description should be updated, but this patch makes
> it in line with what we do for VMs and is preferable IMHO.
> 

The schema is what our ABI is, so if that stated this is optional we can make
it so.

>  src/PVE/CLI/pct.pm | 3 ++-
>  src/PVE/LXC.pm     | 1 -
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
>

applied, thanks!