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)
> }),
> 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.
> + 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?
> +
> + 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
