[pbs-devel] [PATCH backup v3 2/2] api: use if-let pattern for error-only handling
Fabian Grünbichler
f.gruenbichler at proxmox.com
Tue Feb 13 15:04:49 CET 2024
On February 13, 2024 1:43 pm, Maximiliano Sandoval wrote:
> It is more readable than using match. We also inline variables in
> eprintln!.
this does a lot more than what it says here..
>
> Signed-off-by: Maximiliano Sandoval <m.sandoval at proxmox.com>
> ---
> pbs-client/src/pxar/dir_stack.rs | 9 ++--
> pbs-pxar-fuse/src/lib.rs | 47 ++++++++++++---------
> src/api2/access/user.rs | 20 ++-------
> src/bin/proxmox-daily-update.rs | 7 +--
> src/tape/pool_writer/new_chunks_iterator.rs | 9 ++--
> src/tools/parallel_handler.rs | 11 ++---
> src/traffic_control_cache.rs | 7 +--
> 7 files changed, 45 insertions(+), 65 deletions(-)
>
> diff --git a/pbs-client/src/pxar/dir_stack.rs b/pbs-client/src/pxar/dir_stack.rs
> index 43cbee1d..616d7545 100644
> --- a/pbs-client/src/pxar/dir_stack.rs
> +++ b/pbs-client/src/pxar/dir_stack.rs
> @@ -40,16 +40,13 @@ impl PxarDir {
> parent: RawFd,
> allow_existing_dirs: bool,
> ) -> Result<BorrowedFd, Error> {
> - match mkdirat(
> + if let Err(err) = mkdirat(
> parent,
> self.file_name.as_os_str(),
> perms_from_metadata(&self.metadata)?,
> ) {
> - Ok(()) => (),
> - Err(err) => {
> - if !(allow_existing_dirs && err.already_exists()) {
> - return Err(err.into());
> - }
> + if !(allow_existing_dirs && err.already_exists()) {
> + return Err(err.into());
fine
> }
> }
>
> diff --git a/pbs-pxar-fuse/src/lib.rs b/pbs-pxar-fuse/src/lib.rs
> index 98d28c57..5c2b6b49 100644
> --- a/pbs-pxar-fuse/src/lib.rs
> +++ b/pbs-pxar-fuse/src/lib.rs
> @@ -525,9 +525,11 @@ impl SessionImpl {
> let file = file?.decode_entry().await?;
> let stat = to_stat(to_inode(&file), &file)?;
> let name = file.file_name();
> - match request.add_entry(name, &stat, next, 1, f64::MAX, f64::MAX)? {
> - ReplyBufState::Ok => (),
> - ReplyBufState::Full => return Ok(lookups),
> + if request
> + .add_entry(name, &stat, next, 1, f64::MAX, f64::MAX)?
> + .is_full()
> + {
> + return Ok(lookups);
not error handling, but semantically equivalent to if-let, okay..
> }
> lookups.push(self.make_lookup(request.inode, stat.st_ino, &file)?);
> }
> @@ -537,9 +539,11 @@ impl SessionImpl {
> let file = dir.lookup_self().await?;
> let stat = to_stat(to_inode(&file), &file)?;
> let name = OsStr::new(".");
> - match request.add_entry(name, &stat, next, 1, f64::MAX, f64::MAX)? {
> - ReplyBufState::Ok => (),
> - ReplyBufState::Full => return Ok(lookups),
> + if request
> + .add_entry(name, &stat, next, 1, f64::MAX, f64::MAX)?
> + .is_full()
> + {
> + return Ok(lookups);
same
> }
> lookups.push(LookupRef::clone(&dir_lookup));
> }
> @@ -551,9 +555,11 @@ impl SessionImpl {
> let file = parent_dir.lookup_self().await?;
> let stat = to_stat(to_inode(&file), &file)?;
> let name = OsStr::new("..");
> - match request.add_entry(name, &stat, next, 1, f64::MAX, f64::MAX)? {
> - ReplyBufState::Ok => (),
> - ReplyBufState::Full => return Ok(lookups),
> + if request
> + .add_entry(name, &stat, next, 1, f64::MAX, f64::MAX)?
> + .is_full()
> + {
> + return Ok(lookups);
same
> }
> lookups.push(lookup);
> }
> @@ -618,24 +624,25 @@ impl SessionImpl {
> ) -> Result<ReplyBufState, Error> {
> let xattrs = self.listxattrs(request.inode).await?;
>
> - for entry in xattrs {
> - match request.add_c_string(entry.name()) {
> - ReplyBufState::Ok => (),
> - ReplyBufState::Full => return Ok(ReplyBufState::Full),
> - }
> + if xattrs
> + .iter()
> + .any(|entry| request.add_c_string(entry.name()).is_full())
> + {
> + Ok(ReplyBufState::Full)
> + } else {
> + Ok(ReplyBufState::Ok)
this is not an if-let at all (and also not error handling ;)), even
though the end result is of course the same..
> }
> -
> - Ok(ReplyBufState::Ok)
> }
>
> async fn getxattr(&self, inode: u64, xattr: &OsStr) -> Result<pxar::format::XAttr, Error> {
> // TODO: pxar::Accessor could probably get a more optimized method to fetch a specific
> // xattr for an entry...
> let xattrs = self.listxattrs(inode).await?;
> - for entry in xattrs {
> - if entry.name().to_bytes() == xattr.as_bytes() {
> - return Ok(entry);
> - }
> + if xattrs
> + .iter()
> + .any(|entry| request.add_c_string(entry.name()).is_full())
> + {
> + return Ok(entry);
but this here is an obvious copy paste error that doesn't even compile!
please check patches before sending, even if they are supposedly trivial
style fixes!
> }
> io_return!(libc::ENODATA);
> }
> diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
> index 118838ce..a0be6111 100644
> --- a/src/api2/access/user.rs
> +++ b/src/api2/access/user.rs
> @@ -381,28 +381,16 @@ pub fn delete_user(userid: Userid, digest: Option<String>) -> Result<(), Error>
> pbs_config::user::save_config(&config)?;
>
> let authenticator = crate::auth::lookup_authenticator(userid.realm())?;
> - match authenticator.remove_password(userid.name()) {
> - Ok(()) => {}
> - Err(err) => {
> - eprintln!(
> - "error removing password after deleting user {:?}: {}",
> - userid, err
> - );
> - }
> + if let Err(err) = authenticator.remove_password(userid.name()) {
> + eprintln!("error removing password after deleting user {userid:?}: {err}",);
this one seems fine
> }
>
> - match crate::config::tfa::read().and_then(|mut cfg| {
> + if let Err(err) = crate::config::tfa::read().and_then(|mut cfg| {
> let _: proxmox_tfa::api::NeedsSaving =
> cfg.remove_user(&crate::config::tfa::UserAccess, userid.as_str())?;
> crate::config::tfa::write(&cfg)
> }) {
> - Ok(()) => (),
> - Err(err) => {
> - eprintln!(
> - "error updating TFA config after deleting user {:?}: {}",
> - userid, err
> - );
> - }
> + eprintln!("error updating TFA config after deleting user {userid:?} {err}",);
this one as well
> }
>
> Ok(())
> diff --git a/src/bin/proxmox-daily-update.rs b/src/bin/proxmox-daily-update.rs
> index ae3744c5..c22609c5 100644
> --- a/src/bin/proxmox-daily-update.rs
> +++ b/src/bin/proxmox-daily-update.rs
> @@ -55,11 +55,8 @@ async fn do_update(rpcenv: &mut dyn RpcEnvironment) -> Result<(), Error> {
> _ => unreachable!(),
> };
>
> - match check_acme_certificates(rpcenv).await {
> - Ok(()) => (),
> - Err(err) => {
> - log::error!("error checking certificates: {}", err);
> - }
> + if let Err(err) = check_acme_certificates(rpcenv).await {
> + log::error!("error checking certificates: {err}");
okay as well
> }
>
> // TODO: cleanup tasks like in PVE?
> diff --git a/src/tape/pool_writer/new_chunks_iterator.rs b/src/tape/pool_writer/new_chunks_iterator.rs
> index ae75b7b1..1454b33d 100644
> --- a/src/tape/pool_writer/new_chunks_iterator.rs
> +++ b/src/tape/pool_writer/new_chunks_iterator.rs
> @@ -57,12 +57,9 @@ impl NewChunksIterator {
>
> let blob = datastore.load_chunk(&digest)?;
> //println!("LOAD CHUNK {}", hex::encode(&digest));
> - match tx.send(Ok(Some((digest, blob)))) {
> - Ok(()) => {}
> - Err(err) => {
> - eprintln!("could not send chunk to reader thread: {}", err);
> - break;
> - }
> + if let Err(err) = tx.send(Ok(Some((digest, blob)))) {
> + eprintln!("could not send chunk to reader thread: {err}");
> + break;
okay
> }
>
> chunk_index.insert(digest);
> diff --git a/src/tools/parallel_handler.rs b/src/tools/parallel_handler.rs
> index c4316ad0..17f70179 100644
> --- a/src/tools/parallel_handler.rs
> +++ b/src/tools/parallel_handler.rs
> @@ -80,13 +80,10 @@ impl<I: Send + 'static> ParallelHandler<I> {
> Ok(data) => data,
> Err(_) => return,
> };
> - match (handler_fn)(data) {
> - Ok(()) => (),
> - Err(err) => {
> - let mut guard = abort.lock().unwrap();
> - if guard.is_none() {
> - *guard = Some(err.to_string());
> - }
> + if let Err(err) = (handler_fn)(data) {
> + let mut guard = abort.lock().unwrap();
> + if guard.is_none() {
> + *guard = Some(err.to_string());
okay
> }
> }
> })
> diff --git a/src/traffic_control_cache.rs b/src/traffic_control_cache.rs
> index 2e097d70..4c3bccee 100644
> --- a/src/traffic_control_cache.rs
> +++ b/src/traffic_control_cache.rs
> @@ -164,11 +164,8 @@ impl TrafficControlCache {
> self.last_traffic_control_generation = traffic_control_generation;
> self.last_update = now;
>
> - match self.reload_impl() {
> - Ok(()) => (),
> - Err(err) => {
> - log::error!("TrafficControlCache::reload failed -> {}", err);
> - }
> + if let Err(err) = self.reload_impl() {
> + log::error!("TrafficControlCache::reload failed -> {err}");
okay
> }
> }
>
> --
> 2.39.2
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
More information about the pbs-devel
mailing list