From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 2E51C1FF2A8 for ; Tue, 2 Jul 2024 17:24:19 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 677333152F; Tue, 2 Jul 2024 17:24:34 +0200 (CEST) Message-ID: <3719b558-83e7-41f4-a4c7-3ad3e269dd32@proxmox.com> Date: Tue, 2 Jul 2024 17:24:30 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Proxmox VE development discussion , Maximiliano Sandoval References: <20240527114219.362428-1-m.sandoval@proxmox.com> From: Aaron Lauterer In-Reply-To: <20240527114219.362428-1-m.sandoval@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.292 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [btrfsplugin.pm, plugin.pm] URI_NOVOWEL 0.5 URI hostname has long non-vowel sequence Subject: Re: [pve-devel] [PATCH storage] fix-4272: btrfs: add rename feature X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 > --- > 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