Re: [PATCH v1 proxmox-backup 11/11] tape: media catalog: use Flock wrapper instead of deprecated function

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On February 26, 2026 3:40 pm, Robert Obkircher wrote:
> Switch to the new wrapper type to fix the deprecation warnings. Panic
> if unlock fails, because there is no easy way to return the File that
> was temporarily moved out of the field.
> 
> An alternative to moving out of the optional field would be to use
> `File::try_clone` but that isn't ideal either because duplicated file
> descriptors share the same lock status. That also doesn't solve the
> problem that propagating the error without leaking the locked file
> descriptor requires unsafe code or a change to nix.
> 
> [1] https://lore.proxmox.com/pbs-devel/1763634535.5sqd1r4buu.astroid@yuna.none/
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
>  src/tape/media_catalog.rs | 49 +++++++++++++++++++++------------------
>  1 file changed, 27 insertions(+), 22 deletions(-)
> 
> diff --git a/src/tape/media_catalog.rs b/src/tape/media_catalog.rs
> index 63329a65..372bf59d 100644
> --- a/src/tape/media_catalog.rs
> +++ b/src/tape/media_catalog.rs
> @@ -6,6 +6,7 @@ use std::path::{Path, PathBuf};
>  
>  use anyhow::{bail, format_err, Error};
>  use endian_trait::Endian;
> +use nix::fcntl::{Flock, FlockArg};
>  
>  use proxmox_sys::fs::read_subdir;
>  
> @@ -194,7 +195,7 @@ impl MediaCatalog {
>          let me = proxmox_lang::try_block!({
>              Self::create_basedir(base_path)?;
>  
> -            let mut file = std::fs::OpenOptions::new()
> +            let file = std::fs::OpenOptions::new()
>                  .read(true)
>                  .write(write)
>                  .create(create)
> @@ -219,9 +220,10 @@ impl MediaCatalog {
>              };
>  
>              // Note: lock file, to get a consistent view with load_catalog
> -            nix::fcntl::flock(file.as_raw_fd(), nix::fcntl::FlockArg::LockExclusive)?;
> +            let mut file = Flock::lock(file, FlockArg::LockExclusive)
> +                .map_err(|e| format_err!("flock failed: {e:?}"))?;
>              let result = me.load_catalog(&mut file, media_id.media_set_label.as_ref());
> -            nix::fcntl::flock(file.as_raw_fd(), nix::fcntl::FlockArg::Unlock)?;
> +            let file = file.unlock().expect("shouldn't fail for valid fd");

here we could just return an error instead of panicing? we only read
from the file anyway, and bubbling up the error should be fine..

>  
>              let (found_magic_number, _) = result?;
>  
> @@ -367,27 +369,30 @@ impl MediaCatalog {
>              return Ok(());
>          }
>  
> -        match self.file {
> -            Some(ref mut file) => {
> -                let pending = &self.pending;
> -                // Note: lock file, to get a consistent view with load_catalog
> -                nix::fcntl::flock(file.as_raw_fd(), nix::fcntl::FlockArg::LockExclusive)?;
> -                let result: Result<(), Error> = proxmox_lang::try_block!({
> -                    file.write_all(pending)?;
> -                    file.flush()?;
> -                    file.sync_data()?;
> -                    Ok(())
> -                });
> -                nix::fcntl::flock(file.as_raw_fd(), nix::fcntl::FlockArg::Unlock)?;
> -
> -                result?;
> -            }
> -            None => bail!("media catalog not writable (opened read only)"),
> -        }
> +        let Some(file) = self.file.take() else {
> +            bail!("media catalog not writable (opened read only)");
> +        };
> +
> +        let mut file = Flock::lock(file, FlockArg::LockExclusive).map_err(|(f, e)| {
> +            self.file = Some(f);
> +            format_err!("flock failed: {:?} - {e:?}", self.file)
> +        })?;
>  
> -        self.pending = Vec::new();
> +        let pending = &self.pending;
> +        // Note: lock file, to get a consistent view with load_catalog
> +        let result: Result<(), Error> = proxmox_lang::try_block!({
> +            file.write_all(pending)?;
> +            file.flush()?;
> +            file.sync_data()?;
> +            Ok(())
> +        });
>  
> -        Ok(())
> +        self.file = Some(file.unlock().expect("shouldn't fail for valid fd"));

here the situation is a bit more complicated, since we wrote to the file
above.. but looking at the call graphs/error behaviour, it seems to me
that we are often operating on temporary files (though we do not seem to
clean them up in the error path?), or failure to commit herer would
bubble up and abort writing to tape anyway..

triggering a panic here doesn't help in any case - the possibly
inconsistent writes/reads have already happened, at best we could maybe
(try to) invalidate the catalog as part of error handling to force
re-reading it from tape? or we could bubble up the error to abort
whatever operation is attempting to commit, and log the error
prominently - taking down the whole PBS instance doesn't buy us anything
AFAICT?

> +
> +        if result.is_ok() {
> +            self.pending = Vec::new();
> +        }
> +        result
>      }
>  
>      /// Conditionally commit if in pending data is large (> 1Mb)
> -- 
> 2.47.3
> 
> 
> 
> 
> 
>