[pve-devel] [PATCH common v2 3/4] section config: allow separated property lists for plugins

[Dominik Csapak <d.csapak@proxmox.com>]

when using 'init(1)'. This saves the property lists per type instead of
a big one, and using create/updateSchema creates a new schema with the
options as 'oneOf' and/or 'instance-types' (depending if the schemas
match).

with that, we change how we work with the options hash:

it's not needed anymore to specify options that are specified in the
type specific propertyList, except if it's 'fixed => 1' (since that does
not exist in the schema)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* don't use the __global hack anymore, we now construct the
  plugin-specific options hash in `init`
* all 'global' properties are added in the api as they are
* improved the compare function and moved to PVE::Tools as a generic
  recursive variable compare
* removed copy_property
* optimized the genrated schema: when a property is used by all types,
  the 'type-property' and 'instance-types' is removed, this makes the
  docs a bit shorter when many of such options exist
* added an is_separated call

 src/PVE/SectionConfig.pm | 259 ++++++++++++++++++++++++++++++---------
 src/PVE/Tools.pm         |  33 +++++
 2 files changed, 235 insertions(+), 57 deletions(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index 5690476..eb39b41 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -8,6 +8,7 @@ use Digest::SHA;
 
 use PVE::Exception qw(raise_param_exc);
 use PVE::JSONSchema qw(get_standard_option);
+use PVE::Tools qw(compare_deeply);
 
 my $defaultData = {
     options => {},
@@ -51,6 +52,61 @@ sub plugindata {
     return {};
 }
 
+sub is_separated {
+    my ($class) = @_;
+
+    return $class->private()->{propertyListSeparated}->%* > 0;
+}
+
+my sub cmp_property {
+    my ($a, $b, $skip_opts) = @_;
+
+    my $merged = {$a->%*, $b->%*};
+    for my $opt ($skip_opts->@*) {
+	delete $merged->{$opt};
+    }
+    for my $opt (keys $merged->%*) {
+	return 0 if !compare_deeply($a->{$opt}, $b->{$opt});
+    }
+
+    return 1;
+};
+
+my sub add_property {
+    my ($props, $key, $prop, $type) = @_;
+
+    if (!defined($props->{$key})) {
+	$props->{$key} = $prop;
+	return;
+    }
+
+    if (!defined($props->{$key}->{oneOf})) {
+	if (cmp_property($props->{$key}, $prop, ['instance-types'])) {
+	    push $props->{$key}->{'instance-types'}->@*, $type;
+	} else {
+	    my $new_prop = delete $props->{$key};
+	    delete $new_prop->{'type-property'};
+	    delete $prop->{'type-property'};
+	    $props->{$key} = {
+		'type-property' => 'type',
+		oneOf => [
+		    $new_prop,
+		    $prop,
+		],
+	    };
+	}
+    } else {
+	for my $existing_prop ($props->{$key}->{oneOf}->@*) {
+	    if (cmp_property($existing_prop, $prop, ['instance-types', 'type-property'])) {
+		push $existing_prop->{'instance-types'}->@*, $type;
+		return;
+	    }
+	}
+
+	push $props->{$key}->{oneOf}->@*, $prop;
+    }
+};
+
 sub createSchema {
     my ($class, $skip_type, $base) = @_;
 
@@ -60,42 +116,61 @@ sub createSchema {
 
     my $props = $base || {};
 
-    my $copy_property = sub {
-	my ($src) = @_;
-
-	my $res = {};
-	foreach my $k (keys %$src) {
-	    $res->{$k} = $src->{$k};
-	}
+    if (!$class->is_separated()) {
+	foreach my $p (keys %$propertyList) {
+	    next if $skip_type && $p eq 'type';
 
-	return $res;
-    };
+	    if (!$propertyList->{$p}->{optional}) {
+		$props->{$p} = $propertyList->{$p};
+		next;
+	    }
 
-    foreach my $p (keys %$propertyList) {
-	next if $skip_type && $p eq 'type';
+	    my $required = 1;
 
-	if (!$propertyList->{$p}->{optional}) {
-	    $props->{$p} = $propertyList->{$p};
-	    next;
-	}
+	    my $copts = $class->options();
+	    $required = 0 if defined($copts->{$p}) && $copts->{$p}->{optional};
 
-	my $required = 1;
-
-	my $copts = $class->options();
-	$required = 0 if defined($copts->{$p}) && $copts->{$p}->{optional};
+	    foreach my $t (keys %$plugins) {
+		my $opts = $pdata->{options}->{$t} || {};
+		$required = 0 if !defined($opts->{$p}) || $opts->{$p}->{optional};
+	    }
 
-	foreach my $t (keys %$plugins) {
-	    my $opts = $pdata->{options}->{$t} || {};
-	    $required = 0 if !defined($opts->{$p}) || $opts->{$p}->{optional};
+	    if ($required) {
+		# make a copy, because we modify the optional property
+		my $res = {$propertyList->{$p}->%*}; # shallow copy
+		$res->{optional} = 0;
+		$props->{$p} = $res;
+	    } else {
+		$props->{$p} = $propertyList->{$p};
+	    }
 	}
-
-	if ($required) {
-	    # make a copy, because we modify the optional property
-	    my $res = &$copy_property($propertyList->{$p});
-	    $res->{optional} = 0;
-	    $props->{$p} = $res;
-	} else {
-	    $props->{$p} = $propertyList->{$p};
+    } else {
+	for my $type (sort keys %$plugins) {
+	    my $opts = $pdata->{options}->{$type} || {};
+	    for my $key (sort keys $opts->%*) {
+		my $schema = $class->get_property_schema($type, $key);
+		my $prop = {$schema->%*};
+		$prop->{'instance-types'} = [$type];
+		$prop->{'type-property'} = 'type';
+		$prop->{optional} = 1 if $opts->{$key}->{optional};
+
+		add_property($props, $key, $prop, $type);
+	    }
+	}
+	# add remaining global properties
+	for my $opt (keys $propertyList->%*) {
+	    next if $props->{$opt};
+	    $props->{$opt} = {$propertyList->{$opt}->%*};
+	}
+	for my $opt (keys $props->%*) {
+	    if (my $necessaryTypes = $props->{$opt}->{'instance-types'}) {
+		if ($necessaryTypes->@* == scalar(keys $plugins->%*)) {
+		    delete $props->{$opt}->{'instance-types'};
+		    delete $props->{$opt}->{'type-property'};
+		} else {
+		    $props->{$opt}->{optional} = 1;
+		}
+	    }
 	}
     }
 
@@ -117,30 +192,61 @@ sub updateSchema {
 
     my $filter_type = $single_class ? $class->type() : undef;
 
-    foreach my $p (keys %$propertyList) {
-	next if $p eq 'type';
+    if (!$class->is_separated()) {
+	foreach my $p (keys %$propertyList) {
+	    next if $p eq 'type';
 
-	my $copts = $class->options();
+	    my $copts = $class->options();
 
-	next if defined($filter_type) && !defined($copts->{$p});
+	    next if defined($filter_type) && !defined($copts->{$p});
 
-	if (!$propertyList->{$p}->{optional}) {
-	    $props->{$p} = $propertyList->{$p};
-	    next;
-	}
+	    if (!$propertyList->{$p}->{optional}) {
+		$props->{$p} = $propertyList->{$p};
+		next;
+	    }
 
-	my $modifyable = 0;
+	    my $modifyable = 0;
 
-	$modifyable = 1 if defined($copts->{$p}) && !$copts->{$p}->{fixed};
+	    $modifyable = 1 if defined($copts->{$p}) && !$copts->{$p}->{fixed};
 
-	foreach my $t (keys %$plugins) {
-	    my $opts = $pdata->{options}->{$t} || {};
-	    next if !defined($opts->{$p});
-	    $modifyable = 1 if !$opts->{$p}->{fixed};
+	    foreach my $t (keys %$plugins) {
+		my $opts = $pdata->{options}->{$t} || {};
+		next if !defined($opts->{$p});
+		$modifyable = 1 if !$opts->{$p}->{fixed};
+	    }
+	    next if !$modifyable;
+
+	    $props->{$p} = $propertyList->{$p};
+	}
+    } else {
+	for my $type (sort keys %$plugins) {
+	    my $opts = $pdata->{options}->{$type} || {};
+	    for my $key (sort keys $opts->%*) {
+		next if $opts->{$key}->{fixed};
+
+		my $schema = $class->get_property_schema($type, $key);
+		my $prop = {$schema->%*};
+		$prop->{'instance-types'} = [$type];
+		$prop->{'type-property'} = 'type';
+		$prop->{optional} = 1;
+
+		add_property($props, $key, $prop, $type);
+	    }
 	}
-	next if !$modifyable;
 
-	$props->{$p} = $propertyList->{$p};
+	for my $opt (keys $propertyList->%*) {
+	    next if $props->{$opt};
+	    $props->{$opt} = {$propertyList->{$opt}->%*};
+	}
+
+	for my $opt (keys $props->%*) {
+	    if (my $necessaryTypes = $props->{$opt}->{'instance-types'}) {
+		if ($necessaryTypes->@* == scalar(keys $plugins->%*)) {
+		    delete $props->{$opt}->{'instance-types'};
+		    delete $props->{$opt}->{'type-property'};
+		}
+	    }
+	}
     }
 
     $props->{digest} = get_standard_option('pve-config-digest');
@@ -160,22 +266,28 @@ sub updateSchema {
 }
 
 sub init {
-    my ($class) = @_;
+    my ($class, $separate_properties) = @_;
 
     my $pdata = $class->private();
 
-    foreach my $k (qw(options plugins plugindata propertyList)) {
+    foreach my $k (qw(options plugins plugindata propertyList propertyListSeparated)) {
 	$pdata->{$k} = {} if !$pdata->{$k};
     }
 
     my $plugins = $pdata->{plugins};
     my $propertyList = $pdata->{propertyList};
+    my $propertyListSeparated = $pdata->{propertyListSeparated};
 
     foreach my $type (keys %$plugins) {
 	my $props = $plugins->{$type}->properties();
 	foreach my $p (keys %$props) {
-	    die "duplicate property '$p'" if defined($propertyList->{$p});
-	    my $res = $propertyList->{$p} = {};
+	    my $res;
+	    if ($separate_properties) {
+		$res = $propertyListSeparated->{$type}->{$p} = {};
+	    } else {
+		die "duplicate property '$p'" if defined($propertyList->{$p});
+		$res = $propertyList->{$p} = {};
+	    }
 	    my $data = $props->{$p};
 	    for my $a (keys %$data) {
 		$res->{$a} = $data->{$a};
@@ -187,8 +299,23 @@ sub init {
     foreach my $type (keys %$plugins) {
 	my $opts = $plugins->{$type}->options();
 	foreach my $p (keys %$opts) {
-	    die "undefined property '$p'" if !$propertyList->{$p};
+	    my $prop;
+	    if ($separate_properties) {
+		$prop = $propertyListSeparated->{$type}->{$p};
+	    }
+	    $prop //= $propertyList->{$p};
+	    die "undefined property '$p'" if !$prop;
 	}
+
+	# automatically the properties to options (if not specified explicitely)
+	if ($separate_properties) {
+	    foreach my $p (keys $propertyListSeparated->{$type}->%*) {
+		next if $opts->{$p};
+		$opts->{$p} = {};
+		$opts->{$p}->{optional} = 1 if $propertyListSeparated->{$type}->{$p}->{optional};
+	    }
+	}
+
 	$pdata->{options}->{$type} = $opts;
     }
 
@@ -237,11 +364,12 @@ sub check_value {
     return $value if $key eq 'type' && $type eq $value;
 
     my $opts = $pdata->{options}->{$type};
+
     die "unknown section type '$type'\n" if !$opts;
 
     die "unexpected property '$key'\n" if !defined($opts->{$key});
 
-    my $schema = $pdata->{propertyList}->{$key};
+    my $schema = $class->get_property_schema($type, $key);
     die "unknown property type\n" if !$schema;
 
     my $ct = $schema->{type};
@@ -295,6 +423,20 @@ sub format_section_header {
     return "$type: $sectionId\n";
 }
 
+sub get_property_schema {
+    my ($class, $type, $key) = @_;
+
+    my $pdata = $class->private();
+    my $opts = $pdata->{options}->{$type};
+
+    my $schema;
+    if ($class->is_separated()) {
+	$schema = $pdata->{propertyListSeparated}->{$type}->{$key};
+    }
+    $schema //= $pdata->{propertyList}->{$key};
+
+    return $schema;
+}
 
 sub parse_config {
     my ($class, $filename, $raw, $allow_unknown) = @_;
@@ -322,7 +464,7 @@ sub parse_config {
     my $is_array = sub {
 	my ($type, $key) = @_;
 
-	my $schema = $pdata->{propertyList}->{$key};
+	my $schema = $class->get_property_schema($type, $key);
 	die "unknown property type\n" if !$schema;
 
 	return $schema->{type} eq 'array';
@@ -494,7 +636,6 @@ sub write_config {
     my ($class, $filename, $cfg, $allow_unknown) = @_;
 
     my $pdata = $class->private();
-    my $propertyList = $pdata->{propertyList};
 
     my $out = '';
 
@@ -516,6 +657,7 @@ sub write_config {
 	my $scfg = $ids->{$sectionId};
 	my $type = $scfg->{type};
 	my $opts = $pdata->{options}->{$type};
+	my $global_opts = $pdata->{options}->{__global};
 
 	die "unknown section type '$type'\n" if !$opts && !$allow_unknown;
 
@@ -545,7 +687,8 @@ sub write_config {
 	if ($scfg->{comment} && !$done_hash->{comment}) {
 	    my $k = 'comment';
 	    my $v = $class->encode_value($type, $k, $scfg->{$k});
-	    $data .= &$format_config_line($propertyList->{$k}, $k, $v);
+	    my $prop = $class->get_property_schema($type, $k);
+	    $data .= &$format_config_line($prop, $k, $v);
 	}
 
 	$data .= "\tdisable\n" if $scfg->{disable} && !$done_hash->{disable};
@@ -562,7 +705,8 @@ sub write_config {
 	    die "section '$sectionId' - missing value for required option '$k'\n"
 		if !defined ($v);
 	    $v = $class->encode_value($type, $k, $v);
-	    $data .= &$format_config_line($propertyList->{$k}, $k, $v);
+	    my $prop = $class->get_property_schema($type, $k);
+	    $data .= &$format_config_line($prop, $k, $v);
 	}
 
 	foreach my $k (@option_keys) {
@@ -570,7 +714,8 @@ sub write_config {
 	    my $v = $scfg->{$k};
 	    next if !defined($v);
 	    $v = $class->encode_value($type, $k, $v);
-	    $data .= &$format_config_line($propertyList->{$k}, $k, $v);
+	    my $prop = $class->get_property_schema($type, $k);
+	    $data .= &$format_config_line($prop, $k, $v);
 	}
 
 	$out .= "$data\n";
diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index b3af2c6..23ed069 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -36,6 +36,7 @@ no warnings 'portable'; # Support for 64-bit ints required
 our @EXPORT_OK = qw(
 $IPV6RE
 $IPV4RE
+compare_deeply
 lock_file
 lock_file_full
 run_command
@@ -2150,4 +2151,36 @@ sub get_file_hash {
     return lc($digest);
 }
 
+# compare two perl variables recursively, so this works for scalars, nested
+# hashes and nested arrays
+sub compare_deeply {
+    my ($a, $b) = @_;
+
+    return 0 if defined($a) != defined($b);
+    return 1 if !defined($a); # both are undef
+
+    my $ref_a = ref($a);
+    my $ref_b = ref($b);
+
+    # scalar case
+    return 0 if !$ref_a && !$ref_b && "$a" ne "$b";
+
+    # different types
+    return 0 if $ref_a ne $ref_b;
+
+    if ($ref_a eq 'HASH') {
+	return 0 if $a->%* != $b->%*;
+	for my $opt (keys $a->%*) {
+	    return 0 if !compare_deeply->($a->{$opt}, $b->{$opt});
+	}
+    } elsif ($ref_a eq 'ARRAY') {
+	return 0 if $a->@* != $b->@*;
+	for (my $i = 0; $i < $a->@*; $i++) {
+	    return 0 if !compare_deeply->($a->[$i], $b->[$i]);
+	}
+    }
+
+    return 1;
+}
+
 1;
-- 
2.30.2