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

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

[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].

[1]: https://bugzilla.proxmox.com/show_bug.cgi?id=3921

Signed-off-by: Gabriel Goller <g.goller@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))
     }
 
     /// Execute a special GET request and send output to a writer
-- 
2.39.2

Re: [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@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

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

[Gabriel Goller]

On 9/1/23 13:51, Thomas Lamprecht wrote:
> 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.

We return the error above. We have this regex
in place for all file-names:
`PROXMOX_SAFE_ID_REGEX_STR { () => { r"(?:[A-Za-z0-9_][A-Za-z0-9._\-]*)" 
}; }`

> 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.
I don't think so to be honest, because AFAIK we don't support directories
in snapshots (?) so it doesn't make sense to pass "./test.pxar".

Stripping is also wrong, because the . (dot) would be an accepted character
in the file-name and thus we cannot remove it. Though we could remove every
prohibited character (the / in this case) and the characters around
(the . in our example)?

>> [1]: https://bugzilla.proxmox.com/show_bug.cgi?id=3921
>>
>> Signed-off-by: Gabriel Goller <g.goller@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?
Yes, because the `err` is a multi-line string here. On the api we have a 
`Vec` of errors and return it as a string using `\n`
as a delimiter.
>>       }
>>   
>>       /// Execute a special GET request and send output to a writer

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

[Gabriel Goller]

Submitted v2.

On 9/1/23 10:02, Gabriel Goller wrote:
> 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].
>
> [1]: https://bugzilla.proxmox.com/show_bug.cgi?id=3921
>
> Signed-off-by: Gabriel Goller <g.goller@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))
>       }
>   
>       /// Execute a special GET request and send output to a writer