Re: [pbs-devel] [PATCH proxmox-backup] api: config: use guard for unmounting on failed datastore creation

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Thu, Mar 20, 2025 at 10:48:51AM +0100, Hannes Laimer wrote:
> Currently if any `?`/`bail!` happens between mounting and completing
> the creation process unmounting will be skipped. Adding this guard
> solves that problem and makes it easier to add things in the future
> without having to worry about a disk not being unmounted in case of a
> failed creation.
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> Reported-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
>  src/api2/config/datastore.rs | 86 ++++++++++++++++++++++--------------
>  1 file changed, 52 insertions(+), 34 deletions(-)
> 
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index fe3260f6d..99c699814 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -1,7 +1,7 @@
>  use std::path::{Path, PathBuf};
>  
>  use ::serde::{Deserialize, Serialize};
> -use anyhow::{bail, format_err, Error};
> +use anyhow::{bail, Error};
>  use hex::FromHex;
>  use serde_json::Value;
>  use tracing::warn;
> @@ -70,6 +70,33 @@ pub fn list_datastores(
>      Ok(list.into_iter().filter(filter_by_privs).collect())
>  }
>  
> +struct UnmountGuard {
> +    path: PathBuf,
> +    should_unmount: bool,

Instead of the extra `bool` it would be cheaper if we just make `path`
an `Option<PathBuf>`.

> +}
> +
> +impl UnmountGuard {
> +    fn new(path: PathBuf, should_unmount: bool) -> Self {
> +        UnmountGuard {
> +            path,
> +            should_unmount,
> +        }
> +    }
> +    fn disable(&mut self) {

This could take `self` by-move since we don't intend to re-enable it and
then we don't need the guard to be mutable down where it's used.

> +        self.should_unmount = false;
> +    }
> +}
> +
> +impl Drop for UnmountGuard {
> +    fn drop(&mut self) {
> +        if self.should_unmount {
> +            if let Err(e) = unmount_by_mountpoint(&self.path) {
> +                warn!("could not unmount device: {e}");
> +            }
> +        }
> +    }
> +}
> +
>  pub(crate) fn do_create_datastore(
>      _lock: BackupLockGuard,
>      mut config: SectionConfigData,
> @@ -87,59 +114,50 @@ pub(crate) fn do_create_datastore(
>          param_bail!("path", err);
>      }
>  
> -    let need_unmount = datastore.backing_device.is_some();
> -    if need_unmount {
> -        do_mount_device(datastore.clone())?;
> -    };
> -
>      let tuning: DatastoreTuning = serde_json::from_value(
>          DatastoreTuning::API_SCHEMA
>              .parse_property_string(datastore.tuning.as_deref().unwrap_or(""))?,
>      )?;
>  
> -    let res = if reuse_datastore {
> -        ChunkStore::verify_chunkstore(&path)
> +    let mut unmount_guard = if datastore.backing_device.is_some() {
> +        do_mount_device(datastore.clone())?;
> +        UnmountGuard::new(path.clone(), true)
> +    } else {
> +        UnmountGuard::new(path.clone(), false)

With `path` as `Option` this could be a cheap `UnmountGuard::new(None)`.

> +    };
> +
> +    if reuse_datastore {
> +        ChunkStore::verify_chunkstore(&path)?;
>      } else {
> -        let mut is_empty = true;
>          if let Ok(dir) = std::fs::read_dir(&path) {
>              for file in dir {
>                  let name = file?.file_name();
>                  let name = name.to_str();
>                  if !name.is_some_and(|name| name.starts_with('.') || name == "lost+found") {
> -                    is_empty = false;
> -                    break;
> +                    bail!("datastore path not empty");
>                  }
>              }
>          }
> -        if is_empty {
> -            let backup_user = pbs_config::backup_user()?;
> -            ChunkStore::create(
> -                &datastore.name,
> -                path.clone(),
> -                backup_user.uid,
> -                backup_user.gid,
> -                tuning.sync_level.unwrap_or_default(),
> -            )
> -            .map(|_| ())
> -        } else {
> -            Err(format_err!("datastore path not empty"))
> -        }
> +        let backup_user = pbs_config::backup_user()?;
> +        ChunkStore::create(
> +            &datastore.name,
> +            path.clone(),
> +            backup_user.uid,
> +            backup_user.gid,
> +            tuning.sync_level.unwrap_or_default(),
> +        )
> +        .map(|_| ())?;
>      };
>  
> -    if res.is_err() {
> -        if need_unmount {
> -            if let Err(e) = unmount_by_mountpoint(&path) {
> -                warn!("could not unmount device: {e}");
> -            }
> -        }
> -        return res;
> -    }
> -
>      config.set_data(&datastore.name, "datastore", &datastore)?;
>  
>      pbs_config::datastore::save_config(&config)?;
>  
> -    jobstate::create_state_file("garbage_collection", &datastore.name)
> +    jobstate::create_state_file("garbage_collection", &datastore.name)?;
> +
> +    unmount_guard.disable();
> +
> +    Ok(())
>  }
>  
>  #[api(
> -- 
> 2.39.5


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel