Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replacement nix::fcntl::Flock does not cover all use cases for deprecated nix::fcntl::flock #2356

Closed
allenap opened this issue Apr 7, 2024 · 2 comments · Fixed by #2407
Closed
Milestone

Comments

@allenap
Copy link

allenap commented Apr 7, 2024

#2170 deprecated flock and replaced it with a Flock struct, allowing for automatic unlock-on-drop behaviour. So far, so good, but I think there are several ways in which flock can be used that cannot be modelled with this replacement. For example, upgrading or downgrading locks.

Suppose I have a shared lock and I want to upgrade it to an exclusive lock. With flock, if there's any error, e.g. EWOULDBLOCK/EAGAIN (same thing), I typically would not lose the existing shared lock.

However, using Flock, I cannot call Flock::lock with the Flock itself, so I would have to call Flock::unlock first to return the enclosed Flockable, before then calling Flock::lock. An error in that last step would leave me with no lock.

Some ideas for how to resolve this:

  • Remove the deprecation of flock.
  • Add a Flock::relock method that allows for upgrading/downgrading locks.
  • Add an impl Flockable for Flock<T> so that one can call Flock::lock with a Flock.
@SteveLauC
Copy link
Member

For example, upgrading or downgrading locks.

Yeah, this is something not covered by the current interface.

Some ideas for how to resolve this:

  • Remove the deprecation of flock.
  • Add a Flock::relock method that allows for upgrading/downgrading locks.
  • Add an impl Flockable for Flock so that one can call Flock::lock with a Flock.

impl Flockable for Flock<T> where T: Flockable is the most Rusty one, though Flock::unlock() would return a Ok(Flock<File>) o n success, which does not seem to be good.

So I think we should do it with the second approach, something like:

impl<T: Flockable> Flock<T> {
    /// Do another lock operation, can be used to upgrade locks.
    pub fn relock(&self, arg: FlockArg) -> Result<()> {
        let fd = self.0.as_raw_fd();
         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,
            #[allow(deprecated)]
            FlockArg::Unlock | FlockArg::UnlockNonblock => return Err((t, Errno::EINVAL)),
        };
        Errno::result(unsafe { libc::flock(t.as_raw_fd(), flags) }).map(drop)
    }
}

but I think there are several ways in which flock can be used that cannot be modelled with this replacement.

Would you like to show me other cases that are not handled by the current impl so that we can probably also give them a fix.

@allenap
Copy link
Author

allenap commented Apr 25, 2024

Would you like to show me other cases that are not handled by the current impl so that we can probably also give them a fix.

Sorry, I suspect I was careless with my words. Right now I cannot think of another case that isn't handled.

@asomers asomers added this to the 0.29.0 milestone May 16, 2024
asomers added a commit to asomers/nix that referenced this issue May 16, 2024
It can upgrade or downgrade a lock.

Fixes nix-rust#2356
@asomers asomers mentioned this issue May 16, 2024
3 tasks
github-merge-queue bot pushed a commit that referenced this issue May 16, 2024
It can upgrade or downgrade a lock.

Fixes #2356
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants