[pbs-devel] [PATCH proxmox{, -backup} v3 0/4] output full anyhow context in client

[pbs-devel] [PATCH proxmox{, -backup} v3 0/4] output full anyhow context in client

[Gabriel Goller]

Instead of using format_err! and format! to create error messages, we
want to move to anyhow::Context and add information to already
existing anyhow::Error's. 
Before we start to gradually phase out the format! error calls, we
need to print the whole context + error, because the default 
anyhow::Error Display implementation doesn't print the actual error.

This series starts with the proxmox-backup-client and includes a patch
from Fabian that introduces pretty-printing of errors (including 
the context) and some other high-level stuff from me. The scope 
is not to remove every format call and add a context everywhere, 
but to enable this change in the future.

v3: 
 - rebased
 - applied Max's fixup

v2:
 - moved explanation to readme.rst file
 - remove unwrap, use take() in `if let`
 - cargo fmt

proxmox:

Fabian Grünbichler (1):
  CLI: print fatal errors including causes

 proxmox-router/README.rst         | 95 +++++++++++++++++++++++++++++++
 proxmox-router/src/cli/command.rs |  4 +-
 2 files changed, 97 insertions(+), 2 deletions(-)
 create mode 100644 proxmox-router/README.rst


proxmox-backup:

Gabriel Goller (3):
  pxar: remove ArchiveError
  pxar: add UniqueContext helper
  pxar: use anyhow::Error in PxarBackupStream

 pbs-client/src/pxar/create.rs        | 48 +++++++++++++---------------
 pbs-client/src/pxar_backup_stream.rs | 18 +++++------
 2 files changed, 32 insertions(+), 34 deletions(-)


Summary over all repositories:
  4 files changed, 129 insertions(+), 36 deletions(-)

-- 
Generated by git-murpp 0.5.0


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox v3 1/4] CLI: print fatal errors including causes

[Gabriel Goller]

From: Fabian Grünbichler <f.gruenbichler@proxmox.com>

as a first step of improving our error handling story, printing context
and causes if the error contains them.

The downside to adding context is that the default Display implementation
will *just* print the context, which hides the root cause. This is why
we print the errors using the pretty-print formatter in this change.

More info in `proxmox-router/README.rst`.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 proxmox-router/README.rst         | 95 +++++++++++++++++++++++++++++++
 proxmox-router/src/cli/command.rs |  4 +-
 2 files changed, 97 insertions(+), 2 deletions(-)
 create mode 100644 proxmox-router/README.rst

