Re: [pbs-devel] [PATCH vma-to-pbs 6/9] refactor error handling

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

On Wed Apr 3, 2024 at 11:49 AM CEST, Filip Schauer wrote:
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  src/vma.rs     | 14 +++++++-------
>  src/vma2pbs.rs | 33 ++++++++++++++++++---------------
>  2 files changed, 25 insertions(+), 22 deletions(-)
>
> diff --git a/src/vma.rs b/src/vma.rs
> index 447e8db..f7944cc 100644
> --- a/src/vma.rs
> +++ b/src/vma.rs
> @@ -3,7 +3,7 @@ use std::io::Read;
>  use std::mem::size_of;
>  use std::str;
>  
> -use anyhow::{anyhow, bail, Result};
> +use anyhow::{bail, Context, Result};
>  use bincode::Options;
>  use serde::{Deserialize, Serialize};
>  use serde_big_array::BigArray;
> @@ -135,11 +135,11 @@ impl<T: Read> VmaReader<T> {
>          let mut vma_header: VmaHeader = bincode_options.deserialize(&buffer)?;
>  
>          if vma_header.magic != VMA_HEADER_MAGIC {
> -            return Err(anyhow!("Invalid magic number"));
> +            bail!("Invalid magic number");
>          }
>  
>          if vma_header.version != 1 {
> -            return Err(anyhow!("Invalid VMA version {}", vma_header.version));
> +            bail!("Invalid VMA version {}", vma_header.version);
>          }
>  
>          buffer.resize(vma_header.header_size as usize, 0);
> @@ -150,7 +150,7 @@ impl<T: Read> VmaReader<T> {
>          let computed_md5sum: [u8; 16] = md5::compute(&buffer).into();
>  
>          if vma_header.md5sum != computed_md5sum {
> -            return Err(anyhow!("Wrong VMA header checksum"));
> +            bail!("Wrong VMA header checksum");
>          }
>  
>          let blob_buffer = &buffer[VMA_HEADER_SIZE_NO_BLOB_BUFFER..vma_header.header_size as usize];
> @@ -233,7 +233,7 @@ impl<T: Read> VmaReader<T> {
>          let vma_extent_header: VmaExtentHeader = bincode_options.deserialize(&buffer)?;
>  
>          if vma_extent_header.magic != VMA_EXTENT_HEADER_MAGIC {
> -            return Err(anyhow!("Invalid magic number"));
> +            bail!("Invalid magic number");
>          }
>  
>          // Fill the MD5 sum field with zeros to compute the MD5 sum
> @@ -241,7 +241,7 @@ impl<T: Read> VmaReader<T> {
>          let computed_md5sum: [u8; 16] = md5::compute(&buffer).into();
>  
>          if vma_extent_header.md5sum != computed_md5sum {
> -            return Err(anyhow!("Wrong VMA extent header checksum"));
> +            bail!("Wrong VMA extent header checksum");
>          }
>  
>          Ok(vma_extent_header)
> @@ -303,7 +303,7 @@ impl<T: Read> VmaReader<T> {
>                          if ioerr.kind() == std::io::ErrorKind::UnexpectedEof {
>                              break; // Break out of the loop since the end of the file was reached.
>                          } else {
> -                            return Err(anyhow!("Failed to read VMA file: {}", ioerr));
> +                            return Err(anyhow::format_err!(e)).context("Failed to read VMA file");

You can import `format_err` directly for this. ;)

>                          }
>                      }
>                      _ => {
> diff --git a/src/vma2pbs.rs b/src/vma2pbs.rs
> index 5b9e20f..9483f6e 100644
> --- a/src/vma2pbs.rs
> +++ b/src/vma2pbs.rs
> @@ -39,6 +39,16 @@ struct BlockDeviceInfo {
>      pub device_size: u64,
>  }
>  
> +fn handle_pbs_error(pbs_err: *mut c_char, function_name: &str) -> Result<()> {
> +    if pbs_err.is_null() {
> +        bail!("{function_name} failed without error message");
> +    }
> +
> +    let pbs_err_cstr = unsafe { CStr::from_ptr(pbs_err) };
> +    let pbs_err_str = pbs_err_cstr.to_string_lossy();
> +    bail!("{function_name} failed: {pbs_err_str}");
> +}
> +

Absolutely great that you added this! At the same time however, you
could've just added it in patch 04 where you introduced all the other
changes.

The hunks in this file ('vma2pbs.rs') can most likely just be shoved
into patch 04 with `git commit --fixup=...` followed by
`git rebase -i --autosquash ...`.

Alternatively, you can also use `git absorb` [0] instead of making
manual fixups.

This doesn't just go for the hunks here - check if this applies to some
other changes you've made as well.

Fixups and rebasing is a great way to keep your own history clean if
you're not already doing that :)

[0]: https://github.com/tummychow/git-absorb

>  fn create_pbs_backup_task(args: BackupVmaToPbsArgs) -> Result<*mut ProxmoxBackupHandle> {
>      println!("PBS repository: {}", args.pbs_repository);
>      println!("PBS fingerprint: {}", args.fingerprint);
> @@ -88,8 +98,7 @@ fn create_pbs_backup_task(args: BackupVmaToPbsArgs) -> Result<*mut ProxmoxBackup
>      );
>  
>      if pbs.is_null() {
> -        let pbs_err_cstr = unsafe { CStr::from_ptr(pbs_err) };
> -        bail!("proxmox_backup_new_ns failed: {pbs_err_cstr:?}");
> +        handle_pbs_error(pbs_err, "proxmox_backup_new_ns")?;
>      }
>  
>      Ok(pbs)
> @@ -117,8 +126,7 @@ where
>              &mut pbs_err,
>          ) < 0
>          {
> -            let pbs_err_cstr = unsafe { CStr::from_ptr(pbs_err) };
> -            bail!("proxmox_backup_add_config failed: {pbs_err_cstr:?}");
> +            handle_pbs_error(pbs_err, "proxmox_backup_add_config")?;
>          }
>      }
>  
> @@ -158,8 +166,7 @@ where
>          );
>  
>          if pbs_device_id < 0 {
> -            let pbs_err_cstr = unsafe { CStr::from_ptr(pbs_err) };
> -            bail!("proxmox_backup_register_image failed: {pbs_err_cstr:?}");
> +            handle_pbs_error(pbs_err, "proxmox_backup_register_image")?;
>          }
>  
>          let block_device_info = BlockDeviceInfo {
> @@ -254,11 +261,10 @@ where
>              );
>  
>              if write_data_result < 0 {
> -                let pbs_err_cstr = unsafe { CStr::from_ptr(pbs_err) };
> -                bail!("proxmox_backup_write_data failed: {pbs_err_cstr:?}");
> +                handle_pbs_error(pbs_err, "proxmox_backup_write_data")?;
>              }
>  
> -            Ok(())
> +            Ok::<(), anyhow::Error>(())

As mentioned in my response to your cover letter, we usually don't
import `anyhow::Result` but instead import `anyhow::Error` - so you
don't need to fully qualify `Error` here once it's imported.

>          };
>  
>          let insert_image_chunk = |image_chunks: &mut HashMap<u64, ImageChunk>,
> @@ -325,8 +331,7 @@ where
>          let pbs_device_id = block_device_info.pbs_device_id;
>  
>          if proxmox_backup_close_image(pbs, pbs_device_id, &mut pbs_err) < 0 {
> -            let pbs_err_cstr = unsafe { CStr::from_ptr(pbs_err) };
> -            bail!("proxmox_backup_close_image failed: {pbs_err_cstr:?}");
> +            handle_pbs_error(pbs_err, "proxmox_backup_close_image")?;
>          }
>      }
>  
> @@ -353,8 +358,7 @@ pub fn backup_vma_to_pbs(args: BackupVmaToPbsArgs) -> Result<()> {
>      let connect_result = proxmox_backup_connect(pbs, &mut pbs_err);
>  
>      if connect_result < 0 {
> -        let pbs_err_cstr = unsafe { CStr::from_ptr(pbs_err) };
> -        bail!("proxmox_backup_connect failed: {pbs_err_cstr:?}");
> +        handle_pbs_error(pbs_err, "proxmox_backup_connect")?;
>      }
>  
>      println!("Connected to Proxmox Backup Server");
> @@ -365,8 +369,7 @@ pub fn backup_vma_to_pbs(args: BackupVmaToPbsArgs) -> Result<()> {
>      upload_block_devices(vma_reader, pbs)?;
>  
>      if proxmox_backup_finish(pbs, &mut pbs_err) < 0 {
> -        let pbs_err_cstr = unsafe { CStr::from_ptr(pbs_err) };
> -        bail!("proxmox_backup_finish failed: {pbs_err_cstr:?}");
> +        handle_pbs_error(pbs_err, "proxmox_backup_finish")?;
>      }
>  
>      let elapsed_ms = SystemTime::now()