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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

thanks for the review, something on top inline.

On 06.05.21 12:04, Oguz Bektas wrote:
> On Thu, May 06, 2021 at 11:11:00AM +0200, Lorenz Stechauner wrote:

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

True, but would be a separate series out of scope here, needs to gain
support in datacenter.cfg
https://pve.proxmox.com/pve-docs/datacenter.cfg.5.html#_options

May be relevant to talk with Dietmar about the upcomming possibilities in PBS,
he checked out HTTP proxies quite closely recently.

> [snip]
>> +
>> +sub check_file_hash {
>> +    my ($checksums, $filename, $noerr) = @_;
>> +
>> +    my $digest;
>> +    my $expected;
>> +
>> +    eval {
>> +	open(my $fh, '<', $filename) or die "Can't open '$filename': $!";


as already mentioned in a previous review, add the trailing new line "\n" to
die statements, else they will get ugly by adding internal information!

>> +	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) {

use for over foreach, and a list can be used directly, no need for a useless
intermediate variable:

for my $foo ('a', 'b', 'c') {
    ...


>     if (defined($checksums->{"sha$algorithm"})) {
> 	$expected = $checksums->{"sha$algorithm"};
> 	$digest = Digest::SHA->new($algorithm)->addfile($fh)->hexdigest;

You can also use strings as module in perl:

$digest = "Digest::$algorithm"->new->addfile...


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

with < 10 elements that can be fine, but here the whole method is weird in UX
IMO and could be improved in general by:

1. pass alogrirhm and expected hash string directly
2. use a map for the different modules
3. let the caller handle the error (albeit no hard feelings here)

sub check_file_hash
   my ($algorithm, $expected, $file) = @_;

   my $algorithm_map = {
      'sha256' => sub { Digest::SHA->new(512) },
      'sha512' => sub { Digest::SHA->new(512) },
      # etc...
   };

   my $digester = $algorithm_map->{$algorithm}->() or die "unknown algorithm '$algorithm'\n";

   open(my $fh, '<', $filename) or die "cannot open file '$file': $!\n";

   my $got = $digester->addfile($fh)->hexdigest;
   close($fh);

   return lc($digest) eq lc($expected);
}


IMO much simpler/shorter and still easy to grasp.

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