diff --git a/proxmox-router/README.rst b/proxmox-router/README.rst
new file mode 100644
index 00000000..5fef633f
--- /dev/null
+++ b/proxmox-router/README.rst
@@ -0,0 +1,95 @@
+proxmox-router
+==============
+
+Command Line
+------------
+
+To improve our error handling story, we use anyhow and ``.context()`` on
+all errors. This means no more ``format_err()`` and ``format()`` of errors.
+
+For example, with two calls to ``.with_context`` when bubbling up errors in
+proxmox-offline-mirror:
+
+.. code-block::
+    diff --git a/src/bin/proxmox-offline-mirror.rs b/src/bin/proxmox-offline-mirror.rs
+    index bec366a..403a2f5 100644
+    --- a/src/bin/proxmox-offline-mirror.rs
+    +++ b/src/bin/proxmox-offline-mirror.rs
+    @@ -1,7 +1,7 @@
+     use std::fmt::Display;
+     use std::path::Path;
+
+    -use anyhow::{bail, Error};
+    +use anyhow::{bail, format_err, Context, Error};
+     use serde_json::Value;
+
+     use proxmox_router::cli::{run_cli_command, CliCommand, CliCommandMap, CliEnvironment};
+    @@ -676,7 +676,8 @@ async fn setup(config: Option<String>, _param: Value) -> Result<(), Error> {
+                 Action::AddMirror => {
+                     for mirror_config in action_add_mirror(&config)? {
+                         let id = mirror_config.id.clone();
+    -                    mirror::init(&mirror_config)?;
+    +                    mirror::init(&mirror_config)
+    +                        .with_context(|| format!("Failed to initialize mirror '{id}'"))?;
+                         config.set_data(&id, "mirror", mirror_config)?;
+                         save_config(&config_file, &config)?;
+                         println!("Config entry '{id}' added");
+    diff --git a/src/pool.rs b/src/pool.rs
+    index 3da8c08..ecf3f6f 100644
+    --- a/src/pool.rs
+    +++ b/src/pool.rs
+    @@ -7,7 +7,7 @@ use std::{
+         path::{Path, PathBuf},
+     };
+
+    -use anyhow::{bail, format_err, Error};
+    +use anyhow::{bail, format_err, Context, Error};
+     use nix::{unistd, NixPath};
+
+     use proxmox_apt::deb822::CheckSums;
+    @@ -45,10 +45,12 @@ impl Pool {
+             }
+
+             if !pool.exists() {
+    -            create_path(pool, None, None)?;
+    +            create_path(pool, None, None)
+    +                .with_context(|| format!("Failed to create pool dir {pool:?}"))?;
+             }
+
+    -        create_path(link_dir, None, None)?;
+    +        create_path(link_dir, None, None)
+    +            .with_context(|| format!("Failed to create link dir {link_dir:?}"))?;
+
+             Ok(Self {
+                 pool_dir: pool.to_path_buf(),
+
+We'd get the following output::
+
+    Error: Failed to initialize mirror 'debian_bullseye_main'
+
+    Caused by:
+        0: Failed to create pool dir "/var/lib/proxmox-offline-mirror/mirrors//.pool"
+        1: EACCES: Permission denied
+
+Instead of the original::
+
+    Error: EACCESS: Permission denied
+
+Which is not really helpful without knowing the path.
+
+For non-fatal cases or logging inside tasks, ``{:#}`` could be used which just
+prints the causes/contexts in a single line like this::
+
+    Failed to initialize mirror 'debian_bullseye_main': Failed to create pool dir "/var/lib/proxmox-offline-mirror/mirrors//.pool": EACCES: Permission denied
+
+but for that usage, the context should be kept short to avoid the line getting overly long.
+
+One downside to adding context is that the default ``Display`` implementation will
+*just* print the context, which hides the root cause::
+
+    Error: Failed to initialize mirror 'debian_bullseye_main'
+
+When adding context to existing error handling (or when replacing manual
+context adding via format_err!), call sites need to be adapted to ensure the
+causes are not accidentally hidden.
+
diff --git a/proxmox-router/src/cli/command.rs b/proxmox-router/src/cli/command.rs
index 7a26ffb9..d5522f12 100644
--- a/proxmox-router/src/cli/command.rs
+++ b/proxmox-router/src/cli/command.rs
@@ -83,7 +83,7 @@ async fn handle_simple_command_future(
             }
         }
         Err(err) => {
-            eprintln!("Error: {}", err);
+            eprintln!("Error: {:?}", err);
             return Err(err);
         }
     }
@@ -135,7 +135,7 @@ fn handle_simple_command(
             }
         }
         Err(err) => {
-            eprintln!("Error: {}", err);
+            eprintln!("Error: {:?}", err);
             return Err(err);
         }
     }
-- 
2.43.0



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox v3 1/4] CLI: print fatal errors including causes

[Wolfgang Bumiller]

On Mon, Jun 17, 2024 at 10:12:51AM GMT, Gabriel Goller wrote:
> From: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> 
> as a first step of improving our error handling story, printing context
> and causes if the error contains them.
> 
> The downside to adding context is that the default Display implementation
> will *just* print the context, which hides the root cause. This is why
> we print the errors using the pretty-print formatter in this change.
> 
> More info in `proxmox-router/README.rst`.

This file will not be seen by anyone. It's also not where I'd expect
generic coding style advice and doesn't *really* have much to do with
the router crate.

This info might be better in the developer docs / wiki.

So please drop the file for now.

(Also: such files are better as a separate patch, so it can easily be
dropped while applying.)

> diff --git a/proxmox-router/src/cli/command.rs b/proxmox-router/src/cli/command.rs
> index 7a26ffb9..d5522f12 100644
> --- a/proxmox-router/src/cli/command.rs
> +++ b/proxmox-router/src/cli/command.rs
> @@ -83,7 +83,7 @@ async fn handle_simple_command_future(
>              }
>          }
>          Err(err) => {
> -            eprintln!("Error: {}", err);
> +            eprintln!("Error: {:?}", err);

Could go straight to 

    eprintln!("Error: {err:?}");

in both cases.

>              return Err(err);
>          }
>      }
> @@ -135,7 +135,7 @@ fn handle_simple_command(
>              }
>          }
>          Err(err) => {
> -            eprintln!("Error: {}", err);
> +            eprintln!("Error: {:?}", err);
>              return Err(err);
>          }
>      }
> -- 
> 2.43.0


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox v3 1/4] CLI: print fatal errors including causes

