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

Add support for openat2. #2339

Merged
merged 1 commit into from
Mar 25, 2024
Merged

Conversation

blinsay
Copy link
Contributor

@blinsay blinsay commented Mar 20, 2024

What does this PR do

Adds support for openat2 on linux. Includes a new ResolveFlag struct to pass resolve flags, which is passed directly to the new fcntl::openat2 function. libc::open_how isn't exposed in any way here, which will mean this API needs to change if there's an update that extends libc::open_how with new fields.

Given that the whole point of libc::open_how is that it's extensible, I'm not sure that this is the right way to go about adding this API. I could be convinced to go back and add something that looks more like the stdlib's OpenOptions that would allow adding more arguments to this function without a breaking change. I would love some input/direction here.

Fixes #1400.

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

@blinsay
Copy link
Contributor Author

blinsay commented Mar 20, 2024

Also, I haven't added the changelog yet because I wasn't sure what my PR number would be. I'll go ahead and add that once I get some input on what the API should look like.

@blinsay blinsay force-pushed the blinsay/openat2 branch 5 times, most recently from bd3a7f1 to 54c97de Compare March 20, 2024 22:49
@SteveLauC
Copy link
Member

Hi, thanks for this, I will take a look at this PR this weekend:)

@SteveLauC
Copy link
Member

Hi, thanks for the PR and thoughts on the API design:)

which will mean this API needs to change if there's an update that extends libc::open_how with new fields.

Yeah, this is bad and we should avoid it.

I could be convinced to go back and add something that looks more like the stdlib's OpenOptions that would allow adding more arguments to this function without a breaking change.

Yeah, a builder pattern for OpenHow would work well:

  1. Nix won't introduce breaking changes when the Linux kernel extends this structure, we can simply add new .with_new_field(field_value) methods.

  2. The manual mentions that:

    Therefore, a user must zero-fill this structure on initialization.

    We can enforce this with a builder pattern.


A draft code would be something like this:

#[repr(transparent)]
pub struct OpenHow(libc::open_how);

pub struct OpenHowBuilder {
    open_how: libc::open_how,
}

impl OpenHowBuilder {
    pub fn new() -> Self {
        // SAFETY:
        // From the manual: https://man7.org/linux/man-pages/man2/open_how.2type.html
        //
        // > Therefore, a user must zero-fill this structure on initialization.
        //
        // Initializing `open_how` with 0s is valid and a necessity.
        let zero_inited = unsafe {
            std::mem::MaybeUninit::zeroed().assume_init()
        };
        Self {
            open_how: zero_inited
        }
    }

    pub fn with_flags(&mut self, flags: nix::fcntl::OFlag) -> &mut Self {
        let flags = flags.bits();
        self.open_how.flags = flags.try_into().expect("failed to cast it to u64");

        self
    }

    pub fn with_mode(&mut self, mode: nix::sys::stat::Mode) -> &mut Self {
        let mode = mode.bits();
        self.open_how.mode = mode.try_into().expect("failed to cast it to u64");

        self
    }

    pub fn with_resolve(&mut self, resolve: nix::fcntl::ResolveFlag) -> &mut Self {
        let resolve = resolve.bits();
        self.open_how.resolve = resolve;

        self
    }
    
    // We can add new methods for future kernel extensions

    pub fn build(self) -> OpenHow {
        OpenHow(self.open_how)
    }
}

@blinsay
Copy link
Contributor Author

blinsay commented Mar 23, 2024

Therefore, a user must zero-fill this structure on initialization.

Whoops, that's a big oversight on my part! Thanks for catching it.


I went with a#[repr(transparent)] struct with builder-style method, the extra layer of builder didn't feel worth it.

I think the slightly more interesting choice is having openat2 taking an owned OpenHow instead of a &mut OpenHow. It felt right given that the #![deny(missing_copy_implementations)] lint suggested I make OpenHow be Copy, but we may want to just make it Clone in case any of the future fields aren't easily Copy-able.

Thanks for the quick review. :)

@blinsay blinsay force-pushed the blinsay/openat2 branch 3 times, most recently from d394b5d to 1b49aba Compare March 23, 2024 18:14
@blinsay
Copy link
Contributor Author

blinsay commented Mar 23, 2024

Sorry for all the force pushes. I switched to using cfg_if to try and make it harder to mess up platform support.

@blinsay
Copy link
Contributor Author

blinsay commented Mar 23, 2024

Looking at CI it looks like there are tier3 failures for unrelated things and some failures where the syscall is returning ENOSYS. Is there nix way to detect (lack of) support or should I document that openat2 requires at least 5.6 and move on?

UPDATE: oops, I put #[cfg_attr(qemu, ignore)] on test_openat2 but not test_openat2_forbidden so one of the tests was running when it shouldn't have. Let's see if that fixes this.

@SteveLauC
Copy link
Member

SteveLauC commented Mar 24, 2024

Looking at CI it looks like there are tier3 failures for unrelated things

Yeah, they are unrelated and fixed in #2344.

Is there nix way to detect (lack of) support or should I document that openat2 requires at least 5.6

I guess no

Copy link
Member

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch looks great! Thanks! Don't forget to add a changelog entry and rebase your branch to have CI issues resolved:)

Adds an openat2 function with an OpenHow and ResolveFlag options for
configuring path resolution.

Fixes nix-rust#1400.
@SteveLauC SteveLauC added this pull request to the merge queue Mar 25, 2024
Merged via the queue into nix-rust:master with commit 4ab23c3 Mar 25, 2024
34 checks passed
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 this pull request may close these issues.

Support for openat2()
2 participants