Stoiko Ivanov <s.iva...@proxmox.com> writes: > In general I think the addition of bz2 for ISOs makes sense (if only for > OPNSense's sake) - Thanks for addressing this! > > You want to add bzip2 as dependency to debian/control > we probably should also add lzop, zstd, gzip to the dependencies > although they are pulled in as dependency of pve-manger (due to > vzdump being there) - please do so in a separate commit
👌 > On Fri, 2 Aug 2024 16:34:14 +0200 > Maximiliano Sandoval <m.sando...@proxmox.com> wrote: > >> This was requested at [1]. > add a 'close #5267' to the commit message as suggested in the > developer-docs: > https://pve.proxmox.com/wiki/Developer_Documentation#Commits_and_Commit_Messages > Your colleagues preparing the release-notes will be grateful. > #5267 also asks for xz support and it is loosely titled "Please support more decompression algorithms" hence why this does not really fix this issue. >> >> [1] https://bugzilla.proxmox.com/show_bug.cgi?id=5267 >> >> Suggested-by: Stoiko Ivanov <s.iva...@proxmox.com> >> Signed-off-by: Maximiliano Sandoval <m.sando...@proxmox.com> >> --- >> >> For the sake of not having a commit where tests fail, this is all in one >> commit. >> Please tell me if I should split it. > having each commit working is worth a lot if you have to bisect at some > point - and in this case following the changes works well for me - so I'd > agree to keep this as one commit > 👌 >> >> The idea is to expose this algorithm only for bz2 decompression in the web UI >> when downloading ISOs. A followup will contain the patch for pve-manager. > you say you only want it for ISOs here ... >> >> An popular iso that is compressed with bz2 is opnsense [2]. > That motivation could also be useful in the commit-message for the future. > (I did not expect that bz2 as sole compressed format is something I'd run > into nowadays) > > will give it a spin with the pve-manager commit. > > >> >> [2] https://opnsense.org/download/ >> >> src/PVE/Storage.pm | 3 +++ >> src/PVE/Storage/Plugin.pm | 2 +- >> src/test/archive_info_test.pm | 6 ++++-- >> src/test/list_volumes_test.pm | 11 ++++++++++- >> src/test/parse_volname_test.pm | 4 ++-- >> src/test/path_to_volume_id_test.pm | 13 ++++++++----- >> 6 files changed, 28 insertions(+), 11 deletions(-) >> >> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm >> index 57b2038..0caac71 100755 >> --- a/src/PVE/Storage.pm >> +++ b/src/PVE/Storage.pm >> @@ -1550,16 +1550,19 @@ sub decompressor_info { >> gz => ['tar', '-z'], >> lzo => ['tar', '--lzop'], >> zst => ['tar', '--zstd'], >> + bz2 => ['tar', '--bzip2'], >> }, >> vma => { >> gz => ['zcat'], >> lzo => ['lzop', '-d', '-c'], >> zst => ['zstd', '-q', '-d', '-c'], >> + bz2 => ['bzcat', '-q'], > ... but add it for tar and vma here? > (haven't tried if adding it only for one of the formats works explicitly > though) The patch requires adding a new value to `KNOWN_COMPRESSION_FORMATS` which at least for the current usecase is used to define the supported formats for the API. this change can potentially break something else if these methods are not implemented. > > also - but this is really in the realm of cosmetics - why do we have > zcat/bzcat, but not zstdcat (lzop does not ship a link > to lzopcat ..)? (but again - I don't think there's a need to change > stuff here) > > >> }, >> iso => { >> gz => ['zcat'], >> lzo => ['lzop', '-d', '-c'], >> zst => ['zstd', '-q', '-d', '-c'], >> + bz2 => ['bzcat', '-q'], >> }, >> }; >> >> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm >> index 6444390..e5ef9de 100644 >> --- a/src/PVE/Storage/Plugin.pm >> +++ b/src/PVE/Storage/Plugin.pm >> @@ -19,7 +19,7 @@ use JSON; >> >> use base qw(PVE::SectionConfig); >> >> -use constant KNOWN_COMPRESSION_FORMATS => ('gz', 'lzo', 'zst'); >> +use constant KNOWN_COMPRESSION_FORMATS => ('gz', 'lzo', 'zst', 'bz2'); >> use constant COMPRESSOR_RE => join('|', KNOWN_COMPRESSION_FORMATS); >> >> use constant LOG_EXT => ".log"; >> diff --git a/src/test/archive_info_test.pm b/src/test/archive_info_test.pm >> index cf03c3d..53e37be 100644 >> --- a/src/test/archive_info_test.pm >> +++ b/src/test/archive_info_test.pm >> @@ -121,11 +121,13 @@ my $decompressor = { >> gz => ['tar', '-z'], >> lzo => ['tar', '--lzop'], >> zst => ['tar', '--zstd'], >> + bz2 => ['tar', '--bzip2'], >> }, >> vma => { >> gz => ['zcat'], >> lzo => ['lzop', '-d', '-c'], >> zst => ['zstd', '-q', '-d', '-c'], >> + bz2 => ['bzcat', '-q'], >> }, >> }; >> >> @@ -163,8 +165,8 @@ for my $virt (sort keys %$bkp_suffix) { >> >> # add compression formats to test failed matches >> my $non_bkp_suffix = { >> - 'openvz' => [ 'zip', 'tgz.lzo', 'tar.bz2', 'zip.gz', '', ], >> - 'lxc' => [ 'zip', 'tgz.lzo', 'tar.bz2', 'zip.gz', '', ], >> + 'openvz' => [ 'zip', 'tgz.lzo', 'zip.gz', '', ], >> + 'lxc' => [ 'zip', 'tgz.lzo', 'zip.gz', '', ], >> 'qemu' => [ 'vma.xz', 'vms.gz', 'vmx.zst', '', ], >> 'none' => [ 'tar.gz', ], >> }; >> diff --git a/src/test/list_volumes_test.pm b/src/test/list_volumes_test.pm >> index d155cb9..ba959ff 100644 >> --- a/src/test/list_volumes_test.pm >> +++ b/src/test/list_volumes_test.pm >> @@ -189,6 +189,7 @@ my @tests = ( >> "$storage_dir/dump/vzdump-lxc-16112-2020_03_30-21_49_30.tar.gz", >> "$storage_dir/dump/vzdump-lxc-16112-2020_03_30-21_49_30.tar.zst", >> "$storage_dir/dump/vzdump-lxc-16112-2020_03_30-21_59_30.tgz", >> + "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2", >> ], >> expected => [ >> { >> @@ -237,6 +238,15 @@ my @tests = ( >> 'vmid' => '16112', >> 'volid' => >> 'local:backup/vzdump-lxc-16112-2020_03_30-21_59_30.tgz', >> }, >> + { >> + 'content' => 'backup', >> + 'ctime' => 1585604370, >> + 'format' => 'tar.bz2', >> + 'size' => DEFAULT_SIZE, >> + 'subtype' => 'openvz', >> + 'vmid' => '16112', >> + 'volid' => >> 'local:backup/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2', >> + }, >> ], >> }, >> { >> @@ -440,7 +450,6 @@ my @tests = ( >> >> "$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.zip.gz", >> >> "$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.tar.bz2", >> "$storage_dir/private/subvol-19254-disk-0/19254", >> - "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2", >> "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.zip.gz", >> "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tgz.lzo", >> "$storage_dir/dump/vzdump-qemu-16110-2020_03_30-21_12_40.vma.xz", >> diff --git a/src/test/parse_volname_test.pm b/src/test/parse_volname_test.pm >> index d6ac885..5f63941 100644 >> --- a/src/test/parse_volname_test.pm >> +++ b/src/test/parse_volname_test.pm >> @@ -177,7 +177,7 @@ foreach my $s (@$disk_suffix) { >> # create more test cases for backup files matches >> my $bkp_suffix = { >> qemu => [ 'vma', 'vma.gz', 'vma.lzo', 'vma.zst' ], >> - lxc => [ 'tar', 'tgz', 'tar.gz', 'tar.lzo', 'tar.zst' ], >> + lxc => [ 'tar', 'tgz', 'tar.gz', 'tar.lzo', 'tar.zst', 'tar.bz2' ], >> openvz => [ 'tar', 'tgz', 'tar.gz', 'tar.lzo', 'tar.zst' ], >> }; >> >> @@ -204,7 +204,7 @@ foreach my $virt (keys %$bkp_suffix) { >> # create more test cases for failed backup files matches >> my $non_bkp_suffix = { >> qemu => [ 'vms.gz', 'vma.xz' ], >> - lxc => [ 'tar.bz2', 'zip.gz', 'tgz.lzo' ], >> + lxc => [ 'zip.gz', 'tgz.lzo' ], >> }; >> foreach my $virt (keys %$non_bkp_suffix) { >> my $suffix = $non_bkp_suffix->{$virt}; >> diff --git a/src/test/path_to_volume_id_test.pm >> b/src/test/path_to_volume_id_test.pm >> index 8149c88..0f39b03 100644 >> --- a/src/test/path_to_volume_id_test.pm >> +++ b/src/test/path_to_volume_id_test.pm >> @@ -116,6 +116,14 @@ my @tests = ( >> 'local:backup/vzdump-lxc-16112-2020_03_30-21_39_30.tar.zst' >> ], >> }, >> + { >> + description => 'Backup, tar.bz2', >> + volname => >> "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2", >> + expected => [ >> + 'backup', >> + 'local:backup/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2', >> + ], >> + }, >> >> { >> description => 'ISO file', >> @@ -201,11 +209,6 @@ my @tests = ( >> volname => "$storage_dir/private/subvol-19254-disk-0/", >> expected => [''], >> }, >> - { >> - description => 'Backup, wrong ending, openvz, tar.bz2', >> - volname => >> "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2", >> - expected => [''], >> - }, >> { >> description => 'Backup, wrong format, openvz, zip.gz', >> volname => >> "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.zip.gz", I will add more info to the commit message and add dependencies to d/control before sending v2. -- Maximiliano _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
