[pve-devel] [PATCH pve-zsync] fix #1860 added ability to specify source and destination user

[Thomas Lamprecht]

On 8/22/18 10:47 AM, Wolfgang Bumiller wrote:
> On Tue, Aug 21, 2018 at 10:05:39AM +0200, Thomas Lamprecht wrote:
>> On 8/13/18 10:47 AM, David wrote:
>>> From: David Limbeck <d.limbeck at proxmox.com>
> [snip]
>>> @@ -210,6 +210,8 @@ sub parse_argv {
>>>      $param->{name} = undef;
>>>      $param->{skip} = undef;
>>>      $param->{method} = undef;
>>> +    $param->{source_user} = undef;
>>> +    $param->{dest_user} = undef;
>>
>> this was/is noisy, maybe:
>>
>>     my $param = { map { $_ => undef } qw(dest source verbose limit maxsnap name skip method source_user dest_user) };
> 
> I'd prefer the qw() contents in a new line though, due to the length,
> perhaps even one line per entry. Or even just:
>     my $param = {
>         a => undef,
>         b => undef,
>     }

will do this, had it as option but went the map way, as map is fun.

I also had a halfway implemented schema approach cutting down a lot
of lines, but as this is more in maintenance mode (superseded by pvesr)
and I do not want to break things with refactoring I'll omit that.

> 
> Really anything's better than repeating hash accesses so many times.
> 
> (Since it's all initialized to undef, we could just skip this altogether
> even, as the reference-taking in the GetOptionsFromArray call is going
> to "autovivify" them as undef anyway ;-) (and we obviously can't be
> using any exists() checks anywhere atm.).
> Then again, I don't think we like autovivification much :-D )
> 
>>
>>>  
>>>      my ($ret, $ar) = GetOptionsFromArray(\@arg,
>>>  					 'dest=s' => \$param->{dest},
>>> @@ -219,7 +221,9 @@ sub parse_argv {
>>>  					 'maxsnap=i' => \$param->{maxsnap},
>>>  					 'name=s' => \$param->{name},
>>>  					 'skip' => \$param->{skip},
>>> -					 'method=s' => \$param->{method});
>>> +					 'method=s' => \$param->{method},
>>> +                     'source-user=s' => \$param->{source_user},
>>> +                     'dest-user=s' => \$param->{dest_user});
>>
>> indentation error...
>>
>>>  
>>>      if ($ret == 0) {
>>>  	die "can't parse options\n";
>>> @@ -266,6 +270,8 @@ sub encode_cron {
>>>  	    $cfg->{$param->{source}}->{$param->{name}}->{maxsnap} = $param->{maxsnap};
>>>  	    $cfg->{$param->{source}}->{$param->{name}}->{skip} = $param->{skip};
>>>  	    $cfg->{$param->{source}}->{$param->{name}}->{method} = $param->{method};
>>> +	    $cfg->{$param->{source}}->{$param->{name}}->{source_user} = $param->{source_user};
>>> +	    $cfg->{$param->{source}}->{$param->{name}}->{dest_user} = $param->{dest_user};
>>
>> this block was already strange, maybe we can clean it up in a separate commit
>> to resemble something like:
>>
>>         if ($param->{source} && $param->{dest}) {
>>             my $source = delete $param->{source};
>>             my $name = delete $param->{name};
>>
>>             $cfg->{$source}->{$name} = $param;
>>         }
>>
>> as parse_argv already has a defined fixed set of possible params and we copy
>> all but source/name here anyway...
> 
> Also, as a general note, I'd like to not see long hash access chains
> in new code, but rather use helper variables, like:
> 
> 	my $src = $param->{source};
> 	my $dest = $param->{dest};
> 	my $cfgentry = $cfg->{$src}->{$dst}; # do this only once
> 	$cfgentry->{a} = $param->{a};
> 	$cfgentry->{b} = $param->{b};
> 	$cfgentry->{c} = $param->{c};
> 
> I know it's tempting to just copy a line from such a block of
> assignments and change a couple of letters, but we should try to
> improve code like that if we can.
> And the original code accesses a nested hash via keys stored in yet
> another hash... that's just painful to read.
> 

+1