[RFC PATCH common] section config: correctly handle required options in write_config

[RFC PATCH common] section config: correctly handle required options in write_config

[Daniel Kral]

parse_config(...) warns about and skips sections, which do not specify
all required properties according to their schema, if $allow_unknown is
not set.

Meanwhile, write_config(...) will not write the config line for some
required options, as format_config_line(...) does return an empty string
for empty array properties or other empty non-boolean properties.

To ensure that required options are always written to the config to not
be skipped by parse_config(...), check whether format_config_line(...)
produces any output.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
This came up while fixing [0] as I noticed that at least for required
options with the '*-list' format, the parameter can still be an empty
string / list. AFAICT this is because we check for definedness of the
property value only.

Even though I considered changing this in PVE::JSONSchema first, it
could break existing code where this behavior is currently expected.
Essentially, the check would need to be specified for types, e.g., to
ensure that setting '0' for booleans, integer and numbers is still
interpreted as a set required property.

The behavior is much more apparent for section configs, where
parse_config(...) expects these properties to be written in the config,
while write_config(...) will drop these for empty strings and will
result in warn log messages for any subsequent reads with $allow_unknown
set. Therefore I propose the change only for section config for now.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=7399

 src/PVE/SectionConfig.pm | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index 84ff81a..d0332f9 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -1572,11 +1572,12 @@ sub write_config {
             next if $opts->{$k}->{optional};
             $done_hash->{$k} = 1;
             my $v = $scfg->{$k};
-            die "section '$sectionId' - missing value for required option '$k'\n"
-                if !defined($v);
             $v = $class->encode_value($type, $k, $v);
             my $prop = $class->get_property_schema($type, $k);
-            $data .= format_config_line($prop, $k, $v);
+            my $cfg_line = format_config_line($prop, $k, $v);
+            die "section '$sectionId' - missing value for required option '$k'\n"
+                if !$cfg_line;
+            $data .= $cfg_line;
         }
 
         for my $k (@option_keys) {
-- 
2.47.3

Re: [RFC PATCH common] section config: correctly handle required options in write_config

[Max R. Carrara]

On Wed Mar 11, 2026 at 1:26 PM CET, Daniel Kral wrote:
> parse_config(...) warns about and skips sections, which do not specify
> all required properties according to their schema, if $allow_unknown is
> not set.
>
> Meanwhile, write_config(...) will not write the config line for some
> required options, as format_config_line(...) does return an empty string
> for empty array properties or other empty non-boolean properties.
>
> To ensure that required options are always written to the config to not
> be skipped by parse_config(...), check whether format_config_line(...)
> produces any output.
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
> This came up while fixing [0] as I noticed that at least for required
> options with the '*-list' format, the parameter can still be an empty
> string / list. AFAICT this is because we check for definedness of the
> property value only.
>
> Even though I considered changing this in PVE::JSONSchema first, it
> could break existing code where this behavior is currently expected.
> Essentially, the check would need to be specified for types, e.g., to
> ensure that setting '0' for booleans, integer and numbers is still
> interpreted as a set required property.
>
> The behavior is much more apparent for section configs, where
> parse_config(...) expects these properties to be written in the config,
> while write_config(...) will drop these for empty strings and will
> result in warn log messages for any subsequent reads with $allow_unknown
> set. Therefore I propose the change only for section config for now.
>
> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=7399

Just had a look at your patch—nice that you spotted this!

I think this fix here should be fine, but I think it would be nice if
you added a test: Replicate the broken behavior first, and then fix it
together with this patch here. So, basically a little bit of TDD.

I'm not a big fan of TDD myself, but I think in this specific case it
would be beneficial for us to have a test like that—it's also
documentation at the same time.

Perhaps a nice spot for that would be in the SectionConfig tests
directory [1] as a separate script or something—see the other scripts
for how the tests in there work. (ofc you can holler at me too!)

Regarding JSONSchema: I think the definedness check itself is fine—an
empty array is still "something" rather than nothing, so ...

We could instead implement some of the other validation keywords [2] for
arrays, like e.g. `minItems`. I think an array that's `required` and
also has `minItems` set to `1` would allow you to express what you
initially wanted where you fixed #7399 [0], right?

I just hope that this is possible in your current JSONSchema—Wolfgang
and I have spoken about some things related to this a (longer) while
ago; I believe it was about empty strings / zeroes as well. *That* might
be a bit more complicated to solve, though and might even require a
JSONSchema v2 of some sorts, but I don't have all the details in my head
at the moment.

[1]: https://git.proxmox.com/?p=pve-common.git;a=tree;f=test/SectionConfig;h=0967fb1eebf84b4ecad696901572fa7550d49941;hb=refs/heads/master
[2]: https://json-schema.org/draft/2020-12/json-schema-validation#name-validation-keywords-for-arr

>
>  src/PVE/SectionConfig.pm | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
> index 84ff81a..d0332f9 100644
> --- a/src/PVE/SectionConfig.pm
> +++ b/src/PVE/SectionConfig.pm
> @@ -1572,11 +1572,12 @@ sub write_config {
>              next if $opts->{$k}->{optional};
>              $done_hash->{$k} = 1;
>              my $v = $scfg->{$k};
> -            die "section '$sectionId' - missing value for required option '$k'\n"
> -                if !defined($v);
>              $v = $class->encode_value($type, $k, $v);
>              my $prop = $class->get_property_schema($type, $k);
> -            $data .= format_config_line($prop, $k, $v);
> +            my $cfg_line = format_config_line($prop, $k, $v);
> +            die "section '$sectionId' - missing value for required option '$k'\n"
> +                if !$cfg_line;
> +            $data .= $cfg_line;
>          }
>
>          for my $k (@option_keys) {

Re: [RFC PATCH common] section config: correctly handle required options in write_config

[Daniel Kral]

On Thu Mar 12, 2026 at 10:47 AM CET, Max R. Carrara wrote:
> Just had a look at your patch—nice that you spotted this!
>
> I think this fix here should be fine, but I think it would be nice if
> you added a test: Replicate the broken behavior first, and then fix it
> together with this patch here. So, basically a little bit of TDD.
>
> I'm not a big fan of TDD myself, but I think in this specific case it
> would be beneficial for us to have a test like that—it's also
> documentation at the same time.
>
> Perhaps a nice spot for that would be in the SectionConfig tests
> directory [1] as a separate script or something—see the other scripts
> for how the tests in there work. (ofc you can holler at me too!)

Thanks for the quick feedback!

Yeah, but for something integral as the section config adding some test
cases documenting the changes would be great, I'll do that for the v2!

>
> Regarding JSONSchema: I think the definedness check itself is fine—an
> empty array is still "something" rather than nothing, so ...
>
> We could instead implement some of the other validation keywords [2] for
> arrays, like e.g. `minItems`. I think an array that's `required` and
> also has `minItems` set to `1` would allow you to express what you
> initially wanted where you fixed #7399 [0], right?
>
> I just hope that this is possible in your current JSONSchema—Wolfgang
> and I have spoken about some things related to this a (longer) while
> ago; I believe it was about empty strings / zeroes as well. *That* might
> be a bit more complicated to solve, though and might even require a
> JSONSchema v2 of some sorts, but I don't have all the details in my head
> at the moment.

Thanks for the insights, I haven't looked too closely at the json schema
draft yet!

You're right, even though it seems that the draft doesn't specify any
behavior regarding our `optional` property, my proposed change in our
json schema code would break assumptions around our code base and with
all the `min*` properties in mind it indeed would be inconsistent if
those `min*` properties default to `0`. So let's discard that idea.

>
> [1]: https://git.proxmox.com/?p=pve-common.git;a=tree;f=test/SectionConfig;h=0967fb1eebf84b4ecad696901572fa7550d49941;hb=refs/heads/master
> [2]: https://json-schema.org/draft/2020-12/json-schema-validation#name-validation-keywords-for-arr

Re: [RFC PATCH common] section config: correctly handle required options in write_config

[Wolfgang Bumiller]

On Thu, Mar 12, 2026 at 01:19:53PM +0100, Daniel Kral wrote:
> On Thu Mar 12, 2026 at 10:47 AM CET, Max R. Carrara wrote:
> > Just had a look at your patch—nice that you spotted this!
> >
> > I think this fix here should be fine, but I think it would be nice if
> > you added a test: Replicate the broken behavior first, and then fix it
> > together with this patch here. So, basically a little bit of TDD.
> >
> > I'm not a big fan of TDD myself, but I think in this specific case it
> > would be beneficial for us to have a test like that—it's also
> > documentation at the same time.
> >
> > Perhaps a nice spot for that would be in the SectionConfig tests
> > directory [1] as a separate script or something—see the other scripts
> > for how the tests in there work. (ofc you can holler at me too!)
> 
> Thanks for the quick feedback!
> 
> Yeah, but for something integral as the section config adding some test
> cases documenting the changes would be great, I'll do that for the v2!
> 
> >
> > Regarding JSONSchema: I think the definedness check itself is fine—an
> > empty array is still "something" rather than nothing, so ...
> >
> > We could instead implement some of the other validation keywords [2] for
> > arrays, like e.g. `minItems`. I think an array that's `required` and
> > also has `minItems` set to `1` would allow you to express what you
> > initially wanted where you fixed #7399 [0], right?
> >
> > I just hope that this is possible in your current JSONSchema—Wolfgang
> > and I have spoken about some things related to this a (longer) while
> > ago; I believe it was about empty strings / zeroes as well. *That* might
> > be a bit more complicated to solve, though and might even require a
> > JSONSchema v2 of some sorts, but I don't have all the details in my head
> > at the moment.
> 
> Thanks for the insights, I haven't looked too closely at the json schema
> draft yet!
> 
> You're right, even though it seems that the draft doesn't specify any
> behavior regarding our `optional` property, my proposed change in our

Our optional property is just an "inlined" version of *not* putting
something in the enclosing object's `required` array. (IIRC we discussed
the possibility of adding that and slowly moving over to using that as
it would also make the perl<->rust conversion/interop easier in the long
run.

> json schema code would break assumptions around our code base and with
> all the `min*` properties in mind it indeed would be inconsistent if
> those `min*` properties default to `0`. So let's discard that idea.
> 
> >
> > [1]: https://git.proxmox.com/?p=pve-common.git;a=tree;f=test/SectionConfig;h=0967fb1eebf84b4ecad696901572fa7550d49941;hb=refs/heads/master
> > [2]: https://json-schema.org/draft/2020-12/json-schema-validation#name-validation-keywords-for-arr

Re: [RFC PATCH common] section config: correctly handle required options in write_config

[Wolfgang Bumiller]

On Wed, Mar 11, 2026 at 01:26:18PM +0100, Daniel Kral wrote:
> parse_config(...) warns about and skips sections, which do not specify
> all required properties according to their schema, if $allow_unknown is
> not set.
> 
> Meanwhile, write_config(...) will not write the config line for some
> required options, as format_config_line(...) does return an empty string
> for empty array properties or other empty non-boolean properties.
> 
> To ensure that required options are always written to the config to not
> be skipped by parse_config(...), check whether format_config_line(...)
> produces any output.
> 
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
> This came up while fixing [0] as I noticed that at least for required
> options with the '*-list' format, the parameter can still be an empty
> string / list. AFAICT this is because we check for definedness of the
> property value only.
> 
> Even though I considered changing this in PVE::JSONSchema first, it
> could break existing code where this behavior is currently expected.
> Essentially, the check would need to be specified for types, e.g., to
> ensure that setting '0' for booleans, integer and numbers is still
> interpreted as a set required property.
> 
> The behavior is much more apparent for section configs, where
> parse_config(...) expects these properties to be written in the config,
> while write_config(...) will drop these for empty strings and will
> result in warn log messages for any subsequent reads with $allow_unknown
> set. Therefore I propose the change only for section config for now.
> 
> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=7399
> 
>  src/PVE/SectionConfig.pm | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
> index 84ff81a..d0332f9 100644
> --- a/src/PVE/SectionConfig.pm
> +++ b/src/PVE/SectionConfig.pm
> @@ -1572,11 +1572,12 @@ sub write_config {
>              next if $opts->{$k}->{optional};
>              $done_hash->{$k} = 1;
>              my $v = $scfg->{$k};
> -            die "section '$sectionId' - missing value for required option '$k'\n"
> -                if !defined($v);
>              $v = $class->encode_value($type, $k, $v);

Note that this also changes the public section config API by requiring
`encode_value()` to deal with the `$value` parameter being `undef` -
previously this would not happen here.

Its description would need to be updated, and its example as well, since
it only checks the `$key` before then dereferencing `$value` as a hash.

A quick grep shows a bunch of unprotected uses:
- PVE::Storage::Plugin for `nodes` or `content{,-dirs}`
- PVE::SDN::Zones::Plugin
- PVE::ACME::Challenge
- PVE::HA::Rules
- PVE::HA::Groups
- plugins for PVE::Job::Registry

>              my $prop = $class->get_property_schema($type, $k);
> -            $data .= format_config_line($prop, $k, $v);
> +            my $cfg_line = format_config_line($prop, $k, $v);
> +            die "section '$sectionId' - missing value for required option '$k'\n"
> +                if !$cfg_line;
> +            $data .= $cfg_line;
>          }
>  
>          for my $k (@option_keys) {
> -- 
> 2.47.3

Re: [RFC PATCH common] section config: correctly handle required options in write_config

[Daniel Kral]

On Thu Mar 12, 2026 at 2:06 PM CET, Wolfgang Bumiller wrote:
> On Wed, Mar 11, 2026 at 01:26:18PM +0100, Daniel Kral wrote:
>> diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
>> index 84ff81a..d0332f9 100644
>> --- a/src/PVE/SectionConfig.pm
>> +++ b/src/PVE/SectionConfig.pm
>> @@ -1572,11 +1572,12 @@ sub write_config {
>>              next if $opts->{$k}->{optional};
>>              $done_hash->{$k} = 1;
>>              my $v = $scfg->{$k};
>> -            die "section '$sectionId' - missing value for required option '$k'\n"
>> -                if !defined($v);
>>              $v = $class->encode_value($type, $k, $v);
>
> Note that this also changes the public section config API by requiring
> `encode_value()` to deal with the `$value` parameter being `undef` -
> previously this would not happen here.

Right, good catch! I had a bad gut feeling about moving it farther here
and only did so that the die is coupled to whether
format_config_line(...) does add a line to the config $data string.

I'd go for having both the definedness check above and the 'is the data
actually written to the config file?' check below with a more telling
error message in a v2, if there are no objections.

>
> Its description would need to be updated, and its example as well, since
> it only checks the `$key` before then dereferencing `$value` as a hash.
>
> A quick grep shows a bunch of unprotected uses:
> - PVE::Storage::Plugin for `nodes` or `content{,-dirs}`
> - PVE::SDN::Zones::Plugin
> - PVE::ACME::Challenge
> - PVE::HA::Rules
> - PVE::HA::Groups
> - plugins for PVE::Job::Registry
>
>>              my $prop = $class->get_property_schema($type, $k);
>> -            $data .= format_config_line($prop, $k, $v);
>> +            my $cfg_line = format_config_line($prop, $k, $v);
>> +            die "section '$sectionId' - missing value for required option '$k'\n"
>> +                if !$cfg_line;
>> +            $data .= $cfg_line;
>>          }
>>  
>>          for my $k (@option_keys) {
>> -- 
>> 2.47.3