Re: [pbs-devel] [PATCH proxmox-backup] tape: media_catalog: replace deprecated flock() with Flock

[Hannes Laimer <h.laimer@proxmox.com>]

not sure why, but my brain skipped this mail...
anyway, thanks for taking a look! comments inline

On 11/13/25 09:39, Fabian Grünbichler wrote:
> On November 12, 2025 10:18 am, Hannes Laimer wrote:
>> nix 0.28.0 deprecated fcntl::flock() in favor of fcntl::Flock.
>> In commit() lock a cloned fd to avoid moving self.file
>>
>> No functional change intended. Fixes deprecation warnings.
>>
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>>   src/tape/media_catalog.rs | 27 ++++++++++++++++++---------
>>   1 file changed, 18 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/tape/media_catalog.rs b/src/tape/media_catalog.rs
>> index 63329a65..7307ca0f 100644
>> --- a/src/tape/media_catalog.rs
>> +++ b/src/tape/media_catalog.rs
>> @@ -9,6 +9,7 @@ use endian_trait::Endian;
>>   
>>   use proxmox_sys::fs::read_subdir;
>>   
>> +use nix::fcntl;
>>   use proxmox_io::{ReadExt, WriteExt};
>>   use proxmox_sys::fs::{create_path, fchown, CreateOptions};
>>   use proxmox_uuid::Uuid;
>> @@ -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,12 @@ 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 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 mut locked = fcntl::Flock::lock(file, nix::fcntl::FlockArg::LockExclusive)
>> +                .map_err(|(_, e)| format_err!("flock failed - {}", e))?;
> 
> this part here is fine
> 
>> +            let result = me.load_catalog(&mut locked, media_id.media_set_label.as_ref());
>> +            let file = locked
>> +                .unlock()
>> +                .map_err(|(_, e)| format_err!("flock unlock failed - {}", e))?;
> 
> but this is not - if unlocking fails, the returned Error tuple has the
> Flock instance as first member. and if you drop an Flock, it will try to
> unlock it again, and panic if that fails (which seems likely if the
> first unlock attempt failed, e.g. because the FD got closed for some
> reason, or the underlying FS went away, or ..).
> 

good catch! I think a
`std::mem::forget(_guard);`
in the mapper should fix that. As for the migration in proxmox-sys...



> if we switch to Flock here, we should also migrate our helpers in
> proxmox_sys over, and ideally, make them usable here as well ;)
> 

... I'm not sure that that would work? I don't think we could do timeout.
we could add a separate/new `safe-locking-thing` that uses the new Flock
with RAII though... but I'm not sure we want two

>>   
>>               let (found_magic_number, _) = result?;
>>   
>> @@ -371,14 +375,19 @@ impl MediaCatalog {
>>               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 file_clone = file.try_clone()?;
>> +                let mut locked =
>> +                    fcntl::Flock::lock(file_clone, nix::fcntl::FlockArg::LockExclusive)
>> +                        .map_err(|(_, e)| format_err!("flock failed - {}", e))?;
>>                   let result: Result<(), Error> = proxmox_lang::try_block!({
>> -                    file.write_all(pending)?;
>> -                    file.flush()?;
>> -                    file.sync_data()?;
>> +                    locked.write_all(pending)?;
>> +                    locked.flush()?;
>> +                    locked.sync_data()?;
>>                       Ok(())
>>                   });
>> -                nix::fcntl::flock(file.as_raw_fd(), nix::fcntl::FlockArg::Unlock)?;
>> +                let _ = locked
>> +                    .unlock()
>> +                    .map_err(|(_, e)| format_err!("flock unlock failed - {}", e))?;
> 
> same here
> 
>>   
>>                   result?;
>>               }
>> -- 
>> 2.47.3
>>
>>
>>
>> _______________________________________________
>> pbs-devel mailing list
>> pbs-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>
>>
>>
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 



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