* [pbs-devel] [PATCH proxmox 1/4] CLI: print fatal errors including causes
2024-02-16 15:33 [pbs-devel] [PATCH proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
@ 2024-02-16 15:33 ` Gabriel Goller
2024-02-16 15:33 ` [pbs-devel] [PATCH proxmox-backup 2/4] pxar: remove ArchiveError Gabriel Goller
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Gabriel Goller @ 2024-02-16 15:33 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.
For example, with two calls to `.with_context` when bubbling up errors in
proxmox-offline-mirror:
> ----8<----
> 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(),
> ---->8----
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.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
proxmox-router/src/cli/command.rs | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
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] 12+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 2/4] pxar: remove ArchiveError
2024-02-16 15:33 [pbs-devel] [PATCH proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
2024-02-16 15:33 ` [pbs-devel] [PATCH proxmox 1/4] CLI: print fatal errors including causes Gabriel Goller
@ 2024-02-16 15:33 ` Gabriel Goller
2024-02-16 15:33 ` [pbs-devel] [PATCH proxmox-backup 3/4] pxar: add UniqueContext helper Gabriel Goller
` (3 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Gabriel Goller @ 2024-02-16 15:33 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] 12+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 3/4] pxar: add UniqueContext helper
2024-02-16 15:33 [pbs-devel] [PATCH proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
2024-02-16 15:33 ` [pbs-devel] [PATCH proxmox 1/4] CLI: print fatal errors including causes Gabriel Goller
2024-02-16 15:33 ` [pbs-devel] [PATCH proxmox-backup 2/4] pxar: remove ArchiveError Gabriel Goller
@ 2024-02-16 15:33 ` Gabriel Goller
2024-02-16 17:44 ` Max Carrara
2024-02-16 15:33 ` [pbs-devel] [PATCH proxmox-backup 4/4] pxar: use anyhow::Error in PxarBackupStream Gabriel Goller
` (2 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Gabriel Goller @ 2024-02-16 15:33 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 | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index 06f396e0..7bec35c4 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,8 +89,30 @@ 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))
+ },
+ }
}
}
@@ -242,7 +265,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.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] 12+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 3/4] pxar: add UniqueContext helper
2024-02-16 15:33 ` [pbs-devel] [PATCH proxmox-backup 3/4] pxar: add UniqueContext helper Gabriel Goller
@ 2024-02-16 17:44 ` Max Carrara
0 siblings, 0 replies; 12+ messages in thread
From: Max Carrara @ 2024-02-16 17:44 UTC (permalink / raw)
To: pbs-devel
On 2/16/24 16:33, Gabriel Goller wrote:
> 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, 24 insertions(+), 1 deletion(-)
>
> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
> index 06f396e0..7bec35c4 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,8 +89,30 @@ 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))
> + },
> + }
> }
> }
The trait plus the impl could perhaps be inlined inside the body of
`Archiver::archive_dir_contents`, as `unique_context()` is only used in there,
but I think it's also fine if it says where it is. Looks good otherwise!
>
> @@ -242,7 +265,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.context(format!("error at {:?}", self.path))?;
> + .await.unique_context(format!("error at {:?}", self.path))?;
> }
> self.path = old_path;
> self.entry_counter = entry_counter;
^ permalink raw reply [flat|nested] 12+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 4/4] pxar: use anyhow::Error in PxarBackupStream
2024-02-16 15:33 [pbs-devel] [PATCH proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
` (2 preceding siblings ...)
2024-02-16 15:33 ` [pbs-devel] [PATCH proxmox-backup 3/4] pxar: add UniqueContext helper Gabriel Goller
@ 2024-02-16 15:33 ` Gabriel Goller
2024-02-16 17:47 ` Max Carrara
2024-02-16 17:49 ` [pbs-devel] [PATCH proxmox{, -backup} 0/4] output full anyhow context in client Max Carrara
2024-02-20 10:30 ` Gabriel Goller
5 siblings, 1 reply; 12+ messages in thread
From: Gabriel Goller @ 2024-02-16 15:33 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 | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/pbs-client/src/pxar_backup_stream.rs b/pbs-client/src/pxar_backup_stream.rs
index 22a6ffdc..4dbfd472 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,20 @@ 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 error.is_some() {
+ let err = error.take().unwrap();
+ 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 error.is_some() {
+ let err = error.take().unwrap();
+ return Poll::Ready(Some(Err(err)));
}
Poll::Ready(None) // channel closed, no error
}
--
2.43.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 4/4] pxar: use anyhow::Error in PxarBackupStream
2024-02-16 15:33 ` [pbs-devel] [PATCH proxmox-backup 4/4] pxar: use anyhow::Error in PxarBackupStream Gabriel Goller
@ 2024-02-16 17:47 ` Max Carrara
2024-02-19 10:53 ` Gabriel Goller
0 siblings, 1 reply; 12+ messages in thread
From: Max Carrara @ 2024-02-16 17:47 UTC (permalink / raw)
To: pbs-devel
On 2/16/24 16:33, Gabriel Goller wrote:
> 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 | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/pbs-client/src/pxar_backup_stream.rs b/pbs-client/src/pxar_backup_stream.rs
> index 22a6ffdc..4dbfd472 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,20 @@ 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))));
This here:
> + let mut error = self.error.lock().unwrap();
> + if error.is_some() {
> + let err = error.take().unwrap();
> + return Poll::Ready(Some(Err(err)));
> }
... could be replaced with the following:
let mut error = self.error.lock().unwrap();
if let Some(err) = error.take() {
return Poll::Ready(Some(Err(err)));
}
`Option::take()` also returns an `Option`, allowing you to omit the `unwrap()`.
> }
>
> 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 error.is_some() {
> + let err = error.take().unwrap();
> + return Poll::Ready(Some(Err(err)));
> }
Same as above here.
> Poll::Ready(None) // channel closed, no error
> }
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 4/4] pxar: use anyhow::Error in PxarBackupStream
2024-02-16 17:47 ` Max Carrara
@ 2024-02-19 10:53 ` Gabriel Goller
0 siblings, 0 replies; 12+ messages in thread
From: Gabriel Goller @ 2024-02-19 10:53 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On Fri Feb 16, 2024 at 6:47 PM CET, Max Carrara wrote:
> On 2/16/24 16:33, Gabriel Goller wrote:
> > + let mut error = self.error.lock().unwrap();
> > + if error.is_some() {
> > + let err = error.take().unwrap();
> > + return Poll::Ready(Some(Err(err)));
> > }
>
> ... could be replaced with the following:
>
> let mut error = self.error.lock().unwrap();
> if let Some(err) = error.take() {
> return Poll::Ready(Some(Err(err)));
> }
>
> `Option::take()` also returns an `Option`, allowing you to omit the `unwrap()`.
Good point!
Will be fixed in the next version!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [pbs-devel] [PATCH proxmox{, -backup} 0/4] output full anyhow context in client
2024-02-16 15:33 [pbs-devel] [PATCH proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
` (3 preceding siblings ...)
2024-02-16 15:33 ` [pbs-devel] [PATCH proxmox-backup 4/4] pxar: use anyhow::Error in PxarBackupStream Gabriel Goller
@ 2024-02-16 17:49 ` Max Carrara
2024-02-16 17:55 ` Max Carrara
2024-02-20 10:30 ` Gabriel Goller
5 siblings, 1 reply; 12+ messages in thread
From: Max Carrara @ 2024-02-16 17:49 UTC (permalink / raw)
To: pbs-devel
On 2/16/24 16:33, Gabriel Goller wrote:
> 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.
>
Some comments inline, but otherwise a good way to clean up some of our
error handling and how we display things. Will test on Monday, but apart
from what I've already mentioned, looks pretty good!
Reviewed-by: Max Carrara <m.carrara@proxmox.com>
>
>
> proxmox:
>
> Fabian Grünbichler (1):
> CLI: print fatal errors including causes
>
> proxmox-router/src/cli/command.rs | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>
> proxmox-backup:
>
> Gabriel Goller (3):
> pxar: remove ArchiveError
> pxar: add UniqueContext helper
> pxar: use anyhow::Error in PxarBackupStream
>
> pbs-client/src/pxar/create.rs | 49 ++++++++++++++--------------
> pbs-client/src/pxar_backup_stream.rs | 20 +++++++-----
> 2 files changed, 35 insertions(+), 34 deletions(-)
>
>
> Summary over all repositories:
> 3 files changed, 37 insertions(+), 36 deletions(-)
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [pbs-devel] [PATCH proxmox{, -backup} 0/4] output full anyhow context in client
2024-02-16 17:49 ` [pbs-devel] [PATCH proxmox{, -backup} 0/4] output full anyhow context in client Max Carrara
@ 2024-02-16 17:55 ` Max Carrara
2024-02-19 10:54 ` Gabriel Goller
0 siblings, 1 reply; 12+ messages in thread
From: Max Carrara @ 2024-02-16 17:55 UTC (permalink / raw)
To: pbs-devel
On 2/16/24 18:49, Max Carrara wrote:
> On 2/16/24 16:33, Gabriel Goller wrote:
>> 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.
>>
>
> Some comments inline, but otherwise a good way to clean up some of our
> error handling and how we display things. Will test on Monday, but apart
> from what I've already mentioned, looks pretty good!
>
> Reviewed-by: Max Carrara <m.carrara@proxmox.com>
Oh, one more thing I forgot to mention: Run `cargo fmt` please ;)
>
>>
>>
>> proxmox:
>>
>> Fabian Grünbichler (1):
>> CLI: print fatal errors including causes
>>
>> proxmox-router/src/cli/command.rs | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>>
>> proxmox-backup:
>>
>> Gabriel Goller (3):
>> pxar: remove ArchiveError
>> pxar: add UniqueContext helper
>> pxar: use anyhow::Error in PxarBackupStream
>>
>> pbs-client/src/pxar/create.rs | 49 ++++++++++++++--------------
>> pbs-client/src/pxar_backup_stream.rs | 20 +++++++-----
>> 2 files changed, 35 insertions(+), 34 deletions(-)
>>
>>
>> Summary over all repositories:
>> 3 files changed, 37 insertions(+), 36 deletions(-)
>>
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [pbs-devel] [PATCH proxmox{, -backup} 0/4] output full anyhow context in client
2024-02-16 15:33 [pbs-devel] [PATCH proxmox{, -backup} 0/4] output full anyhow context in client Gabriel Goller
` (4 preceding siblings ...)
2024-02-16 17:49 ` [pbs-devel] [PATCH proxmox{, -backup} 0/4] output full anyhow context in client Max Carrara
@ 2024-02-20 10:30 ` Gabriel Goller
5 siblings, 0 replies; 12+ messages in thread
From: Gabriel Goller @ 2024-02-20 10:30 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
sent a v2!
^ permalink raw reply [flat|nested] 12+ messages in thread