Add System V shared memory APIs #1989
Conversation
There was a problem hiding this comment.
First off, I'm not affiliated with nix, so take this review with a grain of salt.
However I do see a big issue with your approach. It is a very low level API. It basically just exposes the raw libc API. Is there a way to build a memory safe API on top instead?
Even if we wrap key_t, the various flags etc in rust abstractions, that is still rather low level. Can we do better? What would a struct ShmMemory look like? Is it possible to make something like that will be generally useful (not restricting certain use cases) and that doesn't need the user to use unsafe? With proper lifetimes (and using a null pointer to shmat to make it pick a suitable memory address)?
| /// For more information, see [`shmget(2)`]. | ||
| /// | ||
| /// [`shmget(2)`]: https://man7.org/linux/man-pages/man2/shmget.2.html | ||
| pub fn shmget(key: key_t, size: size_t, shmflg: c_int) -> Result<c_int> { |
There was a problem hiding this comment.
In general (not just for shmget): I would not expose key_t directly. It is just a type alias, and thus not safe. Looking at the docs, some sort of newtype or enum would be more appropriate.
| /// For more information, see [`shmget(2)`]. | ||
| /// | ||
| /// [`shmget(2)`]: https://man7.org/linux/man-pages/man2/shmget.2.html | ||
| pub fn shmget(key: key_t, size: size_t, shmflg: c_int) -> Result<c_int> { |
There was a problem hiding this comment.
shmflg should be an enum of the valid values, not a raw C integer. I believe that there is a special macro in nix (libc_enum! or something like that) that will streamline the creation of this.
| pub unsafe fn shmat( | ||
| shmid: c_int, | ||
| shmaddr: *const c_void, | ||
| shmflg: c_int, |
There was a problem hiding this comment.
Again: shmflg, shmid should be wrapped in safe abstractions. Reading the man pages it even seems like shmat and shmget take different sets of flags. So two different enums probably?
| /// For more information, see [`shmget(2)`]. | ||
| /// | ||
| /// [`shmget(2)`]: https://man7.org/linux/man-pages/man2/shmget.2.html | ||
| pub fn shmget(key: key_t, size: size_t, shmflg: c_int) -> Result<c_int> { |
There was a problem hiding this comment.
At the very least the result type should be a newtype pattern. I.e:
-> Result<ShmKey>where
#[derive(Debug, PartialEq, Eq, Hash)]
struct ShmKey(c_int)
impl ShmKey {
unsafe fn to_raw(&self) -> c_int {
self.0
}
// unsafe from_raw, other methods
}This helps ensure that you will get sensible compiler errors.
| /// All arguments should be valid and meet the requirements described in the [`shmctl(2)`] man page. | ||
| /// | ||
| /// [`shmctl(2)`]: https://man7.org/linux/man-pages/man2/shmctl.2.html | ||
| pub unsafe fn shmctl( |
There was a problem hiding this comment.
Again: shmflg & cmd should be wrapped in safe abstractions. Note that at least on Linux there seems to be some Linux specific commands. So cfg compile time guards will be needed to construct the enum.
We also don't want to expose shmid_ds directly to the user I would say. It too needs rustification (though I haven't looked into the details of that).
| cmd: c_int, | ||
| buf: *mut shmid_ds, | ||
| ) -> Result<c_int> { | ||
| Errno::result(libc::shmctl(shmid, cmd, buf)) |
There was a problem hiding this comment.
It also seems the meaning of the return value changes depending on the specific command, so it is not correct to treat it as errno unconditionally:
Quoting man 2 shmctl on my system:
A successful IPC_INFO or SHM_INFO operation returns the index of the highest used entry in the kernel's internal array recording information about all shared memory segments. (This information can be used with repeated SHM_STAT or SHM_STAT_ANY operations to obtain information about all shared memory segments on the system.) A successful SHM_STAT operation returns the identifier of the shared memory segment whose index was given in shmid. Other operations return 0 on success.
First draft to add System V shared memory APIs.
I would like some help with these questions:
unsafeor not? For now, I've marked functions taking in any pointer arguments as unsafe.libc_bitflagsforshmflgorcmd? There aren't a set of constants with a fixed prefix as described in the CONVENTIONS.md file. There are a fewIPC_*andSHM_*constants for them.Partially addresses #1718.