Re: [pve-devel] [pbs-devel] [PATCH proxmox-backup 1/2] restore-daemon: add 'format' parameter to the 'extract' handler

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Tue, May 31, 2022 at 01:17:22PM +0200, Dominik Csapak wrote:
> this can be 'plain', 'pxar', 'zip' or 'tar.zst',  and it returns the
> content in the given format (with fallback to the old behaviour if not
> given)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> note: needs a bumped 'proxmox-compression' in the Cargo.toml to build
> 
>  .../src/proxmox_restore_daemon/api.rs         | 49 ++++++++++++++++---
>  1 file changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/proxmox-restore-daemon/src/proxmox_restore_daemon/api.rs b/proxmox-restore-daemon/src/proxmox_restore_daemon/api.rs
> index aeb5a71d..c4977ce6 100644
> --- a/proxmox-restore-daemon/src/proxmox_restore_daemon/api.rs
> +++ b/proxmox-restore-daemon/src/proxmox_restore_daemon/api.rs
> @@ -13,7 +13,7 @@ use serde_json::Value;
>  use tokio::sync::Semaphore;
>  
>  use pathpatterns::{MatchEntry, MatchPattern, MatchType, Pattern};
> -use proxmox_compression::zip::zip_directory;
> +use proxmox_compression::{tar::tar_directory, zip::zip_directory, zstd::ZstdEncoder};
>  use proxmox_router::{
>      list_subdirs_api_method, ApiHandler, ApiMethod, ApiResponseFuture, Permission, Router,
>      RpcEnvironment, SubdirMap,
> @@ -236,11 +236,24 @@ pub const API_METHOD_EXTRACT: ApiMethod = ApiMethod::new(
>                  true,
>                  &BooleanSchema::new(concat!(
>                      "if true, return a pxar archive, otherwise either the ",
> -                    "file content or the directory as a zip file"
> +                    "file content or the directory as a zip file. DEPRECATED: use 'format' instead."
>                  ))
>                  .default(true)
>                  .schema()
> -            )
> +            ),
> +            (
> +                "format",
> +                true,
> +                &StringSchema::new("The desired format of the result.")
> +                    .format(&ApiStringFormat::Enum(&[
> +                        EnumEntry::new("plain", "Plain file (only works for single files)"),
> +                        EnumEntry::new("pxar", "PXAR archive"),
> +                        EnumEntry::new("zip", "ZIP archive"),
> +                        EnumEntry::new("tar.zst", "Zstd compressed TAR archive"),
> +                    ]))
> +                    .default("pxar")
> +                    .schema()

If you make an `#[api] enum RestoreFormat {}` and `use proxmox_schema::ApiType` you
can pass the enum's `RestoreFormat::API_SCHEMA` in here.
Would be nicer than using strings... (you can use `#[serde(rename)]` for
the "tar.zst" value).

On the other hand... We actually use a *tar* encoder, not a *tar.zstd*
encoder (obviously, given the nature of how this works), which makes me
wonder, shouldn't `zstd` maybe be a separate boolean?

I do think for a large single file "plain+zstd" makes sense, also
zstd-compressing a pxar might be worthwhile...
(And while zip+zstd doesn't make *that* much sense... I wouldn't really
care much either ;-) )

> +            ),
>          ]),
>      ),
>  )
> @@ -271,6 +284,11 @@ fn extract(
>          let path = Path::new(OsStr::from_bytes(&path[..]));
>  
>          let pxar = param["pxar"].as_bool().unwrap_or(true);
> +        let format = match param["format"].as_str() {
> +            Some(format) => format.to_string(),

Maybe consider erroring if both `pxar` and `format` are set here?
Otherwise move the pxar binding into the None scope so it's gone
afterwards, it's a long function ;-)

> +            // FIXME: old default was plain or zip, remove that with 3.0
> +            None => if pxar { "pxar".to_string() } else { String::new() }
> +        };
>  
>          let query_result = proxmox_async::runtime::block_in_place(move || {
>              let mut disk_state = crate::DISK_STATE.lock().unwrap();