From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id A30DA1FF391 for ; Wed, 12 Jun 2024 18:01:44 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E0DF714ABC; Wed, 12 Jun 2024 18:02:21 +0200 (CEST) Date: Wed, 12 Jun 2024 18:01:47 +0200 Message-Id: To: "Proxmox VE development discussion" From: "Max Carrara" Mime-Version: 1.0 X-Mailer: aerc 0.17.0-72-g6a84f1331f1c References: <20240524132209.703402-1-d.csapak@proxmox.com> <20240524132209.703402-17-d.csapak@proxmox.com> In-Reply-To: <20240524132209.703402-17-d.csapak@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.034 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [qemu.pm] Subject: Re: [pve-devel] [PATCH qemu-server v4 4/4] api: create: add 'import-extraction-storage' parameter X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On Fri May 24, 2024 at 3:22 PM CEST, Dominik Csapak wrote: > this is to override the target extraction storage for the option disk > extraction for 'import-from'. This way if the storage does not > supports the content type 'images', one can give an alternative one. > > Signed-off-by: Dominik Csapak > --- > PVE/API2/Qemu.pm | 46 +++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 37 insertions(+), 9 deletions(-) > > diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm > index 8c335ac4..80ea52c5 100644 > --- a/PVE/API2/Qemu.pm > +++ b/PVE/API2/Qemu.pm > @@ -128,7 +128,7 @@ my $check_drive_param = sub { > }; > > my $check_storage_access = sub { > - my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage) = @_; > + my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage, $extraction_storage) = @_; > > $foreach_volume_with_alloc->($settings, sub { > my ($ds, $drive) = @_; > @@ -169,9 +169,18 @@ my $check_storage_access = sub { > if $vtype ne 'images' && $vtype ne 'import'; > > if (PVE::QemuServer::Helpers::needs_extraction($vtype, $fmt)) { > - raise_param_exc({ $ds => "$src_image is not on an storage with 'images' content type."}) > - if !$scfg->{content}->{images}; > - $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']); > + if (defined($extraction_storage)) { > + my $extraction_scfg = PVE::Storage::storage_config($storecfg, $extraction_storage); > + raise_param_exc({ 'import-extraction-storage' => "$extraction_storage does not support" > + ." 'images' content type or is not file based."}) Is there perhaps a way to display "Disk images" like in the storage config drop down menu? Also, this is where the error handling could be more fine-grained so the user immediately knows what's wrong, as mentioned in my response to your cover letter. > + if !$extraction_scfg->{content}->{images} || !$extraction_scfg->{path}; Style: The `raise_param_exc` plus `if` here should IMO be indented like you did ... > + $rpcenv->check($authuser, "/storage/$extraction_storage", ['Datastore.AllocateSpace']); > + } else { > + raise_param_exc({ $ds => "$src_image is not on an storage with 'images'" s/an storage/a storage > + ." content type and no 'import-extraction-storage' was given."}) > + if !$scfg->{content}->{images}; ... here. > + $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']); > + } > } > } > > @@ -326,7 +335,7 @@ my $import_from_volid = sub { > > # Note: $pool is only needed when creating a VM, because pool permissions > # are automatically inherited if VM already exists inside a pool. > -my sub create_disks : prototype($$$$$$$$$$) { > +my sub create_disks : prototype($$$$$$$$$$$) { > my ( > $rpcenv, > $authuser, > @@ -338,6 +347,7 @@ my sub create_disks : prototype($$$$$$$$$$) { > $settings, > $default_storage, > $is_live_import, > + $extraction_storage, > ) = @_; > > my $vollist = []; > @@ -407,8 +417,8 @@ my sub create_disks : prototype($$$$$$$$$$) { > my $needs_extraction = PVE::QemuServer::Helpers::needs_extraction($vtype, $fmt); > if ($needs_extraction) { > print "extracting $source\n"; > - my $extracted_volid > - = PVE::GuestImport::extract_disk_from_import_file($source, $vmid); > + my $extracted_volid = PVE::GuestImport::extract_disk_from_import_file( > + $source, $vmid, $extraction_storage); > print "finished extracting to $extracted_volid\n"; > push @$vollist, $extracted_volid; > $source = $extracted_volid; > @@ -941,6 +951,12 @@ __PACKAGE__->register_method({ > default => 0, > description => "Start VM after it was created successfully.", > }, > + 'import-extraction-storage' => get_standard_option('pve-storage-id', { > + description => "Storage for temporarily extracted images 'import-from' image" > + ." files (default: import source storage)", > + optional => 1, > + completion => \&PVE::QemuServer::complete_storage, > + }), > }, > 1, # with_disk_alloc > ), > @@ -967,6 +983,7 @@ __PACKAGE__->register_method({ > my $storage = extract_param($param, 'storage'); > my $unique = extract_param($param, 'unique'); > my $live_restore = extract_param($param, 'live-restore'); > + my $extraction_storage = extract_param($param, 'import-extraction-storage'); > > if (defined(my $ssh_keys = $param->{sshkeys})) { > $ssh_keys = URI::Escape::uri_unescape($ssh_keys); > @@ -1026,7 +1043,8 @@ __PACKAGE__->register_method({ > if (scalar(keys $param->%*) > 0) { > &$resolve_cdrom_alias($param); > > - &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param, $storage); > + &$check_storage_access( > + $rpcenv, $authuser, $storecfg, $vmid, $param, $storage, $extraction_storage); > > &$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, $pool, [ keys %$param]); > > @@ -1141,6 +1159,7 @@ __PACKAGE__->register_method({ > $param, > $storage, > $live_restore, > + $extraction_storage > ); > $conf->{$_} = $created_opts->{$_} for keys $created_opts->%*; > > @@ -1683,6 +1702,8 @@ my $update_vm_api = sub { > > my $skip_cloud_init = extract_param($param, 'skip_cloud_init'); > > + my $extraction_storage = extract_param($param, 'import-extraction-storage'); > + > if (defined(my $cipassword = $param->{cipassword})) { > # Same logic as in cloud-init (but with the regex fixed...) > $param->{cipassword} = PVE::Tools::encrypt_pw($cipassword) > @@ -1802,7 +1823,7 @@ my $update_vm_api = sub { > > &$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, undef, [keys %$param]); > > - &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param); > + &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param, undef, $extraction_storage); Style: Should perhaps be indented like above: &$check_storage_access( $rpcenv, $authuser, $storecfg, $vmid, $param, $storage, $extraction_storage); > > PVE::QemuServer::check_bridge_access($rpcenv, $authuser, $param); > > @@ -1984,6 +2005,7 @@ my $update_vm_api = sub { > {$opt => $param->{$opt}}, > undef, > undef, > + $extraction_storage, > ); > $conf->{pending}->{$_} = $created_opts->{$_} for keys $created_opts->%*; > > @@ -2179,6 +2201,12 @@ __PACKAGE__->register_method({ > maximum => 30, > optional => 1, > }, > + 'import-extraction-storage' => get_standard_option('pve-storage-id', { > + description => "Storage for temporarily extracted images 'import-from' image" > + ." files (default: import source storage)", > + optional => 1, > + completion => \&PVE::QemuServer::complete_storage, > + }), > }, > 1, # with_disk_alloc > ), _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel