Re: [pve-devel] [PATCH v4 common 2/7] tools: add download_file_from_url

[Oguz Bektas <o.bektas@proxmox.com>]

hi,

see inline for some small suggestions :)

On Thu, May 06, 2021 at 11:11:00AM +0200, Lorenz Stechauner wrote:
> code is based on
> manager:PVE/API2/Nodes.pm:aplinfo
> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  src/PVE/Tools.pm | 123 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 123 insertions(+)
> 
> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index 16ae3d2..c751426 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -1829,4 +1829,127 @@ sub safe_compare {
>      return $cmp->($left, $right);
>  }
>  
> +
> +# opts
> +#  -> hash_required
> +#  -> http_proxy
> +#  -> verify_certificates
> +#  -> sha(1|224|256|384|512)sum
> +#  -> md5sum
> +sub download_file_from_url {
> +    my ($dest, $url, $opts) = @_;
> +
> +    my $tmpdest = "$dest.tmp.$$";
> +
> +    my $worker = sub  {
> +	my $upid = shift;
> +
> +	print "donwloading $url to $dest\n";

small typo here

> +
> +	eval {
> +	    if (-f $dest) {
> +		print "calculating checksum of existing file...\n";
> +		my ($correct, $hash, $expected) = check_file_hash($opts, $dest, 1);
> +
> +		if ($hash && $correct) {
> +		    print "file already exists - no need to download\n";
> +		    return;
> +		} else {
> +		    print "mismatch, downloading\n";
> +		}
> +	    }
> +
> +	    my @cmd = ('/usr/bin/wget', '--progress=dot:mega', '-O', $tmpdest, $url);
> +
> +	    local %ENV;
> +	    if ($opts->{http_proxy}) {
> +		$ENV{http_proxy} = $opts->{http_proxy};

might be worth it to also add https_proxy here

> +	    }
> +
> +	    if (defined($opts->{verify_certificates}) && $opts->{verify_certificates} == 0) {
> +		push @cmd, '--no-check-certificate';
> +	    }
> +
> +	    if (system(@cmd) != 0) {
> +		die "download failed - $!\n";
> +	    }

we don't use 'system' for executing commands (especially when a command
parameter is supplied by a user!). see the 'run_command' helper in
pve-common (which also does shellquoting)

> +
> +	    print "trying to calculate checksum...\n";
> +
> +	    my ($correct, $hash, $expected) = check_file_hash($opts, $tmpdest, !$opts->{hash_required});

is it necessary to call check_file_hash unless the option hash_required
is passed?

> +
> +	    die "could not calculate checksum\n" if ($opts->{hash_required} && !$hash);
> +
> +	    if ($hash) {
> +		if ($correct) {
> +		    print "checksum verified\n";
> +		} else {
> +		    die "wrong checksum: $hash != $expected\n";
> +		}
> +	    } else {
> +		print "no checksum for verification specified\n";
> +	    }
> +
> +	    if (!rename($tmpdest, $dest)) {
> +		die "unable to save file - $!\n";
> +	    }
> +	};
> +	my $err = $@;
> +
> +	unlink $tmpdest;
> +
> +	if ($err) {
> +	    print "\n";
> +	    die $err;
> +	}
> +
> +	print "download finished\n";
> +    };
> +
> +    my $rpcenv = PVE::RPCEnvironment::get();
> +    my $user = $rpcenv->get_user();
> +
> +    (my $filename = $dest) =~ s!.*/([^/]*)$!\1!;
> +
> +    return $rpcenv->fork_worker('download', $filename, $user, $worker);
> +}
> +
> +sub check_file_hash {
> +    my ($checksums, $filename, $noerr) = @_;
> +
> +    my $digest;
> +    my $expected;
> +
> +    eval {
> +	open(my $fh, '<', $filename) or die "Can't open '$filename': $!";
> +	binmode($fh);
> +	if (defined($checksums->{sha512sum})) {
> +	    $expected = $checksums->{sha512sum};
> +	    $digest = Digest::SHA->new(512)->addfile($fh)->hexdigest;
> +	} elsif (defined($checksums->{sha384sum})) {
> +	    $expected = $checksums->{sha384sum};
> +	    $digest = Digest::SHA->new(384)->addfile($fh)->hexdigest;
> +	} elsif (defined($checksums->{sha256sum})) {
> +	    $expected = $checksums->{sha256sum};
> +	    $digest = Digest::SHA->new(256)->addfile($fh)->hexdigest;
> +	} elsif (defined($checksums->{sha224sum})) {
> +	    $expected = $checksums->{sha224sum};
> +	    $digest = Digest::SHA->new(224)->addfile($fh)->hexdigest;
> +	} elsif (defined($checksums->{sha1sum})) {
> +	    $expected = $checksums->{sha1sum};
> +	    $digest = Digest::SHA->new(1)->addfile($fh)->hexdigest;
> +	} elsif (defined($checksums->{md5sum})) {
> +	    $expected = $checksums->{md5sum};
> +	    $digest = Digest::MD5->new->addfile($fh)->hexdigest;

hmm not necessary but maybe you could also do something like this (not
tested):

...
my $sha_algorithms = ('1', '224', '256', '384', '512');
foreach my $algorithm (@$sha_algorithms) {
    if (defined($checksums->{"sha$algorithm"})) {
	$expected = $checksums->{"sha$algorithm"};
	$digest = Digest::SHA->new($algorithm)->addfile($fh)->hexdigest;
    }
}

to avoid having a lot of if/elsif clauses (md5 would probably have another
clause but 2 is better than 5-6).


> +	} else {
> +	    die "no expected checksum defined";
> +	}
> +	close($fh);
> +    };
> +
> +    die "checking hash failed - $@\n" if $@ && !$noerr;
> +
> +    return (($digest ? lc($digest) eq lc($expected) : 0), $digest, $expected);
> +}
> +
>  1;
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>