From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 9ACAD91425 for ; Wed, 3 Apr 2024 16:53:10 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 808F01B1BF for ; Wed, 3 Apr 2024 16:52:40 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 3 Apr 2024 16:52:39 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 9B05044B4E for ; Wed, 3 Apr 2024 16:52:39 +0200 (CEST) Content-Type: text/plain; charset=UTF-8 Date: Wed, 03 Apr 2024 16:52:38 +0200 Message-Id: To: "Proxmox Backup Server development discussion" From: "Max Carrara" Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 X-Mailer: aerc 0.17.0-72-g6a84f1331f1c References: <20240403094913.107177-1-f.schauer@proxmox.com> <20240403094913.107177-7-f.schauer@proxmox.com> In-Reply-To: <20240403094913.107177-7-f.schauer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.027 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH vma-to-pbs 6/9] refactor error handling X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Apr 2024 14:53:10 -0000 On Wed Apr 3, 2024 at 11:49 AM CEST, Filip Schauer wrote: > Signed-off-by: Filip Schauer > --- > 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; > =20 > -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 VmaReader { > let mut vma_header: VmaHeader =3D bincode_options.deserialize(&b= uffer)?; > =20 > if vma_header.magic !=3D VMA_HEADER_MAGIC { > - return Err(anyhow!("Invalid magic number")); > + bail!("Invalid magic number"); > } > =20 > if vma_header.version !=3D 1 { > - return Err(anyhow!("Invalid VMA version {}", vma_header.vers= ion)); > + bail!("Invalid VMA version {}", vma_header.version); > } > =20 > buffer.resize(vma_header.header_size as usize, 0); > @@ -150,7 +150,7 @@ impl VmaReader { > let computed_md5sum: [u8; 16] =3D md5::compute(&buffer).into(); > =20 > if vma_header.md5sum !=3D computed_md5sum { > - return Err(anyhow!("Wrong VMA header checksum")); > + bail!("Wrong VMA header checksum"); > } > =20 > let blob_buffer =3D &buffer[VMA_HEADER_SIZE_NO_BLOB_BUFFER..vma_= header.header_size as usize]; > @@ -233,7 +233,7 @@ impl VmaReader { > let vma_extent_header: VmaExtentHeader =3D bincode_options.deser= ialize(&buffer)?; > =20 > if vma_extent_header.magic !=3D VMA_EXTENT_HEADER_MAGIC { > - return Err(anyhow!("Invalid magic number")); > + bail!("Invalid magic number"); > } > =20 > // Fill the MD5 sum field with zeros to compute the MD5 sum > @@ -241,7 +241,7 @@ impl VmaReader { > let computed_md5sum: [u8; 16] =3D md5::compute(&buffer).into(); > =20 > if vma_extent_header.md5sum !=3D computed_md5sum { > - return Err(anyhow!("Wrong VMA extent header checksum")); > + bail!("Wrong VMA extent header checksum"); > } > =20 > Ok(vma_extent_header) > @@ -303,7 +303,7 @@ impl VmaReader { > if ioerr.kind() =3D=3D std::io::ErrorKind::Unexp= ectedEof { > break; // Break out of the loop since the en= d 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. ;) > } > } > _ =3D> { > 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, > } > =20 > +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 =3D unsafe { CStr::from_ptr(pbs_err) }; > + let pbs_err_str =3D 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=3D...` 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 Proxm= oxBackupHandle> { > 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 > ); > =20 > if pbs.is_null() { > - let pbs_err_cstr =3D unsafe { CStr::from_ptr(pbs_err) }; > - bail!("proxmox_backup_new_ns failed: {pbs_err_cstr:?}"); > + handle_pbs_error(pbs_err, "proxmox_backup_new_ns")?; > } > =20 > Ok(pbs) > @@ -117,8 +126,7 @@ where > &mut pbs_err, > ) < 0 > { > - let pbs_err_cstr =3D unsafe { CStr::from_ptr(pbs_err) }; > - bail!("proxmox_backup_add_config failed: {pbs_err_cstr:?}"); > + handle_pbs_error(pbs_err, "proxmox_backup_add_config")?; > } > } > =20 > @@ -158,8 +166,7 @@ where > ); > =20 > if pbs_device_id < 0 { > - let pbs_err_cstr =3D unsafe { CStr::from_ptr(pbs_err) }; > - bail!("proxmox_backup_register_image failed: {pbs_err_cstr:?= }"); > + handle_pbs_error(pbs_err, "proxmox_backup_register_image")?; > } > =20 > let block_device_info =3D BlockDeviceInfo { > @@ -254,11 +261,10 @@ where > ); > =20 > if write_data_result < 0 { > - let pbs_err_cstr =3D unsafe { CStr::from_ptr(pbs_err) }; > - bail!("proxmox_backup_write_data failed: {pbs_err_cstr:?= }"); > + handle_pbs_error(pbs_err, "proxmox_backup_write_data")?; > } > =20 > - 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. > }; > =20 > let insert_image_chunk =3D |image_chunks: &mut HashMap, > @@ -325,8 +331,7 @@ where > let pbs_device_id =3D block_device_info.pbs_device_id; > =20 > if proxmox_backup_close_image(pbs, pbs_device_id, &mut pbs_err) = < 0 { > - let pbs_err_cstr =3D unsafe { CStr::from_ptr(pbs_err) }; > - bail!("proxmox_backup_close_image failed: {pbs_err_cstr:?}")= ; > + handle_pbs_error(pbs_err, "proxmox_backup_close_image")?; > } > } > =20 > @@ -353,8 +358,7 @@ pub fn backup_vma_to_pbs(args: BackupVmaToPbsArgs) ->= Result<()> { > let connect_result =3D proxmox_backup_connect(pbs, &mut pbs_err); > =20 > if connect_result < 0 { > - let pbs_err_cstr =3D unsafe { CStr::from_ptr(pbs_err) }; > - bail!("proxmox_backup_connect failed: {pbs_err_cstr:?}"); > + handle_pbs_error(pbs_err, "proxmox_backup_connect")?; > } > =20 > 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)?; > =20 > if proxmox_backup_finish(pbs, &mut pbs_err) < 0 { > - let pbs_err_cstr =3D unsafe { CStr::from_ptr(pbs_err) }; > - bail!("proxmox_backup_finish failed: {pbs_err_cstr:?}"); > + handle_pbs_error(pbs_err, "proxmox_backup_finish")?; > } > =20 > let elapsed_ms =3D SystemTime::now()