Re: [pve-devel] [PATCH common 1/1] tools: add extract_sensitive_params

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 02.12.20 10:21, Dominik Csapak wrote:
> moved and generalized from pve-storage, since we'll need it
> in more places
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/Tools.pm | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index 4b445ea..bda236a 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -48,6 +48,7 @@ template_replace
>  safe_print
>  trim
>  extract_param
> +extract_sensitive_params
>  file_copy
>  get_host_arch
>  O_PATH
> @@ -807,6 +808,29 @@ sub extract_param {
>      return $res;
>  }
>  

can we have some short comment about what this does and when/why it can be useful here

> +sub extract_sensitive_params :prototype($$$) {
> +    my ($param, $sensitive_list, $delete_list) = @_;
> +
> +    my $sensitive;

I know auto vivification and such things exist, but I'd feel more comfortable
to set above explicitly to and empty hash {} .

> +
> +    my %delete = map { $_ => 1 } ($delete_list || [])->@*;
> +
> +    # always extract sensitive keys, so they don't get written to the www-data readable scfg

not only for scfg anymore, would drop that comment actually completely, that's rather
something for a method comment (see above)

> +    for my $opt (@$sensitive_list) {
> +	# First handle deletions as explicitly setting `undef`, afterwards new values may override
> +	# it.

I know this is just copied, but there's no actual reason for setting to undef vs.
using delete encoded in that comment, it's just merely describing what one sees
when reading the code anyhow..

@Wolfgang, you as original author (pve-storage commit 72385de9e23df) why did you
used undef vs. delete?

> +	if (exists($delete{$opt})) {
> +	    $sensitive->{$opt} = undef;
> +	}
> +
> +	if (defined(my $value = extract_param($param, $opt))) {
> +	    $sensitive->{$opt} = $value;
> +	}
> +    }
> +
> +    return $sensitive;
> +}
> +
>  # Note: we use this to wait until vncterm/spiceterm is ready
>  sub wait_for_vnc_port {
>      my ($port, $family, $timeout) = @_;
>