public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH v2 proxmox{, -backup} 0/4] output full anyhow context in client
@ 2024-02-20 10:28 Gabriel Goller
  2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox 1/4] CLI: print fatal errors including causes Gabriel Goller
                   ` (5 more replies)
  0 siblings, 6 replies; 9+ messages in thread
From: Gabriel Goller @ 2024-02-20 10:28 UTC (permalink / raw)
  To: pbs-devel

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.

Changelog 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(-)

-- 
murpp v0.4.0





^ permalink raw reply	[flat|nested] 9+ messages in thread

* [pbs-devel] [PATCH v2 proxmox 1/4] CLI: print fatal errors including causes
  2024-02-20 10:28 [pbs-devel] [PATCH v2 proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
@ 2024-02-20 10:28 ` Gabriel Goller
  2024-02-21 16:45   ` Max Carrara
  2024-02-21 16:52   ` [pbs-devel] [PATCH proxmox] fixup! " Max Carrara
  2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox-backup 2/4] pxar: remove ArchiveError Gabriel Goller
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 9+ messages in thread
From: Gabriel Goller @ 2024-02-20 10:28 UTC (permalink / raw)
  To: pbs-devel

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 0000000..9c7c213
--- /dev/null
+++ b/proxmox-router/README.rst
@@ -0,0 +1,95 @@
+================
+ proxmox-router
+================
+
+cli
+==================
+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 7a26ffb..d5522f1 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





^ permalink raw reply	[flat|nested] 9+ messages in thread

* [pbs-devel] [PATCH v2 proxmox-backup 2/4] pxar: remove ArchiveError
  2024-02-20 10:28 [pbs-devel] [PATCH v2 proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
  2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox 1/4] CLI: print fatal errors including causes Gabriel Goller
@ 2024-02-20 10:28 ` Gabriel Goller
  2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox-backup 3/4] pxar: add UniqueContext helper Gabriel Goller
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Gabriel Goller @ 2024-02-20 10:28 UTC (permalink / raw)
  To: pbs-devel

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 | 26 +-------------------------
 1 file changed, 1 insertion(+), 25 deletions(-)

diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index 75376c0c..06f396e0 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::os::unix::ffi::OsStrExt;
 use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, OwnedFd, RawFd};
@@ -89,25 +88,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)]
 struct HardLinkInfo {
@@ -219,14 +204,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>,
@@ -265,8 +242,7 @@ impl Archiver {
                 (self.callback)(&file_entry.path)?;
                 self.path = file_entry.path;
                 self.add_entry(encoder, dir_fd, &file_entry.name, &file_entry.stat)
-                    .await
-                    .map_err(|err| self.wrap_err(err))?;
+                    .await.context(format!("error at {:?}", self.path))?;
             }
             self.path = old_path;
             self.entry_counter = entry_counter;
-- 
2.43.0





^ permalink raw reply	[flat|nested] 9+ messages in thread

* [pbs-devel] [PATCH v2 proxmox-backup 3/4] pxar: add UniqueContext helper
  2024-02-20 10:28 [pbs-devel] [PATCH v2 proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
  2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox 1/4] CLI: print fatal errors including causes Gabriel Goller
  2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox-backup 2/4] pxar: remove ArchiveError Gabriel Goller
@ 2024-02-20 10:28 ` Gabriel Goller
  2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox-backup 4/4] pxar: use anyhow::Error in PxarBackupStream Gabriel Goller
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Gabriel Goller @ 2024-02-20 10:28 UTC (permalink / raw)
  To: pbs-devel

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 | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index 06f396e0..679b3c62 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::os::unix::ffi::OsStrExt;
 use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, OwnedFd, RawFd};
