* [pve-devel] [PATCH storage] fix-4272: btrfs: add rename feature
@ 2024-05-27 11:42 Maximiliano Sandoval
2024-07-02 13:16 ` Maximiliano Sandoval
2024-07-02 15:24 ` Aaron Lauterer
0 siblings, 2 replies; 4+ messages in thread
From: Maximiliano Sandoval @ 2024-05-27 11:42 UTC (permalink / raw)
To: pve-devel
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';
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 (
+ $vtype,
+ $source_image,
+ $source_vmid,
+ $base_name,
+ $base_vmid,
+ 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH storage] fix-4272: btrfs: add rename feature
2024-05-27 11:42 [pve-devel] [PATCH storage] fix-4272: btrfs: add rename feature Maximiliano Sandoval
@ 2024-07-02 13:16 ` Maximiliano Sandoval
2024-07-02 15:24 ` Aaron Lauterer
1 sibling, 0 replies; 4+ messages in thread
From: Maximiliano Sandoval @ 2024-07-02 13:16 UTC (permalink / raw)
To: pve-devel
ping
Maximiliano Sandoval <m.sandoval@proxmox.com> writes:
> 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';
> 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 (
> + $vtype,
> + $source_image,
> + $source_vmid,
> + $base_name,
> + $base_vmid,
> + 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
--
Maximiliano
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH storage] fix-4272: btrfs: add rename feature
2024-05-27 11:42 [pve-devel] [PATCH storage] fix-4272: btrfs: add rename feature Maximiliano Sandoval
2024-07-02 13:16 ` Maximiliano Sandoval
@ 2024-07-02 15:24 ` Aaron Lauterer
[not found] ` <s8o1q4a3bbj.fsf@proxmox.com>
1 sibling, 1 reply; 4+ messages in thread
From: Aaron Lauterer @ 2024-07-02 15:24 UTC (permalink / raw)
To: Proxmox VE development discussion, Maximiliano Sandoval
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH storage] fix-4272: btrfs: add rename feature
[not found] ` <s8o1q4a3bbj.fsf@proxmox.com>
@ 2024-07-03 13:08 ` Aaron Lauterer
0 siblings, 0 replies; 4+ messages in thread
From: Aaron Lauterer @ 2024-07-03 13:08 UTC (permalink / raw)
To: Maximiliano Sandoval, PVE development discussion
On 2024-07-03 14:32, Maximiliano Sandoval wrote:
> Aaron Lauterer <a.lauterer@proxmox.com> writes:
>
>> 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.
>
> Thanks for testing!
>
[...]
>> 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.
>
> While it is true that others methods call the SUPER method, I didn't
> find any other implementation using SUPER::rename_volume.
Not surprising, as this behavior seems to be a BTRFS specific one for
the edge case that we are dealing with a regular file (non .raw) instead
of how the plugin usually wants it. AFAICT this is a safety mechanism,
just in case.
For example (IIRC): on BTRFS the layout if usually:
images/{vmid}/vm-{vmid}-disk-X/disk.raw
where the `vm-{vmid}-disk-X` part is a BTRFS subvolume. But it would
also be possible that someone (manually) builds it like a regular
directory based storage plugin:
images/{vmid}/vm-{vmid}-disk-X.qcow2
where everything is just a path/file. How well does the rename_volume
code handle this? Or should it call the SUPER method?
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-07-03 13:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-27 11:42 [pve-devel] [PATCH storage] fix-4272: btrfs: add rename feature Maximiliano Sandoval
2024-07-02 13:16 ` Maximiliano Sandoval
2024-07-02 15:24 ` Aaron Lauterer
[not found] ` <s8o1q4a3bbj.fsf@proxmox.com>
2024-07-03 13:08 ` Aaron Lauterer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox