From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <o.bektas@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by lists.proxmox.com (Postfix) with ESMTPS id 327C079FA7
 for <pve-devel@lists.proxmox.com>; Thu,  6 May 2021 12:04:47 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id 255551ECAB
 for <pve-devel@lists.proxmox.com>; Thu,  6 May 2021 12:04:47 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [94.136.29.106])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS id 209391EC9F
 for <pve-devel@lists.proxmox.com>; Thu,  6 May 2021 12:04:46 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with ESMTP id F136342AC1
 for <pve-devel@lists.proxmox.com>; Thu,  6 May 2021 12:04:45 +0200 (CEST)
Date: Thu, 6 May 2021 12:04:43 +0200
From: Oguz Bektas <o.bektas@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Message-ID: <20210506100443.GA12590@gaia.proxmox.com>
Mail-Followup-To: Oguz Bektas <o.bektas@proxmox.com>,
 Proxmox VE development discussion <pve-devel@lists.proxmox.com>
References: <20210506091010.40737-1-l.stechauner@proxmox.com>
 <20210506091105.40976-1-l.stechauner@proxmox.com>
 <20210506091105.40976-2-l.stechauner@proxmox.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210506091105.40976-2-l.stechauner@proxmox.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
X-SPAM-LEVEL: Spam detection results:  1
 AWL 1.263 Adjusted score from AWL reputation of From: address
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
 http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
 information. [proxmox.com, tools.pm]
Subject: Re: [pve-devel] [PATCH v4 common 2/7] tools: add
 download_file_from_url
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Thu, 06 May 2021 10:04:47 -0000

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