bytecodealliance / rustix

Safe Rust bindings to POSIX-ish APIs
Other
1.5k stars 161 forks source link

openat2 extensibility #1186

Open cyphar opened 1 month ago

cyphar commented 1 month ago

The kernel API for openat2 is designed to be extensible but the API binding provided by rustix is done in a way that would result in API breakage if a new field was added to openat2 in the future. Ideally, the API would look something more like (idk if AsRef<Openat2How> or Into<Openat2How> is more preferable):

#[non_exhaustive]
#[derive(Clone, Debug, Default)]
pub struct Openat2How {
    // NOTE: This is actually a u64 but OFlags is i32. There have been
    // discussions about making openat2-only flags before so maybe this should
    // be O2Flags or something.
    pub flags: OFlags,
    pub mode: Mode,
    pub resolve: ResolveFlags,
}

pub fn openat2<Fd: AsFd, P: Arg>(dirfd: Fd, path: P, how: &Openat2How) -> Result<OwnedFd>

Sadly, the most ergonomic way of instantiating Openat2How wouldn't work:

let how = Openat2How {
    flags: ...,
    resolve: ...,
    ..Default::default()
};

because #[non_exhaustive] blocks that too. But they could at least do:

let mut how = Openat2How::default();
how.flags = ...;
how.resolve = ...;

And then rustix would use std::mem::size_of::<Openat2How>() as the size argument to the syscall. This would allow for future extensions to be added to Openat2How transparently without causing breakages for Rust users -- allowing us to provide the same backward-compatibility that C users of openat2 get.

Because of this limitation, I can't switch my last syscall wrapper in libpathrs from raw libc calls to rustix because I sometimes test extensions in my Rust code and the current API doesn't let you express extensions.

Since changing this would be a breakage and would require a new minor bump for rustix, I'm opening an issue before sending a PR for this. If this API would a bit too ugly to use for most people, then maybe we could make it openat2_raw or something?

sunfishcode commented 1 month ago

Rustix tends to avoid "just pass the bits through" interfaces, because one of rustix's big goals is safe APIs. openat2 in rustix is currently declared as a safe function. If it were extensible with arbitrary fields, and those field ever contained raw pointers or raw file descriptors, then it would become unsound. I don't know how likely it is that the Linux developers would ever add pointers or file descriptors to open_how, but they don't guarantee that they won't, and it doesn't seem implausible.

sunfishcode commented 1 month ago

Oh, rereading this, I think I may have misunderstood your request here. Perhaps you're not asking to expose the raw Linux open_how bits; perhaps you're asking for rustix to have something like its own OpenHow type which is #[non_exhautive] so that rustix can add new features over time without API breaks?

I looked a little at libpathrs and don't see a place where openat2 is exposed in the public API, so I'm not quite clear on how this would work in practice. Would you mind going into a little more detail of how you'd want to use this?

cyphar commented 1 month ago

We don't expose it in libpathrs, this is mainly a request as a user of rustix.

However (as the author of openat2), there are quite a few extra features we plan to add and it would be nice if Rust programs could make use of them without having to wait for rustix to have a SemVer-incompatibility version bump when a new field is added. I don't know what the long-term plan is for rustix (will there ever be a 1.0 or will it be like libc where it is going to forever be non-1.0?) but making it difficult to use newer openat2 features (having to wait for a 0.39 or 0.40 release because of back-compat issues) would make me hesitant about using the rustix openat2 wrappers.

This would also allow libraries that expose Openat2How to update to use newer openat2 features with rustix without breaking their users and allow their users to make use of newer features fairly easily (though in the Rust community it seems that re-exporting types directly is something most people avoid due to risks of breakage when updating dependencies, so in practice this might not actually be that important).