-
Notifications
You must be signed in to change notification settings - Fork 654
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
System V shared memory APIs (2nd draft) #2314
base: master
Are you sure you want to change the base?
Conversation
I am trying to add support for System V message queue in #2318 |
Next step would be to increase coverage by trying the different flags
SystemV is disabled on Android, see [here](https://android.googlesource.com/platform/ndk/+/4e159d95ebf23b5f72bb707b0cb1518ef96b3d03/docs/system/libc/SYSV-IPC.TXT)
Hello @SteveLauC, I think this PR is pretty good now. I've added some basic test, with around 30% coverage of the new function. I also didn't implement the I don't understand why BSD isn't able to build successfully, when it should be working fine. Is there anything left to be done before a review and an eventual merge? |
Hi @IzawGithub, sorry for the late reply, I plan to take a look at this PR this weekend :) |
src/sys/mod.rs
Outdated
@@ -143,6 +143,10 @@ feature! { | |||
#[allow(missing_docs)] | |||
pub mod sysinfo; | |||
|
|||
#[allow(missing_docs)] | |||
#[cfg(any(bsd, target_os = "linux", not(target_os = "android")))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not(target_os = "android")
necessary as any(bsd, target_os = "linux")
does not include android
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it is always ok to not support all the platforms, we can add them incrementally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the redundant android
target. BSD is still not building and I don't really understand why. Something to do with macro? I'm not proficient with them yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BSD is still not building and I don't really understand why.
The OpenBSD and NetBSD CI failed because some missing constants in the libc crate, which means we need to add them before merging this PR if we want to have #[cfg(bsd)
s supported.
Let me take a closer look at this and probably give it a fix!
src/sys/system_v.rs
Outdated
/// # Ok(()) | ||
/// # } | ||
/// ``` | ||
pub struct Permissions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the last 9 bits of those flg
arguments, Nix already has a type for it,
nix::sys::stat::Mode
, so a new type is probably not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, wasn't aware of this type. This makes it depend on the fs
module but I'm assuming it's fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes it depend on the fs module but I'm assuming it's fine?
Yeah, this is unfortunate, ideally, this Mode
type should be available if #[cfg(any(feature="fs", feature="system_v")]
, doing this will require some extra work, so I think it is ok to make system_v
depend on fs
.
Some thoughts about these APIs:
|
Hello, Thanks for the comprehensive review! Those are all fair issue, I'll work on them this week. For your last question about wrapping all the functions, this is certainly doable, but I wasn't sure if it was prefered to match libc as closely as possible or not. I can definitely try to make something more Rusty. Worst case scenario, if it's not ok we could always revert to what's currently there. Also, once the PR is done, should I clean the history with a rebase before a merge or not? |
Nix uses squash merge, so it's fine to leave the commit history here:) |
Hello, all of the issue listed above should be fixed, expect for the Nice catch on the UB! I'll work on the semaphores during the coming week. |
src/sys/system_v/shm.rs
Outdated
/// # Ok::<(), Errno>(()) | ||
/// ``` | ||
/// | ||
pub fn create_and_connect(key: key_t, mode: Mode) -> Result<Self> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious about why is this called connect()
, IMHO, connecting to a segment is kinda similar to attaching to it, which make me a little bit confused, thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I didn't really put too much though on the name. Maybe create_and_get
? We're creating the segment by using shmget. I'm not the best at naming things, so if you've got a better idea, feel free!
Hi, I just finished another round of review, and realized we can still have UB even with use nix::sys::{
stat::Mode,
system_v::shm::{Shm, ShmatFlag},
};
#[tokio::main(flavor = "current_thread")]
async fn main() {
let mode = Mode::S_IRWXU;
let shm = Shm::<&()>::create_and_connect(65536, mode).unwrap();
let mem_segment = shm.attach(ShmatFlag::empty()).unwrap();
println!("reference = {:p}", *mem_segment);
} In the above code snippet, we create a new shared memory segment and attach to it, then print the value stored there, a Rust reference can never be NULL, i.e, 0, but with the above code, I got: $ cargo r -q
reference = 0x0 With the current impl, there is no guarantee that the value stored in a segment is valid for
A |
Hello, this is harder than I though! For your example, after a bit of debugging, I think that pointing to 0x0 is because we're trying to store a type that's a ref, when SharedMemory actually expect a non ref. The rest of the issue are pretty simple fix I'll work on right away. |
This make it possible to delete a memory segment without attaching.
Hi, sorry for the late response, been busy with my work and can barely do open source things on work days.
Even if we restrict From my observation, that memory segment will be zero-initialized, 0 is a valid type for use nix::sys::{
stat::Mode,
system_v::shm::{Shm, ShmatFlag},
};
fn main() {
let mode = Mode::S_IRWXU;
let shm = Shm::<std::num::NonZeroU8>::create_and_connect(65536, mode).unwrap();
let mem_segment = shm.attach(ShmatFlag::empty()).unwrap();
println!("number = {}", *mem_segment);
} $ cargo r
Finished dev [unoptimized + debuginfo] target(s) in 0.03s
Running `target/debug/rust`
number = 0 |
What does this PR do
Checklist:
CONTRIBUTING.md
I've seen issue #1718 and the pull request #1989 made by @arpankapoor from a year ago, that added some support for the System V API.
This is a continuation of this PR, where I tried to implement most of the change suggested by @VorpalBlade.
What's missing
I haven't added any unit test yet, as somehow I'm getting a lot of compilation error on the test, about some missing symbol, but I've got a closed source project where I'm using this draft as a "eat your own dog food", with unit test, and it seems to work fine.
Some of the bitflags, 2 in
ShmgetFlag
and a lot inSemctlCmd
do not exist in libc, even though they are documented.So to merge this, there is a need to create a PR for libc that add those.
What can be better
Permissions
: Could maybe be moved elsewhere? I haven't found any struct already implemented that wrap the octal permission system unix use, so I threw this in 5m.shmat
: I'm really not a fan of returning a void ptr. I've tried to wrap it in aWeak<RefCell<T>>
but I get a segfault when it gets dropped.What would be ideal is for the user to give a generic type T that could be wrapped in a non owning smart pointer that could also be null. That's why I though Weak would be the answer, but I'm not good enough to find the problem.
Currently, the user has to cast it in an unsafe block, to be able to use the value stored, which is awkward and error prone.
I'm not too sure about the linux only bitflags compilation. We should test it for android too, but I don't know how to do that.