[pve-devel] [PATCH manager] ui: util: simplify volume_is_qemu_backup again

[pve-devel] [PATCH manager] ui: util: simplify volume_is_qemu_backup again

[Fiona Ebner]

Commit f8087e0f ("ui: fix regression with checking if volume is QEMU
backup") opted for making the function support multiple types of
callers making the function more complex than it needs to be. Simply
adapt the rest of the call sites that the commit introducing the
regression missed, i.e. commit 3f8246030 ("ui: backup: also check for
backup subtype to classify archive").

By always checking the subtype, this also makes the function work
correctly should there ever be another storage type supporting file
restore  with different format names than PBS or volid patterns than
directory storages.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 www/manager6/Utils.js              | 15 +++------------
 www/manager6/grid/BackupView.js    |  2 +-
 www/manager6/storage/BackupView.js |  2 +-
 3 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 1f6778cd..63f6b2ce 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -694,18 +694,9 @@ Ext.define('PVE.Utils', {
 	'import': gettext('Import'),
     },
 
-     // volume can be a full volume info object, in which case the format parameter is ignored, or
-     // you can pass the volume ID and format as separate string parameters.
-    volume_is_qemu_backup: function(volume, format) {
-	let volid, subtype;
-	if (typeof volume === 'string') {
-	    volid = volume;
-	} else if (typeof volume === 'object') {
-	    ({ volid, format, subtype } = volume);
-	} else {
-	    console.error("internal error - unexpected type", volume);
-	}
-	return format === 'pbs-vm' || volid.match(':backup/vzdump-qemu-') || subtype === 'qemu';
+    volume_is_qemu_backup: function(volume) {
+	return volume.format === 'pbs-vm' || volume.volid.match(':backup/vzdump-qemu-') ||
+	    volume.subtype === 'qemu';
     },
 
     volume_is_lxc_backup: function(volume) {
diff --git a/www/manager6/grid/BackupView.js b/www/manager6/grid/BackupView.js
index 0f68a25e..610dda8d 100644
--- a/www/manager6/grid/BackupView.js
+++ b/www/manager6/grid/BackupView.js
@@ -244,7 +244,7 @@ Ext.define('PVE.grid.BackupView', {
 	    hidden: !isPBS,
 	    handler: function(b, e, rec) {
 		let storage = storagesel.getValue();
-		let isVMArchive = PVE.Utils.volume_is_qemu_backup(rec.data.volid, rec.data.format);
+		let isVMArchive = PVE.Utils.volume_is_qemu_backup(rec.data);
 		Ext.create('Proxmox.window.FileBrowser', {
 		    title: gettext('File Restore') + " - " + rec.data.text,
 		    listURL: `/api2/json/nodes/localhost/storage/${storage}/file-restore/list`,
diff --git a/www/manager6/storage/BackupView.js b/www/manager6/storage/BackupView.js
index 749c2136..1247be81 100644
--- a/www/manager6/storage/BackupView.js
+++ b/www/manager6/storage/BackupView.js
@@ -115,7 +115,7 @@ Ext.define('PVE.storage.BackupView', {
 		disabled: true,
 		selModel: sm,
 		handler: function(b, e, rec) {
-		    let isVMArchive = PVE.Utils.volume_is_qemu_backup(rec.data.volid, rec.data.format);
+		    let isVMArchive = PVE.Utils.volume_is_qemu_backup(rec.data);
 		    Ext.create('Proxmox.window.FileBrowser', {
 			title: gettext('File Restore') + " - " + rec.data.text,
 			listURL: `/api2/json/nodes/localhost/storage/${me.storage}/file-restore/list`,
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH manager] ui: util: simplify volume_is_qemu_backup again

[Fiona Ebner]

Am 10.04.25 um 10:12 schrieb Thomas Lamprecht:
> Am 10.04.25 um 09:16 schrieb Fiona Ebner:
>> Commit f8087e0f ("ui: fix regression with checking if volume is QEMU
>> backup") opted for making the function support multiple types of
>> callers making the function more complex than it needs to be. Simply
> 
> it's a simple if/else though, so complex is really relative.

Yes, I used a relative statement: "more than it needs to be".

>> adapt the rest of the call sites that the commit introducing the
>> regression missed, i.e. commit 3f8246030 ("ui: backup: also check for
>> backup subtype to classify archive").
>>
>> By always checking the subtype, this also makes the function work
>> correctly should there ever be another storage type supporting file
>> restore  with different format names than PBS or volid patterns than
>> directory storages.
> 
> FWIW I weighted both options when fixing this and used the central
> one by choice for more flexibility, passing opaque objects is a bit
> of a later move at best IMO move to a single string per clearly named
> parameter, or does yours have any advantage?
> 
> Some actual type that can be checked at a compile time would be a
> benefit, but that's another language story.

Agreed and I get your point. It just felt more natural to go with the
object approach since all call sites have that very object and would
need to do the very same deconstruction if going with function(volume,
format, subtype). Independent of the chosen approach, keeping it
consistent with the volume_is_lxc_backup() helper is also an advantage.

In any case, nothing urgent in this follow-up and thank you for the
quick fix!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH manager] ui: util: simplify volume_is_qemu_backup again

[Thomas Lamprecht]

Am 10.04.25 um 10:29 schrieb Fiona Ebner:
> Agreed and I get your point. It just felt more natural to go with the
> object approach since all call sites have that very object and would
> need to do the very same deconstruction if going with function(volume,
> format, subtype). Independent of the chosen approach, keeping it
> consistent with the volume_is_lxc_backup() helper is also an advantage.

Hmm, that one could be reworked too to a simpler/clearer signature.
Such methods in Utils are far from ideal in the first place, having
just an opaque signature does not really help here.

But yeah, does not really matter, one can hold it rather easily wrong
either way.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel