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

["Max R. Carrara" <m.carrara@proxmox.com>]

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) {