Re: [pve-devel] [PATCH storage v2] fix-4272: btrfs: add rename feature

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

CC'ing pve-devel again.

Am 04/07/2024 um 11:42 schrieb Maximiliano Sandoval:
> Thomas Lamprecht <t.lamprecht@proxmox.com> writes:
>> Am 03/07/2024 um 14:41 schrieb Maximiliano Sandoval:
>>> Adds the ability to change the owner of a guest image.
>>>
>>> Btrfs does not need special commands to rename a subvolume and this can
>>> be achieved the same as in Storage/plugin.pm's rename_volume taking
>>> special care of how the directory structure used by Btrfs.
>>>
>>> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
>>> ---
>>
>> this missing a v1 -> v2 changelog here.
> 
> Right.
> 
> The changelog is basically:
> 
> Differences from v1:
>  - Rename unused variables to undef
> 

Thanks for providing one, it's also very important for small/trivial
changes, as then one can know that the basics did not change, which allows
one to spent less time on reviewing the whole thing.

The changelog might be slightly easier to understand, at least to me, if
worded:
- avoid assigning unused values of returned list to variables

btw. one should be also able to do something like:

my $format = ($class->parse_volname($source_volname))[6];

here.

While not perfect, that would be IMO not really be worse than having 5
undef's bloating up code line in this case.

Orthogonal to this: adding a "parse_volume_format" (wrapper) method to
the storage plugin might really be worth it, as IIRC we have this use case
on quite a few places. But that certainly does not need to happen before
this patch.


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