From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 9BD8BBA7AA for ; Thu, 14 Dec 2023 15:24:13 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 769D617E0D for ; Thu, 14 Dec 2023 15:23:43 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 14 Dec 2023 15:23:42 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 9DCEF4761D for ; Thu, 14 Dec 2023 15:23:42 +0100 (CET) Message-ID: Date: Thu, 14 Dec 2023 15:23:41 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Proxmox VE development discussion , Hannes Duerr References: <20231207091203.87763-1-h.duerr@proxmox.com> <20231207091203.87763-3-h.duerr@proxmox.com> From: Fiona Ebner In-Reply-To: <20231207091203.87763-3-h.duerr@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.076 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 T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH v2 qemu-server pve-storage 2/2] fix #1611: implement import of base-images for LVM-thin Storage 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: , X-List-Received-Date: Thu, 14 Dec 2023 14:24:13 -0000 Am 07.12.23 um 10:12 schrieb Hannes Duerr: > for base images we call the volume_import of the parent plugin and pass > it as vm-image instead of base-image, then convert it back as base-image > > Signed-off-by: Hannes Duerr > --- > src/PVE/Storage/LvmThinPlugin.pm | 60 ++++++++++++++++++++++++++++++++ > 1 file changed, 60 insertions(+) > > diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm > index 1d2e37c..6c95919 100644 > --- a/src/PVE/Storage/LvmThinPlugin.pm > +++ b/src/PVE/Storage/LvmThinPlugin.pm > @@ -383,6 +383,66 @@ sub volume_has_feature { > return undef; > } > > +sub volume_import { > + my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $base_snapshot, $with_snapshots, $allow_rename) = @_; > + die "volume import format $format not available for $class\n" > + if $format ne 'raw+size'; > + die "cannot import volumes together with their snapshots in $class\n" > + if $with_snapshots; > + die "cannot import an incremental stream in $class\n" if defined($base_snapshot); > + Nit: Not really hurting, but since we use the parent's import method, I think we can be fine relying on the checks there. If the parent's method gains support for more in the future and can drop a check, we'd just need to remove the check here too. > + my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $file_format) = > + $class->parse_volname($volname); > + die "cannot import format $format into a file of format $file_format\n" > + if $file_format ne 'raw'; > + > + my $oldbasename; Nit: This variable is not used in the if branch, so declaration could be moved into the else branch. > + if (!$isBase) { > + ($storeid, $volname) = split (/:/, $class->SUPER::volume_import( Could also just be return $class->SUPER... instead of doing the split and re-assembling for return below. Also avoids overwriting the passed-in params $storeid and $volname. > + $scfg, > + $storeid, > + $fh, > + $name, I'd still pass $volname just to be more future-proof > + $format, > + $snapshot, > + $base_snapshot, > + $with_snapshots, > + $allow_rename > + )); > + } else { > + my $vg = $scfg->{vgname}; > + my $lvs = PVE::Storage::LVMPlugin::lvm_list_volumes($vg); > + if ($lvs->{$vg}->{$volname}) { > + die "volume $vg/$volname already exists\n" if !$allow_rename; > + warn "volume $vg/$volname already exists - importing with a different name\n"; > + > + $volname = $class->find_free_diskname($storeid, $scfg, $vmid);> + } else { > + $oldbasename = $volname; > + $volname =~ s/base/vm/; > + } Nit: I'd prefer to have new variables for both the temporary volname we pass to the parent's volume_import and the new volname. Since they serve different purposes. > + > + ($storeid, $volname) = split (/:/, $class->SUPER::volume_import( Using parse_volid() would be more robust than using an ad-hoc split. > + $scfg, > + $storeid, > + $fh, > + $volname, > + $format, > + $snapshot, > + $base_snapshot, > + $with_snapshots, > + $allow_rename > + )); > + > + $volname = $class->create_base($storeid, $scfg, $volname); > + if ($oldbasename) { > + $volname= $oldbasename; > + } create_base() will tell you the actual name, you should not override it with the old one. We expect it to match, but then there's no need for the assignment. If it doesn't match, you'd be returning something wrong. Or what am I missing? > + } > + > + return "$storeid:$volname"; > +} > + > # used in LVMPlugin->volume_import > sub volume_import_write { > my ($class, $input_fh, $output_file) = @_;