From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Subject: Re: [pbs-devel] [PATCH backup v3 2/2] api: use if-let pattern for error-only handling
Date: Tue, 13 Feb 2024 15:04:49 +0100 [thread overview]
Message-ID: <1707832707.8atxyms769.astroid@yuna.none> (raw)
In-Reply-To: <20240213124329.411737-2-m.sandoval@proxmox.com>
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@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@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
next prev parent reply other threads:[~2024-02-13 14:05 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-13 12:43 [pbs-devel] [PATCH backup v3 1/2] backup-proxy: avoid block in if condition Maximiliano Sandoval
2024-02-13 12:43 ` [pbs-devel] [PATCH backup v3 2/2] api: use if-let pattern for error-only handling Maximiliano Sandoval
2024-02-13 14:04 ` Fabian Grünbichler [this message]
2024-02-13 15:49 ` Maximiliano Sandoval
2024-02-13 13:58 ` [pbs-devel] applied: [PATCH backup v3 1/2] backup-proxy: avoid block in if condition Fabian Grünbichler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1707832707.8atxyms769.astroid@yuna.none \
--to=f.gruenbichler@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.