From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <m.carrara@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by lists.proxmox.com (Postfix) with ESMTPS id 7C529BA449
 for <pve-devel@lists.proxmox.com>; Tue, 19 Mar 2024 16:58:44 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id 54B483961
 for <pve-devel@lists.proxmox.com>; Tue, 19 Mar 2024 16:58:14 +0100 (CET)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [94.136.29.106])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS
 for <pve-devel@lists.proxmox.com>; Tue, 19 Mar 2024 16:58:12 +0100 (CET)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 11B2646915
 for <pve-devel@lists.proxmox.com>; Tue, 19 Mar 2024 16:58:12 +0100 (CET)
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Tue, 19 Mar 2024 16:58:10 +0100
Message-Id: <CZXUMSU5CMIG.SDGNQJ13WCCH@proxmox.com>
From: "Max Carrara" <m.carrara@proxmox.com>
To: "Proxmox VE development discussion" <pve-devel@lists.proxmox.com>
X-Mailer: aerc 0.17.0-72-g6a84f1331f1c
References: <20240305150758.252669-1-m.carrara@proxmox.com>
 <20240305150758.252669-6-m.carrara@proxmox.com>
 <1710838924.n32t2u8k60.astroid@yuna.none>
In-Reply-To: <1710838924.n32t2u8k60.astroid@yuna.none>
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.021 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
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
 T_SCC_BODY_TEXT_LINE    -0.01 -
 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
 http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
 information. [cephconfig.pm, proxmox.com, ceph.com]
Subject: Re: [pve-devel] [PATCH v4 pve-storage 05/16] cephconfig: align our
 parser more with Ceph's parser
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Tue, 19 Mar 2024 15:58:44 -0000

On Tue Mar 19, 2024 at 10:38 AM CET, Fabian Gr=C3=BCnbichler wrote:
> On March 5, 2024 4:07 pm, Max Carrara wrote:
> >  1. Comments, irrespective of whether they start with '#' or ';' are
> >     now treated the same. Otherwise, sections and key-value pairs with
> >     a trailing comment starting with ';' are still parsed. Consider
> >     this example:
> >=20
> >       [some.section] # inline comment after section
> >       foo =3D bar ; inline comment after value
> >=20
> >      The '[some.section]' section in the example above would otherwise
> >      not be parsed at all, while in the key-value definition 'foo'
> >      parses as the key, which is correct, but 'bar ; inline comment
> >      after value' parses as value, which is incorrect according to
> >      Ceph's grammar [0][1].
> >=20
> >  2. Sections may now contain any character, including whitespace, but
> >     not '\n' or a comment literal '#' or ';'. The case for comment
> >     literals is handled in 1. above.
> >=20
> >  3. Instead of treating '-', '_' and ' ' as the same, only '_' and ' '
> >     are treated the same, like in Ceph's parser [2].
> >=20
> >  4. Although not crucial for Ceph, our parser now also supports empty
> >     sections. When a section header is successfully parsed, it gets
> >     added to the configuration hash and the parser continues operating
> >     on the next line.
> >=20
> > [0]: https://git.proxmox.com/?p=3Dceph.git;a=3Dblob;f=3Dceph/src/common=
/ConfUtils.cc;h=3D2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=3Drefs/heads/=
master#l178
> > [1]: https://git.proxmox.com/?p=3Dceph.git;a=3Dblob;f=3Dceph/src/common=
/ConfUtils.cc;h=3D2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=3Drefs/heads/=
master#l194
> > [2]: https://git.proxmox.com/?p=3Dceph.git;a=3Dblob;f=3Dceph/src/common=
/ConfUtils.cc;h=3D2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=3Drefs/heads/=
master#l294
> >=20
> > Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> > ---
> > Changes v1 --> v2:
> >   * new
> > Changes v2 --> v3:
> >   * support comment literals (4.)
> > Changes v3 --> v4:
> >   * support empty sections
> >   * fix and move support for comment literals to separate patch
> >=20
> >  src/PVE/CephConfig.pm | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> >=20
> > diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
> > index 6b10d46..74a92eb 100644
> > --- a/src/PVE/CephConfig.pm
> > +++ b/src/PVE/CephConfig.pm
> > @@ -10,6 +10,8 @@ cfs_register_file('ceph.conf',
> >  		  \&parse_ceph_config,
> >  		  \&write_ceph_config);
> > =20
> > +# For more details on how Ceph's config parser works, see:
> > +# https://git.proxmox.com/?p=3Dceph.git;a=3Dblob;f=3Dceph/src/common/C=
onfUtils.cc;h=3D2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=3Drefs/heads/ma=
ster
> >  sub parse_ceph_config {
> >      my ($filename, $raw) =3D @_;
> > =20
> > @@ -20,14 +22,18 @@ sub parse_ceph_config {
> > =20
> >      my $section;
> > =20
> > -    foreach my $line (@lines) {
> > -	$line =3D~ s/#.*$//;
> > +    for my $line (@lines) {
> > +	$line =3D~ s/(?<!\\)(#|;).*$//;
>
> nit: [#;] instead of the group works as well, and is a bit more
> idiomatic IMHO.

Good catch, thanks!

>
> so this seems to agree with the parser grammar, but it kind of disagrees
> with the docs that for example state:
>
> > mon_host
> >=20
> > This is a list of IP addresses or hostnames that are separated by comma=
s, whitespace, or semicolons.=20
>
> https://docs.ceph.com/en/latest/rados/configuration/ceph-conf/#confval-mo=
n_host
>
> well, they don't mention there that you need to escape the `;` for that
> to work, else only the first "entry" takes, and the rest is silently
> interpreted as comment. if you quote the "list", then it's even a parse
> error since the closing quote is interpreted as part of the comment ;)
>
> but unfortunately there is one more issue:
>
> \\;
>
> still marks the start of a comment (since it's the backlash that is
> being escaped) in ceph's parser (same applies to '#' as well).

Oh, that's interesting. Very good catch, thank you! Seems like I'll have
to revise the whole parsing logic again in the next series, *sigh* ...

>
> what a mess this file format is..

I wholeheartedly agree.

>
> >  	$line =3D~ s/^\s+//;
> > -	$line =3D~ s/^;.*$//;
> >  	$line =3D~ s/\s+$//;
> >  	next if !$line;
> > =20
> > -	$section =3D $1 if $line =3D~ m/^\[(\S+)\]$/;
> > +	if ($line =3D~ m/^\[(.+)\]$/) {
> > +	    $section =3D $1;
> > +	    $cfg->{$section} =3D {} if !exists($cfg->{$section});
> > +	    next;
> > +	}
> > +
> >  	if (!$section) {
> >  	    warn "no section - skip: $line\n";
> >  	    next;
> > @@ -35,11 +41,12 @@ sub parse_ceph_config {
> > =20
> >  	if ($line =3D~ m/^(.*?\S)\s*=3D\s*(\S.*)$/) {
> >  	    my ($key, $val) =3D ($1, $2);
> > -	    # ceph treats ' ', '_' and '-' in keys the same, so lets do too
> > -	    $key =3D~ s/[-\ ]/_/g;
> > +	    # ceph treats ' ' and '_' in keys the same, so lets do too
> > +	    $key =3D~ s/ /_/g;
> >  	    $cfg->{$section}->{$key} =3D $val;
> > -	}
> > =20
> > +	    next;
>
> this next serves no purpose? (ah, it does with one of the later patches
> that adds code below, so please move this to that patch!)

My bad, will do!

>
> > +	}
> >      }
> > =20
> >      return $cfg;
> > --=20
> > 2.39.2
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel