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

[Fiona Ebner <f.ebner@proxmox.com>]

Am 15.11.24 um 16:17 schrieb Dominik Csapak:
> since we want to handle ova files (which are only ovf+images bundled in
> a tar file) for import, add code that handles that.
> 
> we introduce a valid volname for files contained in ovas like this:
> 
>  storage:import/archive.ova/disk-1.vmdk
> 
> by basically treating the last part of the path as the name for the
> contained disk we want.
> 
> in that case we return 'import' as type with 'vmdk/qcow2/raw' as format
> (we cannot use something like 'ova+vmdk' without extending the 'format'
> parsing to that for all storages/formats. This is because it runs
> though a verify format check at least once)
> 
> we then provide a function to use for that:
> 
> * extract_disk_from_import_file: this actually extracts the file from
>   the archive. Currently only ova is supported, so the extraction with
>   'tar' is hardcoded, but again we can easily extend/modify that should
>   we need to.
> 
>   we currently extract into the either the import storage or a given
>   target storage in the images directory so if the cleanup does not
>   happen, the user can still see and interact with the image via
>   api/cli/gui
> 
> 
> we have to modify the `parse_ovf` a bit to handle the missing disk
> images, and we parse the size out of the ovf part (since this is
> informal only, it should be no problem if we cannot parse it sometimes)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>

One minor nit below, but:

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 3655e6a..eed764d 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -663,6 +663,10 @@ sub parse_volname {
>  	return ('backup', $fn);
>      } elsif ($volname =~ m!^snippets/([^/]+)$!) {
>  	return ('snippets', $1);
> +    } elsif ($volname =~ m!^import/(${PVE::Storage::SAFE_CHAR_CLASS_RE}+\.ova\/${PVE::Storage::OVA_CONTENT_RE_1})$!) {
> +	my $archive = $1;

Nit: That's the volname for the disk inside the archive, so 'archive' is
not the best variable name.

> +	my $format = $2;
> +	return ('import', $archive, undef, undef, undef, undef, "ova+$format");
>      } elsif ($volname =~ m!^import/(${PVE::Storage::SAFE_CHAR_CLASS_RE}+$PVE::Storage::IMPORT_EXT_RE_1)$!) {
>  	return ('import', $1, undef, undef, undef, undef, $2);
>      }


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