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

[Thomas Lamprecht]

On 22.08.19 14:39, 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 images can be uploaded but might not work in VMs.
> 
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
> 
> v1 -> v2:
> * Refactored regex into variable
> 
> I left the [Ii][Ss]... since we don't know if the variable is used with //i
> or not.

1. used my instead of our scope, but used the variable in another module,
   that cannot work... Now it was poassed a undefined regex value to all
   out-of-module users and thus the ending wasn't checked at all anymore,
   effectively allowing all endings..

2. You could just added the case insensitive flag to the base regex?

perl -we 'my $re = qr/foo/i; "Foo" =~ /^$re/ or die "not matched\n"'
works just fine...

Applied, with 1. and 2. fixed as followup...

> 
>  PVE/API2/Storage/Status.pm | 4 ++--
>  PVE/Storage.pm             | 4 +++-
>  PVE/Storage/Plugin.pm      | 4 ++--
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
> index ce7e040..91946ac 100644
> --- a/PVE/API2/Storage/Status.pm
> +++ b/PVE/API2/Storage/Status.pm
> @@ -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![^/]+$PVE::Storage::iso_extension_re$!) {
> +		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..08b6e14 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -99,6 +99,8 @@ PVE::Storage::Plugin->init();
>  
>  my $UDEVADM = '/sbin/udevadm';
>  
> +my $iso_extension_re = qr/\.(?:[Ii][Ss][Oo]|[Ii][Mm][Gg])/;
> +
>  #  PVE::Storage utility functions
>  
>  sub config {
> @@ -501,7 +503,7 @@ sub path_to_volume_id {
>  		    return ('images', $info->{volid});
>  		}
>  	    }
> -	} elsif ($path =~ m!^$isodir/([^/]+\.[Ii][Ss][Oo])$!) {
> +	} elsif ($path =~ m!^$isodir/([^/]+$iso_extension_re)$!) {
>  	    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..9a419f1 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/([^/]+$PVE::Storage::iso_extension_re)$!) {
>  	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!/([^/]+$PVE::Storage::iso_extension_re)$!i;
>  
>  	    $info = { volid => "$sid:iso/$1", format => 'iso' };
>  
>