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.scha...@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