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

[pbs-devel] [PATCH proxmox{, -backup} 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.



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

-- 
murpp v0.4.0

[pbs-devel] [PATCH proxmox 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.

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

[pbs-devel] [PATCH proxmox-backup 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 | 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

[pbs-devel] [PATCH proxmox-backup 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, 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

[pbs-devel] [PATCH proxmox-backup 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 | 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

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

[Max Carrara]

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;

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

[Max Carrara]

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

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

[Max Carrara]

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

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

[Max Carrara]

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

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

[Gabriel Goller]

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!

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

[Gabriel Goller]

On Fri Feb 16, 2024 at 6:55 PM CET, Max Carrara wrote:
> On 2/16/24 18:49, Max Carrara wrote:
> Oh, one more thing I forgot to mention: Run `cargo fmt` please ;)

Done!
Thanks for the review!

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

[Gabriel Goller]

sent a v2!