Re: [pve-devel] [PATCH qemu-server 07/10] mtunnel: add API endpoints

[Fabian Ebner <f.ebner@proxmox.com>]

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?

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

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

---snip---