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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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..

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 ;)

>  	    }
>  	    $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;
>  }