Re: [pve-devel] [PATCH manager v8 1/2] fix #4849: api: download to storage: automatically dectect and configure compression

[Philipp Hufnagl <p.hufnagl@proxmox.com>]

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 <p.hufnagl@proxmox.com>
>> ---
>>  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!