From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 7184B984D1 for ; Wed, 15 Nov 2023 10:36:16 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 56B61290F for ; Wed, 15 Nov 2023 10:36:16 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 15 Nov 2023 10:36:15 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id D332B42F22 for ; Wed, 15 Nov 2023 10:36:14 +0100 (CET) Date: Wed, 15 Nov 2023 10:36:12 +0100 From: Wolfgang Bumiller To: Dominik Csapak Cc: pve-devel@lists.proxmox.com Message-ID: References: <20231114103340.2850162-1-d.csapak@proxmox.com> <20231114103340.2850162-4-d.csapak@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20231114103340.2850162-4-d.csapak@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.102 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [sectionconfig.pm] Subject: Re: [pve-devel] [PATCH common 3/4] section config: allow separated property lists for plugins X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 15 Nov 2023 09:36:16 -0000 On Tue, Nov 14, 2023 at 11:33:38AM +0100, Dominik Csapak wrote: > 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). > > for that to work there are a few changes in how to use the 'options' hash: > * when wanting to use a property that is not defined in either the local > list or the global one, the 'type' property has to be given to specify > which plugin type this should be taken from > * if it's desired to always have some global properties in the schema, > they have to be given in the 'global' options section now (the perl > package where the base plugin is defined) > > for now, we then save the global options in the section '__global', > which is now a forbidden plugin type, but that should be fine as we > don't use that > > Signed-off-by: Dominik Csapak > --- > src/PVE/SectionConfig.pm | 256 ++++++++++++++++++++++++++++++--------- > 1 file changed, 198 insertions(+), 58 deletions(-) > > diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm > index 5690476..1e44c3d 100644 > --- a/src/PVE/SectionConfig.pm > +++ b/src/PVE/SectionConfig.pm > @@ -30,6 +30,9 @@ sub register { > die "duplicate plugin registration (type = $type)" > if defined($pdata->{plugins}->{$type}); > > + die "invalid plugin type (type = '__global')" > + if $type eq '__global'; > + > my $plugindata = $class->plugindata(); > $pdata->{plugindata}->{$type} = $plugindata; > $pdata->{plugins}->{$type} = $class; > @@ -51,6 +54,67 @@ sub plugindata { > return {}; > } > > +my sub cmp_property { > + my ($src, $tgt, $skip_opts) = @_; "compare" vs "source/target", I'd argue 'a' and 'b' would be a better choice for the parameter names. > + > + my $merged = {$src->%*, $tgt->%*}; > + for my $opt ($skip_opts->@*) { > + delete $merged->{$opt}; > + } > + for my $opt (keys $merged->%*) { > + return 0 if !defined($src->{$opt}) || !defined($tgt->{$opt}) || $tgt->{$opt} ne $src->{$opt}; ^ might be worth checking that they aren't `ref()`s, as you don't want to `ne` a potential hash here (eg. an `items` in an array schema?) I'm not sure we have such types in the section config currently, but IIRC you wanted to add that at some point? But apart from array/object types we also have `requires => [array]` - that one in particular should be compared recursively if we use this to optimize the `instance-types` arrays. > + } > + > + return 1; > +} > + > +my sub copy_property { ^ This thing again. 😒 At the very least make its body just: return { $_[0]->%* }; Or just kill it. > + my ($src) = @_; > + > + my $res = {}; > + foreach my $k (keys %$src) { > + $res->{$k} = $src->{$k}; > + } > + > + return $res; > +} > + > + > +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) = @_; > > @@ -58,44 +122,55 @@ sub createSchema { > my $propertyList = $pdata->{propertyList}; > my $plugins = $pdata->{plugins}; > > - my $props = $base || {}; > - > - my $copy_property = sub { > - my ($src) = @_; > + my $global_options = $pdata->{options}->{__global}; > + my $propertyListSeparated = $pdata->{propertyListSeparated}; > > - my $res = {}; > - foreach my $k (keys %$src) { > - $res->{$k} = $src->{$k}; > - } > + my $props = $base || {}; > > - return $res; > - }; > + if (!defined($global_options)) { > + foreach my $p (keys %$propertyList) { > + next if $skip_type && $p eq 'type'; > > - foreach my $p (keys %$propertyList) { > - next if $skip_type && $p eq 'type'; > + if (!$propertyList->{$p}->{optional}) { > + $props->{$p} = $propertyList->{$p}; > + next; > + } > > - if (!$propertyList->{$p}->{optional}) { > - $props->{$p} = $propertyList->{$p}; > - next; > - } > + my $required = 1; > > - my $required = 1; > + my $copts = $class->options(); > + $required = 0 if defined($copts->{$p}) && $copts->{$p}->{optional}; > > - 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 = copy_property($propertyList->{$p}); > + $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 = copy_property($schema); > + $prop->{'instance-types'} = [$type]; > + $prop->{'type-property'} = 'type'; > + $prop->{optional} = 1 if $opts->{$key}->{optional}; > + > + add_property($props, $key, $prop, $type); > + } > + } > + for my $opt (keys $global_options->%*) { > + $props->{$opt} = copy_property($propertyList->{$opt}); > + $props->{$opt}->{optional} = 1 if $global_options->{$opt}->{optional}; > } > } > > @@ -113,34 +188,61 @@ sub updateSchema { > my $propertyList = $pdata->{propertyList}; > my $plugins = $pdata->{plugins}; > > + my $global_options = $pdata->{options}->{__global}; > + my $propertyListSeparated = $pdata->{propertyListSeparated}; > + > my $props = $base || {}; > > my $filter_type = $single_class ? $class->type() : undef; > > - foreach my $p (keys %$propertyList) { > - next if $p eq 'type'; > + if (!defined($global_options)) { > + 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}; > + } > + next if !$modifyable; > > - foreach my $t (keys %$plugins) { > - my $opts = $pdata->{options}->{$t} || {}; > - next if !defined($opts->{$p}); > - $modifyable = 1 if !$opts->{$p}->{fixed}; > + $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 = copy_property($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 $global_options->%*) { > + next if $global_options->{$opt}->{fixed}; > + > + $props->{$opt} = copy_property($propertyList->{$opt}); > + $props->{$opt}->{optional} = 1 if $global_options->{$opt}->{optional}; > + } > } > > $props->{digest} = get_standard_option('pve-config-digest'); > @@ -160,22 +262,33 @@ 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}; > } > > + if ($separate_properties) { > + $pdata->{options}->{__global} = delete $pdata->{options}; > + } > + > 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) { > + die "duplicate property '$p'" if defined($propertyListSeparated->{$type}->{$p}); ^ Can this even happen? $props is a hash per $type after all, and we're now namespaced by $type Should we instead check it against the global options? > + $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,7 +300,13 @@ 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) { > + my $opt_type = $opts->{$p}->{type} // $type; ^ Similarly, we're coming from a fixed $type and query the `options` from that very type, does it make sense for there to be a `type` property present? > + $prop = $propertyListSeparated->{$opt_type}->{$p}; > + } > + $prop //= $propertyList->{$p}; > + die "undefined property '$p'" if !$prop; > } > $pdata->{options}->{$type} = $opts; > } > @@ -237,11 +356,13 @@ sub check_value { > return $value if $key eq 'type' && $type eq $value; > > my $opts = $pdata->{options}->{$type}; > + my $global_opts = $pdata->{options}->{__global}; > + > die "unknown section type '$type'\n" if !$opts; > > - die "unexpected property '$key'\n" if !defined($opts->{$key}); > + die "unexpected property '$key'\n" if !defined($opts->{$key}) && !defined($global_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 +416,23 @@ 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 $separated = defined($pdata->{options}->{__global}); ^ would it make sense to have this condition as a `$class->is_separated()` sub? > + > + my $schema; > + if ($separated) { > + my $opt_type = $opts->{$key}->{type} // $type; > + $schema = $pdata->{propertyListSeparated}->{$opt_type}->{$key}; > + } > + $schema //= $pdata->{propertyList}->{$key}; > + > + return $schema; > +} > > sub parse_config { > my ($class, $filename, $raw, $allow_unknown) = @_; > @@ -322,7 +460,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 +632,6 @@ sub write_config { > my ($class, $filename, $cfg, $allow_unknown) = @_; > > my $pdata = $class->private(); > - my $propertyList = $pdata->{propertyList}; > > my $out = ''; > > @@ -545,7 +682,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 +700,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 +709,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"; > -- > 2.30.2