[Gabriel Goller]

On 18.06.2024 15:42, Wolfgang Bumiller wrote:
>On Mon, Jun 17, 2024 at 10:12:51AM GMT, Gabriel Goller wrote:
>> From: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>>
>> as a first step of improving our error handling story, printing context
>> and causes if the error contains them.
>>
>> The downside to adding context is that the default Display implementation
>> will *just* print the context, which hides the root cause. This is why
>> we print the errors using the pretty-print formatter in this change.
>>
>> More info in `proxmox-router/README.rst`.
>
>This file will not be seen by anyone. It's also not where I'd expect
>generic coding style advice and doesn't *really* have much to do with
>the router crate.
>
>This info might be better in the developer docs / wiki.

I agree and I'm going to remove this from the next version.

When this is merged where do you think we should put this? I don't think
a new header in
https://pbs.proxmox.com/wiki/index.php/Developer_Documentation is really
fitting. Maybe a new site `Development Best Practices` with 'use context
instead of format_err'? :)

>> diff --git a/proxmox-router/src/cli/command.rs b/proxmox-router/src/cli/command.rs
>> index 7a26ffb9..d5522f12 100644
>> --- a/proxmox-router/src/cli/command.rs
>> +++ b/proxmox-router/src/cli/command.rs
>> @@ -83,7 +83,7 @@ async fn handle_simple_command_future(
>>              }
>>          }
>>          Err(err) => {
>> -            eprintln!("Error: {}", err);
>> +            eprintln!("Error: {:?}", err);
>
>Could go straight to
>
>    eprintln!("Error: {err:?}");
>
>in both cases.

Submitted a v4 already!


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup v3 2/4] pxar: remove ArchiveError

[Gabriel Goller]

The sole purpose of the ArchiveError was to add the file-path to the
error. Using anyhow::Error we can add this information using the context
and don't need this struct anymore.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 pbs-client/src/pxar/create.rs | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index 9a41bf3e..c9a8c744 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -1,6 +1,5 @@
 use std::collections::{HashMap, HashSet};
 use std::ffi::{CStr, CString, OsStr};
-use std::fmt;
 use std::io::{self, Read};
 use std::mem::size_of;
 use std::ops::Range;
@@ -117,25 +116,11 @@ pub fn is_virtual_file_system(magic: i64) -> bool {
         SYSFS_MAGIC)
 }
 
-#[derive(Debug)]
-struct ArchiveError {
-    path: PathBuf,
-    error: Error,
 }
 
-impl ArchiveError {
-    fn new(path: PathBuf, error: Error) -> Self {
-        Self { path, error }
     }
 }
 
-impl std::error::Error for ArchiveError {}
-
-impl fmt::Display for ArchiveError {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(f, "error at {:?}: {}", self.path, self.error)
-    }
-}
 
 #[derive(Eq, PartialEq, Hash)]
 pub(crate) struct HardLinkInfo {
@@ -353,14 +338,6 @@ impl Archiver {
         self.feature_flags & self.fs_feature_flags
     }
 
-    fn wrap_err(&self, err: Error) -> Error {
-        if err.downcast_ref::<ArchiveError>().is_some() {
-            err
-        } else {
-            ArchiveError::new(self.path.clone(), err).into()
-        }
-    }
-
     fn archive_dir_contents<'a, T: SeqWrite + Send>(
         &'a mut self,
         encoder: &'a mut Encoder<'_, T>,
@@ -409,7 +386,7 @@ impl Archiver {
                     &file_entry.stat,
                 )
                 .await
-                .map_err(|err| self.wrap_err(err))?;
+                .context(format!("error at {:?}", self.path))?;
             }
             self.path = old_path;
             self.entry_counter = entry_counter;
-- 
2.43.0



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup v3 3/4] pxar: add UniqueContext helper

[Gabriel Goller]

To create a pxar archive, we recursively traverse the target folder.
If there is an error further down and we add a context using anyhow,
the context will be duplicated and we get an output like:

> Error: error at "xattr/xattr.txt": error at "xattr/xattr.txt": E2BIG [skip]

This is obviously not optimal, so in recursive contexts we can use the
UniqueContext, which quickly checks the context from the last item in
the error chain and only adds it if it is unique.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 pbs-client/src/pxar/create.rs | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index c9a8c744..89a5b5ab 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -1,5 +1,6 @@
 use std::collections::{HashMap, HashSet};
 use std::ffi::{CStr, CString, OsStr};
