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

[Thomas Lamprecht]

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) };

>  
>      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...

> [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->{$_};
>