From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 8AACB1FF17E for ; Thu, 13 Nov 2025 09:39:00 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 1FCE914DC4; Thu, 13 Nov 2025 09:39:55 +0100 (CET) Date: Thu, 13 Nov 2025 09:39:46 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20251112091856.73930-1-h.laimer@proxmox.com> In-Reply-To: <20251112091856.73930-1-h.laimer@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1763022246.tus7z1wzjz.astroid@yuna.none> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1763023163905 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.047 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup] tape: media_catalog: replace deprecated flock() with Flock X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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 > --- > 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