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.stechau...@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
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