@@ -88,12 +89,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)]
 struct HardLinkInfo {
     st_dev: u64,
@@ -242,7 +263,8 @@ impl Archiver {
                 (self.callback)(&file_entry.path)?;
                 self.path = file_entry.path;
                 self.add_entry(encoder, dir_fd, &file_entry.name, &file_entry.stat)
-                    .await.context(format!("error at {:?}", self.path))?;
+                    .await
+                    .unique_context(format!("error at {:?}", self.path))?;
             }
             self.path = old_path;
             self.entry_counter = entry_counter;
-- 
2.43.0





^ permalink raw reply	[flat|nested] 9+ messages in thread

* [pbs-devel] [PATCH v2 proxmox-backup 4/4] pxar: use anyhow::Error in PxarBackupStream
  2024-02-20 10:28 [pbs-devel] [PATCH v2 proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
                   ` (2 preceding siblings ...)
  2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox-backup 3/4] pxar: add UniqueContext helper Gabriel Goller
@ 2024-02-20 10:28 ` Gabriel Goller
  2024-04-26 12:00 ` [pbs-devel] [PATCH v2 proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
  2024-06-17  8:12 ` Gabriel Goller
  5 siblings, 0 replies; 9+ messages in thread
From: Gabriel Goller @ 2024-02-20 10:28 UTC (permalink / raw)
  To: pbs-devel

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 22a6ffdc..0fe83342 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::{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;
@@ -25,7 +25,7 @@ use pbs_datastore::catalog::CatalogWriter;
 pub struct PxarBackupStream {
     rx: Option<std::sync::mpsc::Receiver<Result<Vec<u8>, Error>>>,
     handle: Option<AbortHandle>,
-    error: Arc<Mutex<Option<String>>>,
+    error: Arc<Mutex<Option<Error>>>,
 }
 
 impl Drop for PxarBackupStream {
@@ -68,7 +68,7 @@ impl PxarBackupStream {
             .await
             {
                 let mut error = error2.lock().unwrap();
-                *error = Some(err.to_string());
+                *error = Some(err);
             }
         };
 
@@ -100,18 +100,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





^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [pbs-devel] [PATCH v2 proxmox 1/4] CLI: print fatal errors including causes
  2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox 1/4] CLI: print fatal errors including causes Gabriel Goller
@ 2024-02-21 16:45   ` Max Carrara
  2024-02-21 16:52   ` [pbs-devel] [PATCH proxmox] fixup! " Max Carrara
  1 sibling, 0 replies; 9+ messages in thread
From: Max Carrara @ 2024-02-21 16:45 UTC (permalink / raw)
  To: pbs-devel

On 2/20/24 11:28, 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`.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---

There are some lines in `README.rst` that contain only whitespace and some
things regarding ReST formatting. See comments inline.

As these things are non-issues IMO and easily fixed, I'll send in a follow-up fixup
that may be applied and rebased if desired.

>  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 0000000..9c7c213
> --- /dev/null
> +++ b/proxmox-router/README.rst
> @@ -0,0 +1,95 @@
> +================
> + proxmox-router
> +================

AFAIK, our top-level sections consist of the title just being underlined with '='s, as in:

proxmox-router
==============

> +
> +cli
> +==================

This should then instead be underlined with '-' - and the title could be prettier IMO:

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:

In this paragraph (and others) you're using backticks to denote code, but those actually
mark something as "interpreted text" [0]. Instead, double backticks (e.g. ``.with_context``)
are used to mark something as an inline literal [1] (oiw. code).

I know this may be somewhat annoying (as it's not the same as in Markdown), but that's
just how the format works.

[0]: https://docutils.sourceforge.io/docs/ref/rst/restructuredtext.html#interpreted-text
[1]: https://docutils.sourceforge.io/docs/ref/rst/restructuredtext.html#inline-literals

> +
> +.. 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;
> +    

Whitespace in this line here ^

> +    -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},
> +     };
> +    

... and here ^ and also in the remaining inline diff below.

> +    -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

Double backticks should be used here as well ...

> +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

... and here.

> +*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 7a26ffb..d5522f1 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);
>          }
>      }





^ permalink raw reply	[flat|nested] 9+ messages in thread

* [pbs-devel] [PATCH proxmox] fixup! CLI: print fatal errors including causes
  2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox 1/4] CLI: print fatal errors including causes Gabriel Goller
  2024-02-21 16:45   ` Max Carrara
@ 2024-02-21 16:52   ` Max Carrara
  1 sibling, 0 replies; 9+ messages in thread
From: Max Carrara @ 2024-02-21 16:52 UTC (permalink / raw)
  To: pbs-devel

---
 proxmox-router/README.rst | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/proxmox-router/README.rst b/proxmox-router/README.rst
index 9c7c2139..5fef633f 100644
--- a/proxmox-router/README.rst
+++ b/proxmox-router/README.rst
@@ -1,13 +1,13 @@
-================
- proxmox-router
-================
+proxmox-router
+==============
 
-cli
-==================
-To improve our error handling story, we use anyhow and `.context()` on
-all errors. This means no more `format_err()` and `format()` of errors.
+Command Line
+------------
 
-For example, with two calls to `.with_context` when bubbling up errors in
+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::
@@ -18,11 +18,11 @@ proxmox-offline-mirror:
     @@ -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 => {
@@ -41,7 +41,7 @@ proxmox-offline-mirror:
     @@ -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};
@@ -49,7 +49,7 @@ proxmox-offline-mirror:
      use proxmox_apt::deb822::CheckSums;
     @@ -45,10 +45,12 @@ impl Pool {
              }
-    
+
              if !pool.exists() {
     -            create_path(pool, None, None)?;
     +            create_path(pool, None, None)
@@ -59,7 +59,7 @@ proxmox-offline-mirror:
     -        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(),
 
@@ -77,14 +77,14 @@ Instead of the original::
 
 Which is not really helpful without knowing the path.
 
-For non-fatal cases or logging inside tasks, `{:#}` could be used which just
+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
+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'
-- 
2.39.2





^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [pbs-devel] [PATCH v2 proxmox{, -backup} 0/4] output full anyhow context in client
  2024-02-20 10:28 [pbs-devel] [PATCH v2 proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
                   ` (3 preceding siblings ...)
  2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox-backup 4/4] pxar: use anyhow::Error in PxarBackupStream Gabriel Goller
@ 2024-04-26 12:00 ` Gabriel Goller
  2024-06-17  8:12 ` Gabriel Goller
  5 siblings, 0 replies; 9+ messages in thread
From: Gabriel Goller @ 2024-04-26 12:00 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion

bump.


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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [pbs-devel] [PATCH v2 proxmox{, -backup} 0/4] output full anyhow context in client
  2024-02-20 10:28 [pbs-devel] [PATCH v2 proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
                   ` (4 preceding siblings ...)
  2024-04-26 12:00 ` [pbs-devel] [PATCH v2 proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
@ 2024-06-17  8:12 ` Gabriel Goller
  5 siblings, 0 replies; 9+ messages in thread
From: Gabriel Goller @ 2024-06-17  8:12 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion

Submitted a v3!


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


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-06-17  8:12 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-20 10:28 [pbs-devel] [PATCH v2 proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox 1/4] CLI: print fatal errors including causes Gabriel Goller
2024-02-21 16:45   ` Max Carrara
2024-02-21 16:52   ` [pbs-devel] [PATCH proxmox] fixup! " Max Carrara
2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox-backup 2/4] pxar: remove ArchiveError Gabriel Goller
2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox-backup 3/4] pxar: add UniqueContext helper Gabriel Goller
2024-02-20 10:28 ` [pbs-devel] [PATCH v2 proxmox-backup 4/4] pxar: use anyhow::Error in PxarBackupStream Gabriel Goller
2024-04-26 12:00 ` [pbs-devel] [PATCH v2 proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
2024-06-17  8:12 ` Gabriel Goller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal