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

[Wolfgang Bumiller]

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>
> > 
> 
> some comments inline, did not test to much yet...
> 
> > source user and destination user can be specified with -source-user and
> > -dest-user, root is chosen if none is specified
> 
> I'm not to sure about source user, #1860 only asks for a destination user,
> source is probably always root in our use cases, but we allow use cases
> where it makes sense so I'm not really opposed...
> 
> maybe add at least some input validation if a VMID is passed as source?
> (makes no sense to have source_user there, we mirror from local and run
> as root already...)
> 
> > requires zfs permissions on source and destination target
> > destination dataset has to be created already but not mounted
> > ---
> >  pve-zsync | 117 +++++++++++++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 73 insertions(+), 44 deletions(-)
> > 
> > diff --git a/pve-zsync b/pve-zsync
> > index 9938b17..c40df35 100755
> > --- a/pve-zsync
> > +++ b/pve-zsync
> > @@ -124,12 +124,12 @@ sub get_status {
> >  }
> >  
> >  sub check_pool_exists {
> > -    my ($target) = @_;
> > +    my ($target, $user) = @_;
> >  
> >      my $cmd = [];
> >  
> >      if ($target->{ip}) {
> > -	push @$cmd, 'ssh', "root\@$target->{ip}", '--';
> > +	push @$cmd, 'ssh', "$user\@$target->{ip}", '--';
> >      }
> >      push @$cmd, 'zfs', 'list', '-H', '--', $target->{all};
> >      eval {
> > @@ -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,
    }

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.

> 
> > [snip]> @@ -1156,6 +1182,9 @@ if (!$command) {
> >  my @arg = @ARGV;
> >  my $param = parse_argv(@arg);
> >  
> > +$param->{source_user} = "root" if(!$param->{source_user});
> > +$param->{dest_user} = "root" if(!$param->{dest_user});
> 
> why don't you put this in the parse_argv sub?
> Else this gets missed on all other place we pare params, e.g., when reading
> the crontab... In the aforementioned sub we already set a few defaults so it
> seems the right place to ensure we never miss it...
> 
> > +
> >  sub check_params {
> >      for (@_) {
> >  	die "$cmd_help->{$command}\n" if !$param->{$_};
> >