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

[Fiona Ebner <f.ebner@proxmox.com>]

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