Re: [pve-devel] [PATCH storage v3 03/10] plugin: dir: handle ova files for import

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On April 29, 2024 1:21 pm, Dominik Csapak wrote:
> diff --git a/src/PVE/GuestImport.pm b/src/PVE/GuestImport.pm
> new file mode 100644
> index 0000000..d405e30
> --- /dev/null
> +++ b/src/PVE/GuestImport.pm
> @@ -0,0 +1,100 @@
> +package PVE::GuestImport;
> +
> +use strict;
> +use warnings;
> +
> +use File::Path;
> +
> +use PVE::Storage;
> +use PVE::Tools qw(run_command);
> +
> +sub copy_needs_extraction {
> +    my ($volid) = @_;
> +    my $cfg = PVE::Storage::config();
> +    my ($vtype, $name, undef, undef, undef, undef, $fmt) = PVE::Storage::parse_volname($cfg, $volid);
> +
> +    # only volumes inside ovas need extraction
> +    return $vtype eq 'import' && $fmt =~ m/^ova\+(.*)$/;
> +}
> +
> +sub extract_disk_from_import_file {
> +    my ($volid, $vmid, $target_storeid) = @_;
> +
> +    my ($source_storeid, $volname) = PVE::Storage::parse_volume_id($volid);
> +    $target_storeid //= $source_storeid;
> +    my $cfg = PVE::Storage::config();
> +    my $source_scfg = PVE::Storage::storage_config($cfg, $source_storeid);
> +    my $source_plugin = PVE::Storage::Plugin->lookup($source_scfg->{type});
> +
> +    my ($vtype, $name, undef, undef, undef, undef, $fmt) =
> +	$source_plugin->parse_volname($volname);
> +
> +    die "only files with content type 'import' can be extracted\n"
> +	if $vtype ne 'import' || $fmt !~ m/^ova\+/;
> +
> +    # extract the inner file from the name
> +    my $archive;
> +    my $inner_file;
> +    if ($name =~ m!^(.*\.ova)/(${PVE::Storage::SAFE_CHAR_CLASS_RE}+)$!) {
> +	$archive = "import/$1";
> +	$inner_file = $2;
> +	($fmt) = $fmt =~ /^ova\+(.*)$/;
> +    } else {
> +	die "cannot extract $volid - invalid volname $volname\n";
> +    }
> +
> +    my ($ova_path) = $source_plugin->path($source_scfg, $archive, $source_storeid);
> +
> +    my $target_scfg = PVE::Storage::storage_config($cfg, $target_storeid);
> +    my $target_plugin = PVE::Storage::Plugin->lookup($target_scfg->{type});
> +
> +    my $destdir = $target_plugin->get_subdir($target_scfg, 'images');
> +
> +    my $pid = $$;
> +    $destdir .= "/tmp_${pid}_${vmid}";
> +    mkpath $destdir;
> +
> +    ($ova_path) = $ova_path =~ m|^(.*)$|; # untaint
> +
> +    my $source_path = "$destdir/$inner_file";
> +    my $target_path;
> +    my $target_volname;
> +    eval {
> +	run_command(['tar', '-x', '--force-local', '-C', $destdir, '-f', $ova_path, $inner_file]);
> +
> +	# check for symlinks and other non regular files
> +	if (-l $source_path || ! -f $source_path) {
> +	    die "only regular files are allowed\n";
> +	}
> +
> +	my $target_diskname
> +	    = $target_plugin->find_free_diskname($target_storeid, $target_scfg, $vmid, $fmt, 1);

thought some more about this part. I don't think we currently consider
find_free_diskname to be public API for consumption outside of plugins
(rightfully so, IMHO).

I wonder how we could avoid that problem here. we could extend the
existing rename feature to allow moving from an arbitrary path to the
target storage (but that is risky, since it might mean we are copying
and not moving/renaming, unless we add extra checks)?

or we could handle the "temp extracted volume" as a volume, allowing a
regular PVE::Storage::rename_volume call to work?

maybe would risk breaking existing external plugins, but we could bump
APIVER and APIAGE and check APIVER to only allow storages as extraction
target that explicitly opted into it?

> +	$target_volname = "$vmid/" . $target_diskname;
> +	$target_path = $target_plugin->filesystem_path($target_scfg, $target_volname);
> +
> +	print "renaming $source_path to $target_path\n";
> +	my $imagedir = $target_plugin->get_subdir($target_scfg, 'images');
> +	mkpath "$imagedir/$vmid";
> +
> +	rename($source_path, $target_path) or die "unable to move - $!\n";
> +    };
> +    if (my $err = $@) {
> +	unlink $source_path;
> +	unlink $target_path if defined($target_path);
> +	rmdir $destdir;
> +	die "error during extraction: $err\n";
> +    }
> +
> +    rmdir $destdir;
> +
> +    return "$target_storeid:$target_volname";
> +}
> +
> +sub cleanup_extracted_image {
> +    my ($source) = @_;
> +
> +    my $cfg = PVE::Storage::config();
> +    PVE::Storage::vdisk_free($cfg, $source);
> +}
> +
> +1;


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel