On April 2, 2020 6:34 pm, Thomas Lamprecht wrote:
> On 3/30/20 1:41 PM, Fabian Grünbichler wrote:
>> the syntax is backwards compatible, providing a single storage ID or '1'
>> works like before. the new helper ensures consistent behaviour at all
>> call sites.
>
> looks OK in general, some small things/nits inside (best to read them
> bottom up ^^)
>
>>
>> Signed-off-by: Fabian Grünbichler <f.gruenbich...@proxmox.com>
>> ---
>>
>> Notes:
>> needs a versioned dep on pve-common with the new format and parse_idmap
>>
>> PVE/API2/Qemu.pm | 41 ++++++++++++++++++++++++++++-------------
>> PVE/QemuMigrate.pm | 24 +++++++++++-------------
>> PVE/QemuServer.pm | 34 ++++++++++++++++++++++++++++------
>> 3 files changed, 67 insertions(+), 32 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 788db93..6eba8d0 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> [ -- 8< -- snip -- 8< --]
>> @@ -2067,8 +2063,15 @@ __PACKAGE__->register_method({
>> my $migration_network = $get_root_param->('migration_network');
>> my $targetstorage = $get_root_param->('targetstorage');
>>
>> - raise_param_exc({ targetstorage => "targetstorage can only by used with
>> migratedfrom." })
>> - if $targetstorage && !$migratedfrom;
>> + my $storagemap;
>> +
>> + if ($targetstorage) {
>> + raise_param_exc({ targetstorage => "targetstorage can only by used
>> with migratedfrom." })
>> + if !$migratedfrom;
>
> see below for above also
>
>> + $storagemap = eval { PVE::JSONSchema::parse_idmap($targetstorage,
>> 'pve-storage-id') };
>> + raise_param_exc({ targetstorage => "failed to parse targetstorage
>> map: $@" })
>> + if $@;
>
> see below about duplicated "targetstorage" in raised exception message.
> (reviewed this one bottom up, sorry ^^)
>
>> + }
>>
>> # read spice ticket from STDIN
>> my $spice_ticket;
>
>> @@ -3385,9 +3388,7 @@ __PACKAGE__->register_method({
>> description => "Enable live storage migration for local disk",
>> optional => 1,
>> },
>> - targetstorage => get_standard_option('pve-storage-id', {
>> - description => "Default target storage.",
>> - optional => 1,
>> + targetstorage => get_standard_option('pve-targetstorage', {
>> completion => \&PVE::QemuServer::complete_migration_storage,
>
> Doing a cool completion handler for this will be fun ;) Actually, it should
> be to hard (and for intra cluster fast enough)
yes, for intra cluster it shouldn't be much trouble. I think completion
for remote cluster will be ENOTIMPLEMENTED here, that one needs a proper
multi-cluster client that then re-uses this API call ;)
>> }),
>> bwlimit => {
>> @@ -3451,8 +3452,22 @@ __PACKAGE__->register_method({
>>
>> my $storecfg = PVE::Storage::config();
>>
>> - if( $param->{targetstorage}) {
>> - PVE::Storage::storage_check_node($storecfg,
>> $param->{targetstorage}, $target);
>> + if (my $targetstorage = $param->{targetstorage}) {
>> + my $storagemap = eval {
>> PVE::JSONSchema::parse_idmap($targetstorage, 'pve-storage-id') };
>> + raise_param_exc({ targetstorage => "failed to parse targetstorage
>> map: $@" })
>
> This results in:
>
>> 400 Parameter verification failed.
>> targetstorage: failed to parse targetstorage map: $@
>
> You could thus do a s/targetstorage map/storage map/ do avoid to much
> redundancy in the raised error message.
yes, that makes sense :)
>
>> + if $@;
>> +
>> + foreach my $source (keys %{$storagemap->{entries}}) {
>> + PVE::Storage::storage_check_node($storecfg,
>> $storagemap->{entries}->{$source}, $target);
>> + }
>
> Was there any reason to not iterate over `values %hash`, like:
>
> foreach my $source (values %{$storagemap->{entries}}) {
> PVE::Storage::storage_check_node($storecfg, $source, $target);
> }
>
> for above?
none other than habit :)
>
>> +
>> + PVE::Storage::storage_check_node($storecfg, $storagemap->{default},
>> $target)
>> + if $storagemap->{default};
>> +
>> + PVE::QemuServer::check_storage_availability($storecfg, $conf,
>> $target)
>> + if $storagemap->{identity};
>> +
>> + $param->{storagemap} = $storagemap;
>> } else {
>> PVE::QemuServer::check_storage_availability($storecfg, $conf,
>> $target);
>> }
>> [ -- 8< -- snip -- 8< --]> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 1ac8643..cd534f4 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -97,6 +97,28 @@
>> PVE::JSONSchema::register_standard_option('pve-qemu-machine', {
>> optional => 1,
>> });
>>
>> +
>> +sub map_storage {
>> + my ($map, $source) = @_;
>> +
>> + return $source if !defined($map);
>> +
>> + return $map->{entries}->{$source}
>> + if defined($map->{entries}) && $map->{entries}->{$source};
>
> should/could be:
>
> if $map->{entries} && exists $map->{entries}->{$source};
>
>> +
>> + return $map->{default} if $map->{default};
>> +
>> + # identity (fallback)
>> + return $source;
>> +}
>> [ -- 8< -- snip -- 8< --]
>
>
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
