[pve-devel] [PATCH common v2 3/4] section config: allow separated property lists for plugins
Thomas Lamprecht
t.lamprecht at proxmox.com
Wed Nov 15 16:46:14 CET 2023
Am 15/11/2023 um 14:51 schrieb Dominik Csapak:
> 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
"it's no longer necessary [...]" would be a bit easier to read.
> type specific propertyList, except if it's 'fixed => 1' (since that does
> not exist in the schema)
>
> Signed-off-by: Dominik Csapak <d.csapak at 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;
the propertyListSeparated name is not telling me much about what this is for.
This is for property isolation? Maybe going with the term in that direction
would be better.
"private" is also often used for such things, but we already use that for
another thing IIRC or "shared" vs "exclusive", e.g., shared_schema.
Adding a comment at the top of the file describing describing how the schemas
are (or are not) merged from all plugin-types would help others to easier
understand this too I think.
But from top of my head I have no Real Good™ proposal for what to use, but I do
not really like the term "separated" here and just think that it might be a bit
to hard to understand what it means.
That naming is actually my biggest gripe with the patch (but I didn't look that
closely), most other things are (stylistic) nits that I'd not block this on.
btw. can we be a bit more explicit here:
my $place_holder_name = $class->private()->{placeHolderName};
return defined($place_holder_name) && scalar(keys $place_holder_name->%*) > 0;
> +}
> +
> +my sub cmp_property {
s/cmp/compare/ but not sure if that name fits that well either with the
$skip_opts stuff, but it's small enough to still be fine.
> + my ($a, $b, $skip_opts) = @_;
> +
> + my $merged = {$a->%*, $b->%*};
> + for my $opt ($skip_opts->@*) {
> + delete $merged->{$opt};
> + }
no hard feelings, but could be:
delete $merged->{$_} for $skip_opts->@*;
> + 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};
> +
unrelated white space change
> 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
do we really need to allow export for a single use-case?
> 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 {
could mirror the one from Test::More and name it is_deeply
Also, could be added in it's own commit with a handful of unit-tests thrown in.
> + 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);
nit: could be
my ($ref_a, $ref_b) = (ref($a), ref($b));
Less for the single line saved, but it would IMO fit this method (but
just a tiny subjective nit)
> +
> + # scalar case
> + return 0 if !$ref_a && !$ref_b && "$a" ne "$b";
> +
> + # different types
> + return 0 if $ref_a ne $ref_b;
this can result in a undef warning, as, e.g., $ref_a equaling 'HASH' and $ref_b
equaling undef is not caught by the check before this.
Maybe add a
return 0 if defined($ref_a) != defined($ref_b);
directly after assigning those variables?
> +
> + if ($ref_a eq 'HASH') {
> + return 0 if $a->%* != $b->%*;
nit: we often use `keys %$a)` for getting the size, as that makes it a bit more explicit
> + for my $opt (keys $a->%*) {
> + return 0 if !compare_deeply->($a->{$opt}, $b->{$opt});
why the -> call style for the method, maybe this was a code ref inside a variable once?
> + }
> + } elsif ($ref_a eq 'ARRAY') {
> + return 0 if $a->@* != $b->@*;
same nit here w.r.t. (not) using scalar
> + for (my $i = 0; $i < $a->@*; $i++) {
> + return 0 if !compare_deeply->($a->[$i], $b->[$i]);
same here w.r.t method call style
> + }
> + }
> +
> + return 1;
> +}
> +
> 1;
More information about the pve-devel
mailing list