On November 10, 2021 8:40 am, Fabian Ebner wrote:
> Am 09.11.21 um 13:46 schrieb Fabian Ebner:
>> Am 05.11.21 um 14:03 schrieb Fabian Grünbichler:
>
> ---snip---
>
>>> use IO::Socket::IP;
>>> +use IO::Socket::UNIX;
>>> +use IPC::Open3;
>>> +use JSON;
>>> +use MIME::Base64;
>
> Forgot to ask: is this import needed or a left-over from development?
yes
>
> ---snip---
>
>>
>>> +
>>> + my $migration_snapshot;
>>> + if ($scfg->{type} eq 'zfspool' || $scfg->{type} eq
>>> 'btrfs') {
>>> + $migration_snapshot = '__migration__';
>>> + }
>>> +
>>> + my $volid = "$storeid:$volname";
>>> +
>>> + # find common import/export format, taken from PVE::Storage
>>> + my @import_formats =
>>> PVE::Storage::volume_import_formats($state->{storecfg}, $volid,
>>> $migration_snapshot, undef, $with_snapshots);
>>> + my @export_formats =
>>> PVE::Tools::split_list($params->{'export-formats'});
>>> + my %import_hash = map { $_ => 1 } @import_formats;
>>> + my @common = grep { $import_hash{$_} } @export_formats;
>>> + die "no matching import/export format found for storage
>>> '$storeid'\n"
>>> + if !@common;
>>> + $format = $common[0];
>>> +
>>> + my $input = IO::File->new();
>>> + my $info = IO::File->new();
>>> + my $unix = "/run/qemu-server/$vmid.storage";
>>> +
>>> + my $import_cmd = ['pvesm', 'import', $volid, $format,
>>> "unix://$unix", '-with-snapshots', $with_snapshots];
>>> + if ($params->{'allow-rename'}) {
>>> + push @$import_cmd, '-allow-rename',
>>> $params->{'allow-rename'};
>>> + }
>>> + if ($migration_snapshot) {
>>> + push @$import_cmd, '-delete-snapshot', $migration_snapshot;
>>
>> Missing '-snapshot $migration_snapshot'? While the parameter is ignored
>> by our ZFSPoolPlugin, the BTRFSPlugin aborts if it's not specified
>> AFAICS. And external plugins might require it too.
>
> That is, for the 'btrfs' format. In the patch with the export command, a
> snapshot is only used for ZFS, so it would already fail on export for
> BTRFS with 'btrfs' format. For external plugins we also don't use a
> migration snapshot in storage_migrate(), so please disregard that part.
done
>
>>
>> In general, we'll need to be careful not to introduce mismatches between
>> the import and the export parameters. Might it be better if the client
>> would pass along (most of) the parameters for the import command (which
>> basically is how it's done for the existing storage_migrate)?
>>
>
> On the other hand, that would require being very careful with input
> validation.
yeah, and since we are crossing a trust boundary here (between two
clusters) we have to be careful. if we change the export/import code, we
can always also bump the tunnel API if needed (either to selectively use
new features only if supported, or to error out early if there was a
breaking change). just passing in "pass this to `pvesm import`" is
potentially dangerous if we don't carefully validate the 'this', and
that is easier if it's structured data ;) so I'd rather do this explicit
even if it means extending two places when we change the interface.
>
> ---snip---
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
