[pve-devel] [PATCH manager v8 0/2] fix #4849: allow download of compressed ISOs

[pve-devel] [PATCH manager v8 0/2] fix #4849: allow download of compressed ISOs

[Philipp Hufnagl]

Many web pages offer the download of the disk images compressed. This
patch allows the download of archives (like .gz), automatically detects
the format and decompresses it

Changes since v7:
  * added detect-compression API parameter again

Changes since v6:
  * remove detect-compression API parameter
  * add compression field with cbind

Changes since v5:
  * split manager patch into frontend/backend
  * remove not needed regex group

Changes since v4:
  * add commit messages
  * fix nit

Changes since v3:
  * generate compression regex from compression list
  * fix logic errors

Changes since v2:
  * move compression code to the download function in common
  * minor code improvements


Changes since v1:
  * Improve code quality as suggested by feedbackfix #4849: allow download of compressed ISOs

Many web pages offer the download of the disk images compressed. This
patch allows the download of archives (like .gz), automatically detects
the format and decompresses it

Changes since v7:
  * added detect-compression API parameter again

Changes since v6:
  * remove detect-compression API parameter
  * add compression field with cbind

Changes since v5:
  * split manager patch into frontend/backend
  * remove not needed regex group

Changes since v4:
  * add commit messages
  * fix nit

Changes since v3:
  * generate compression regex from compression list
  * fix logic errors

Changes since v2:
  * move compression code to the download function in common
  * minor code improvements


Changes since v1:
  * Improve code quality as suggested by feedback
Philipp Hufnagl (2):
  fix #4849: api: download to storage: automatically dectect and
    configure compression
  fix #4849: ui: download to storage: automatically dectect and
    configure compression

 PVE/API2/Nodes.pm                           | 22 ++++++++++++++++++++-
 www/manager6/Makefile                       |  1 +
 www/manager6/form/DecompressionSelector.js  | 13 ++++++++++++
 www/manager6/window/DownloadUrlToStorage.js | 14 ++++++++++++-
 4 files changed, 48 insertions(+), 2 deletions(-)
 create mode 100644 www/manager6/form/DecompressionSelector.js

-- 
2.39.2

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

[Philipp Hufnagl]

extends the query_url_metadata callback with the functionallity to
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",
+		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;
 
 	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;
 	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})$!) {
+	    $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;
     }});
-- 
2.39.2

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

[Philipp Hufnagl]

extends the download iso prompt with a "compression algorithm" drop down
under advanced. User can configure there if a decompression algorithm
should be used from the storage backend. The compression algorithm will
be automatically guessed when calling query_url_metadata

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 www/manager6/Makefile                       |  1 +
 www/manager6/form/DecompressionSelector.js  | 13 +++++++++++++
 www/manager6/window/DownloadUrlToStorage.js | 14 +++++++++++++-
 3 files changed, 27 insertions(+), 1 deletion(-)
 create mode 100644 www/manager6/form/DecompressionSelector.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 59a5d8a7..87e66ece 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -34,6 +34,7 @@ JSSRC= 							\
 	form/ContentTypeSelector.js			\
 	form/ControllerSelector.js			\
 	form/DayOfWeekSelector.js			\
+	form/DecompressionSelector.js			\
 	form/DiskFormatSelector.js			\
 	form/DiskStorageSelector.js			\
 	form/FileSelector.js				\
diff --git a/www/manager6/form/DecompressionSelector.js b/www/manager6/form/DecompressionSelector.js
new file mode 100644
index 00000000..abd19316
--- /dev/null
+++ b/www/manager6/form/DecompressionSelector.js
@@ -0,0 +1,13 @@
+Ext.define('PVE.form.DecompressionSelector', {
+    extend: 'Proxmox.form.KVComboBox',
+    alias: ['widget.pveDecompressionSelector'],
+    config: {
+	deleteEmpty: false,
+    },
+    comboItems: [
+		['__default__', Proxmox.Utils.NoneText],
+		['lzo', 'LZO'],
+		['gz', 'GZIP'],
+		['zst', 'ZSTD'],
+    ],
+});
diff --git a/www/manager6/window/DownloadUrlToStorage.js b/www/manager6/window/DownloadUrlToStorage.js
index 90320da4..36ad13fa 100644
--- a/www/manager6/window/DownloadUrlToStorage.js
+++ b/www/manager6/window/DownloadUrlToStorage.js
@@ -66,6 +66,7 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
 		params: {
 		    url: queryParam.url,
 		    'verify-certificates': queryParam['verify-certificates'],
+		    'detect-compression': view.content === 'iso' ? 1 : 0,
 		},
 		waitMsgTarget: view,
 		failure: res => {
@@ -84,6 +85,7 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
 			filename: data.filename || "",
 			size: (data.size && Proxmox.Utils.format_size(data.size)) || gettext("Unknown"),
 			mimetype: data.mimetype || gettext("Unknown"),
+			compression: data.compression || '__default__',
 		    });
 		},
 	    });
