[pve-devel] [PATCH storage v2 2/3] storage: merge archive format/compressor

[Thomas Lamprecht]

On 1/31/20 5:01 PM, Alwin Antreich wrote:
> detection into a separate function to reduce code duplication and allow
> for easier modification.
> 
> Signed-off-by: Alwin Antreich <a.antreich at proxmox.com>
> ---
>  PVE/Storage.pm | 78 ++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 57 insertions(+), 21 deletions(-)
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 1688077..390b343 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -1265,6 +1265,52 @@ sub foreach_volid {
>      }
>  }
>  
> +sub archive_info {

this mixes two things which shouldn't be mixed, IMO.
1. getting the archive info 
2. getting a decompressor from $format and $compressor

I'd split that up in two.

> +    my ($archive, $comp, $format) = @_;

With the split up you can then remove $com and $format here, they make for
a strange method signature IMO, that should be handled by the caller.
But as the single caller even passing this at all (qemu-server patch 1/2) is
interested for the decompressor only anyway the aforementioned split up
will solve that for you anyway and give a much nicer interface and two
method doing only one thing, not two mixed with edge cases.

A few other comments below, but I wrote them before I checked the overall
design. You may read them, but I suggest in doing the split up first before
all - it should solve most issues.

> +    my $type;
> +
> +    if (!defined($comp) || !defined($format)) {
> +	my $volid = basename($archive);
> +	if ($volid =~ /vzdump-(lxc|openvz|qemu)-\d+-(\d{4})_(\d{2})_(\d{2})-(\d{2})_(\d{2})_(\d{2})\.(tgz|((tar|vma)(\.(gz|lzo))?))$/) {

please use non-capturing groups, it would be great if we could get the  
$format and $compressor without the case separation below.

> +	    $type = $1;
> +
> +	    if ($8 eq 'tgz') {
> +		$format = 'tar';
> +		$comp = 'gz';
> +	    } else {
> +		$format = $10;
> +		$comp = $12 if defined($12);
> +	    }
> +	} else {
> +	    die "ERROR: couldn't determine format and compression type\n";
> +	}
> +    }
> +
> +    my $decompressor = {
> +	gz  => {
> +	    'vma' => [ "zcat", $archive ],
> +	    'tar' => [ "tar", "-z", $archive ],
> +	},
> +	lzo => {
> +	    'vma' => [ "lzop", "-d", "-c", $archive ],
> +	    'tar' => [ "tar", "--lzop", $archive ],
> +	},

I'd rather inverse it and add tgz as "virtual format"

vma => {
    ...
},
tar => {
    ...
},
tgz => [],


die "..." if !defined($format); # always an error

my $decomp = $decompressor->{$format};


> +    };
> +
> +    my $info;
> +    $info->{'format'} = $format;
> +    $info->{'type'} = $type;
> +    $info->{'compression'} = $comp;

please just set it as hash directly:

my $info = {
    format => $format,
    type => $type,
    compressor => $comp,
};

> +
> +    if (defined($comp) && defined($format)) {
> +	my $dcomp = $decompressor->{$comp}->{$format};
> +	pop(@$dcomp) if !defined($archive);

doesn't above just produces a empty list? seems like a confusing and
complicated way for a fall back value...
either just include $archive in the check above or just directly assign it?

a die like it existed befor below for the ($comp && !$decompressor) case
would be more sensible, IMO.

> +	$info->{'decompressor'} = $dcomp;
> +    }
> +
> +    return $info;
> +}
> +
>  sub extract_vzdump_config_tar {
>      my ($archive, $conf_re) = @_;
>  
> @@ -1310,16 +1356,12 @@ sub extract_vzdump_config_vma {
>      };
>  
>  
> +    my $info = archive_info($archive);
> +    $comp //= $info->{compression};
> +    my $decompressor = $info->{decompressor};
> +
>      if ($comp) {
> -	my $uncomp;
> -	if ($comp eq 'gz') {
> -	    $uncomp = ["zcat", $archive];
> -	} elsif ($comp eq 'lzo') {
> -	    $uncomp = ["lzop", "-d", "-c", $archive];
> -	} else {
> -	    die "unknown compression method '$comp'\n";
> -	}
> -	$cmd = [$uncomp, ["vma", "config", "-"]];
> +	$cmd = [ $decompressor, ["vma", "config", "-"] ];
>  
>  	# in some cases, lzop/zcat exits with 1 when its stdout pipe is
>  	# closed early by vma, detect this and ignore the exit code later
> @@ -1360,20 +1402,14 @@ sub extract_vzdump_config {
>      my ($cfg, $volid) = @_;
>  
>      my $archive = abs_filesystem_path($cfg, $volid);
> +    my $info = archive_info($archive);
> +    my $format = $info->{format};
> +    my $comp = $info->{compression};
> +    my $type = $info->{type};
>  
> -    if ($volid =~ /vzdump-(lxc|openvz)-\d+-(\d{4})_(\d{2})_(\d{2})-(\d{2})_(\d{2})_(\d{2})\.(tgz|(tar(\.(gz|lzo))?))$/) {
> +    if ($type eq 'lxc' || $type eq 'openvz') {
>  	return extract_vzdump_config_tar($archive, qr!^(\./etc/vzdump/(pct|vps)\.conf)$!);
> -    } elsif ($volid =~ /vzdump-qemu-\d+-(\d{4})_(\d{2})_(\d{2})-(\d{2})_(\d{2})_(\d{2})\.(tgz|((tar|vma)(\.(gz|lzo))?))$/) {
> -	my $format;
> -	my $comp;
> -	if ($7 eq 'tgz') {
> -	    $format = 'tar';
> -	    $comp = 'gz';
> -	} else {
> -	    $format = $9;
> -	    $comp = $11 if defined($11);
> -	}
> -
> +    } elsif ($type eq 'qemu') {
>  	if ($format eq 'tar') {
>  	    return extract_vzdump_config_tar($archive, qr!\(\./qemu-server\.conf\)!);
>  	} else {
>