+use std::fmt::Display;
 use std::io::{self, Read};
 use std::mem::size_of;
 use std::ops::Range;
@@ -116,12 +117,32 @@ pub fn is_virtual_file_system(magic: i64) -> bool {
         SYSFS_MAGIC)
 }
 
+trait UniqueContext<T> {
+    fn unique_context<S>(self, context: S) -> Result<T, anyhow::Error>
+    where
+        S: Display + Send + Sync + 'static;
 }
 
+impl<T> UniqueContext<T> for Result<T, anyhow::Error> {
+    fn unique_context<S>(self, context: S) -> Result<T, anyhow::Error>
+    where
+        S: Display + Send + Sync + 'static,
+    {
+        match self {
+            Ok(ok) => Ok(ok),
+            Err(err) => {
+                let last_error = err.chain().next();
+                if let Some(e) = last_error {
+                    if e.to_string() == context.to_string() {
+                        return Err(err);
+                    }
+                }
+                Err(err.context(context))
+            }
+        }
     }
 }
 
-
 #[derive(Eq, PartialEq, Hash)]
 pub(crate) struct HardLinkInfo {
     st_dev: u64,
@@ -386,7 +407,7 @@ impl Archiver {
                     &file_entry.stat,
                 )
                 .await
-                .context(format!("error at {:?}", self.path))?;
+                .unique_context(format!("error at {:?}", self.path))?;
             }
             self.path = old_path;
             self.entry_counter = entry_counter;
-- 
2.43.0



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup v3 4/4] pxar: use anyhow::Error in PxarBackupStream

[Gabriel Goller]

Instead of storing the error as a string in the PxarBackupStream, we
store it as an anyhow::Error. As we can't clone an anyhow::Error, we take
it out from the mutex and return it. This won't change anything as
the consumation of the stream will stop if it gets a Some(Err(..)).

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 pbs-client/src/pxar_backup_stream.rs | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/pbs-client/src/pxar_backup_stream.rs b/pbs-client/src/pxar_backup_stream.rs
index cdbcdfec..4370da6c 100644
--- a/pbs-client/src/pxar_backup_stream.rs
+++ b/pbs-client/src/pxar_backup_stream.rs
@@ -5,7 +5,7 @@ use std::pin::Pin;
 use std::sync::{mpsc, Arc, Mutex};
 use std::task::{Context, Poll};
 
-use anyhow::{format_err, Error};
+use anyhow::Error;
 use futures::future::{AbortHandle, Abortable};
 use futures::stream::Stream;
 use nix::dir::Dir;
@@ -29,7 +29,7 @@ pub struct PxarBackupStream {
     rx: Option<std::sync::mpsc::Receiver<Result<Vec<u8>, Error>>>,
     pub suggested_boundaries: Option<std::sync::mpsc::Receiver<u64>>,
     handle: Option<AbortHandle>,
-    error: Arc<Mutex<Option<String>>>,
+    error: Arc<Mutex<Option<Error>>>,
 }
 
 impl Drop for PxarBackupStream {
@@ -98,7 +98,7 @@ impl PxarBackupStream {
             .await
             {
                 let mut error = error2.lock().unwrap();
-                *error = Some(err.to_string());
+                *error = Some(err);
             }
         };
 
@@ -142,18 +142,18 @@ impl Stream for PxarBackupStream {
     fn poll_next(self: Pin<&mut Self>, _cx: &mut Context) -> Poll<Option<Self::Item>> {
         {
             // limit lock scope
-            let error = self.error.lock().unwrap();
-            if let Some(ref msg) = *error {
-                return Poll::Ready(Some(Err(format_err!("{}", msg))));
+            let mut error = self.error.lock().unwrap();
+            if let Some(err) = error.take() {
+                return Poll::Ready(Some(Err(err)));
             }
         }
 
         match proxmox_async::runtime::block_in_place(|| self.rx.as_ref().unwrap().recv()) {
             Ok(data) => Poll::Ready(Some(data)),
             Err(_) => {
-                let error = self.error.lock().unwrap();
-                if let Some(ref msg) = *error {
-                    return Poll::Ready(Some(Err(format_err!("{}", msg))));
+                let mut error = self.error.lock().unwrap();
+                if let Some(err) = error.take() {
+                    return Poll::Ready(Some(Err(err)));
                 }
                 Poll::Ready(None) // channel closed, no error
             }
-- 
2.43.0



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel