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
> ---
> 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';
> -
