[pve-devel] [PATCH pve-zsync] fix #1860 added ability to specify source and destination user
Thomas Lamprecht
t.lamprecht at proxmox.com
Wed Aug 22 11:13:53 CEST 2018
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
More information about the pve-devel
mailing list