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

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

On November 20, 2025 11:11 am, Hannes Laimer wrote:
> 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...

I guess we cannot really do much more, yeah.. in practice this should
only happen if we closed the FD behind the Flock's back, or the FS went
away in the meantime..

>> 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

well, we do want to get rid of the deprecation there as well..

I am not sure why the timeout wouldn't be possible? but yes, it would
need a different interface, since it would "consume" the file and return
a lock guard.. but that would mean we could use the helper here, whereas
we can't use the current one ;)

> 
>>>   
>>>               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