[pve-devel] [PATCH container] Fix #2124: restore: support external de-compressor

[Thomas Lamprecht]

On 6/14/19 3:37 PM, Alwin Antreich wrote:
> This patch adds support to restore archives that have been compressed
> with a compressor not natively supported by tar. This had to be added
> for zstd support.

yeah, tha's nice, but that's "what" not how/why...

I.e., no words about that the zstd support should come through
"decompressor_info". And why you do it now half/half adding effectively
two places the CT de-compressor info is hard coded? I don't want that
just complicates things, so either we have it here alone or there, but
not semi-mixed.

Why not just use decompressor_info alone?  And the _info from that
method is misleading IMO, as it's currently it better should be called
_cmd, or (maybe better) have a return signature like:

return wantarray ? $cmd : ($cmd, $format, $comp);

> 
> Signed-off-by: Alwin Antreich <a.antreich at proxmox.com>
> ---
>  src/PVE/LXC/Create.pm | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/src/PVE/LXC/Create.pm b/src/PVE/LXC/Create.pm
> index 029c940..a7aec9a 100644
> --- a/src/PVE/LXC/Create.pm
> +++ b/src/PVE/LXC/Create.pm
> @@ -71,21 +71,28 @@ sub restore_archive {
>      my $archive_fh;
>      my $tar_input = '<&STDIN';
>      my @compression_opt;
> +    my $decomp;

s/decomp/decompressor/

>      if ($archive ne '-') {
>  	# GNU tar refuses to autodetect this... *sigh*
>  	my %compression_map = (
> -	    '.gz'  => '-z',
> -	    '.bz2' => '-j',
> -	    '.xz'  => '-J',
> -	    '.lzo'  => '--lzop',
> +	    'gz'  => '-z',
> +	    'bz2' => '-j',
> +	    'xz'  => '-J',
> +	    'lzo'  => '--lzop',
>  	);
> -	if ($archive =~ /\.tar(\.[^.]+)?$/) {
> -	    if (defined($1)) {
> -		die "unrecognized compression format: $1\n" if !defined($compression_map{$1});
> -		@compression_opt = $compression_map{$1};
> +
> +	$archive =~ /\.(tar)\.?([^.]+)?$/;
> +	    my $format = $1;
> +	    my $comp = $2;

my ($format, $compressor) = ($1, $2);

> +
> +	if (defined($comp)) {
> +	    if (defined($compression_map{$comp})) {
> +		@compression_opt = $compression_map{$comp};
> +	    } else {
> +		$decomp = PVE::Storage::decompressor_info(undef, $comp);
>  	    }
>  	} else {
> -	    die "file does not look like a template archive: $archive\n";
> +	    die "file does not look like a template archive: $archive\n" if ($format ne 'tar');

what? why is $format even introduced here? It has nothing to do 
with zstd support? 

Also _if_ we would something like this then please no post-if here,
if one would do this then by changing the else to an elsif, but as the
regex is fixed to tar anyway it makes no sense in this patch?

>  	}
>  	sysopen($archive_fh, $archive, O_RDONLY)
>  	    or die "failed to open '$archive': $!\n";
> @@ -106,8 +113,12 @@ sub restore_archive {
>      push @$cmd, '--anchored';
>      push @$cmd, '--exclude' , './dev/*';
>  
> +    $cmd = [$decomp, $cmd] if defined($decomp);


> +
>      if (defined($bwlimit)) {
> -	$cmd = [ ['cstream', '-t', $bwlimit*1024], $cmd ];
> +	    my $cstream = ['cstream', '-t', $bwlimit*1024];
> +	    # unpack if $decomp, otherwise to many array levels
> +	    $cmd = defined($decomp) ? [ $cstream, @$cmd ] : [ $cstream, $cmd ];

just unshift $cstream into @$cmd? e.g.:

unsift @$cmd, [ 'cstream', '-t', $bwlimit*1024 ];

should work. And please keep spaces at start and end of such command array reference
declarations. /Most/ of the time it makes them easier to read.

>      }
>  
>      if ($archive eq '-') {
>