Re: [pve-devel] [PATCH firewall 1/4] api: factor out renaming parameters to more descriptive names

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Thu, Jan 26, 2023 at 03:30:16PM +0100, Leo Nunner wrote:
> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
> ---
>  src/PVE/API2/Firewall/Aliases.pm | 20 ++++++------
>  src/PVE/API2/Firewall/Groups.pm  | 53 ++++++++++++++++----------------
>  src/PVE/API2/Firewall/IPSet.pm   | 39 ++++++++++++-----------
>  3 files changed, 56 insertions(+), 56 deletions(-)
> 
> diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
> index 33ac669..88f20a0 100644
> --- a/src/PVE/API2/Firewall/Aliases.pm
> +++ b/src/PVE/API2/Firewall/Aliases.pm
> @@ -234,24 +234,22 @@ sub register_update_alias {
>  
>  		PVE::Tools::assert_if_modified($digest, $param->{digest});
>  
> -		my $name = lc($param->{name});
> +		my $rename_to = lc($param->{rename}) if defined($param->{rename});

Conditional variable declaration leads to really bad undefined behavior
in perl, please keep this split in 2 as it was before, otherwise this
might retain values from previous times the api call was used...

> +		my $rename_from = lc($param->{name});
>  
> -		raise_param_exc({ name => "no such alias" }) if !$aliases->{$name};
> +		raise_param_exc({ name => "no such alias" }) if !$aliases->{$rename_from};
>  
>  		my $data = { name => $param->{name}, cidr => $param->{cidr} };
>  		$data->{comment} = $param->{comment} if $param->{comment};
>  
> -		$aliases->{$name} = $data;
> -
> -		my $rename = $param->{rename};
> -		$rename = lc($rename) if $rename;
> +		$aliases->{$rename_from} = $data;
>  
> -		if ($rename && ($name ne $rename)) {
> +		if ($rename_to && ($rename_from ne $rename_to)) {
>  		    raise_param_exc({ name => "alias '$param->{rename}' already exists" })
> -			if defined($aliases->{$rename});
> -		    $aliases->{$name}->{name} = $param->{rename};
> -		    $aliases->{$rename} = $aliases->{$name};
> -		    delete $aliases->{$name};
> +			if defined($aliases->{$rename_to});
> +		    $aliases->{$rename_from}->{name} = $param->{rename};
> +		    $aliases->{$rename_to} = $aliases->{$rename_from};
> +		    delete $aliases->{$rename_from};
>  		}
>  
>  		$class->save_aliases($param, $fw_conf, $aliases);