[pve-devel] [PATCH-SERIES v2 manager] notes-template followups

[pve-devel] [PATCH-SERIES v2 manager] notes-template followups

[Fabian Ebner]

Decided to collect the patches in a series to make it easier to keep
track. Used v2 because everything besides the first patch is.

Fabian Ebner (6):
  vzdump: verify parameters: properly verify notes-template
  vzdump: generate notes: initialize potentially undef values
  ui: manual backup: list possible template variables directly
  vzdump: avoid 'requires' constraint when parsing defaults
  ui: manual backup: also set notes-template default
  configs: vzdump: add notes-template default

 PVE/VZDump.pm                 | 48 +++++++++++++++++++++++++++++------
 configs/vzdump.conf           |  1 +
 www/manager6/Utils.js         |  2 ++
 www/manager6/dc/Backup.js     |  2 +-
 www/manager6/window/Backup.js | 20 ++++++++++-----
 5 files changed, 58 insertions(+), 15 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH v4 manager 1/6] vzdump: verify parameters: properly verify notes-template

[Fabian Ebner]

instead of just checking for a newline, do a full check already.

Also do the check at the beginning of generate_notes() for consistency
and remove the check after expansion to avoid failing late for things
like '{{cl{{node}}er}}' (which can even expand to a valid variable
making the error even more confusing).

Co-developed-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Re-sent v3.

Changes from v2:
    * Check early rather than injecting error into notes.

 PVE/VZDump.pm | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index edcab696..0dbf8928 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -70,9 +70,32 @@ sub run_command {
     PVE::Tools::run_command($cmdstr, %param, logfunc => $logfunc);
 }
 
