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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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