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)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id C0157988F0 for ; Wed, 15 Nov 2023 16:46:48 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9B757946A for ; Wed, 15 Nov 2023 16:46:18 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 15 Nov 2023 16:46:17 +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 F16BC43290 for ; Wed, 15 Nov 2023 16:46:16 +0100 (CET) Message-ID: Date: Wed, 15 Nov 2023 16:46:14 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Content-Language: en-GB, de-AT To: Proxmox VE development discussion , Dominik Csapak References: <20231115135137.2265822-1-d.csapak@proxmox.com> <20231115135137.2265822-4-d.csapak@proxmox.com> From: Thomas Lamprecht Autocrypt: addr=t.lamprecht@proxmox.com; keydata= xsFNBFsLjcYBEACsaQP6uTtw/xHTUCKF4VD4/Wfg7gGn47+OfCKJQAD+Oyb3HSBkjclopC5J uXsB1vVOfqVYE6PO8FlD2L5nxgT3SWkc6Ka634G/yGDU3ZC3C/7NcDVKhSBI5E0ww4Qj8s9w OQRloemb5LOBkJNEUshkWRTHHOmk6QqFB/qBPW2COpAx6oyxVUvBCgm/1S0dAZ9gfkvpqFSD 90B5j3bL6i9FIv3YGUCgz6Ue3f7u+HsEAew6TMtlt90XV3vT4M2IOuECG/pXwTy7NtmHaBQ7 UJBcwSOpDEweNob50+9B4KbnVn1ydx+K6UnEcGDvUWBkREccvuExvupYYYQ5dIhRFf3fkS4+ wMlyAFh8PQUgauod+vqs45FJaSgTqIALSBsEHKEs6IoTXtnnpbhu3p6XBin4hunwoBFiyYt6 YHLAM1yLfCyX510DFzX/Ze2hLqatqzY5Wa7NIXqYYelz7tXiuCLHP84+sV6JtEkeSUCuOiUY virj6nT/nJK8m0BzdR6FgGtNxp7RVXFRz/+mwijJVLpFsyG1i0Hmv2zTn3h2nyGK/I6yhFNt dX69y5hbo6LAsRjLUvZeHXpTU4TrpN/WiCjJblbj5um5eEr4yhcwhVmG102puTtuCECsDucZ jpKpUqzXlpLbzG/dp9dXFH3MivvfuaHrg3MtjXY1i+/Oxyp5iwARAQABzTNUaG9tYXMgTGFt cHJlY2h0IChBdXRoLTQpIDx0LmxhbXByZWNodEBwcm94bW94LmNvbT7CwY4EEwEIADgWIQQO R4qbEl/pah9K6VrTZCM6gDZWBgUCWwuNxgIbAwULCQgHAgYVCAkKCwIEFgIDAQIeAQIXgAAK CRDTZCM6gDZWBm/jD/4+6JB2s67eaqoP6x9VGaXNGJPCscwzLuxDTCG90G9FYu29VcXtubH/ bPwsyBbNUQpqTm/s4XboU2qpS5ykCuTjqavrcP33tdkYfGcItj2xMipJ1i3TWvpikQVsX42R G64wovLs/dvpTYphRZkg5DwhgTmy3mRkmofFCTa+//MOcNOORltemp984tWjpR3bUJETNWpF sKGZHa3N4kCNxb7A+VMsJZ/1gN3jbQbQG7GkJtnHlWkw9rKCYqBtWrnrHa4UAvSa9M/XCIAB FThFGqZI1ojdVlv5gd6b/nWxfOPrLlSxbUo5FZ1i/ycj7/24nznW1V4ykG9iUld4uYUY86bB UGSjew1KYp9FmvKiwEoB+zxNnuEQfS7/Bj1X9nxizgweiHIyFsRqgogTvLh403QMSGNSoArk tqkorf1U+VhEncIn4H3KksJF0njZKfilrieOO7Vuot1xKr9QnYrZzJ7m7ZxJ/JfKGaRHXkE1 feMmrvZD1AtdUATZkoeQtTOpMu4r6IQRfSdwm/CkppZXfDe50DJxAMDWwfK2rr2bVkNg/yZI tKLBS0YgRTIynkvv0h8d9dIjiicw3RMeYXyqOnSWVva2r+tl+JBaenr8YTQw0zARrhC0mttu cIZGnVEvQuDwib57QLqMjQaC1gazKHvhA15H5MNxUhwm229UmdH3KM7BTQRbC43GARAAyTkR D6KRJ9Xa2fVMh+6f186q0M3ni+5tsaVhUiykxjsPgkuWXWW9MbLpYXkzX6h/RIEKlo2BGA95 QwG5+Ya2Bo3g7FGJHAkXY6loq7DgMp5/TVQ8phsSv3WxPTJLCBq6vNBamp5hda4cfXFUymsy HsJy4dtgkrPQ/bnsdFDCRUuhJHopnAzKHN8APXpKU6xV5e3GE4LwFsDhNHfH/m9+2yO/trcD txSFpyftbK2gaMERHgA8SKkzRhiwRTt9w5idOfpJVkYRsgvuSGZ0pcD4kLCOIFrer5xXudk6 NgJc36XkFRMnwqrL/bB4k6Pi2u5leyqcXSLyBgeHsZJxg6Lcr2LZ35+8RQGPOw9C0ItmRjtY ZpGKPlSxjxA1WHT2YlF9CEt3nx7c4C3thHHtqBra6BGPyW8rvtq4zRqZRLPmZ0kt/kiMPhTM 8wZAlObbATVrUMcZ/uNjRv2vU9O5aTAD9E5r1B0dlqKgxyoImUWB0JgpILADaT3VybDd3C8X s6Jt8MytUP+1cEWt9VKo4vY4Jh5vwrJUDLJvzpN+TsYCZPNVj18+jf9uGRaoK6W++DdMAr5l gQiwsNgf9372dbMI7pt2gnT5/YdG+ZHnIIlXC6OUonA1Ro/Itg90Q7iQySnKKkqqnWVc+qO9 GJbzcGykxD6EQtCSlurt3/5IXTA7t6sAEQEAAcLBdgQYAQgAIBYhBA5HipsSX+lqH0rpWtNk IzqANlYGBQJbC43GAhsMAAoJENNkIzqANlYGD1sP/ikKgHgcspEKqDED9gQrTBvipH85si0j /Jwu/tBtnYjLgKLh2cjv1JkgYYjb3DyZa1pLsIv6rGnPX9bH9IN03nqirC/Q1Y1lnbNTynPk IflgvsJjoTNZjgu1wUdQlBgL/JhUp1sIYID11jZphgzfDgp/E6ve/8xE2HMAnf4zAfJaKgD0 F+fL1DlcdYUditAiYEuN40Ns/abKs8I1MYx7Yglu3RzJfBzV4t86DAR+OvuF9v188WrFwXCS RSf4DmJ8tntyNej+DVGUnmKHupLQJO7uqCKB/1HLlMKc5G3GLoGqJliHjUHUAXNzinlpE2Vj C78pxpwxRNg2ilE3AhPoAXrY5qED5PLE9sLnmQ9AzRcMMJUXjTNEDxEYbF55SdGBHHOAcZtA kEQKub86e+GHA+Z8oXQSGeSGOkqHi7zfgW1UexddTvaRwE6AyZ6FxTApm8wq8NT2cryWPWTF BDSGB3ujWHMM8ERRYJPcBSjTvt0GcEqnd+OSGgxTkGOdufn51oz82zfpVo1t+J/FNz6MRMcg 8nEC+uKvgzH1nujxJ5pRCBOquFZaGn/p71Yr0oVitkttLKblFsqwa+10Lt6HBxm+2+VLp4Ja 0WZNncZciz3V3cuArpan/ZhhyiWYV5FD0pOXPCJIx7WS9PTtxiv0AOS4ScWEUmBxyhFeOpYa DrEx In-Reply-To: <20231115135137.2265822-4-d.csapak@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL -0.066 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. [tools.pm, sectionconfig.pm] Subject: Re: [pve-devel] [PATCH common v2 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 15:46:48 -0000 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). >=20 > with that, we change how we work with the options hash: >=20 > 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 =3D> 1' (since that d= oes > not exist in the schema) >=20 > Signed-off-by: Dominik Csapak > --- > 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 >=20 > src/PVE/SectionConfig.pm | 259 ++++++++++++++++++++++++++++++---------= > src/PVE/Tools.pm | 33 +++++ > 2 files changed, 235 insertions(+), 57 deletions(-) >=20 > 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; > =20 > use PVE::Exception qw(raise_param_exc); > use PVE::JSONSchema qw(get_standard_option); > +use PVE::Tools qw(compare_deeply); > =20 > my $defaultData =3D { > options =3D> {}, > @@ -51,6 +52,61 @@ sub plugindata { > return {}; > } > =20 > +sub is_separated { > + my ($class) =3D @_; > + > + 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 directi= on 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 sch= emas 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=E2=84=A2 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 loo= k 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 =3D $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) =3D @_; > + > + my $merged =3D {$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) =3D @_; > + > + if (!defined($props->{$key})) { > + $props->{$key} =3D $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 =3D delete $props->{$key}; > + delete $new_prop->{'type-property'}; > + delete $prop->{'type-property'}; > + $props->{$key} =3D { > + 'type-property' =3D> 'type', > + oneOf =3D> [ > + $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) =3D @_; > =20 > @@ -60,42 +116,61 @@ sub createSchema { > =20 > my $props =3D $base || {}; > =20 > - my $copy_property =3D sub { > - my ($src) =3D @_; > - > - my $res =3D {}; > - foreach my $k (keys %$src) { > - $res->{$k} =3D $src->{$k}; > - } > + if (!$class->is_separated()) { > + foreach my $p (keys %$propertyList) { > + next if $skip_type && $p eq 'type'; > =20 > - return $res; > - }; > + if (!$propertyList->{$p}->{optional}) { > + $props->{$p} =3D $propertyList->{$p}; > + next; > + } > =20 > - foreach my $p (keys %$propertyList) { > - next if $skip_type && $p eq 'type'; > + my $required =3D 1; > =20 > - if (!$propertyList->{$p}->{optional}) { > - $props->{$p} =3D $propertyList->{$p}; > - next; > - } > + my $copts =3D $class->options(); > + $required =3D 0 if defined($copts->{$p}) && $copts->{$p}->{option= al}; > =20 > - my $required =3D 1; > - > - my $copts =3D $class->options(); > - $required =3D 0 if defined($copts->{$p}) && $copts->{$p}->{optional};= > + foreach my $t (keys %$plugins) { > + my $opts =3D $pdata->{options}->{$t} || {}; > + $required =3D 0 if !defined($opts->{$p}) || $opts->{$p}->{optional};= > + } > =20 > - foreach my $t (keys %$plugins) { > - my $opts =3D $pdata->{options}->{$t} || {}; > - $required =3D 0 if !defined($opts->{$p}) || $opts->{$p}->{optiona= l}; > + if ($required) { > + # make a copy, because we modify the optional property > + my $res =3D {$propertyList->{$p}->%*}; # shallow copy > + $res->{optional} =3D 0; > + $props->{$p} =3D $res; > + } else { > + $props->{$p} =3D $propertyList->{$p}; > + } > } > - > - if ($required) { > - # make a copy, because we modify the optional property > - my $res =3D &$copy_property($propertyList->{$p}); > - $res->{optional} =3D 0; > - $props->{$p} =3D $res; > - } else { > - $props->{$p} =3D $propertyList->{$p}; > + } else { > + for my $type (sort keys %$plugins) { > + my $opts =3D $pdata->{options}->{$type} || {}; > + for my $key (sort keys $opts->%*) { > + my $schema =3D $class->get_property_schema($type, $key); > + my $prop =3D {$schema->%*}; > + $prop->{'instance-types'} =3D [$type]; > + $prop->{'type-property'} =3D 'type'; > + $prop->{optional} =3D 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} =3D {$propertyList->{$opt}->%*}; > + } > + for my $opt (keys $props->%*) { > + if (my $necessaryTypes =3D $props->{$opt}->{'instance-types'}) { > + if ($necessaryTypes->@* =3D=3D scalar(keys $plugins->%*)) { > + delete $props->{$opt}->{'instance-types'}; > + delete $props->{$opt}->{'type-property'}; > + } else { > + $props->{$opt}->{optional} =3D 1; > + } > + } > } > } > =20 > @@ -117,30 +192,61 @@ sub updateSchema { > =20 > my $filter_type =3D $single_class ? $class->type() : undef; > =20 > - foreach my $p (keys %$propertyList) { > - next if $p eq 'type'; > + if (!$class->is_separated()) { > + foreach my $p (keys %$propertyList) { > + next if $p eq 'type'; > =20 > - my $copts =3D $class->options(); > + my $copts =3D $class->options(); > =20 > - next if defined($filter_type) && !defined($copts->{$p}); > + next if defined($filter_type) && !defined($copts->{$p}); > =20 > - if (!$propertyList->{$p}->{optional}) { > - $props->{$p} =3D $propertyList->{$p}; > - next; > - } > + if (!$propertyList->{$p}->{optional}) { > + $props->{$p} =3D $propertyList->{$p}; > + next; > + } > =20 > - my $modifyable =3D 0; > + my $modifyable =3D 0; > =20 > - $modifyable =3D 1 if defined($copts->{$p}) && !$copts->{$p}->{fixed};= > + $modifyable =3D 1 if defined($copts->{$p}) && !$copts->{$p}->{fix= ed}; > =20 > - foreach my $t (keys %$plugins) { > - my $opts =3D $pdata->{options}->{$t} || {}; > - next if !defined($opts->{$p}); > - $modifyable =3D 1 if !$opts->{$p}->{fixed}; > + foreach my $t (keys %$plugins) { > + my $opts =3D $pdata->{options}->{$t} || {}; > + next if !defined($opts->{$p}); > + $modifyable =3D 1 if !$opts->{$p}->{fixed}; > + } > + next if !$modifyable; > + > + $props->{$p} =3D $propertyList->{$p}; > + } > + } else { > + for my $type (sort keys %$plugins) { > + my $opts =3D $pdata->{options}->{$type} || {}; > + for my $key (sort keys $opts->%*) { > + next if $opts->{$key}->{fixed}; > + > + my $schema =3D $class->get_property_schema($type, $key); > + my $prop =3D {$schema->%*}; > + $prop->{'instance-types'} =3D [$type]; > + $prop->{'type-property'} =3D 'type'; > + $prop->{optional} =3D 1; > + > + add_property($props, $key, $prop, $type); > + } > } > - next if !$modifyable; > =20 > - $props->{$p} =3D $propertyList->{$p}; > + for my $opt (keys $propertyList->%*) { > + next if $props->{$opt}; > + $props->{$opt} =3D {$propertyList->{$opt}->%*}; > + } > + > + for my $opt (keys $props->%*) { > + if (my $necessaryTypes =3D $props->{$opt}->{'instance-types'}) { > + if ($necessaryTypes->@* =3D=3D scalar(keys $plugins->%*)) { > + delete $props->{$opt}->{'instance-types'}; > + delete $props->{$opt}->{'type-property'}; > + } > + } > + } > } > =20 > $props->{digest} =3D get_standard_option('pve-config-digest'); > @@ -160,22 +266,28 @@ sub updateSchema { > } > =20 > sub init { > - my ($class) =3D @_; > + my ($class, $separate_properties) =3D @_; > =20 > my $pdata =3D $class->private(); > =20 > - foreach my $k (qw(options plugins plugindata propertyList)) { > + foreach my $k (qw(options plugins plugindata propertyList property= ListSeparated)) { > $pdata->{$k} =3D {} if !$pdata->{$k}; > } > =20 > my $plugins =3D $pdata->{plugins}; > my $propertyList =3D $pdata->{propertyList}; > + my $propertyListSeparated =3D $pdata->{propertyListSeparated}; > =20 > foreach my $type (keys %$plugins) { > my $props =3D $plugins->{$type}->properties(); > foreach my $p (keys %$props) { > - die "duplicate property '$p'" if defined($propertyList->{$p}); > - my $res =3D $propertyList->{$p} =3D {}; > + my $res; > + if ($separate_properties) { > + $res =3D $propertyListSeparated->{$type}->{$p} =3D {}; > + } else { > + die "duplicate property '$p'" if defined($propertyList->{$p}); > + $res =3D $propertyList->{$p} =3D {}; > + } > my $data =3D $props->{$p}; > for my $a (keys %$data) { > $res->{$a} =3D $data->{$a}; > @@ -187,8 +299,23 @@ sub init { > foreach my $type (keys %$plugins) { > my $opts =3D $plugins->{$type}->options(); > foreach my $p (keys %$opts) { > - die "undefined property '$p'" if !$propertyList->{$p}; > + my $prop; > + if ($separate_properties) { > + $prop =3D $propertyListSeparated->{$type}->{$p}; > + } > + $prop //=3D $propertyList->{$p}; > + die "undefined property '$p'" if !$prop; > } > + > + # automatically the properties to options (if not specified explicite= ly) > + if ($separate_properties) { > + foreach my $p (keys $propertyListSeparated->{$type}->%*) { > + next if $opts->{$p}; > + $opts->{$p} =3D {}; > + $opts->{$p}->{optional} =3D 1 if $propertyListSeparated->{$type}->{$= p}->{optional}; > + } > + } > + > $pdata->{options}->{$type} =3D $opts; > } > =20 > @@ -237,11 +364,12 @@ sub check_value { > return $value if $key eq 'type' && $type eq $value; > =20 > my $opts =3D $pdata->{options}->{$type}; > + unrelated white space change > die "unknown section type '$type'\n" if !$opts; > =20 > die "unexpected property '$key'\n" if !defined($opts->{$key}); > =20 > - my $schema =3D $pdata->{propertyList}->{$key}; > + my $schema =3D $class->get_property_schema($type, $key); > die "unknown property type\n" if !$schema; > =20 > my $ct =3D $schema->{type}; > @@ -295,6 +423,20 @@ sub format_section_header { > return "$type: $sectionId\n"; > } > =20 > +sub get_property_schema { > + my ($class, $type, $key) =3D @_; > + > + my $pdata =3D $class->private(); > + my $opts =3D $pdata->{options}->{$type}; > + > + my $schema; > + if ($class->is_separated()) { > + $schema =3D $pdata->{propertyListSeparated}->{$type}->{$key}; > + } > + $schema //=3D $pdata->{propertyList}->{$key}; > + > + return $schema; > +} > =20 > sub parse_config { > my ($class, $filename, $raw, $allow_unknown) =3D @_; > @@ -322,7 +464,7 @@ sub parse_config { > my $is_array =3D sub { > my ($type, $key) =3D @_; > =20 > - my $schema =3D $pdata->{propertyList}->{$key}; > + my $schema =3D $class->get_property_schema($type, $key); > die "unknown property type\n" if !$schema; > =20 > return $schema->{type} eq 'array'; > @@ -494,7 +636,6 @@ sub write_config { > my ($class, $filename, $cfg, $allow_unknown) =3D @_; > =20 > my $pdata =3D $class->private(); > - my $propertyList =3D $pdata->{propertyList}; > =20 > my $out =3D ''; > =20 > @@ -516,6 +657,7 @@ sub write_config { > my $scfg =3D $ids->{$sectionId}; > my $type =3D $scfg->{type}; > my $opts =3D $pdata->{options}->{$type}; > + my $global_opts =3D $pdata->{options}->{__global}; > =20 > die "unknown section type '$type'\n" if !$opts && !$allow_unknown; > =20 > @@ -545,7 +687,8 @@ sub write_config { > if ($scfg->{comment} && !$done_hash->{comment}) { > my $k =3D 'comment'; > my $v =3D $class->encode_value($type, $k, $scfg->{$k}); > - $data .=3D &$format_config_line($propertyList->{$k}, $k, $v); > + my $prop =3D $class->get_property_schema($type, $k); > + $data .=3D &$format_config_line($prop, $k, $v); > } > =20 > $data .=3D "\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 =3D $class->encode_value($type, $k, $v); > - $data .=3D &$format_config_line($propertyList->{$k}, $k, $v); > + my $prop =3D $class->get_property_schema($type, $k); > + $data .=3D &$format_config_line($prop, $k, $v); > } > =20 > foreach my $k (@option_keys) { > @@ -570,7 +714,8 @@ sub write_config { > my $v =3D $scfg->{$k}; > next if !defined($v); > $v =3D $class->encode_value($type, $k, $v); > - $data .=3D &$format_config_line($propertyList->{$k}, $k, $v); > + my $prop =3D $class->get_property_schema($type, $k); > + $data .=3D &$format_config_line($prop, $k, $v); > } > =20 > $out .=3D "$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 req= uired > our @EXPORT_OK =3D 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); > } > =20 > +# compare two perl variables recursively, so this works for scalars, n= ested > +# 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 thro= wn in. > + my ($a, $b) =3D @_; > + > + return 0 if defined($a) !=3D defined($b); > + return 1 if !defined($a); # both are undef > + > + my $ref_a =3D ref($a); > + my $ref_b =3D ref($b); nit: could be=20 my ($ref_a, $ref_b) =3D (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=20 return 0 if defined($ref_a) !=3D defined($ref_b); directly after assigning those variables? > + > + if ($ref_a eq 'HASH') { > + return 0 if $a->%* !=3D $b->%*; nit: we often use `keys %$a)` for getting the size, as that makes it a bi= t 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->@* !=3D $b->@*; same nit here w.r.t. (not) using scalar > + for (my $i =3D 0; $i < $a->@*; $i++) { > + return 0 if !compare_deeply->($a->[$i], $b->[$i]); same here w.r.t method call style > + } > + } > + > + return 1; > +} > + > 1;