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

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

On 2/12/24 14:33, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 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.
> 
> these seem sensible - what about line continuations? ;)

Yeah, those get swallowed. Will be corrected in v3, thanks for pointing
this out!

> 
>>
>>  3. Instead of treating '-', '_' and ' ' as the same, only '_' and ' '
>>     are treated the same, like in Ceph's parser [2].
> 
> the ceph docs state something else - which is wrong? ;)
> 
> https://docs.ceph.com/en/latest/rados/configuration/ceph-conf/ says:
> 
>> Each of the Ceph configuration options has a unique name that consists
>> of words formed with lowercase characters and connected with
>> underscore characters (_).
> 
> this would seem to agree
> 
>> When option names are specified on the command line, underscore (_)
>> and dash (-) characters can be used interchangeably (for example,
>> --mon-host is equivalent to --mon_host).
> 
> okay, this is CLI which might just have its own mapping
> 
>> When option names appear in configuration files, spaces can also be
>> used in place of underscores or dashes. However, for the sake of
>> clarity and convenience, we suggest that you consistently use
>> underscores, as we do throughout this documentation.
> 
> but this now says that dash in config files is OK?

The docs and the code don't seem to agree in that regard; at least the
INI parser in C++ does *not* treat `-` the same [0]:

> /* Normalize a key name.
>  *
>  * Normalized key names have no leading or trailing whitespace, and all
>  * whitespace is stored as underscores.  The main reason for selecting this
>  * normal form is so that in common/config.cc, we can use a macro to stringify
>  * the field names of md_config_t and get a key in normal form.
>  */

(The code also actually does what it says on the tin. ;) )

So, I think it would be sensible to stick to how the Code does it, in this
case. Or was the original treatment of dashes in our code included in order
to guard against user errors, perhaps?

[0]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l294

> 
>> [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
>>
>>  src/PVE/CephConfig.pm | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
>> index 6b10d46..77b745f 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,13 @@ sub parse_ceph_config {
>>  
>>      my $section;
>>  
>> -    foreach my $line (@lines) {
>> -	$line =~ s/#.*$//;
>> +    for my $line (@lines) {
>> +	$line =~ s/(#|;).*$//;
>>  	$line =~ s/^\s+//;
>> -	$line =~ s/^;.*$//;
>>  	$line =~ s/\s+$//;
>>  	next if !$line;
>>  
>> -	$section = $1 if $line =~ m/^\[(\S+)\]$/;
>> +	$section = $1 if $line =~ m/^\[(.+)\]$/;
>>  	if (!$section) {
>>  	    warn "no section - skip: $line\n";
>>  	    next;
>> @@ -35,11 +36,10 @@ 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;
>>  	}
>> -
>>      }
>>  
>>      return $cfg;
>> -- 
>> 2.39.2
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>