* [pbs-devel] [PATCH proxmox-backup] tape: media_catalog: replace deprecated flock() with Flock
@ 2025-11-12 9:18 Hannes Laimer
2025-11-13 8:39 ` Fabian Grünbichler
0 siblings, 1 reply; 2+ messages in thread
From: Hannes Laimer @ 2025-11-12 9:18 UTC (permalink / raw)
To: pbs-devel
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))?;
+ 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))?;
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))?;
result?;
}
--
2.47.3
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup] tape: media_catalog: replace deprecated flock() with Flock
2025-11-12 9:18 [pbs-devel] [PATCH proxmox-backup] tape: media_catalog: replace deprecated flock() with Flock Hannes Laimer
@ 2025-11-13 8:39 ` Fabian Grünbichler
0 siblings, 0 replies; 2+ messages in thread
From: Fabian Grünbichler @ 2025-11-13 8:39 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
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 ..).
if we switch to Flock here, we should also migrate our helpers in
proxmox_sys over, and ideally, make them usable here as well ;)
>
> 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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-11-13 8:39 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-11-12 9:18 [pbs-devel] [PATCH proxmox-backup] tape: media_catalog: replace deprecated flock() with Flock Hannes Laimer
2025-11-13 8:39 ` Fabian Grünbichler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox