From bdc7e8e1098e4b2a3587207b8f8e0c5efa6042ce Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Tue, 10 Oct 2023 19:45:04 -0400 Subject: [PATCH 01/33] Added object for automatic unlock on drop. --- src/fcntl.rs | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/fcntl.rs b/src/fcntl.rs index 5dd05751cf..490f5dd9e0 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -631,6 +631,42 @@ pub fn flock(fd: RawFd, arg: FlockArg) -> Result<()> { Errno::result(res).map(drop) } + +/// Represents a file lock on a particular [RawFd], which unlocks when dropped. +/// +/// See [fcntl::flock] for details on locking semantics. +#[cfg(not(any(target_os = "redox", target_os = "solaris")))] +#[derive(Debug, Eq, Hash, PartialEq)] +pub struct Flock(RawFd); + +#[cfg(not(any(target_os = "redox", target_os = "solaris")))] +impl Drop for Flock { + fn drop(&mut self) { + _ = flock(self.0, FlockArg::Unlock); + } +} + +#[cfg(not(any(target_os = "redox", target_os = "solaris")))] +impl Flock { + /// Lock the given `fd` using [flock]. + /// + /// # Example + /// ``` + /// # use std::os::fd::RawFd; + /// # use nix::fcntl::{Flock, FlockArg}; + /// fn do_stuff(fd: RawFd) -> nix::Result<()> { + /// let lock = Flock::lock(fd, FlockArg::LockExclusive)?; + /// + /// // Do stuff + /// + /// Ok(()) + /// } // File is unlocked once `lock` goes out of scope. + pub fn lock(fd: RawFd, args: FlockArg) -> Result { + flock(fd, args)?; + + Ok(Self(fd)) + } +} } #[cfg(any(target_os = "android", target_os = "linux"))] From 174013113682db493116baf7eb274755c7232a12 Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Tue, 10 Oct 2023 19:52:51 -0400 Subject: [PATCH 02/33] Added appropriate changelog file. --- changelog/2170.added.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/2170.added.md diff --git a/changelog/2170.added.md b/changelog/2170.added.md new file mode 100644 index 0000000000..5875b84f5a --- /dev/null +++ b/changelog/2170.added.md @@ -0,0 +1 @@ +Added newtype `Flock` to automatically unlock a held flock upon drop. From aa8369d046bde1fc93c72d4c1db44ee70b5e678c Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Tue, 10 Oct 2023 19:53:48 -0400 Subject: [PATCH 03/33] Fixed doc error on `Flock`. --- src/fcntl.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index 490f5dd9e0..0fc6e1bed2 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -634,7 +634,7 @@ pub fn flock(fd: RawFd, arg: FlockArg) -> Result<()> { /// Represents a file lock on a particular [RawFd], which unlocks when dropped. /// -/// See [fcntl::flock] for details on locking semantics. +/// See [flock] for details on locking semantics. #[cfg(not(any(target_os = "redox", target_os = "solaris")))] #[derive(Debug, Eq, Hash, PartialEq)] pub struct Flock(RawFd); From a566bb2f46cd9e3025e3b65bf66ab540da610127 Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Sat, 14 Oct 2023 11:00:12 -0400 Subject: [PATCH 04/33] Requested changes to make `fcntl::flock` private and OwnedFd instead of RawFd. --- changelog/2170.removed.md | 1 + src/fcntl.rs | 19 ++++++++++--------- 2 files changed, 11 insertions(+), 9 deletions(-) create mode 100644 changelog/2170.removed.md diff --git a/changelog/2170.removed.md b/changelog/2170.removed.md new file mode 100644 index 0000000000..f038b47cc5 --- /dev/null +++ b/changelog/2170.removed.md @@ -0,0 +1 @@ +Made `fcntl::flock` private in favor of new Flock wrapper. diff --git a/src/fcntl.rs b/src/fcntl.rs index 0fc6e1bed2..69207b443a 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -14,7 +14,7 @@ use std::ffi::OsString; #[cfg(not(target_os = "redox"))] use std::os::raw; use std::os::unix::ffi::OsStringExt; -use std::os::unix::io::RawFd; +use std::os::unix::io::{OwnedFd, RawFd}; // For splice and copy_file_range #[cfg(any( target_os = "netbsd", @@ -611,7 +611,7 @@ pub enum FlockArg { } #[cfg(not(any(target_os = "redox", target_os = "solaris")))] -pub fn flock(fd: RawFd, arg: FlockArg) -> Result<()> { +fn flock(fd: RawFd, arg: FlockArg) -> Result<()> { use self::FlockArg::*; let res = unsafe { @@ -636,13 +636,14 @@ pub fn flock(fd: RawFd, arg: FlockArg) -> Result<()> { /// /// See [flock] for details on locking semantics. #[cfg(not(any(target_os = "redox", target_os = "solaris")))] -#[derive(Debug, Eq, Hash, PartialEq)] -pub struct Flock(RawFd); +#[derive(Debug)] +pub struct Flock(OwnedFd); #[cfg(not(any(target_os = "redox", target_os = "solaris")))] impl Drop for Flock { fn drop(&mut self) { - _ = flock(self.0, FlockArg::Unlock); + // Result is ignored because flock has no documented failure cases. + _ = flock(self.0.as_raw_fd(), FlockArg::Unlock); } } @@ -652,17 +653,17 @@ impl Flock { /// /// # Example /// ``` - /// # use std::os::fd::RawFd; + /// # use std::os::unix::io::OwnedFd; /// # use nix::fcntl::{Flock, FlockArg}; - /// fn do_stuff(fd: RawFd) -> nix::Result<()> { + /// fn do_stuff(fd: OwnedFd) -> nix::Result<()> { /// let lock = Flock::lock(fd, FlockArg::LockExclusive)?; /// /// // Do stuff /// /// Ok(()) /// } // File is unlocked once `lock` goes out of scope. - pub fn lock(fd: RawFd, args: FlockArg) -> Result { - flock(fd, args)?; + pub fn lock(fd: OwnedFd, args: FlockArg) -> Result { + flock(fd.as_raw_fd(), args)?; Ok(Self(fd)) } From 7912e8867dd5def08e2962f1693913b5e5d6b069 Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Sat, 14 Oct 2023 11:01:31 -0400 Subject: [PATCH 05/33] Indent fix. --- src/fcntl.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index 69207b443a..a96cb6e874 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -642,7 +642,7 @@ pub struct Flock(OwnedFd); #[cfg(not(any(target_os = "redox", target_os = "solaris")))] impl Drop for Flock { fn drop(&mut self) { - // Result is ignored because flock has no documented failure cases. + // Result is ignored because flock has no documented failure cases. _ = flock(self.0.as_raw_fd(), FlockArg::Unlock); } } From d2340546dfea4c2bc5b381654bd2d8b11dbf2280 Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Sat, 14 Oct 2023 11:13:36 -0400 Subject: [PATCH 06/33] Removed doc links to private item, updated imports. --- src/fcntl.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index a96cb6e874..8208156c2d 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -24,11 +24,7 @@ use std::os::unix::io::{OwnedFd, RawFd}; all(target_os = "freebsd", target_arch = "x86_64"), ))] use std::path::PathBuf; -#[cfg(any( - target_os = "android", - target_os = "freebsd", - target_os = "linux" -))] +#[cfg(unix)] use std::{ os::unix::io::{AsFd, AsRawFd}, ptr, @@ -634,7 +630,7 @@ fn flock(fd: RawFd, arg: FlockArg) -> Result<()> { /// Represents a file lock on a particular [RawFd], which unlocks when dropped. /// -/// See [flock] for details on locking semantics. +/// See flock(2) for details on locking semantics. #[cfg(not(any(target_os = "redox", target_os = "solaris")))] #[derive(Debug)] pub struct Flock(OwnedFd); @@ -649,7 +645,7 @@ impl Drop for Flock { #[cfg(not(any(target_os = "redox", target_os = "solaris")))] impl Flock { - /// Lock the given `fd` using [flock]. + /// Lock the given `fd`. /// /// # Example /// ``` From e0c53bbfc81334ae45601bf19ba8f932ff9d2fec Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Sat, 14 Oct 2023 11:29:24 -0400 Subject: [PATCH 07/33] More import fixes. --- src/fcntl.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index 8208156c2d..e17fcf8584 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -24,11 +24,20 @@ use std::os::unix::io::{OwnedFd, RawFd}; all(target_os = "freebsd", target_arch = "x86_64"), ))] use std::path::PathBuf; -#[cfg(unix)] +#[cfg(any( + target_os = "android", + target_os = "freebsd", + target_os = "linux" +))] use std::{ - os::unix::io::{AsFd, AsRawFd}, + os::unix::io::AsFd, ptr, }; +#[cfg(all( + not(any(target_os = "redox", target_os = "solaris")), + unix +))] +use std::os::unix::io::AsRawFd; #[cfg(feature = "fs")] use crate::{sys::stat::Mode, NixPath, Result}; From ad58d40976b498bac9d40700939968dfe4d8378b Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Sat, 14 Oct 2023 11:32:14 -0400 Subject: [PATCH 08/33] Format changes. --- src/fcntl.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index e17fcf8584..09c0cbad05 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -16,6 +16,8 @@ use std::os::raw; use std::os::unix::ffi::OsStringExt; use std::os::unix::io::{OwnedFd, RawFd}; // For splice and copy_file_range +#[cfg(all(not(any(target_os = "redox", target_os = "solaris")), unix))] +use std::os::unix::io::AsRawFd; #[cfg(any( target_os = "netbsd", target_os = "macos", @@ -29,15 +31,7 @@ use std::path::PathBuf; target_os = "freebsd", target_os = "linux" ))] -use std::{ - os::unix::io::AsFd, - ptr, -}; -#[cfg(all( - not(any(target_os = "redox", target_os = "solaris")), - unix -))] -use std::os::unix::io::AsRawFd; +use std::{os::unix::io::AsFd, ptr}; #[cfg(feature = "fs")] use crate::{sys::stat::Mode, NixPath, Result}; From 647059d4c23e7d135a78794f35b38a14452fd6c3 Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Sat, 14 Oct 2023 11:50:31 -0400 Subject: [PATCH 09/33] Remove unused import for redox. --- src/fcntl.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index 09c0cbad05..b2a419fa63 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -14,10 +14,10 @@ use std::ffi::OsString; #[cfg(not(target_os = "redox"))] use std::os::raw; use std::os::unix::ffi::OsStringExt; -use std::os::unix::io::{OwnedFd, RawFd}; +use std::os::unix::io::RawFd; // For splice and copy_file_range #[cfg(all(not(any(target_os = "redox", target_os = "solaris")), unix))] -use std::os::unix::io::AsRawFd; +use std::os::unix::io::{AsRawFd, OwnedFd}; #[cfg(any( target_os = "netbsd", target_os = "macos", From 7cf67654d9633664844ca53c12968c2e1fecc2b0 Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Sun, 19 Nov 2023 13:46:41 -0500 Subject: [PATCH 10/33] Added `Flockable` trait per discussions, added tests for `Flock`. --- changelog/2170.added.md | 1 + src/fcntl.rs | 85 +++++++++++++++++++++++++++++++++++------ test/test_fcntl.rs | 54 ++++++++++++++++++++++++++ 3 files changed, 128 insertions(+), 12 deletions(-) diff --git a/changelog/2170.added.md b/changelog/2170.added.md index 5875b84f5a..6d089a120a 100644 --- a/changelog/2170.added.md +++ b/changelog/2170.added.md @@ -1 +1,2 @@ Added newtype `Flock` to automatically unlock a held flock upon drop. +Added `Flockable` trait to represent valid types for `Flock`. diff --git a/src/fcntl.rs b/src/fcntl.rs index b2a419fa63..38e7733bcf 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -11,13 +11,17 @@ use libc::{self, c_int, c_uint, size_t, ssize_t}; ))] use std::ffi::CStr; use std::ffi::OsString; +#[cfg(not(any(target_os = "redox", target_os = "solaris")))] +use std::mem::ManuallyDrop; +#[cfg(not(any(target_os = "redox", target_os = "solaris")))] +use std::ops::{Deref, DerefMut}; #[cfg(not(target_os = "redox"))] use std::os::raw; use std::os::unix::ffi::OsStringExt; use std::os::unix::io::RawFd; // For splice and copy_file_range #[cfg(all(not(any(target_os = "redox", target_os = "solaris")), unix))] -use std::os::unix::io::{AsRawFd, OwnedFd}; +use std::os::unix::io::AsRawFd; #[cfg(any( target_os = "netbsd", target_os = "macos", @@ -631,15 +635,22 @@ fn flock(fd: RawFd, arg: FlockArg) -> Result<()> { Errno::result(res).map(drop) } -/// Represents a file lock on a particular [RawFd], which unlocks when dropped. +/// Represents valid types for flock. +/// +/// # Safety +/// `T` must be `!Clone`. +pub unsafe trait Flockable: AsRawFd {} + +/// Represents an owned flock, which unlocks on drop. /// /// See flock(2) for details on locking semantics. +// `ManuallyDrop` is necessary to circumvent move out of `Drop` type error. #[cfg(not(any(target_os = "redox", target_os = "solaris")))] #[derive(Debug)] -pub struct Flock(OwnedFd); +pub struct Flock(ManuallyDrop); #[cfg(not(any(target_os = "redox", target_os = "solaris")))] -impl Drop for Flock { +impl Drop for Flock { fn drop(&mut self) { // Result is ignored because flock has no documented failure cases. _ = flock(self.0.as_raw_fd(), FlockArg::Unlock); @@ -647,26 +658,76 @@ impl Drop for Flock { } #[cfg(not(any(target_os = "redox", target_os = "solaris")))] -impl Flock { - /// Lock the given `fd`. +impl Deref for Flock { + type Target = T; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} +#[cfg(not(any(target_os = "redox", target_os = "solaris")))] +impl DerefMut for Flock { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +#[cfg(not(any(target_os = "redox", target_os = "solaris")))] +impl Flock { + /// Obtain a/an flock. /// /// # Example /// ``` - /// # use std::os::unix::io::OwnedFd; + /// # use std::fs::File; /// # use nix::fcntl::{Flock, FlockArg}; - /// fn do_stuff(fd: OwnedFd) -> nix::Result<()> { - /// let lock = Flock::lock(fd, FlockArg::LockExclusive)?; + /// fn do_stuff(file: File) -> nix::Result<()> { + /// let lock = match Flock::lock(file, FlockArg::LockExclusive) { + /// Ok(l) => l, + /// Err((_, e)) => return Err(e), + /// }; /// /// // Do stuff /// /// Ok(()) /// } // File is unlocked once `lock` goes out of scope. - pub fn lock(fd: OwnedFd, args: FlockArg) -> Result { - flock(fd.as_raw_fd(), args)?; + pub fn lock(t: T, args: FlockArg) -> std::result::Result { + match flock(t.as_raw_fd(), args) { + Ok(_) => Ok(Self(ManuallyDrop::new(t))), + Err(e) => Err((t, e)), + } + } - Ok(Self(fd)) + /// Remove the lock and return the object wrapped within. + /// + /// # Example + /// ``` + /// # use std::fs::File; + /// # use nix::fcntl::{Flock, FlockArg}; + /// fn do_stuff(file: File) -> nix::Result<()> { + /// let lock = match Flock::lock(file, FlockArg::LockExclusive) { + /// Ok(l) => l, + /// Err((_,e)) => return Err(e), + /// }; + /// + /// // Do critical section + /// + /// // Unlock + /// let file = lock.unlock(); + /// + /// // Do anything else + /// + /// Ok(()) + /// } // File is unlocked once `lock` goes out of scope. + pub fn unlock(mut self) -> T { + _ = flock(self.0.as_raw_fd(), FlockArg::Unlock); + + // Safety: `self.0` never used again. + unsafe { ManuallyDrop::take(&mut self.0) } } } + +// Safety: `File` is not [std::clone::Clone]. +unsafe impl Flockable for std::fs::File {} } #[cfg(any(target_os = "android", target_os = "linux"))] diff --git a/test/test_fcntl.rs b/test/test_fcntl.rs index 13b6485eb3..174fd9cc3d 100644 --- a/test/test_fcntl.rs +++ b/test/test_fcntl.rs @@ -603,3 +603,57 @@ fn test_f_kinfo() { assert_ne!(res, -1); assert_eq!(path, tmp.path()); } + +/// Test `Flock` and associated functions. +/// +#[cfg(not(any(target_os = "redox", target_os = "solaris")))] +mod test_flock { + use nix::fcntl::*; + use tempfile::NamedTempFile; + + /// Verify that `Flock::lock()` correctly obtains a lock, and subsequently unlocks upon drop. + #[test] + fn verify_lock_and_drop() { + // Get 2 `File` handles to same underlying file. + let file1 = NamedTempFile::new().unwrap(); + let file2 = file1.reopen().unwrap(); + let file1 = file1.into_file(); + + // Lock first handle + let lock1 = Flock::lock(file1, FlockArg::LockExclusive).unwrap(); + + // Attempt to lock second handle + let file2 = match Flock::lock(file2, FlockArg::LockExclusiveNonblock) { + Ok(_) => panic!("Expected second exclusive lock to fail."), + Err((f, _)) => f, + }; + + // Drop first lock + std::mem::drop(lock1); + + // Attempt to lock second handle again (but successfully) + if let Err(_) = Flock::lock(file2, FlockArg::LockExclusiveNonblock) { + panic!("Expected locking to be successful."); + } + } + + /// Verify that `Flock::unlock()` correctly obtains unlocks. + #[test] + fn verify_unlock() { + // Get 2 `File` handles to same underlying file. + let file1 = NamedTempFile::new().unwrap(); + let file2 = file1.reopen().unwrap(); + let file1 = file1.into_file(); + + // Lock first handle + let lock1 = Flock::lock(file1, FlockArg::LockExclusive).unwrap(); + + // Unlock and retain file so any erroneous flocks also remain present. + let _file1 = lock1.unlock(); + + // Attempt to lock second handle. + if let Err(_) = Flock::lock(file2, FlockArg::LockExclusiveNonblock) { + panic!("Expected locking to be successful."); + } + } +} From 99e9f6c4014488f31b6c0c6cab1bb9253d0ad072 Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Sun, 19 Nov 2023 13:57:00 -0500 Subject: [PATCH 11/33] Simplified flock error conditionals. --- test/test_fcntl.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_fcntl.rs b/test/test_fcntl.rs index 174fd9cc3d..895e7709b3 100644 --- a/test/test_fcntl.rs +++ b/test/test_fcntl.rs @@ -632,7 +632,7 @@ mod test_flock { std::mem::drop(lock1); // Attempt to lock second handle again (but successfully) - if let Err(_) = Flock::lock(file2, FlockArg::LockExclusiveNonblock) { + if Flock::lock(file2, FlockArg::LockExclusiveNonblock).is_err() { panic!("Expected locking to be successful."); } } @@ -652,7 +652,7 @@ mod test_flock { let _file1 = lock1.unlock(); // Attempt to lock second handle. - if let Err(_) = Flock::lock(file2, FlockArg::LockExclusiveNonblock) { + if Flock::lock(file2, FlockArg::LockExclusiveNonblock).is_err() { panic!("Expected locking to be successful."); } } From 32443fa303dec09cecadbdfcb10f20789b0204bb Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Sun, 19 Nov 2023 14:27:23 -0500 Subject: [PATCH 12/33] Added missing cfg flags for `Flockable`. --- src/fcntl.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/fcntl.rs b/src/fcntl.rs index 38e7733bcf..ca9f295db9 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -639,6 +639,7 @@ fn flock(fd: RawFd, arg: FlockArg) -> Result<()> { /// /// # Safety /// `T` must be `!Clone`. +#[cfg(not(any(target_os = "redox", target_os = "solaris")))] pub unsafe trait Flockable: AsRawFd {} /// Represents an owned flock, which unlocks on drop. From 81ca0e5845d4234b9b9024a312c2543871ad887a Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Sun, 19 Nov 2023 14:32:47 -0500 Subject: [PATCH 13/33] Added missing cfg flags for impl of `Flockable` on `File`. --- src/fcntl.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/fcntl.rs b/src/fcntl.rs index ca9f295db9..353feaae71 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -728,6 +728,7 @@ impl Flock { } // Safety: `File` is not [std::clone::Clone]. +#[cfg(not(any(target_os = "redox", target_os = "solaris")))] unsafe impl Flockable for std::fs::File {} } From 5704efe3fb63474c61c5881a98d6bb140723b317 Mon Sep 17 00:00:00 2001 From: Chris Date: Mon, 20 Nov 2023 22:52:20 +0000 Subject: [PATCH 14/33] Update src/fcntl.rs Co-authored-by: SteveLauC --- src/fcntl.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index 353feaae71..f77b85deee 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -718,7 +718,7 @@ impl Flock { /// // Do anything else /// /// Ok(()) - /// } // File is unlocked once `lock` goes out of scope. + /// } pub fn unlock(mut self) -> T { _ = flock(self.0.as_raw_fd(), FlockArg::Unlock); From 2103ec7d98b9dea03046826085b385ba1672582e Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Tue, 21 Nov 2023 19:06:45 -0500 Subject: [PATCH 15/33] Updated `Flockable` comment per suggestion. --- src/fcntl.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index a6957e612a..960fbef266 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -636,7 +636,7 @@ fn flock(fd: RawFd, arg: FlockArg) -> Result<()> { /// Represents valid types for flock. /// /// # Safety -/// `T` must be `!Clone`. +/// Types implementing this must be `!Clone`. #[cfg(not(any(target_os = "redox", target_os = "solaris")))] pub unsafe trait Flockable: AsRawFd {} From 5609d07674b108eb011e1897b5a8e0ce630b6de3 Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Tue, 21 Nov 2023 20:18:26 -0500 Subject: [PATCH 16/33] Finalized `FlockArg` as enum and removed TODO accordingly, removed flock wrapper entirely. --- src/fcntl.rs | 41 +++++++++++------------------------------ 1 file changed, 11 insertions(+), 30 deletions(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index 960fbef266..da8c9a1f52 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -599,38 +599,13 @@ pub fn fcntl(fd: RawFd, arg: FcntlArg) -> Result { Errno::result(res) } -// TODO: convert to libc_enum #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] #[non_exhaustive] pub enum FlockArg { LockShared, LockExclusive, - Unlock, LockSharedNonblock, LockExclusiveNonblock, - UnlockNonblock, -} - -#[cfg(not(any(target_os = "redox", target_os = "solaris")))] -fn flock(fd: RawFd, arg: FlockArg) -> Result<()> { - use self::FlockArg::*; - - let res = unsafe { - match arg { - LockShared => libc::flock(fd, libc::LOCK_SH), - LockExclusive => libc::flock(fd, libc::LOCK_EX), - Unlock => libc::flock(fd, libc::LOCK_UN), - LockSharedNonblock => { - libc::flock(fd, libc::LOCK_SH | libc::LOCK_NB) - } - LockExclusiveNonblock => { - libc::flock(fd, libc::LOCK_EX | libc::LOCK_NB) - } - UnlockNonblock => libc::flock(fd, libc::LOCK_UN | libc::LOCK_NB), - } - }; - - Errno::result(res).map(drop) } /// Represents valid types for flock. @@ -652,7 +627,7 @@ pub struct Flock(ManuallyDrop); impl Drop for Flock { fn drop(&mut self) { // Result is ignored because flock has no documented failure cases. - _ = flock(self.0.as_raw_fd(), FlockArg::Unlock); + _ = unsafe { libc::flock(self.0.as_raw_fd(), libc::LOCK_UN) }; } } @@ -690,9 +665,15 @@ impl Flock { /// Ok(()) /// } // File is unlocked once `lock` goes out of scope. pub fn lock(t: T, args: FlockArg) -> std::result::Result { - match flock(t.as_raw_fd(), args) { - Ok(_) => Ok(Self(ManuallyDrop::new(t))), - Err(e) => Err((t, e)), + let flags = match args { + FlockArg::LockShared => libc::LOCK_SH, + FlockArg::LockExclusive => libc::LOCK_EX, + FlockArg::LockSharedNonblock => libc::LOCK_SH | libc::LOCK_NB, + FlockArg::LockExclusiveNonblock => libc::LOCK_EX | libc::LOCK_NB, + }; + match unsafe { libc::flock(t.as_raw_fd(), flags) } { + 0 => Ok(Self(ManuallyDrop::new(t))), + e @ _ => Err((t, Errno::from_i32(e))), } } @@ -718,7 +699,7 @@ impl Flock { /// Ok(()) /// } pub fn unlock(mut self) -> T { - _ = flock(self.0.as_raw_fd(), FlockArg::Unlock); + _ = unsafe { libc::flock(self.0.as_raw_fd(), libc::LOCK_UN) }; // Safety: `self.0` never used again. unsafe { ManuallyDrop::take(&mut self.0) } From 2eaca17d40055e8cc4cf26a620f71f1fde81f8ab Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Tue, 21 Nov 2023 20:46:25 -0500 Subject: [PATCH 17/33] Implemented `Flockable` for `OwnedFd` as well. --- src/fcntl.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index da8c9a1f52..d0908f7cda 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -20,7 +20,7 @@ use std::os::unix::ffi::OsStringExt; use std::os::unix::io::RawFd; // For splice and copy_file_range #[cfg(all(not(any(target_os = "redox", target_os = "solaris")), unix))] -use std::os::unix::io::AsRawFd; +use std::os::unix::io::{AsRawFd, OwnedFd}; #[cfg(any( target_os = "netbsd", apple_targets, @@ -709,6 +709,10 @@ impl Flock { // Safety: `File` is not [std::clone::Clone]. #[cfg(not(any(target_os = "redox", target_os = "solaris")))] unsafe impl Flockable for std::fs::File {} + +// Safety: `OwnedFd` is not [std::clone::Clone]. +#[cfg(not(any(target_os = "redox", target_os = "solaris")))] +unsafe impl Flockable for OwnedFd {} } #[cfg(any(target_os = "android", target_os = "linux"))] From 4919bba425f182b2426b0155f801dc4b16554e86 Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Tue, 21 Nov 2023 20:49:39 -0500 Subject: [PATCH 18/33] Fixed linting error. --- src/fcntl.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index d0908f7cda..76b4534a04 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -673,7 +673,7 @@ impl Flock { }; match unsafe { libc::flock(t.as_raw_fd(), flags) } { 0 => Ok(Self(ManuallyDrop::new(t))), - e @ _ => Err((t, Errno::from_i32(e))), + e => Err((t, Errno::from_i32(e))), } } From 91c9590fca8f6dfd48cc288b0f7155a12874932a Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Tue, 21 Nov 2023 21:20:24 -0500 Subject: [PATCH 19/33] Updated changelog accordingly. --- changelog/2170.removed.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/changelog/2170.removed.md b/changelog/2170.removed.md index f038b47cc5..abc411ffea 100644 --- a/changelog/2170.removed.md +++ b/changelog/2170.removed.md @@ -1 +1,2 @@ -Made `fcntl::flock` private in favor of new Flock wrapper. +Removed `fcntl::flock` in favor of new `Flock` wrapper. +Removed unlock options from `fcntl::FlockArg` From d61c038ae088ae299f349e40337cd20cb0dd4bbd Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Tue, 21 Nov 2023 21:22:13 -0500 Subject: [PATCH 20/33] Corrected errno logic in `Flock::lock`. --- src/fcntl.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index 76b4534a04..11b6510c90 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -671,9 +671,9 @@ impl Flock { FlockArg::LockSharedNonblock => libc::LOCK_SH | libc::LOCK_NB, FlockArg::LockExclusiveNonblock => libc::LOCK_EX | libc::LOCK_NB, }; - match unsafe { libc::flock(t.as_raw_fd(), flags) } { - 0 => Ok(Self(ManuallyDrop::new(t))), - e => Err((t, Errno::from_i32(e))), + match Errno::result(unsafe { libc::flock(t.as_raw_fd(), flags) }) { + Ok(_) => Ok(Self(ManuallyDrop::new(t))), + Err(errno) => Err((t, errno)), } } From 40fda24cfdcccce5b2437e203075ccc651a0870c Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Tue, 21 Nov 2023 21:52:29 -0500 Subject: [PATCH 21/33] Properly dropped inner type for `Flock`. --- src/fcntl.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/fcntl.rs b/src/fcntl.rs index 11b6510c90..b88fb95ab3 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -628,6 +628,9 @@ impl Drop for Flock { fn drop(&mut self) { // Result is ignored because flock has no documented failure cases. _ = unsafe { libc::flock(self.0.as_raw_fd(), libc::LOCK_UN) }; + + // Safety: Neither `self` nor `self.0` will be used again. + unsafe { ManuallyDrop::drop(&mut self.0); } } } From b544cd15192f3840df8428de0b013f0c3ad9e61e Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Tue, 21 Nov 2023 22:18:04 -0500 Subject: [PATCH 22/33] Replaced `ManuallyDrop` with `Option` as inner `Flock` type to avoid double-free. --- src/fcntl.rs | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index b88fb95ab3..04cd90a1cb 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -11,8 +11,6 @@ use libc::{self, c_int, c_uint, size_t, ssize_t}; use std::ffi::CStr; use std::ffi::OsString; #[cfg(not(any(target_os = "redox", target_os = "solaris")))] -use std::mem::ManuallyDrop; -#[cfg(not(any(target_os = "redox", target_os = "solaris")))] use std::ops::{Deref, DerefMut}; #[cfg(not(target_os = "redox"))] use std::os::raw; @@ -621,16 +619,18 @@ pub unsafe trait Flockable: AsRawFd {} // `ManuallyDrop` is necessary to circumvent move out of `Drop` type error. #[cfg(not(any(target_os = "redox", target_os = "solaris")))] #[derive(Debug)] -pub struct Flock(ManuallyDrop); +pub struct Flock(Option); #[cfg(not(any(target_os = "redox", target_os = "solaris")))] impl Drop for Flock { fn drop(&mut self) { - // Result is ignored because flock has no documented failure cases. - _ = unsafe { libc::flock(self.0.as_raw_fd(), libc::LOCK_UN) }; - - // Safety: Neither `self` nor `self.0` will be used again. - unsafe { ManuallyDrop::drop(&mut self.0); } + match self.0 { + Some(ref t) => { + // Result is ignored because flock has no documented failure cases. + _ = unsafe { libc::flock(t.as_raw_fd(), libc::LOCK_UN) }; + }, + None => {} + } } } @@ -639,13 +639,18 @@ impl Deref for Flock { type Target = T; fn deref(&self) -> &Self::Target { - &self.0 - } + match self.0 { + Some(ref t) => t, + None => unreachable!(), + } } } #[cfg(not(any(target_os = "redox", target_os = "solaris")))] impl DerefMut for Flock { fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.0 + match self.0 { + Some(ref mut t) => t, + None => unreachable!(), + } } } @@ -675,7 +680,7 @@ impl Flock { FlockArg::LockExclusiveNonblock => libc::LOCK_EX | libc::LOCK_NB, }; match Errno::result(unsafe { libc::flock(t.as_raw_fd(), flags) }) { - Ok(_) => Ok(Self(ManuallyDrop::new(t))), + Ok(_) => Ok(Self(Some(t))), Err(errno) => Err((t, errno)), } } @@ -702,10 +707,9 @@ impl Flock { /// Ok(()) /// } pub fn unlock(mut self) -> T { - _ = unsafe { libc::flock(self.0.as_raw_fd(), libc::LOCK_UN) }; + _ = unsafe { libc::flock(self.0.as_ref().unwrap().as_raw_fd(), libc::LOCK_UN) }; - // Safety: `self.0` never used again. - unsafe { ManuallyDrop::take(&mut self.0) } + self.0.take().unwrap() } } From 91273fbd473f1fc85a222984518ad6a1c863b0e6 Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Tue, 21 Nov 2023 22:23:49 -0500 Subject: [PATCH 23/33] Fixed linting errors. --- src/fcntl.rs | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index 04cd90a1cb..b61662a1cc 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -624,12 +624,9 @@ pub struct Flock(Option); #[cfg(not(any(target_os = "redox", target_os = "solaris")))] impl Drop for Flock { fn drop(&mut self) { - match self.0 { - Some(ref t) => { - // Result is ignored because flock has no documented failure cases. - _ = unsafe { libc::flock(t.as_raw_fd(), libc::LOCK_UN) }; - }, - None => {} + if let Some(ref t) = self.0 { + // Result is ignored because flock has no documented failure cases. + _ = unsafe { libc::flock(t.as_raw_fd(), libc::LOCK_UN) }; } } } @@ -639,18 +636,17 @@ impl Deref for Flock { type Target = T; fn deref(&self) -> &Self::Target { - match self.0 { - Some(ref t) => t, - None => unreachable!(), - } } + if let Some(ref t) = self.0 { + t + } else { unreachable!() } + } } #[cfg(not(any(target_os = "redox", target_os = "solaris")))] impl DerefMut for Flock { fn deref_mut(&mut self) -> &mut Self::Target { - match self.0 { - Some(ref mut t) => t, - None => unreachable!(), - } + if let Some(ref mut t) = self.0 { + t + } else { unreachable!() } } } From 836986c4e1dc31aa93ca1f65c7f7b390e02b6700 Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Wed, 22 Nov 2023 16:46:44 -0500 Subject: [PATCH 24/33] Removed unnecessary cfg condition, updated documentation. --- src/fcntl.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index b61662a1cc..fab66e3962 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -16,8 +16,7 @@ use std::ops::{Deref, DerefMut}; use std::os::raw; use std::os::unix::ffi::OsStringExt; use std::os::unix::io::RawFd; -// For splice and copy_file_range -#[cfg(all(not(any(target_os = "redox", target_os = "solaris")), unix))] +#[cfg(not(any(target_os = "redox", target_os = "solaris")))] use std::os::unix::io::{AsRawFd, OwnedFd}; #[cfg(any( target_os = "netbsd", @@ -616,7 +615,6 @@ pub unsafe trait Flockable: AsRawFd {} /// Represents an owned flock, which unlocks on drop. /// /// See flock(2) for details on locking semantics. -// `ManuallyDrop` is necessary to circumvent move out of `Drop` type error. #[cfg(not(any(target_os = "redox", target_os = "solaris")))] #[derive(Debug)] pub struct Flock(Option); @@ -667,7 +665,7 @@ impl Flock { /// // Do stuff /// /// Ok(()) - /// } // File is unlocked once `lock` goes out of scope. + /// } pub fn lock(t: T, args: FlockArg) -> std::result::Result { let flags = match args { FlockArg::LockShared => libc::LOCK_SH, From 723c38660da02b1a58070716047eaac74c7c04ef Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Wed, 22 Nov 2023 18:42:55 -0500 Subject: [PATCH 25/33] Modified Flock behavior for drop() and unlock(). --- src/fcntl.rs | 28 ++++++++++++++++++++-------- test/test_fcntl.rs | 2 +- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index fab66e3962..c2a397200f 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -623,8 +623,11 @@ pub struct Flock(Option); impl Drop for Flock { fn drop(&mut self) { if let Some(ref t) = self.0 { - // Result is ignored because flock has no documented failure cases. - _ = unsafe { libc::flock(t.as_raw_fd(), libc::LOCK_UN) }; + if let Err(_) = Errno::result(unsafe { libc::flock(t.as_raw_fd(), libc::LOCK_UN) }) { + if !std::thread::panicking() { + panic!("Failed to remove flock."); + } + } } } } @@ -693,17 +696,26 @@ impl Flock { /// /// // Do critical section /// - /// // Unlock - /// let file = lock.unlock(); + /// // Unlock (don't continue until unlocked) + /// let file = loop { + /// if let Ok(f) = lock.unlock(false) { + /// break f + /// } + /// }; /// /// // Do anything else /// /// Ok(()) /// } - pub fn unlock(mut self) -> T { - _ = unsafe { libc::flock(self.0.as_ref().unwrap().as_raw_fd(), libc::LOCK_UN) }; - - self.0.take().unwrap() + pub fn unlock(mut self, nonblock: bool) -> Result { + let flag = match nonblock { + true => libc::LOCK_UN | libc::LOCK_NB, + false => libc::LOCK_UN, + }; + match Errno::result(unsafe { libc::flock(self.0.as_ref().unwrap().as_raw_fd(), flag) }) { + Ok(_) => Ok(self.0.take().unwrap()), + Err(errno) => Err(errno), + } } } diff --git a/test/test_fcntl.rs b/test/test_fcntl.rs index 7016ae9b00..0f0196716f 100644 --- a/test/test_fcntl.rs +++ b/test/test_fcntl.rs @@ -676,7 +676,7 @@ mod test_flock { let lock1 = Flock::lock(file1, FlockArg::LockExclusive).unwrap(); // Unlock and retain file so any erroneous flocks also remain present. - let _file1 = lock1.unlock(); + let _file1 = lock1.unlock(false).unwrap(); // Attempt to lock second handle. if Flock::lock(file2, FlockArg::LockExclusiveNonblock).is_err() { From 85b76eeff587d7078b98213b63c1857896e41a17 Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Wed, 22 Nov 2023 18:57:00 -0500 Subject: [PATCH 26/33] Reverted changes to original `flock()` and `FlockArg` for deprecation period, added EINVAL case if the unlock operation is given to `Flock::lock()`. --- src/fcntl.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/fcntl.rs b/src/fcntl.rs index c2a397200f..c46e37f92c 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -601,8 +601,33 @@ pub fn fcntl(fd: RawFd, arg: FcntlArg) -> Result { pub enum FlockArg { LockShared, LockExclusive, + Unlock, LockSharedNonblock, LockExclusiveNonblock, + UnlockNonblock, +} + +#[cfg(not(any(target_os = "redox", target_os = "solaris")))] +#[deprecated = "`fcntl::Flock` should be used instead."] +pub fn flock(fd: RawFd, arg: FlockArg) -> Result<()> { + use self::FlockArg::*; + + let res = unsafe { + match arg { + LockShared => libc::flock(fd, libc::LOCK_SH), + LockExclusive => libc::flock(fd, libc::LOCK_EX), + Unlock => libc::flock(fd, libc::LOCK_UN), + LockSharedNonblock => { + libc::flock(fd, libc::LOCK_SH | libc::LOCK_NB) + } + LockExclusiveNonblock => { + libc::flock(fd, libc::LOCK_EX | libc::LOCK_NB) + } + UnlockNonblock => libc::flock(fd, libc::LOCK_UN | libc::LOCK_NB), + } + }; + + Errno::result(res).map(drop) } /// Represents valid types for flock. @@ -675,6 +700,7 @@ impl Flock { FlockArg::LockExclusive => libc::LOCK_EX, FlockArg::LockSharedNonblock => libc::LOCK_SH | libc::LOCK_NB, FlockArg::LockExclusiveNonblock => libc::LOCK_EX | libc::LOCK_NB, + FlockArg::Unlock | FlockArg::UnlockNonblock => return Err((t, Errno::EINVAL)), }; match Errno::result(unsafe { libc::flock(t.as_raw_fd(), flags) }) { Ok(_) => Ok(Self(Some(t))), From c4707526f087d5d812875e469fcdd884686a498b Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Wed, 22 Nov 2023 19:11:11 -0500 Subject: [PATCH 27/33] Refactored `Flock` to wrap T directly and avoid a double-free after `unlock()` is used. --- src/fcntl.rs | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index c46e37f92c..4acd4293d7 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -642,16 +642,14 @@ pub unsafe trait Flockable: AsRawFd {} /// See flock(2) for details on locking semantics. #[cfg(not(any(target_os = "redox", target_os = "solaris")))] #[derive(Debug)] -pub struct Flock(Option); +pub struct Flock(T); #[cfg(not(any(target_os = "redox", target_os = "solaris")))] impl Drop for Flock { fn drop(&mut self) { - if let Some(ref t) = self.0 { - if let Err(_) = Errno::result(unsafe { libc::flock(t.as_raw_fd(), libc::LOCK_UN) }) { - if !std::thread::panicking() { - panic!("Failed to remove flock."); - } + if let Err(_) = Errno::result(unsafe { libc::flock(self.0.as_raw_fd(), libc::LOCK_UN) }) { + if !std::thread::panicking() { + panic!("Failed to remove flock."); } } } @@ -662,17 +660,13 @@ impl Deref for Flock { type Target = T; fn deref(&self) -> &Self::Target { - if let Some(ref t) = self.0 { - t - } else { unreachable!() } + &self.0 } } #[cfg(not(any(target_os = "redox", target_os = "solaris")))] impl DerefMut for Flock { fn deref_mut(&mut self) -> &mut Self::Target { - if let Some(ref mut t) = self.0 { - t - } else { unreachable!() } + &mut self.0 } } @@ -703,7 +697,7 @@ impl Flock { FlockArg::Unlock | FlockArg::UnlockNonblock => return Err((t, Errno::EINVAL)), }; match Errno::result(unsafe { libc::flock(t.as_raw_fd(), flags) }) { - Ok(_) => Ok(Self(Some(t))), + Ok(_) => Ok(Self(t)), Err(errno) => Err((t, errno)), } } @@ -738,10 +732,15 @@ impl Flock { true => libc::LOCK_UN | libc::LOCK_NB, false => libc::LOCK_UN, }; - match Errno::result(unsafe { libc::flock(self.0.as_ref().unwrap().as_raw_fd(), flag) }) { - Ok(_) => Ok(self.0.take().unwrap()), - Err(errno) => Err(errno), - } + let inner = unsafe { + match Errno::result(libc::flock(self.0.as_raw_fd(), flag)) { + Ok(_) => std::ptr::read(&mut self.0), + Err(errno) => return Err(errno), + } + }; + + std::mem::forget(self); + Ok(inner) } } From a28425b772ea0e3fcc0b12df9b33ad7997ffb5bc Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Wed, 22 Nov 2023 19:14:32 -0500 Subject: [PATCH 28/33] Fixed linting errors. --- src/fcntl.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index 4acd4293d7..e2d80e24a3 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -647,7 +647,7 @@ pub struct Flock(T); #[cfg(not(any(target_os = "redox", target_os = "solaris")))] impl Drop for Flock { fn drop(&mut self) { - if let Err(_) = Errno::result(unsafe { libc::flock(self.0.as_raw_fd(), libc::LOCK_UN) }) { + if Errno::result(unsafe { libc::flock(self.0.as_raw_fd(), libc::LOCK_UN) }).is_err() { if !std::thread::panicking() { panic!("Failed to remove flock."); } @@ -727,14 +727,14 @@ impl Flock { /// /// Ok(()) /// } - pub fn unlock(mut self, nonblock: bool) -> Result { + pub fn unlock(self, nonblock: bool) -> Result { let flag = match nonblock { true => libc::LOCK_UN | libc::LOCK_NB, false => libc::LOCK_UN, }; let inner = unsafe { match Errno::result(libc::flock(self.0.as_raw_fd(), flag)) { - Ok(_) => std::ptr::read(&mut self.0), + Ok(_) => std::ptr::read(&self.0), Err(errno) => return Err(errno), } }; From 47e1f5d2589c9b0552779146f6b2a723342d248b Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Wed, 22 Nov 2023 19:16:09 -0500 Subject: [PATCH 29/33] More linting fixes. --- src/fcntl.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index e2d80e24a3..3ed75d20b5 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -647,10 +647,8 @@ pub struct Flock(T); #[cfg(not(any(target_os = "redox", target_os = "solaris")))] impl Drop for Flock { fn drop(&mut self) { - if Errno::result(unsafe { libc::flock(self.0.as_raw_fd(), libc::LOCK_UN) }).is_err() { - if !std::thread::panicking() { - panic!("Failed to remove flock."); - } + if Errno::result(unsafe { libc::flock(self.0.as_raw_fd(), libc::LOCK_UN) }).is_err() && !std::thread::panicking() { + panic!("Failed to remove flock."); } } } From 936e3ceddb6eeeb534a75eff86f3ba7a3833b6b5 Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Wed, 22 Nov 2023 19:25:16 -0500 Subject: [PATCH 30/33] Fixed example code for `Flock::unlock()`. --- src/fcntl.rs | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index 3ed75d20b5..2d70770900 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -707,7 +707,7 @@ impl Flock { /// # use std::fs::File; /// # use nix::fcntl::{Flock, FlockArg}; /// fn do_stuff(file: File) -> nix::Result<()> { - /// let lock = match Flock::lock(file, FlockArg::LockExclusive) { + /// let mut lock = match Flock::lock(file, FlockArg::LockExclusive) { /// Ok(l) => l, /// Err((_,e)) => return Err(e), /// }; @@ -716,26 +716,25 @@ impl Flock { /// /// // Unlock (don't continue until unlocked) /// let file = loop { - /// if let Ok(f) = lock.unlock(false) { - /// break f - /// } + /// lock = match lock.unlock(false) { + /// Ok(f) => break f, + /// Err((l,_)) => l, + /// }; /// }; /// /// // Do anything else /// /// Ok(()) /// } - pub fn unlock(self, nonblock: bool) -> Result { + pub fn unlock(self, nonblock: bool) -> std::result::Result { let flag = match nonblock { true => libc::LOCK_UN | libc::LOCK_NB, false => libc::LOCK_UN, }; - let inner = unsafe { - match Errno::result(libc::flock(self.0.as_raw_fd(), flag)) { - Ok(_) => std::ptr::read(&self.0), - Err(errno) => return Err(errno), - } - }; + let inner = unsafe { match Errno::result(libc::flock(self.0.as_raw_fd(), flag)) { + Ok(_) => std::ptr::read(&self.0), + Err(errno) => return Err((self, errno)), + }}; std::mem::forget(self); Ok(inner) From bcb58800f9b5c463d3448049593e4fcea6215519 Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Sat, 2 Dec 2023 11:29:20 -0500 Subject: [PATCH 31/33] Made requested changes. --- changelog/2170.removed.md | 2 -- src/fcntl.rs | 51 ++++++++++++++++++--------------------- test/test_fcntl.rs | 2 +- 3 files changed, 25 insertions(+), 30 deletions(-) delete mode 100644 changelog/2170.removed.md diff --git a/changelog/2170.removed.md b/changelog/2170.removed.md deleted file mode 100644 index abc411ffea..0000000000 --- a/changelog/2170.removed.md +++ /dev/null @@ -1,2 +0,0 @@ -Removed `fcntl::flock` in favor of new `Flock` wrapper. -Removed unlock options from `fcntl::FlockArg` diff --git a/src/fcntl.rs b/src/fcntl.rs index 2d70770900..fb3918f649 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -608,7 +608,7 @@ pub enum FlockArg { } #[cfg(not(any(target_os = "redox", target_os = "solaris")))] -#[deprecated = "`fcntl::Flock` should be used instead."] +#[deprecated(since = "0.28.0", note = "`fcntl::Flock` should be used instead.")] pub fn flock(fd: RawFd, arg: FlockArg) -> Result<()> { use self::FlockArg::*; @@ -633,13 +633,13 @@ pub fn flock(fd: RawFd, arg: FlockArg) -> Result<()> { /// Represents valid types for flock. /// /// # Safety -/// Types implementing this must be `!Clone`. +/// Types implementing this must not be `Clone`. #[cfg(not(any(target_os = "redox", target_os = "solaris")))] pub unsafe trait Flockable: AsRawFd {} /// Represents an owned flock, which unlocks on drop. /// -/// See flock(2) for details on locking semantics. +/// See [flock(2)](https://linux.die.net/man/2/flock) for details on locking semantics. #[cfg(not(any(target_os = "redox", target_os = "solaris")))] #[derive(Debug)] pub struct Flock(T); @@ -647,9 +647,10 @@ pub struct Flock(T); #[cfg(not(any(target_os = "redox", target_os = "solaris")))] impl Drop for Flock { fn drop(&mut self) { - if Errno::result(unsafe { libc::flock(self.0.as_raw_fd(), libc::LOCK_UN) }).is_err() && !std::thread::panicking() { - panic!("Failed to remove flock."); - } + let res = Errno::result(unsafe { libc::flock(self.0.as_raw_fd(), libc::LOCK_UN) }); + if res.is_err() && !std::thread::panicking() { + panic!("Failed to remove flock: {}", res.unwrap_err()); + } } } @@ -674,18 +675,20 @@ impl Flock { /// /// # Example /// ``` + /// # use std::io::Write; /// # use std::fs::File; /// # use nix::fcntl::{Flock, FlockArg}; - /// fn do_stuff(file: File) -> nix::Result<()> { - /// let lock = match Flock::lock(file, FlockArg::LockExclusive) { - /// Ok(l) => l, - /// Err((_, e)) => return Err(e), - /// }; + /// # fn do_stuff(file: File) { + /// let mut file = match Flock::lock(file, FlockArg::LockExclusive) { + /// Ok(l) => l, + /// Err(_) => return, + /// }; /// - /// // Do stuff - /// - /// Ok(()) - /// } + /// // Do stuff + /// let data = "Foo bar"; + /// _ = file.write(data.as_bytes()); + /// _ = file.sync_data(); + /// # } pub fn lock(t: T, args: FlockArg) -> std::result::Result { let flags = match args { FlockArg::LockShared => libc::LOCK_SH, @@ -714,24 +717,18 @@ impl Flock { /// /// // Do critical section /// - /// // Unlock (don't continue until unlocked) - /// let file = loop { - /// lock = match lock.unlock(false) { - /// Ok(f) => break f, - /// Err((l,_)) => l, - /// }; + /// // Unlock + /// let file = match lock.unlock() { + /// Ok(f) => f, + /// Err((_, e)) => return Err(e), /// }; /// /// // Do anything else /// /// Ok(()) /// } - pub fn unlock(self, nonblock: bool) -> std::result::Result { - let flag = match nonblock { - true => libc::LOCK_UN | libc::LOCK_NB, - false => libc::LOCK_UN, - }; - let inner = unsafe { match Errno::result(libc::flock(self.0.as_raw_fd(), flag)) { + pub fn unlock(self) -> std::result::Result { + let inner = unsafe { match Errno::result(libc::flock(self.0.as_raw_fd(), libc::LOCK_UN)) { Ok(_) => std::ptr::read(&self.0), Err(errno) => return Err((self, errno)), }}; diff --git a/test/test_fcntl.rs b/test/test_fcntl.rs index 0f0196716f..59e77bde46 100644 --- a/test/test_fcntl.rs +++ b/test/test_fcntl.rs @@ -676,7 +676,7 @@ mod test_flock { let lock1 = Flock::lock(file1, FlockArg::LockExclusive).unwrap(); // Unlock and retain file so any erroneous flocks also remain present. - let _file1 = lock1.unlock(false).unwrap(); + let _file1 = lock1.unlock().unwrap(); // Attempt to lock second handle. if Flock::lock(file2, FlockArg::LockExclusiveNonblock).is_err() { From b04334f0a3b5382ce2d4f6315792f56ad66c674d Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Sat, 2 Dec 2023 11:44:01 -0500 Subject: [PATCH 32/33] Removed duplicate import. --- src/fcntl.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index 9a6db45c57..9c14a1e190 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -27,7 +27,7 @@ use std::os::unix::io::{AsRawFd, OwnedFd}; use std::path::PathBuf; #[cfg(any(linux_android, target_os = "freebsd"))] use std::{ - os::unix::io::{AsFd, AsRawFd}, + os::unix::io::AsFd, ptr, }; From d3225bba9e91fba282dfa804cc3a0b1de3d42d73 Mon Sep 17 00:00:00 2001 From: Christopher Skane Date: Sat, 2 Dec 2023 11:45:33 -0500 Subject: [PATCH 33/33] Format change. --- src/fcntl.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/fcntl.rs b/src/fcntl.rs index 9c14a1e190..8f68b498e7 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -26,10 +26,7 @@ use std::os::unix::io::{AsRawFd, OwnedFd}; ))] use std::path::PathBuf; #[cfg(any(linux_android, target_os = "freebsd"))] -use std::{ - os::unix::io::AsFd, - ptr, -}; +use std::{os::unix::io::AsFd, ptr}; #[cfg(feature = "fs")] use crate::{sys::stat::Mode, NixPath, Result};