@@ -203,6 +205,17 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
 			change: 'setQueryEnabled',
 		    },
 		},
+		{
+		    xtype: 'pveDecompressionSelector',
+		    name: 'compression',
+		    fieldLabel: gettext('Decompression algorithm'),
+		    allowBlank: true,
+		    hasNoneOption: true,
+		    value: '__default__',
+		    cbind: {
+			hidden: get => get('content') !== 'iso',
+		    },
+		},
 	    ],
 	},
 	{
@@ -223,7 +236,6 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
 	if (!me.storage) {
 	    throw "no storage ID specified";
 	}
-
 	me.callParent();
     },
 });
-- 
2.39.2

Re: [pve-devel] [PATCH manager v8 0/2] fix #4849: allow download of compressed ISOs

[Dominik Csapak]

LGTM now, and tested fine (this time with ISOs *and* CT templates ;) )

two tiny nits:

the commit message of the first message could mention that the parameter is necessary
to differentiate between different needs for different content types, but
imho not super important now (could be fixed up e.g.)

the last hunk in the second patch seems leftover, but it only removes an empty line,
so it should do any harm.

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>

[pve-devel] applied-series: [PATCH manager v9 0/2] fix #4849: allow download of compressed ISOs

[Fabian Grünbichler]

with some commit message fixups, and d/control update folded in (and
Dominik's T+R-B).

On September 21, 2023 3:09 pm, Philipp Hufnagl wrote:
> Many web pages offer the download of the disk images compressed. This
> patch allows the download of archives (like .gz), automatically detects
> the format and decompresses it
> 
> Changes since v7:
>   * added detect-compression API parameter again
> 
> Changes since v6:
>   * remove detect-compression API parameter
>   * add compression field with cbind
> 
> Changes since v5:
>   * split manager patch into frontend/backend
>   * remove not needed regex group
> 
> Changes since v4:
>   * add commit messages
>   * fix nit
> 
> Changes since v3:
>   * generate compression regex from compression list
>   * fix logic errors
> 
> Changes since v2:
>   * move compression code to the download function in common
>   * minor code improvements
> 
> 
> Changes since v1:
>   * Improve code quality as suggested by feedbackfix #4849: allow download of compressed ISOs
> 
> Many web pages offer the download of the disk images compressed. This
> patch allows the download of archives (like .gz), automatically detects
> the format and decompresses it
> 
> Changes since v7:
>   * added detect-compression API parameter again
> 
> Changes since v6:
>   * remove detect-compression API parameter
>   * add compression field with cbind
> 
> Changes since v5:
>   * split manager patch into frontend/backend
>   * remove not needed regex group
> 
> Changes since v4:
>   * add commit messages
>   * fix nit
> 
> Changes since v3:
>   * generate compression regex from compression list
>   * fix logic errors
> 
> Changes since v2:
>   * move compression code to the download function in common
>   * minor code improvements
> 
> 
> Changes since v1:
>   * Improve code quality as suggested by feedback
> Philipp Hufnagl (2):
>   fix #4849: api: download to storage: automatically dectect and
>     configure compression
>   fix #4849: ui: download to storage: automatically dectect and
>     configure compression
> 
>  PVE/API2/Nodes.pm                           | 22 ++++++++++++++++++++-
>  www/manager6/Makefile                       |  1 +
>  www/manager6/form/DecompressionSelector.js  | 13 ++++++++++++
>  www/manager6/window/DownloadUrlToStorage.js | 14 ++++++++++++-
>  4 files changed, 48 insertions(+), 2 deletions(-)
>  create mode 100644 www/manager6/form/DecompressionSelector.js
> 
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

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

[Thomas Lamprecht]

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

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;
>      }});

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

[Thomas Lamprecht]

