From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <pve-devel-bounces@lists.proxmox.com> Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 768CE1FF16F for <inbox@lore.proxmox.com>; Thu, 13 Feb 2025 18:21:52 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 02A5ABC7B; Thu, 13 Feb 2025 18:21:43 +0100 (CET) Message-ID: <6daf1a2a-e9ec-4c8d-834a-12d352d7db9a@proxmox.com> Date: Thu, 13 Feb 2025 18:21:08 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>, Filip Schauer <f.schauer@proxmox.com> References: <20250120112842.36450-1-f.schauer@proxmox.com> <20250120112842.36450-2-f.schauer@proxmox.com> Content-Language: en-US From: Fiona Ebner <f.ebner@proxmox.com> In-Reply-To: <20250120112842.36450-2-f.schauer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.046 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 Subject: Re: [pve-devel] [PATCH storage v6 1/7] plugin: allow volume import of iso, snippets, vztmpl and import X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com> List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe> List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/> List-Post: <mailto:pve-devel@lists.proxmox.com> List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help> List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe> Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com> Am 20.01.25 um 12:28 schrieb Filip Schauer: > Extend volume import functionality to support 'iso', 'snippets', > 'vztmpl', and 'import' types, in addition to the existing support for > 'images' and 'rootdir'. This is a prerequisite for the ability to move > ISOs, snippets and container templates between nodes. > > Existing behavior for importing VM disks and container volumes remains > unchanged. > > Signed-off-by: Filip Schauer <f.schauer@proxmox.com> > --- > src/PVE/Storage/Plugin.pm | 72 ++++++++++++++++++++++++++------------- > 1 file changed, 48 insertions(+), 24 deletions(-) > > diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm > index 65cf43f..b682362 100644 > --- a/src/PVE/Storage/Plugin.pm > +++ b/src/PVE/Storage/Plugin.pm > @@ -1738,6 +1738,8 @@ sub volume_export_formats { > my $format = ($class->parse_volname($volname))[6]; > my $size = file_size_info($file, undef, $format); > > + return ('raw+size') if !defined($format); Can we rather make this explicitly check for vtype being one of the ones we care about? That reflects more precisely what we want to test for. I'm also thinking whether we want to explicitly include the vtype in the new export format name to avoid potential type confusion down the line. I.e. if a storage wants to export an iso, it should not be importable as a snippet on the other side. We can keep the current formats for backwards-compat, but the formats for the newly supported vtypes could then be $vtype+meta, similar to the backup+size type you add. This would be more explicit and less likely to be(come) exploitable. And we could write the vtype into the header of the stream itself for extra safety. Maybe in a way that allows for some forwards-compatibility, e.g. what if ISO files get notes too at some point. Just a crude outline what the stream could be from the top of my head: number of meta-properties for each meta-property: length of property name, property name, length of property data, property data length of volume, volume data For compatibility/extensibility, the stream consumer can then handle all meta properties it knows and skip the rest (with some informational logging about what was skipped). Or we can add a marker to each property whether it is required or optional and mark all important properties that should rather lead to failure than skipping. Then outdated nodes might not be able to receive certain streams, but we have the flexibility to decide for each property. I'm sorry, it would expand the scope of the series a bit more, but I do think this will save us some headaches and give us more flexibility in the future. And it seems to be a natural generalization of what you already do for backup+size. @other devs: opinions? > + > if ($with_snapshots) { > return ($format.'+size') if ($format eq 'qcow2' || $format eq 'vmdk'); > return (); > @@ -1766,14 +1768,18 @@ sub volume_import { > > # XXX: Should we bother with conversion routines at this level? This won't > # happen without manual CLI usage, so for now we just error out... > - die "cannot import format $format into a file of format $file_format\n" > - if $data_format ne $file_format && !($data_format eq 'tar' && $file_format eq 'subvol'); > + if (($vtype eq 'images' || $vtype eq 'rootdir') && $data_format ne $file_format && > + !($data_format eq 'tar' && $file_format eq 'subvol') Style nit: multiline ifs are written differently: https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Post-If > + ) { > + die "cannot import format $format into a file of format $file_format\n"; > + } > > # Check for an existing file first since interrupting alloc_image doesn't > # free it. > my ($file) = $class->path($scfg, $volname, $storeid); > if (-e $file) { > - die "file '$file' already exists\n" if !$allow_rename; > + die "file '$file' already exists\n" > + if !$allow_rename || ($vtype ne 'images' && $vtype ne 'rootdir'); > warn "file '$file' already exists - importing with a different name\n"; > $name = undef; > } > @@ -1781,29 +1787,44 @@ sub volume_import { > my ($size) = read_common_header($fh); > $size = PVE::Storage::Common::align_size_up($size, 1024) / 1024; > > - eval { > - my $allocname = $class->alloc_image($storeid, $scfg, $vmid, $file_format, $name, $size); > - my $oldname = $volname; > - $volname = $allocname; > - if (defined($name) && $allocname ne $oldname) { > - die "internal error: unexpected allocated name: '$allocname' != '$oldname'\n"; > + if ($vtype eq 'images' || $vtype eq 'rootdir') { > + eval { > + my $allocname = $class->alloc_image($storeid, $scfg, $vmid, $file_format, $name, $size); > + my $oldname = $volname; > + $volname = $allocname; > + if (defined($name) && $allocname ne $oldname) { > + die "internal error: unexpected allocated name: '$allocname' != '$oldname'\n"; > + } > + my ($file) = $class->path($scfg, $volname, $storeid) > + or die "internal error: failed to get path to newly allocated volume $volname\n"; > + if ($data_format eq 'raw' || $data_format eq 'qcow2' || $data_format eq 'vmdk') { > + run_command(['dd', "of=$file", 'conv=sparse', 'bs=64k'], > + input => '<&'.fileno($fh)); > + } elsif ($data_format eq 'tar') { > + run_command(['tar', @COMMON_TAR_FLAGS, '-C', $file, '-xf', '-'], > + input => '<&'.fileno($fh)); > + } else { > + die "volume import format '$format' not available for $class"; > + } > + }; > + if (my $err = $@) { > + eval { $class->free_image($storeid, $scfg, $volname, 0, $file_format) }; > + warn $@ if $@; > + die $err; > } > - my ($file) = $class->path($scfg, $volname, $storeid) > - or die "internal error: failed to get path to newly allocated volume $volname\n"; > - if ($data_format eq 'raw' || $data_format eq 'qcow2' || $data_format eq 'vmdk') { > - run_command(['dd', "of=$file", 'conv=sparse', 'bs=64k'], > - input => '<&'.fileno($fh)); > - } elsif ($data_format eq 'tar') { > - run_command(['tar', @COMMON_TAR_FLAGS, '-C', $file, '-xf', '-'], > - input => '<&'.fileno($fh)); > - } else { > - die "volume import format '$format' not available for $class"; > + } elsif (grep { $vtype eq $_ } qw(import iso snippets vztmpl)) { > + eval { > + run_command(['dd', "of=$file", 'bs=64k'], input => '<&'.fileno($fh)); Please use 'conv=excl' to avoid the race of somebody/something else creating the file between the existence check above and here. > + }; > + if (my $err = $@) { > + if (-e $file) { > + eval { unlink($file) }; > + warn $@ if $@; > + } > + die $err; > } > - }; > - if (my $err = $@) { > - eval { $class->free_image($storeid, $scfg, $volname, 0, $file_format) }; > - warn $@ if $@; > - die $err; > + } else { > + die "importing volume of type '$vtype' not implemented\n"; > } > > return "$storeid:$volname"; > @@ -1813,6 +1834,9 @@ sub volume_import_formats { > my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_; > if ($scfg->{path} && !defined($base_snapshot)) { > my $format = ($class->parse_volname($volname))[6]; > + > + return ('raw+size') if !defined($format); Similar here, I'd like to be more precise and also encode the vtype in the transport format to avoid shenanigans with potential for importing with a different vtype. > + > if ($with_snapshots) { > return ($format.'+size') if ($format eq 'qcow2' || $format eq 'vmdk'); > return (); _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel