[pve-devel] [PATCH common 3/4] section config: allow separated property lists for plugins
Wolfgang Bumiller
w.bumiller at proxmox.com
Wed Nov 15 10:36:12 CET 2023
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 <d.csapak at proxmox.com>
> ---
> 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
More information about the pve-devel
mailing list