From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 744501FF13F for ; Thu, 12 Mar 2026 10:47:42 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0E941C7DE; Thu, 12 Mar 2026 10:47:37 +0100 (CET) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 12 Mar 2026 10:47:31 +0100 Message-Id: From: "Max R. Carrara" Subject: Re: [RFC PATCH common] section config: correctly handle required options in write_config To: "Daniel Kral" , X-Mailer: aerc 0.18.2-0-ge037c095a049 References: <20260311122659.250421-1-d.kral@proxmox.com> In-Reply-To: <20260311122659.250421-1-d.kral@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1773308816074 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.085 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) 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: GDWCZRNKKYVHL4OYUOHN4EAPGQAGSZZC X-Message-ID-Hash: GDWCZRNKKYVHL4OYUOHN4EAPGQAGSZZC X-MailFrom: m.carrara@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: Wolfgang Bumiller 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 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 > --- > 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=3D7399 Just had a look at your patch=E2=80=94nice 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=E2=80=94it'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=E2=80=94see the other scrip= ts for how the tests in there work. (ofc you can holler at me too!) Regarding JSONSchema: I think the definedness check itself is fine=E2=80=94= 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=E2=80=94Wolfga= ng 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=3Dpve-common.git;a=3Dtree;f=3Dtest/SectionC= onfig;h=3D0967fb1eebf84b4ecad696901572fa7550d49941;hb=3Drefs/heads/master [2]: https://json-schema.org/draft/2020-12/json-schema-validation#name-vali= dation-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} =3D 1; > my $v =3D $scfg->{$k}; > - die "section '$sectionId' - missing value for required optio= n '$k'\n" > - if !defined($v); > $v =3D $class->encode_value($type, $k, $v); > my $prop =3D $class->get_property_schema($type, $k); > - $data .=3D format_config_line($prop, $k, $v); > + my $cfg_line =3D format_config_line($prop, $k, $v); > + die "section '$sectionId' - missing value for required optio= n '$k'\n" > + if !$cfg_line; > + $data .=3D $cfg_line; > } > > for my $k (@option_keys) {