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

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

[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>
---
 src/PVE/Storage/BTRFSPlugin.pm | 40 ++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index 42815cb..9f71d78 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -7,6 +7,7 @@ use base qw(PVE::Storage::Plugin);
 
 use Fcntl qw(S_ISDIR O_WRONLY O_CREAT O_EXCL);
 use File::Basename qw(basename dirname);
+use File::Copy 'move';
 use File::Path qw(mkpath);
 use IO::Dir;
 use POSIX qw(EEXIST);
@@ -618,6 +619,9 @@ sub volume_has_feature {
 	    base => { qcow2 => 1, raw => 1, vmdk => 1 },
 	    current => { qcow2 => 1, raw => 1, vmdk => 1 },
 	},
+	rename => {
+	    current => { raw => 1 },
+	},
     };
 
     my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) = $class->parse_volname($volname);
@@ -930,4 +934,40 @@ sub volume_import {
     return "$storeid:$volname";
 }
 
+sub rename_volume {
+    my ($class, $scfg, $storeid, $source_volname, $target_vmid, $target_volname) = @_;
+    die "no path found\n" if !$scfg->{path};
+
+    my (
+	undef,
+	undef,
+	undef,
+	undef,
+	undef
+	undef,
+	$format
+    ) = $class->parse_volname($source_volname);
+
+    my $ppath = $class->filesystem_path($scfg, $source_volname);
+
+    $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid, $format, 1)
+	if !$target_volname;
+    $target_volname = "$target_vmid/$target_volname";
+
+    my $basedir = $class->get_subdir($scfg, 'images');
+
+    mkpath "${basedir}/${target_vmid}";
+    my $source_dir = raw_name_to_dir($source_volname);
+    my $target_dir = raw_name_to_dir($target_volname);
+
+    my $old_path = "${basedir}/${source_dir}";
+    my $new_path = "${basedir}/${target_dir}";
+
+    die "target volume '${target_volname}' already exists\n" if -e $new_path;
+    move $old_path, $new_path ||
+	die "rename '$old_path' to '$new_path' failed - $!\n";
+
+    return "${storeid}:$target_volname";
+}
+
 1
-- 
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 storage v2] fix-4272: btrfs: add rename feature

[Thomas Lamprecht]

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.

>  src/PVE/Storage/BTRFSPlugin.pm | 40 ++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 


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

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

[Fiona Ebner]

For next time, please use the usual prefix for referencing bugzilla
issues in the commit title, i.e. "fix #4272" or since it's an
enhancement, it can also be the alternative "close #4272"


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

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

[Aaron Lauterer]

did you test if it works if a manually placed qcow2 or vmdk disk image 
is used as discussed in my previous responses and in person?

There currently are no checks in place, as there are for all the other 
functions in the BTRFS plugin that deal with creating/changing disk images.

One more thing inline.

On  2024-07-03  14:41, Maximiliano Sandoval wrote:
> 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>
> ---
>   src/PVE/Storage/BTRFSPlugin.pm | 40 ++++++++++++++++++++++++++++++++++
>   1 file changed, 40 insertions(+)
> 
> diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
> index 42815cb..9f71d78 100644
> --- a/src/PVE/Storage/BTRFSPlugin.pm
> +++ b/src/PVE/Storage/BTRFSPlugin.pm
> @@ -7,6 +7,7 @@ use base qw(PVE::Storage::Plugin);
>   
>   use Fcntl qw(S_ISDIR O_WRONLY O_CREAT O_EXCL);
>   use File::Basename qw(basename dirname);
> +use File::Copy 'move';
>   use File::Path qw(mkpath);
>   use IO::Dir;
>   use POSIX qw(EEXIST);
> @@ -618,6 +619,9 @@ sub volume_has_feature {
>   	    base => { qcow2 => 1, raw => 1, vmdk => 1 },
>   	    current => { qcow2 => 1, raw => 1, vmdk => 1 },
>   	},
> +	rename => {
> +	    current => { raw => 1 },
> +	},
>       };
>   
>       my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) = $class->parse_volname($volname);
> @@ -930,4 +934,40 @@ sub volume_import {
>       return "$storeid:$volname";
>   }
>   
> +sub rename_volume {
> +    my ($class, $scfg, $storeid, $source_volname, $target_vmid, $target_volname) = @_;
> +    die "no path found\n" if !$scfg->{path};
> +
> +    my (
> +	undef,
> +	undef,
> +	undef,
> +	undef,
> +	undef
> +	undef,
> +	$format
> +    ) = $class->parse_volname($source_volname);
> +
> +    my $ppath = $class->filesystem_path($scfg, $source_volname);
> +
> +    $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid, $format, 1)
> +	if !$target_volname;
> +    $target_volname = "$target_vmid/$target_volname";
> +
> +    my $basedir = $class->get_subdir($scfg, 'images');
> +
> +    mkpath "${basedir}/${target_vmid}";
> +    my $source_dir = raw_name_to_dir($source_volname);
> +    my $target_dir = raw_name_to_dir($target_volname);
> +
> +    my $old_path = "${basedir}/${source_dir}";
> +    my $new_path = "${basedir}/${target_dir}";
> +
> +    die "target volume '${target_volname}' already exists\n" if -e $new_path;
> +    move $old_path, $new_path ||
did you try if it would just work with 'rename' like in the main 
implementation used in `Plugin.pm`?
Then we could drop the whole import of File::Copy::move

> +	die "rename '$old_path' to '$new_path' failed - $!\n";
> +
> +    return "${storeid}:$target_volname";
> +}
> +
>   1


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

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

[Thomas Lamprecht]

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