Added Flock object for automatic unlock on drop.

[coderBlitz]

What does this PR do

In the spirit of RAII, created the Flock newtype to simplify flock lock handling. Users can pass around an explicit representation of a held lock, automatically unlocking once it goes out of scope.

Checklist:

• I have read CONTRIBUTING.md
• I have written necessary tests and rustdoc comments
• A change log has been added if this PR modifies nix's API

[asomers]

There's little point in wrapping a RawFd, since RawFd implements Copy. It would be way too easy to use accidentally use the RawFd after dropping the lock. If anything, it should wrap OwnedFd. And if we're going to do this, then we should make the flock function private.

[SteveLauC]

There's little point in wrapping a RawFd, since RawFd implements Copy. It would be way too easy to use accidentally use the RawFd after dropping the lock. If anything, it should wrap OwnedFd. And if we're going to do this, then we should make the flock function private.

I kinda think wrapping an OwnedFd is not that easy to use, Let's say that I locked a file exclusively so that I can safely write to it:

fn main() {
    let mut file = File::create("foo").unwrap();

    let flock_guard = Flock::lock(file.into(), FlockArg::LockExclusive).unwrap();

    // error[E0382]: borrow of moved value: `file`
    file.write(b"bar").unwrap();

    // guard dropped here
}

I have to convert the File into an OwnedFd (moved), which means I can not use that File anymore

---

What about we use a BorrowedFd here:

pub struct Flock<'fd>(BorrowedFd<'fd>);

impl<'fd> Drop for Flock<'fd> {
    fn drop(&mut self) {
        _ = flock(self.0.as_raw_fd(), FlockArg::Unlock);
    }
}

impl<'fd> Flock<'fd> {
    pub fn lock(fd: BorrowedFd<'fd>, args: FlockArg) -> anyhow::Result<Self> {
        flock(fd.as_raw_fd(), args)?;

        Ok(Self(fd))
    }
}

Though this won't make the above code compile either:

    let mut file = File::create("foo").unwrap();

    let flock_guard =
        Flock::lock(file.as_fd(), FlockArg::LockExclusive).unwrap();
    // E0502: cannot borrow `file` as mutable because it is also borrowed as immutable
    file.write(b"bar").unwrap();

But we can do this:

    let file = File::create("foo").unwrap();
    
    let flock_guard =
        Flock::lock(file.as_fd(), FlockArg::LockExclusive).unwrap();

    // compiles without any issue
    (&file).write(b"bar").unwrap();

[SteveLauC]

ref: #1750

[coderBlitz]

If flock is only valid on files, then perhaps we could accept a File and implement Deref and DerefMut instead:

pub struct Flock(File);

impl Drop for Flock {
    fn drop(&mut self) {
        _ = flock(self.0.as_raw_fd(), FlockArg::Unlock);
    }
}

impl Deref for Flock {
    type Target = File;
    fn deref(&self) -> &Self::Target {
        &self.0
    }
}
impl DerefMut for Flock {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.0
    }
}

impl Flock {
    pub fn lock(fd: File, args: FlockArg) -> Result<Self> {
        flock(fd.as_raw_fd(), args)?;

        Ok(Self(fd))
    }
    pub fn unlock(self) -> File {
        _ = flock(self.0.as_raw_fd(), FlockArg::Unlock);

        self.0
    }
}

[SteveLauC]

then perhaps we could accept a File and implement Deref and DerefMut instead

Yeah, I agree this is the Rusty way to implement it

If flock is only valid on files

flock(2) can be used for other types of file descriptors

[coderBlitz]

flock(2) can be used for other types of file descriptors

Could you provide some examples or cite what other things it can be used on? I'm looking through a bunch of linux kernel and posix docs and they only specifically call out "files" (and local VFS files at that). The brute force way is to just open every type of file descriptor that we can think of and just see what works and what doesn't, though I guess for the purposes of this we just need 2 valid to make the File option non-viable.