Am 21/09/2023 um 15:09 schrieb Philipp Hufnagl:
> extends the download iso prompt with a "compression algorithm" drop down
> under advanced. User can configure there if a decompression algorithm
> should be used from the storage backend. The compression algorithm will
> be automatically guessed when calling query_url_metadata
> 
> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
>  www/manager6/Makefile                       |  1 +
>  www/manager6/form/DecompressionSelector.js  | 13 +++++++++++++
>  www/manager6/window/DownloadUrlToStorage.js | 14 +++++++++++++-
>  3 files changed, 27 insertions(+), 1 deletion(-)
>  create mode 100644 www/manager6/form/DecompressionSelector.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index 59a5d8a7..87e66ece 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -34,6 +34,7 @@ JSSRC= 							\
>  	form/ContentTypeSelector.js			\
>  	form/ControllerSelector.js			\
>  	form/DayOfWeekSelector.js			\
> +	form/DecompressionSelector.js			\
>  	form/DiskFormatSelector.js			\
>  	form/DiskStorageSelector.js			\
>  	form/FileSelector.js				\
> diff --git a/www/manager6/form/DecompressionSelector.js b/www/manager6/form/DecompressionSelector.js
> new file mode 100644
> index 00000000..abd19316
> --- /dev/null
> +++ b/www/manager6/form/DecompressionSelector.js
> @@ -0,0 +1,13 @@
> +Ext.define('PVE.form.DecompressionSelector', {
> +    extend: 'Proxmox.form.KVComboBox',
> +    alias: ['widget.pveDecompressionSelector'],
> +    config: {
> +	deleteEmpty: false,
> +    },
> +    comboItems: [
> +		['__default__', Proxmox.Utils.NoneText],
> +		['lzo', 'LZO'],
> +		['gz', 'GZIP'],
> +		['zst', 'ZSTD'],
> +    ],
> +});

For such small things, that have no other use sites, I normally
prefer to just define the values inline..

And finding other use sites for such things is not trivial, as that
would mean we will couple the values that both sites can understand,
which for compression might not be the case (e.g., in the future we
might want to support downloading bz2 here, but not want to support
it for backups).

> diff --git a/www/manager6/window/DownloadUrlToStorage.js b/www/manager6/window/DownloadUrlToStorage.js
> index 90320da4..36ad13fa 100644
> --- a/www/manager6/window/DownloadUrlToStorage.js
> +++ b/www/manager6/window/DownloadUrlToStorage.js
> @@ -66,6 +66,7 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
>  		params: {
>  		    url: queryParam.url,
>  		    'verify-certificates': queryParam['verify-certificates'],
> +		    'detect-compression': view.content === 'iso' ? 1 : 0,
>  		},
>  		waitMsgTarget: view,
>  		failure: res => {
> @@ -84,6 +85,7 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
>  			filename: data.filename || "",
>  			size: (data.size && Proxmox.Utils.format_size(data.size)) || gettext("Unknown"),
>  			mimetype: data.mimetype || gettext("Unknown"),
> +			compression: data.compression || '__default__',
>  		    });
>  		},
>  	    });
> @@ -203,6 +205,17 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
>  			change: 'setQueryEnabled',
>  		    },
>  		},
> +		{
> +		    xtype: 'pveDecompressionSelector',
> +		    name: 'compression',
> +		    fieldLabel: gettext('Decompression algorithm'),
> +		    allowBlank: true,
> +		    hasNoneOption: true,
> +		    value: '__default__',
> +		    cbind: {
> +			hidden: get => get('content') !== 'iso',
> +		    },
> +		},
>  	    ],
>  	},
>  	{
> @@ -223,7 +236,6 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
>  	if (!me.storage) {
>  	    throw "no storage ID specified";
>  	}
> -

please avoid unrelated change in the future.

>  	me.callParent();
>      },
>  });

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

[Philipp Hufnagl]

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!

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

[Philipp Hufnagl]

