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

[Max Carrara <m.carrara@proxmox.com>]

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