From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 6859F1FF13F for ; Thu, 12 Mar 2026 14:06:27 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 43FF215073; Thu, 12 Mar 2026 14:06:22 +0100 (CET) Date: Thu, 12 Mar 2026 14:06:15 +0100 From: Wolfgang Bumiller To: Daniel Kral Subject: Re: [RFC PATCH common] section config: correctly handle required options in write_config Message-ID: <5au47ehnodyxkvpthlc7kxsh6ttnbzosexabjsihf64bka3ama@y2jskncbucy7> References: <20260311122659.250421-1-d.kral@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260311122659.250421-1-d.kral@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1773320740284 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.982 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 RCVD_IN_MSPIKE_H2 0.001 Average reputation (+2) RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.408 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.819 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.903 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: RDFLSBUX3MTZENNLH6BVZG45VOBGZIDY X-Message-ID-Hash: RDFLSBUX3MTZENNLH6BVZG45VOBGZIDY X-MailFrom: w.bumiller@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 > --- > 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