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!