[pbs-devel] [PATCH proxmox-backup] fix #3921: client: confusing backup reader error

[Thomas Lamprecht]

Am 01/09/2023 um 10:02 schrieb Gabriel Goller:
> When using the catalog shell command, a common error is that user pass
> "./files.pxar" instead of "files.pxar". This will result in a cryptic error
> "... value does not match regex pattern ...". Added some context to the
> error according to suggestions here [1].

adding error context can be good in general, but the specific case
mentioned could be also solved explicitly by either stripping ./
or handling it explicitly. How does it handle multiple consecutive
slashes in paths? Like "foo///bar"? As that would need similar
normalizing so, if that works, then we maybe got already some place
where we can add handling for ./, and otherwise it might make sense
to start thinking about adding such things.

As when multiple users run into some issue it might be warranted
to check if them "holding it wrong" should be actually a valid way
to hold it.

> 
> [1]: https://bugzilla.proxmox.com/show_bug.cgi?id=3921
> 
> Signed-off-by: Gabriel Goller <g.goller at proxmox.com>
> ---
>  pbs-client/src/backup_reader.rs | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/pbs-client/src/backup_reader.rs b/pbs-client/src/backup_reader.rs
> index 2cd4dc27..138e836a 100644
> --- a/pbs-client/src/backup_reader.rs
> +++ b/pbs-client/src/backup_reader.rs
> @@ -98,7 +98,10 @@ impl BackupReader {
>      pub async fn download<W: Write + Send>(&self, file_name: &str, output: W) -> Result<(), Error> {
>          let path = "download";
>          let param = json!({ "file-name": file_name });
> -        self.h2.download(path, Some(param), output).await
> +        self.h2
> +            .download(path, Some(param), output)
> +            .await
> +            .map_err(|err| format_err!("http2 file download '{}' failed: \n{}", file_name, err))

please use captured identifiers for new format!/format_err! et al.
calls, e.g., here:

.map_err(|err| format_err!("http2 file download '{file_name}' failed - {err}"))

We also normally do not use a newline to separate prefix from actual
error, or would there be a good reason to deviate from the norm here?

>      }
>  
>      /// Execute a special GET request and send output to a writer