+my $verify_notes_template = sub {
+    my ($template) = @_;
+
+    die "contains a line feed\n" if $template =~ /\n/;
+
+    my @problematic = ();
+    while ($template =~ /\\(.)/g) {
+	my $char = $1;
+	push @problematic, "escape sequence '\\$char' at char " . (pos($template) - 2)
+	    if $char !~ /^[n\\]$/;
+    }
+
+    while ($template =~ /\{\{([^\s{}]+)\}\}/g) {
+	my $var = $1;
+	push @problematic, "variable '$var' at char " . (pos($template) - length($var))
+	    if $var !~ /^(cluster|guestname|node|vmid)$/;
+    }
+
+    die "found unknown: " . join(', ', @problematic) . "\n" if scalar(@problematic);
+};
+
 my $generate_notes = sub {
     my ($notes_template, $task) = @_;
 
+    $verify_notes_template->($notes_template);
+
     my $info = {
 	cluster => PVE::Cluster::get_clinfo()->{cluster}->{name},
 	guestname => $task->{hostname},
@@ -92,8 +115,6 @@ my $generate_notes = sub {
     my $vars = join('|', keys $info->%*);
     $notes_template =~ s/\{\{($vars)\}\}/$info->{$1}/g;
 
-    die "unexpected variable name '$1'\n" if $notes_template =~ m/\{\{([^\s}]+)\}\}/;
-
     return $notes_template;
 };
 
@@ -1325,8 +1346,10 @@ sub verify_vzdump_parameters {
 
     $parse_prune_backups_maxfiles->($param, 'CLI parameters');
 
-    raise_param_exc({'notes-template' => "contains a line feed"})
-	if $param->{'notes-template'} && $param->{'notes-template'} =~ m/\n/;
+    if (my $template = $param->{'notes-template'}) {
+	eval { $verify_notes_template->($template); };
+	raise_param_exc({'notes-template' => $@}) if $@;
+    }
 
     $param->{all} = 1 if (defined($param->{exclude}) && !$param->{pool});
 
-- 
2.30.2

[pve-devel] [PATCH v2 manager 2/6] vzdump: generate notes: initialize potentially undef values

[Fabian Ebner]

For VMs, $task->{hostname} might be undef and when running on a
stand-alone node, there is no cluster name.

Reported-by: Marco Gabriel <mgabriel@inett.de>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Changes from v1:
    * Use 'standalone node' rather than empty string if there is no
      cluster.

 PVE/VZDump.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 0dbf8928..86d5a232 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -97,8 +97,8 @@ my $generate_notes = sub {
     $verify_notes_template->($notes_template);
 
     my $info = {
-	cluster => PVE::Cluster::get_clinfo()->{cluster}->{name},
-	guestname => $task->{hostname},
+	cluster => PVE::Cluster::get_clinfo()->{cluster}->{name} // 'standalone node',
+	guestname => $task->{hostname} // "VM $task->{vmid}", # is always set for CTs
 	node => PVE::INotify::nodename(),
 	vmid => $task->{vmid},
     };
-- 
2.30.2

[pve-devel] [PATCH v2 manager 3/6] ui: manual backup: list possible template variables directly

[Fabian Ebner]

rather than as a tooltip.

Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Re-sent v1.

 www/manager6/Utils.js         |  2 ++
 www/manager6/dc/Backup.js     |  2 +-
 www/manager6/window/Backup.js | 16 ++++++++++------
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index b4e2df79..20e9182e 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1801,6 +1801,8 @@ Ext.define('PVE.Utils', {
 	};
 	return value.replace(/(\\\\|\\n)/g, match => replace[match]);
     },
+
+    notesTemplateVars: ['cluster', 'guestname', 'node', 'vmid'],
 },
 
     singleton: true,
diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
index b081be8c..3494aa54 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -411,7 +411,7 @@ Ext.define('PVE.dc.BackupEdit', {
 				      + '<br>'
 				      + Ext.String.format(
 					gettext('Possible template variables are: {0}'),
-					['cluster', 'guestname', 'node', 'vmid'].map(v => `<code>{{${v}}}</code>`).join(', '),
+					PVE.Utils.notesTemplateVars.map(v => `<code>{{${v}}}</code>`).join(', '),
 				    ),
 				},
 			    ],
diff --git a/www/manager6/window/Backup.js b/www/manager6/window/Backup.js
index f77e9ffa..f768aee3 100644
--- a/www/manager6/window/Backup.js
+++ b/www/manager6/window/Backup.js
@@ -181,13 +181,17 @@ Ext.define('PVE.window.Backup', {
 		    fieldLabel: gettext('Notes'),
 		    anchor: '100%',
 		    value: '{{guestname}}',
-		    autoEl: {
-			tag: 'div',
-			'data-qtip': Ext.String.format(
-			    gettext('Notes added to the backup. Possible variables are {0}'),
-			    '{{cluster}}, {{guestname}}, {{node}}, {{vmid}}',
-			),
+		},
+		{
+		    xtype: 'box',
+		    style: {
+			margin: '8px 0px',
+			'line-height': '1.5em',
 		    },
+		    html: Ext.String.format(
+			gettext('Possible template variables are: {0}'),
+			PVE.Utils.notesTemplateVars.map(v => `<code>{{${v}}}</code>`).join(', '),
+		    ),
 		},
 		{
 		    xtype: 'label',
-- 
2.30.2

[pve-devel] [PATCH v2 manager 4/6] vzdump: avoid 'requires' constraint when parsing defaults

[Fabian Ebner]

to avoid warnings like
parse error in '/etc/vzdump.conf' - 'storage': missing property -
'notes-template' requires this property
when there is no default for the required property configured.

In new(), the defaults are mixed in with the regular CLI/API
parameters, so re-check if the required property is set. If it's not,
the defaults do not apply to the current run, and can be dropped.

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

New in v2.

 PVE/VZDump.pm | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 86d5a232..7237823e 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -3,6 +3,7 @@ package PVE::VZDump;
 use strict;
 use warnings;
 
+use Clone;
 use Fcntl ':flock';
 use File::Basename;
 use File::Path;
@@ -35,6 +36,9 @@ my @plugins = qw();
 
 my $confdesc = PVE::VZDump::Common::get_confdesc();
 
+my $confdesc_for_defaults = Clone::clone($confdesc);
+delete $confdesc_for_defaults->{$_}->{requires} for qw(notes-template protected);
+
 # Load available plugins
 my @pve_vzdump_classes = qw(PVE::VZDump::QemuServer PVE::VZDump::LXC);
 foreach my $plug (@pve_vzdump_classes) {
@@ -254,7 +258,7 @@ sub read_vzdump_defaults {
 	map {
 	    my $default = $confdesc->{$_}->{default};
 	     defined($default) ? ($_ => $default) : ()
-	} keys %$confdesc
+	} keys %$confdesc_for_defaults
     };
     $parse_prune_backups_maxfiles->($defaults, "defaults in VZDump schema");
 
@@ -262,7 +266,7 @@ sub read_vzdump_defaults {
     eval { $raw = PVE::Tools::file_get_contents($fn); };
     return $defaults if $@;
 
-    my $conf_schema = { type => 'object', properties => $confdesc, };
+    my $conf_schema = { type => 'object', properties => $confdesc_for_defaults };
     my $res = PVE::JSONSchema::parse_config($conf_schema, $fn, $raw);
     if (my $excludes = $res->{'exclude-path'}) {
 	$res->{'exclude-path'} = PVE::Tools::split_args($excludes);
@@ -546,6 +550,11 @@ sub new {
 	$opts->{storage} = 'local';
     }
 
+    # Enforced by the API too, but these options might come in via defaults. Drop them if necessary.
+    if (!$opts->{storage}) {
+	delete $opts->{$_} for qw(notes-template protected);
+    }
+
     my $errors = '';
     my $add_error = sub {
 	my ($error) = @_;
-- 
2.30.2

[pve-devel] [PATCH v2 manager 5/6] ui: manual backup: also set notes-template default

[Fabian Ebner]

like is done for other vzdump options already.

Requested in the community forum:
https://forum.proxmox.com/threads/108970/#post-468655

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

No changes from v1.

 www/manager6/window/Backup.js | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/www/manager6/window/Backup.js b/www/manager6/window/Backup.js
index f768aee3..510430c0 100644
--- a/www/manager6/window/Backup.js
+++ b/www/manager6/window/Backup.js
@@ -113,6 +113,10 @@ Ext.define('PVE.window.Backup', {
 			    if (!initialDefaults && data.mode !== undefined) {
 				modeSelector.setValue(data.mode);
 			    }
+			    if (!initialDefaults && (data['notes-template'] ?? false)) {
+				me.down('field[name=notes-template]')
+				    .setValue(data['notes-template']);
+			    }
 
 			    initialDefaults = true;
 
-- 
2.30.2

[pve-devel] [PATCH v2 manager 6/6] configs: vzdump: add notes-template default

[Fabian Ebner]

so users can see that it can be configured here.

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

No changes from v1.

Causes a "configuration changed by maintainer dialogue" when
upgrading pve-manager

 configs/vzdump.conf | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/vzdump.conf b/configs/vzdump.conf
index a0075cfa..fe4b18ab 100644
--- a/configs/vzdump.conf
+++ b/configs/vzdump.conf
@@ -14,3 +14,4 @@
 #script: FILENAME
 #exclude-path: PATHLIST
 #pigz: N
+#notes-template: {{guestname}}
-- 
2.30.2

[pve-devel] applied: [PATCH-SERIES v2 manager] notes-template followups

[Thomas Lamprecht]

Am 5/9/22 um 12:34 schrieb Fabian Ebner:
> Decided to collect the patches in a series to make it easier to keep
> track. Used v2 because everything besides the first patch is.
> 
> Fabian Ebner (6):
>   vzdump: verify parameters: properly verify notes-template
>   vzdump: generate notes: initialize potentially undef values
>   ui: manual backup: list possible template variables directly
>   vzdump: avoid 'requires' constraint when parsing defaults
>   ui: manual backup: also set notes-template default
>   configs: vzdump: add notes-template default
> 
>  PVE/VZDump.pm                 | 48 +++++++++++++++++++++++++++++------
>  configs/vzdump.conf           |  1 +
>  www/manager6/Utils.js         |  2 ++
>  www/manager6/dc/Backup.js     |  2 +-
>  www/manager6/window/Backup.js | 20 ++++++++++-----
>  5 files changed, 58 insertions(+), 15 deletions(-)
> 

applied series, thanks!