[pve-devel] [PATCH pve-common 1/3 v2] Add tests for verify_configid

[pve-devel] [PATCH pve-common 1/3 v2] Add tests for verify_configid

[Dominic Jäger]

Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
---
v1->v2: Didn't exist

 test/Makefile       |  2 +-
 test/format_test.pl | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)
 create mode 100755 test/format_test.pl

diff --git a/test/Makefile b/test/Makefile
index b8118c7..85ecac5 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -6,7 +6,7 @@ all:
 
 export PERLLIB=../src
 
-check: lock_file.test calendar_event_test.test convert_size_test.test procfs_tests.test
+check: lock_file.test calendar_event_test.test convert_size_test.test procfs_tests.test format_test.test
 	for d in $(SUBDIRS); do $(MAKE) -C $$d check; done
 
 %.test: %.pl
diff --git a/test/format_test.pl b/test/format_test.pl
new file mode 100755
index 0000000..3f225de
--- /dev/null
+++ b/test/format_test.pl
@@ -0,0 +1,27 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use lib '../src';
+use PVE::JSONSchema;
+
+use Test::More;
+use Test::MockModule;
+
+my $valid_configids = [
+	'aa', 'a0', 'a_', 'a-', 'a-a', 'a'x100, 'Aa', 'AA',
+];
+my $invalid_configids = [
+	'a', 'a+', '1a', '_a', '-a', '+a', 'A',
+];
+
+my $noerr = 1; # easier to test
+foreach my $id (@$valid_configids) {
+    is(PVE::JSONSchema::pve_verify_configid($id, $noerr), $id, 'valid configid');
+}
+foreach my $id (@$invalid_configids) {
+    is(PVE::JSONSchema::pve_verify_configid($id, $noerr), undef, 'invalid configid');
+}
+
+done_testing();
\ No newline at end of file
-- 
2.20.1

[pve-devel] [PATCH pve-common 2/3 v2] Make configid regex public

[Dominic Jäger]

Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
---
v1->v2: Didn't exist

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

diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index e8d7395..29ada5b 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -21,6 +21,8 @@ parse_property_string
 register_standard_option
 );
 
+our $CONFIGID_RE = qr/[a-z][a-z0-9_-]+/i;
+
 # Note: This class implements something similar to JSON schema, but it is not 100% complete.
 # see: http://tools.ietf.org/html/draft-zyp-json-schema-02
 # see: http://json-schema.org/
@@ -177,7 +179,7 @@ register_format('pve-configid', \&pve_verify_configid);
 sub pve_verify_configid {
     my ($id, $noerr) = @_;
 
-    if ($id !~ m/^[a-z][a-z0-9_-]+$/i) {
+    if ($id !~ m/^$CONFIGID_RE$/) {
 	return undef if $noerr;
 	die "invalid configuration ID '$id'\n";
     }
-- 
2.20.1

[pve-devel] [PATCH pve-storage 3/3 v2] lvmthin: Match snapshot remove regex to allowed names

[Dominic Jäger]

We allow snapshot names that match pve-configid but during qm destroy we have
not removed all snapshots that match pve-configid so far. For example, the name
x-y was allowed but the resulting snap_vm-105-disk-0_x-y was not removed.

Reported-by: Hannes Laimer <h.laimer@proxmox.com>
Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
---
Requires the pve-common patches

v1->v2: Use the now public regex instead of the verify function

The "use constant" variant requires a backslash
${\PVE::JSONSchema::CONFIGID_RE} for each usage and the getter/method variant
requires postponed regular subexpressions m/^(??{get_configid_re()})$/), so
"our" looked the most simple to me.

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

diff --git a/PVE/Storage/LvmThinPlugin.pm b/PVE/Storage/LvmThinPlugin.pm
index d1c5b1f..c9e127c 100644
--- a/PVE/Storage/LvmThinPlugin.pm
+++ b/PVE/Storage/LvmThinPlugin.pm
@@ -117,7 +117,7 @@ sub free_image {
 
 	# remove all volume snapshots first
 	foreach my $lv (keys %$dat) {
-	    next if $lv !~ m/^snap_${volname}_(\w+)$/;
+	    next if $lv !~ m/^snap_${volname}_${PVE::JSONSchema::CONFIGID_RE}$/;
 	    my $cmd = ['/sbin/lvremove', '-f', "$vg/$lv"];
 	    run_command($cmd, errmsg => "lvremove snapshot '$vg/$lv' error");
 	}
-- 
2.20.1

[pve-devel] applied: [PATCH pve-common 1/3 v2] Add tests for verify_configid

[Thomas Lamprecht]

On 28.10.20 11:04, Dominic Jäger wrote:
> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
> ---
> v1->v2: Didn't exist
> 
>  test/Makefile       |  2 +-
>  test/format_test.pl | 27 +++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+), 1 deletion(-)
>  create mode 100755 test/format_test.pl
> 
>

applied, thanks!

[pve-devel] applied: [PATCH pve-common 2/3 v2] Make configid regex public

[Thomas Lamprecht]

On 28.10.20 11:04, Dominic Jäger wrote:
> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
> ---
> v1->v2: Didn't exist
> 
>  src/PVE/JSONSchema.pm | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH pve-storage 3/3 v2] lvmthin: Match snapshot remove regex to allowed names

[Thomas Lamprecht]

On 28.10.20 11:04, Dominic Jäger wrote:
> We allow snapshot names that match pve-configid but during qm destroy we have
> not removed all snapshots that match pve-configid so far. For example, the name
> x-y was allowed but the resulting snap_vm-105-disk-0_x-y was not removed.
> 
> Reported-by: Hannes Laimer <h.laimer@proxmox.com>
> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
> ---
> Requires the pve-common patches
> 
> v1->v2: Use the now public regex instead of the verify function
> 
> The "use constant" variant requires a backslash
> ${\PVE::JSONSchema::CONFIGID_RE} for each usage and the getter/method variant
> requires postponed regular subexpressions m/^(??{get_configid_re()})$/), so
> "our" looked the most simple to me.
> 
>  PVE/Storage/LvmThinPlugin.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied, thanks!