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 <[email protected]> > --- > 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 '-') { > _______________________________________________ pve-devel mailing list [email protected] https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
