Re: [pve-devel] [PATCH v2 pve-storage 06/11] cephconfig: allow writing arbitrary sections

[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:
>> This adds support for writing arbitrary sections to 'ceph.conf' while
>> ensuring that already written sections are not duplicated.
>>
>> Sections that are associated with the client, for example
>> '[client.foo]', are written directly after the '[client]' section.
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> 
> Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> 
> would have been easier to parse if the style cleanup and actual
> behaviour change would have been split..

Fair point, acknowledged!

> 
> style change:
> 
>> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
>> index 6b10d46..34c3107 100644
>> --- a/src/PVE/CephConfig.pm
>> +++ b/src/PVE/CephConfig.pm
>> @@ -65,10 +65,10 @@ sub write_ceph_config {
>>      my $cond_write_sec = sub {
>>  	my $re = shift;
>>  
>> -	foreach my $section (sort keys %$cfg) {
>> +	for my $section (sort keys %$cfg) {
>>  	    next if $section !~ m/^$re$/;
>>  	    $out .= "[$section]\n";
>> -	    foreach my $key (sort keys %{$cfg->{$section}}) {
>> +	    for my $key (sort keys %{$cfg->{$section}}) {
>>  		$out .= "\t $key = $cfg->{$section}->{$key}\n";
>>  	    }
>>  	    $out .= "\n";
> 
> actual change (small nits inline!):
> 
>> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
>> index 34c3107..24bc78c 100644
>> --- a/src/PVE/CephConfig.pm
>> +++ b/src/PVE/CephConfig.pm
>> @@ -60,23 +60,30 @@ my $parse_ceph_file = sub {
>>  sub write_ceph_config {
>>      my ($filename, $cfg) = @_;
>>  
>> +    my $written_sections = {};
>>      my $out = '';
>>  
>>      my $cond_write_sec = sub {
>>  	my $re = shift;
>>  
>>  	for my $section (sort keys %$cfg) {
>> -	    next if $section !~ m/^$re$/;
>> +	    if ($section !~ m/^$re$/ || exists($written_sections->{$section})) {
>> +		next;
>> +	    }
> 
> these two could be more clearly written as
> 
> next if $written_sections->{section};
> next if $section !~ m/$re$/;
> 
> since the two checks are not related in any way, other than both having
> the same effect if true (skipping that section). the second line is then
> unchanged, and both the diff and the resulting code is easier to parse
> IMHO.
> 
>> +
>>  	    $out .= "[$section]\n";
>>  	    for my $key (sort keys %{$cfg->{$section}}) {
>> -		$out .= "\t $key = $cfg->{$section}->{$key}\n";
>> +		$out .= "\t$key = $cfg->{$section}->{$key}\n";
> 
> this part here is not mentioned at all, and I might have missed it if I
> hadn't split the diffs ;)

Mea culpa!

Will clean this up in v3 and split the changes accordingly.

> 
>>  	    }
>>  	    $out .= "\n";
>> +
>> +	    $written_sections->{$section} = 1;
>>  	}
>>      };
>>  
>>      &$cond_write_sec('global');
>>      &$cond_write_sec('client');
>> +    &$cond_write_sec('client\..*');
>>  
>>      &$cond_write_sec('mds');
>>      &$cond_write_sec('mon');
>> @@ -88,6 +95,8 @@ sub write_ceph_config {
>>      &$cond_write_sec('osd\..*');
>>      &$cond_write_sec('mgr\..*');
>>  
>> +    &$cond_write_sec('.*');
>> +
>>      return $out;
>>  }
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel