Am 18.04.24 um 11:58 schrieb Dominik Csapak: > On 4/18/24 11:55, Fiona Ebner wrote: >> >> >> Am 18.04.24 um 11:48 schrieb Dominik Csapak: >>> On 4/18/24 11:41, Fiona Ebner wrote: >>>> Am 16.04.24 um 15:19 schrieb Dominik Csapak: >>>>> @@ -391,6 +392,13 @@ my sub create_disks : prototype($$$$$$$$$$) { >>>>> $needs_creation = $live_import; >>>>> + if (PVE::Storage::copy_needs_extraction($source)) { # >>>>> needs extraction beforehand >>>>> + print "extracting $source\n"; >>>>> + $source = >>>>> PVE::Storage::extract_disk_from_import_file($source, $vmid); >>>>> + print "finished extracting to $source\n"; >>>>> + push @$delete_sources, $source; >>>>> + } >>>>> + >>>> >>>> This breaks import from an absolute path: copy_needs_extraction() >>>> expects to be called with a PVE-managed volid, so the above should be >>>> moved into the if below. >>> >>> true, that will be fixed in the next iteration since we then get a >>> pve managed volid back after extraction >>> (see my answer to the cover letter) >>> >> >> Sorry, I don't understand. The breakage is for import from an absolute >> path, because copy_needs_extraction() cannot be called on an absolute >> path. Why does it matter whether extraction returns a managed volid or >> not? >> > > sorry i was a step further along in my mind ^^ > > the reason i put it here was that we got an absolute path back, which > would have been complicated when i'd have put it in the branch > > so with my next patch i'll return a volid again and we can safely put > it there as you suggested >
Ah, I see :) >>>> >>>>> if (PVE::Storage::parse_volume_id($source, 1)) { # >>>>> PVE-managed volume >>>>> if ($live_import && $ds ne 'efidisk0') { >>>>> my $path = PVE::Storage::path($storecfg, $source) >>>>> @@ -514,13 +522,14 @@ my sub create_disks : prototype($$$$$$$$$$) { >>>>> eval { PVE::Storage::vdisk_free($storecfg, $volid); }; >>>>> warn $@ if $@; >>>>> } >>>>> + >>>>> PVE::QemuServer::Helpers::cleanup_extracted_images($delete_sources); >>>>> die $err; >>>>> } >>>>> # don't return empty import mappings >>>>> $live_import_mapping = undef if !%$live_import_mapping; >>>>> - return ($vollist, $res, $live_import_mapping); >>>>> + return ($vollist, $res, $live_import_mapping, $delete_sources); >>>>> }; >>>>> my $check_cpu_model_access = sub { >>>> >>>> The second caller of create_disks(), i.e. when updating an existing VM, >>>> is not updated to handle $delete_sources. (You can also do a disk >>>> import-from from an OVA for an existing VM). >>>> >>>> When I tested that my suspicion is correct I didn't notice initially >>>> that the temporary dirs were hidden, should we really make them so hard >>>> to find? >>> >>> see my recent answer to the cover letter, this shouldn't be an issue >>> when >>> we put the extracted image into a valid image path on the storage >>> >> >> But we should still attempt cleanup and not just ignore the >> $delete_sources for the second caller. > > of course we have to clean up for the other case, i just meant > accidentally left over images can be more easily found and deleted > > sorry for the confusion! > Sorry for not understanding ;) I see what you mean now, thanks for the explanations! _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel