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

[Aaron Lauterer <a.lauterer@proxmox.com>]

Works overall. I did not test edge cases like working around the BTRFS 
plugin to have qcow2 files instead of the raw files in a subvol.

On  2024-05-27  13:42, 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..b85956e 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';

No knowing BTRFS too well, do we need that or could the built-in 
'rename' work as well? A quick test indicates that it does, but I am not 
sure if there are no other reasons I am not aware of.

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

I am not too familiar with how BTRFS and our plugin works, but looking 
at other functions, like 'alloc_image' or 'clone_image' it seems that we 
do have checks regarding the format in place. If it is not a 'raw' or 
'subvol' we call the SUPER from the main Plugin.pm instead of continuing 
in the BTRFS specific implementations that use subvols.

This seems to be missing here, but it might be that it is fine if the 
"move" is working in any way.

> +sub rename_volume {
> +    my ($class, $scfg, $storeid, $source_volname, $target_vmid, $target_volname) = @_;
> +    die "no path found\n" if !$scfg->{path};
> +
> +    my (
> +	$vtype,
> +	$source_image,
> +	$source_vmid,
> +	$base_name,
> +	$base_vmid,
> +	undef,
> +	$format
> +    ) = $class->parse_volname($source_volname);

You only seem to be using '$format' from the returned values? Then the 
others could also be 'undef' if not used as we don't need to allocate them.

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


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