Re: [pbs-devel] [PATCH 1/2] sgutils2: use thiserror to derive Error

[Dominik Csapak <d.csapak@proxmox.com>]

comment inline:

On 4/12/21 11:32, Dietmar Maurer wrote:
> ---
>   Cargo.toml            |  1 +
>   src/tools/sgutils2.rs | 42 ++++++++++--------------------------------
>   2 files changed, 11 insertions(+), 32 deletions(-)
> 
> diff --git a/Cargo.toml b/Cargo.toml
> index 1d47423e..8e85675c 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -32,6 +32,7 @@ endian_trait = { version = "0.6", features = ["arrays"] }
>   env_logger = "0.7"
>   flate2 = "1.0"
>   anyhow = "1.0"
> +thiserror = "1.0"
>   futures = "0.3"
>   h2 = { version = "0.3", features = [ "stream" ] }
>   handlebars = "3.0"
> diff --git a/src/tools/sgutils2.rs b/src/tools/sgutils2.rs
> index 3570aca7..df00fbf5 100644
> --- a/src/tools/sgutils2.rs
> +++ b/src/tools/sgutils2.rs
> @@ -17,16 +17,16 @@ use std::ffi::CStr;
>   
>   use proxmox::tools::io::ReadExt;
>   
> -#[derive(Debug)]
> +#[derive(thiserror::Error, Debug)]
>   pub struct SenseInfo {
>       pub sense_key: u8,
>       pub asc: u8,
>       pub ascq: u8,
>   }
>   
> -impl ToString for SenseInfo {
> +impl std::fmt::Display for SenseInfo {
>   
> -    fn to_string(&self) -> String {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
>   
>           let sense_text = SENSE_KEY_DESCRIPTIONS
>               .get(self.sense_key as usize)
> @@ -34,43 +34,21 @@ impl ToString for SenseInfo {
>               .unwrap_or_else(|| format!("Invalid sense {:02X}", self.sense_key));
>   
>           if self.asc == 0 && self.ascq == 0 {
> -            return sense_text;
> +            write!(f, "{}", sense_text)?;

here a return is missing, else we print sense_text twice and always
get the additional_sense_text

>           }
>   
>           let additional_sense_text = get_asc_ascq_string(self.asc, self.ascq);
>   
> -        format!("{}, {}", sense_text, additional_sense_text)
> +        write!(f, "{}, {}", sense_text, additional_sense_text)
>       }
>   }
>   
> -#[derive(Debug)]
> +#[derive(thiserror::Error, Debug)]
>   pub enum ScsiError {
> -    Error(Error),
> -    Sense(SenseInfo),
> -}
> -
> -impl std::fmt::Display for ScsiError {
> -    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> -        match self {
> -            ScsiError::Error(err) => write!(f, "{}", err),
> -            ScsiError::Sense(sense) =>  write!(f, "{}", sense.to_string()),
> -        }
> -    }
> -}
> -
> -impl std::error::Error for ScsiError {
> -    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
> -         match self {
> -             ScsiError::Error(err) => err.source(),
> -             ScsiError::Sense(_) => None,
> -         }
> -    }
> -}
> -
> -impl From<anyhow::Error> for ScsiError {
> -    fn from(error: anyhow::Error) -> Self {
> -        Self::Error(error)
> -    }
> +    #[error("{0}")]
> +    Error(#[from] Error),
> +    #[error("{0}")]
> +    Sense(#[from] SenseInfo),
>   }
>   
>   impl From<std::io::Error> for ScsiError {
>