On 9/26/23 12:59, Thomas Lamprecht wrote:
> Am 21/09/2023 um 15:09 schrieb Philipp Hufnagl:
>> extends the download iso prompt with a "compression algorithm" drop down
>> under advanced. User can configure there if a decompression algorithm
>> should be used from the storage backend. The compression algorithm will
>> be automatically guessed when calling query_url_metadata
>>
>> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
>> ---
>>  www/manager6/Makefile                       |  1 +
>>  www/manager6/form/DecompressionSelector.js  | 13 +++++++++++++
>>  www/manager6/window/DownloadUrlToStorage.js | 14 +++++++++++++-
>>  3 files changed, 27 insertions(+), 1 deletion(-)
>>  create mode 100644 www/manager6/form/DecompressionSelector.js
>>
>> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
>> index 59a5d8a7..87e66ece 100644
>> --- a/www/manager6/Makefile
>> +++ b/www/manager6/Makefile
>> @@ -34,6 +34,7 @@ JSSRC= 							\
>>  	form/ContentTypeSelector.js			\
>>  	form/ControllerSelector.js			\
>>  	form/DayOfWeekSelector.js			\
>> +	form/DecompressionSelector.js			\
>>  	form/DiskFormatSelector.js			\
>>  	form/DiskStorageSelector.js			\
>>  	form/FileSelector.js				\
>> diff --git a/www/manager6/form/DecompressionSelector.js b/www/manager6/form/DecompressionSelector.js
>> new file mode 100644
>> index 00000000..abd19316
>> --- /dev/null
>> +++ b/www/manager6/form/DecompressionSelector.js
>> @@ -0,0 +1,13 @@
>> +Ext.define('PVE.form.DecompressionSelector', {
>> +    extend: 'Proxmox.form.KVComboBox',
>> +    alias: ['widget.pveDecompressionSelector'],
>> +    config: {
>> +	deleteEmpty: false,
>> +    },
>> +    comboItems: [
>> +		['__default__', Proxmox.Utils.NoneText],
>> +		['lzo', 'LZO'],
>> +		['gz', 'GZIP'],
>> +		['zst', 'ZSTD'],
>> +    ],
>> +});
> 
> For such small things, that have no other use sites, I normally
> prefer to just define the values inline..
> 
> And finding other use sites for such things is not trivial, as that
> would mean we will couple the values that both sites can understand,
> which for compression might not be the case (e.g., in the future we
> might want to support downloading bz2 here, but not want to support
> it for backups).
> 
>> diff --git a/www/manager6/window/DownloadUrlToStorage.js b/www/manager6/window/DownloadUrlToStorage.js
>> index 90320da4..36ad13fa 100644
>> --- a/www/manager6/window/DownloadUrlToStorage.js
>> +++ b/www/manager6/window/DownloadUrlToStorage.js
>> @@ -66,6 +66,7 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
>>  		params: {
>>  		    url: queryParam.url,
>>  		    'verify-certificates': queryParam['verify-certificates'],
>> +		    'detect-compression': view.content === 'iso' ? 1 : 0,
>>  		},
>>  		waitMsgTarget: view,
>>  		failure: res => {
>> @@ -84,6 +85,7 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
>>  			filename: data.filename || "",
>>  			size: (data.size && Proxmox.Utils.format_size(data.size)) || gettext("Unknown"),
>>  			mimetype: data.mimetype || gettext("Unknown"),
>> +			compression: data.compression || '__default__',
>>  		    });
>>  		},
>>  	    });
>> @@ -203,6 +205,17 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
>>  			change: 'setQueryEnabled',
>>  		    },
>>  		},
>> +		{
>> +		    xtype: 'pveDecompressionSelector',
>> +		    name: 'compression',
>> +		    fieldLabel: gettext('Decompression algorithm'),
>> +		    allowBlank: true,
>> +		    hasNoneOption: true,
>> +		    value: '__default__',
>> +		    cbind: {
>> +			hidden: get => get('content') !== 'iso',
>> +		    },
>> +		},
>>  	    ],
>>  	},
>>  	{
>> @@ -223,7 +236,6 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
>>  	if (!me.storage) {
>>  	    throw "no storage ID specified";
>>  	}
>> -
> 
> please avoid unrelated change in the future.
> 
>>  	me.callParent();
>>      },
>>  });
> 

Sorry for the issues. This was my first larger feature. I will
try to improve next time!

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

[Thomas Lamprecht]

Am 26/09/2023 um 14:25 schrieb Philipp Hufnagl:
> 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.

I'm not sure if I understand, I thought that's why we made the link
metadata- query API in the first place (which I obv. do not want to drop
in general)?

As we got the correct (from the PVE node's POV) resolved filename
returned by the metadata query API, so we can just do the regex string
match for detecting a possible compression file extension on that in the
frontend after that API call returns.

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

[Philipp Hufnagl]

On 9/26/23 16:23, Thomas Lamprecht wrote:
> Am 26/09/2023 um 14:25 schrieb Philipp Hufnagl:
>> 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.
> 
> I'm not sure if I understand, I thought that's why we made the link
> metadata- query API in the first place (which I obv. do not want to drop
> in general)?
> 
> As we got the correct (from the PVE node's POV) resolved filename
> returned by the metadata query API, so we can just do the regex string
> match for detecting a possible compression file extension on that in the
> frontend after that API call returns.
> 

Yes that would have been possible, however it would not have saved an
API call since the call is needed anyway. I did it there because I
considered it a cleaner solution to do all handling of metadata in one
place rather then returning a "filename" that has to be further
processed in "filename" and "compression".

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

[Thomas Lamprecht]

Am 26/09/2023 um 16:54 schrieb Philipp Hufnagl:
> On 9/26/23 16:23, Thomas Lamprecht wrote:
>> Am 26/09/2023 um 14:25 schrieb Philipp Hufnagl:
>>> 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.
>>
>> I'm not sure if I understand, I thought that's why we made the link
>> metadata- query API in the first place (which I obv. do not want to drop
>> in general)?
>>
>> As we got the correct (from the PVE node's POV) resolved filename
>> returned by the metadata query API, so we can just do the regex string
>> match for detecting a possible compression file extension on that in the
>> frontend after that API call returns.
>>
> 
> Yes that would have been possible, however it would not have saved an
> API call since the call is needed anyway. I did it there because I
> considered it a cleaner solution to do all handling of metadata in one
> place rather then returning a "filename" that has to be further
> processed in "filename" and "compression".

I never implied that it would save an API call, just that it would
avoid adding unecesarry parameters and return values, bloating up
our API schema for a trivial string match that any client can do
themselves..

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

[Philipp Hufnagl]

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

I will make a new patch resolving this issue. While I am on it, I see
what I can resolve on the other issues.

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

[Thomas Lamprecht]

Am 27/09/2023 um 10:03 schrieb Philipp Hufnagl:
>> 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)
> 
> I will make a new patch resolving this issue. While I am on it, I see
> what I can resolve on the other issues.

FYI, I have a pretty much done patch for this already here, so if
you did not have anything (close to) already finished, I could push
that out – just mentioning to avoid potential duplicate work.

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

[Philipp Hufnagl]

On 9/27/23 10:33, Thomas Lamprecht wrote:
> Am 27/09/2023 um 10:03 schrieb Philipp Hufnagl:
>>> 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)
>>
>> I will make a new patch resolving this issue. While I am on it, I see
>> what I can resolve on the other issues.
> 
> FYI, I have a pretty much done patch for this already here, so if
> you did not have anything (close to) already finished, I could push
> that out – just mentioning to avoid potential duplicate work.


I have nothing close. Sorry for the inconvenience and thank you for
fixing.

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

[Thomas Lamprecht]

Am 27/09/2023 um 10:57 schrieb Philipp Hufnagl:
> On 9/27/23 10:33, Thomas Lamprecht wrote:
>> FYI, I have a pretty much done patch for this already here, so if
>> you did not have anything (close to) already finished, I could push
>> that out – just mentioning to avoid potential duplicate work.
> 
> 
> I have nothing close. Sorry for the inconvenience and thank you for
> fixing.

OK, I now applied below change for doing this in the frontend and
reverted the API change. FWIW, this is not 100% semantically the same,
as I now try to match the file-extension with the case-insensitive flag
to allow matching a, e.g., ISO.GZ too.

----8<----
diff --git a/www/manager6/window/DownloadUrlToStorage.js b/www/manager6/window/DownloadUrlToStorage.js
index 481cb2ed..335d6aa6 100644
--- a/www/manager6/window/DownloadUrlToStorage.js
+++ b/www/manager6/window/DownloadUrlToStorage.js
@@ -66,7 +66,6 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
                params: {
                    url: queryParam.url,
                    'verify-certificates': queryParam['verify-certificates'],
-                   'detect-compression': view.content === 'iso' ? 1 : 0,
                },
                waitMsgTarget: view,
                failure: res => {
@@ -81,11 +80,22 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
                    urlField.validate();
 
                    let data = res.result.data;
+
+                   let filename = data.filename || "";
+                   let compression = '__default__';
+                   if (view.content === 'iso') {
+                       const matches = filename.match(/^(.+)\.(gz|lzo|zst)$/i);
+                       if (matches) {
+                           filename = matches[1];
+                           compression = matches[2];
+                       }
+                   }
+
                    view.setValues({
-                       filename: data.filename || "",
+                       filename,
+                       compression,
                        size: (data.size && Proxmox.Utils.format_size(data.size)) || gettext("Unknown"),
                        mimetype: data.mimetype || gettext("Unknown"),
-                       compression: data.compression || '__default__',
                    });
                },
            });