I could understand an argument that File isn't the only way such files could be represented. Perhaps we should consider some pub trait Flockable: AsRawFd that we can implement for File and anything else we find, and then leave the flock() call public. Nevermind, this defeats the auto-releasing purpose of this PR.

[SteveLauC]

Just checked the FreeBSD manual since this syscall is not POSIX but comes from BSD implementations:

 [EINVAL]		  The argument fd refers to an	object	other  than  a file.

This is not specified in the Linux manual, and I can call flock(2) on a file descriptor created by pipe(2):

#include <stdio.h>
#include <stdlib.h>
#include <sys/file.h>
#include <unistd.h>

int main(void) {
  int fd[2] = {0};
  int res = pipe(fd);

  if (res != 0) {
    perror("pipe(2)");
    exit(EXIT_FAILURE);
  }

  res = flock(fd[0], LOCK_EX);
  if (res != 0) {
    perror("flock(2)");
    exit(EXIT_FAILURE);
  }


  printf("flock acquired on a pipe fd\n");
  exit(EXIT_SUCCESS);
}

$ uname -a
Linux fedora 6.5.9-300.fc39.x86_64 #1 SMP PREEMPT_DYNAMIC Wed Oct 25 21:39:20 UTC 2023 x86_64 GNU/Linux

$ gcc main.c

$ ./a.out
flock acquired on a pipe fd

I think we will get an EINVAL error if we try this program on FreeBSD

---

I agree that most manuals of flock(2) are just talking about files

I didn't see any conclusive doc on whether it can be used on fds that are not files either, so my original statement shouldn't be considered accurate, I am sorry about this

but for systems that don't prohibit this, given that flock is assocaited with the open file objects, if the corresponding file descriptor has a entry in the open file table, then I guess it will work?

[asomers]

So we can't wrap RawFd since it's Copy. And we can't do OwnedFd because it doesn't have enough methods. We don't want to do BorrowedFd because the borrowed fd could still be accessed after dropping the Flock. But we don't want to limit ourselves to std::fs::File because that would exclude stuff like tokio::fs::File. What about being generic, and wrapping anything that isn't Clone? Unfortunately, Rust can't have negative type bounds. But we can get close by defining an unsafe Flockable trait and allowing the user to implement it on anything that isn't Clone. Like this:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1c1ec124c76302e36f5ae4e39ef2b9f3

[coderBlitz]

I think everything is good to go, but looks like the gnu test had a random failure (it succeeded in the previous runs).

[SteveLauC]

But we don't want to limit ourselves to std::fs::File because that would exclude stuff like tokio::fs::File.

If we want to implement Flockable for tokio::fs::File, then it seems that we have to pull the Tokio crate to Nix, is this what we want?

If we want it to be generic without pulling third-party dependencies, i.e.

unsafe impl<T: AsRawFd> Flockable for T { }

Then the unsafe mark seems to be meaningless

[SteveLauC]

BTW, we have migrated our most CI tasks to GHA, the CI in this PR is in a mixed, intermediate state, rebasing your branch should resolve it

[coderBlitz]

But we don't want to limit ourselves to std::fs::File because that would exclude stuff like tokio::fs::File.

If we want to implement Flockable for tokio::fs::File, then it seems that we have to pull the Tokio crate to Nix, is this what we want?

If we want it to be generic without pulling third-party dependencies, i.e.

unsafe impl<T: AsRawFd> Flockable for T { }

Then the unsafe mark seems to be meaningless

I don't think the intent is to pull in tokio (would probably cause circular dependency anyhow), more that tokio would be able to implement it on their end, like so:

unsafe impl nix::fcntl::Flockable for tokio::fs::File {};

Thus the trait is "generic" because anyone can implement it (it is a public trait after all).

[coderBlitz]

Looks like the extra failures were "spurious network errors", then the one expected error with armv7-unknown-linux-uclibceabihf.

[SteveLauC]

We are fixing this, once that PR is merged, rebasing your branch to re-run the jobs should get them all resolved

[SteveLauC]

Gentle ping on the author @coderBlitz, would you like to finish this PR?

[SteveLauC]

Thanks!