Re: [pve-devel] [PATCH v4 pve-storage 05/16] cephconfig: align our parser more with Ceph's parser

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

On Tue Mar 19, 2024 at 10:38 AM CET, Fabian Grünbichler 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:
> > 
> >       [some.section] # inline comment after section
> >       foo = bar ; inline comment after value
> > 
> >      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].
> > 
> >  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.
> > 
> >  3. Instead of treating '-', '_' and ' ' as the same, only '_' and ' '
> >     are treated the same, like in Ceph's parser [2].
> > 
> >  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.
> > 
> > [0]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l178
> > [1]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l194
> > [2]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l294
> > 
> > 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
> > 
> >  src/PVE/CephConfig.pm | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> > 
> > 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);
> >  
> > +# For more details on how Ceph's config parser works, see:
> > +# https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master
> >  sub parse_ceph_config {
> >      my ($filename, $raw) = @_;
> >  
> > @@ -20,14 +22,18 @@ sub parse_ceph_config {
> >  
> >      my $section;
> >  
> > -    foreach my $line (@lines) {
> > -	$line =~ s/#.*$//;
> > +    for my $line (@lines) {
> > +	$line =~ 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
> > 
> > This is a list of IP addresses or hostnames that are separated by commas, whitespace, or semicolons. 
>
> https://docs.ceph.com/en/latest/rados/configuration/ceph-conf/#confval-mon_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 =~ s/^\s+//;
> > -	$line =~ s/^;.*$//;
> >  	$line =~ s/\s+$//;
> >  	next if !$line;
> >  
> > -	$section = $1 if $line =~ m/^\[(\S+)\]$/;
> > +	if ($line =~ m/^\[(.+)\]$/) {
> > +	    $section = $1;
> > +	    $cfg->{$section} = {} if !exists($cfg->{$section});
> > +	    next;
> > +	}
> > +
> >  	if (!$section) {
> >  	    warn "no section - skip: $line\n";
> >  	    next;
> > @@ -35,11 +41,12 @@ sub parse_ceph_config {
> >  
> >  	if ($line =~ m/^(.*?\S)\s*=\s*(\S.*)$/) {
> >  	    my ($key, $val) = ($1, $2);
> > -	    # ceph treats ' ', '_' and '-' in keys the same, so lets do too
> > -	    $key =~ s/[-\ ]/_/g;
> > +	    # ceph treats ' ' and '_' in keys the same, so lets do too
> > +	    $key =~ s/ /_/g;
> >  	    $cfg->{$section}->{$key} = $val;
> > -	}
> >  
> > +	    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!

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