From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 5164A79FAB for ; Thu, 6 May 2021 14:16:03 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 447C320179 for ; Thu, 6 May 2021 14:16:03 +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 4951B2015A for ; Thu, 6 May 2021 14:16:01 +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 1D6CA464AE for ; Thu, 6 May 2021 14:16:01 +0200 (CEST) Message-ID: Date: Thu, 6 May 2021 14:15:59 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:89.0) Gecko/20100101 Thunderbird/89.0 Content-Language: en-US To: Oguz Bektas , Proxmox VE development discussion , Lorenz Stechauner References: <20210506091010.40737-1-l.stechauner@proxmox.com> <20210506091105.40976-1-l.stechauner@proxmox.com> <20210506091105.40976-2-l.stechauner@proxmox.com> <20210506100443.GA12590@gaia.proxmox.com> From: Thomas Lamprecht In-Reply-To: <20210506100443.GA12590@gaia.proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.007 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) 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] 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 May 2021 12:16:03 -0000 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