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 00529E7FD for ; Tue, 26 Sep 2023 14:26:09 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D86804AC for ; Tue, 26 Sep 2023 14:25:39 +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 for ; Tue, 26 Sep 2023 14:25:39 +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 E3A9844646 for ; Tue, 26 Sep 2023 14:25:38 +0200 (CEST) Message-ID: Date: Tue, 26 Sep 2023 14:25:30 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 Content-Language: en-US To: Thomas Lamprecht , Proxmox VE development discussion , =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= References: <20230921130917.2000926-1-p.hufnagl@proxmox.com> <20230921130917.2000926-2-p.hufnagl@proxmox.com> <6aab04ab-2d39-42ac-b389-8e563c7322d0@proxmox.com> From: Philipp Hufnagl In-Reply-To: <6aab04ab-2d39-42ac-b389-8e563c7322d0@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.745 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -1.473 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. [nodes.pm] Subject: Re: [pve-devel] [PATCH manager v8 1/2] fix #4849: api: download to storage: automatically dectect and configure compression 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: Tue, 26 Sep 2023 12:26:10 -0000 On 9/26/23 12:56, Thomas Lamprecht wrote: > while this is already applied, some comments inline, for a possible next > time, and also the big > question if this is even required, after all I can just check the few > compression algorithms easily in the frontend, i.e., offloading a simple > string regex match to the backend seems rather odd to me.. The problem with that is that the point where the iso is stored might not be accessible for the client. If it is done by the PVE, it might resolve the url differently. > > Am 21/09/2023 um 15:09 schrieb Philipp Hufnagl: >> extends the query_url_metadata callback with the functionallity to > > s/functionallity/functionality/ > >> detect used compressions. If a compression is used it tells the ui which >> one >> >> Signed-off-by: Philipp Hufnagl >> --- >> PVE/API2/Nodes.pm | 22 +++++++++++++++++++++- >> 1 file changed, 21 insertions(+), 1 deletion(-) >> >> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm >> index 5a148d1d..1e8ed09e 100644 >> --- a/PVE/API2/Nodes.pm >> +++ b/PVE/API2/Nodes.pm >> @@ -34,6 +34,7 @@ use PVE::RRD; >> use PVE::Report; >> use PVE::SafeSyslog; >> use PVE::Storage; >> +use PVE::Storage::Plugin; >> use PVE::Tools; >> use PVE::pvecfg; >> >> @@ -1564,7 +1565,13 @@ __PACKAGE__->register_method({ >> type => 'boolean', >> optional => 1, >> default => 1, >> - } >> + }, >> + 'detect-compression' => { >> + description => "If true an auto detection of used compression will be attempted", > > Grammatically and semantically slightly better would be something like: > "If true, the queried filename is checked for a compression extension." > > > >> + type => 'boolean', >> + optional => 1, >> + default => 0, >> + }, >> }, >> }, >> returns => { >> @@ -1583,6 +1590,11 @@ __PACKAGE__->register_method({ >> type => 'string', >> optional => 1, >> }, >> + compression => { >> + type => 'string', >> + enum => $PVE::Storage::Plugin::KNOWN_COMPRESSION_FORMATS,> + optional => 1, >> + }, >> }, >> }, >> code => sub { >> @@ -1605,6 +1617,7 @@ __PACKAGE__->register_method({ >> SSL_verify_mode => IO::Socket::SSL::SSL_VERIFY_NONE, >> ); >> } >> + my $detect_compression = $param->{'detect-compression'} // 0; > > It's often best to avoid such intermediate variables if there's just a > single use case and on doesn't needs to "jump through hoops" to get > to the value – e.g., a simple hash access like here if 100% fine, if > the value would to be pulled out of some deeply nested structure, or > assembled or the like, then it might have its merit to use an > intermediate variable. > >> >> my $req = HTTP::Request->new(HEAD => $url); >> my $res = $ua->request($req); >> @@ -1615,6 +1628,7 @@ __PACKAGE__->register_method({ >> my $disposition = $res->header("Content-Disposition"); >> my $type = $res->header("Content-Type"); >> >> + my $compression; > > Keep definition and use closer together (I'd moved this down directly above the if t that sets it) > >> my $filename; >> >> if ($disposition && ($disposition =~ m/filename="([^"]*)"/ || $disposition =~ m/filename=([^;]*)/)) { >> @@ -1628,10 +1642,16 @@ __PACKAGE__->register_method({ >> $type = $1; >> } >> >> + if ($detect_compression && $filename =~ m!^(.+)\.(${\PVE::Storage::Plugin::COMPRESSOR_RE})$!) { > > There are code paths where $filename is not yet defined here, resulting > in a rather ugly warning – so this needs upfront checking too – always > check where the value code path is coming in (yeah, Rust would do that for > you, but most API endpoints are small enough to be able to do so quickly also manually) > >> + $filename = $1; >> + $compression = $2; >> + } >> + >> my $ret = {}; >> $ret->{filename} = $filename if $filename; >> $ret->{size} = $size + 0 if $size; >> $ret->{mimetype} = $type if $type; >> + $ret->{compression} = $compression if $compression; >> >> return $ret; >> }}); > Sorry for the small issues. This was my first larger feature. I will try to improve next time!