[pve-devel] [PATCH storage 1/2] fix #2216: Allow .img files in 'iso' type storages

[Dominik Csapak]

comments inline

On 8/22/19 1:35 PM, Stefan Reiter wrote:
> To maintain full (backwards) compatibility, leave the type name as
> 'iso' - this makes this patch work without changing every consumer of
> storage APIs.
> 
> Note that currently these files can only be attached as a CDROM/DVD
> drive, so USB-only imgs can be uploaded but might not work in VMs.
> 
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
> 
> Note that starting a VM with an img file attached breaks live migration to
> machines without this patch - not sure how relevant, considering this is
> a new addition anyway.
> 
> 
>   PVE/API2/Storage/Status.pm | 6 +++---
>   PVE/Storage.pm             | 2 +-
>   PVE/Storage/Plugin.pm      | 4 ++--
>   3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
> index ce7e040..09cf8b7 100644
> --- a/PVE/API2/Storage/Status.pm
> +++ b/PVE/API2/Storage/Status.pm
> @@ -350,7 +350,7 @@ __PACKAGE__->register_method ({
>       name => 'upload',
>       path => '{storage}/upload',
>       method => 'POST',
> -    description => "Upload templates and ISO images.",
> +    description => "Upload templates and ISO/IMG images.",
>       permissions => {
>   	check => ['perm', '/storage/{storage}', ['Datastore.AllocateTemplate']],
>       },
> @@ -408,8 +408,8 @@ __PACKAGE__->register_method ({
>   	my $path;
>   
>   	if ($content eq 'iso') {
> -	    if ($filename !~ m![^/]+\.[Ii][Ss][Oo]$!) {
> -		raise_param_exc({ filename => "missing '.iso' extension" });
> +	    if ($filename !~ m![^/]+\.(?:[Ii][Ss][Oo]|[Ii][Mm][Gg])$!) {

why not

m![^/]+\.(?:iso|img)$!i

in my opinion, much more readable

> +		raise_param_exc({ filename => "missing '.iso' or '.img' extension" });
>   	    }
>   	    $path = PVE::Storage::get_iso_dir($cfg, $param->{storage});
>   	} elsif ($content eq 'vztmpl') {
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 755eca8..de117a5 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -501,7 +501,7 @@ sub path_to_volume_id {
>   		    return ('images', $info->{volid});
>   		}
>   	    }
> -	} elsif ($path =~ m!^$isodir/([^/]+\.[Ii][Ss][Oo])$!) {
> +	} elsif ($path =~ m!^$isodir/([^/]+\.(?:[Ii][Ss][Oo]|[Ii][Mm][Gg]))$!) {

same here, but we would have to be carefull with $isodir, so maybe not?

>   	    my $name = $1;
>   	    return ('iso', "$sid:iso/$name");
>   	} elsif ($path =~ m!^$tmpldir/([^/]+\.tar\.gz)$!) {
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 27f832f..87a6b9a 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -415,7 +415,7 @@ sub parse_volname {
>   	my ($vmid, $name) = ($1, $2);
>   	my (undef, $format, $isBase) = parse_name_dir($name);
>   	return ('images', $name, $vmid, undef, undef, $isBase, $format);
> -    } elsif ($volname =~ m!^iso/([^/]+\.[Ii][Ss][Oo])$!) {
> +    } elsif ($volname =~ m!^iso/([^/]+\.(?:[Ii][Ss][Oo]|[Ii][Mm][Gg]))$!) {

same here but now with 'iso'...

>   	return ('iso', $1);
>       } elsif ($volname =~ m!^vztmpl/([^/]+\.tar\.[gx]z)$!) {
>   	return ('vztmpl', $1);
> @@ -915,7 +915,7 @@ my $get_subdir_files = sub {
>   	my $info;
>   
>   	if ($tt eq 'iso') {
> -	    next if $fn !~ m!/([^/]+\.iso)$!i;
> +	    next if $fn !~ m!/([^/]+\.(?:[Ii][Ss][Oo]|[Ii][Mm][Gg]))$!i;

here we even have the 'i' already

maybe the '[Ii][Ss]...' part can be refactored into a variable, so that
we do not have to update 4 places everytime we want to add an extension?
(also makes it more readable)

>   
>   	    $info = { volid => "$sid:iso/$1", format => 'iso' };
>   
>