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 shm_open/shm_unlink (POSIX shared memory) #848

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

ids1024
Copy link
Contributor

@ids1024 ids1024 commented Sep 22, 2023

Fixes #705.

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! Overall this looks good.

My main concern here is that I'm not sure shm_open and shm_unlink belong in rustix::fs. There is no clear rule, but my instinct is that the shared-memory namespace is distinct from the filesystem namespace, even if there is aliasing on Linux, that it justifies putting these in a new rustix::shm module. What would you think about that?


pub(crate) fn shm_unlink(name: &CStr) -> io::Result<()> {
let path = get_shm_name(name)?;
let path = &path[..=path.iter().position(|x| *x == 0).unwrap()];
Copy link
Member

Choose a reason for hiding this comment

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

We could avoid these position calls by having get_shm_name also return the length of the name, which it already knows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I originally wrote it with from_bytes_until_nul, but then CI complained since that's too new of a standard library addition.

Either from_bytes_with_nul or from_bytes_until_nul still need to traverse the string. method.from_bytes_with_nul_unchecked would avoid that.

@ids1024
Copy link
Contributor Author

ids1024 commented Sep 25, 2023

I put it next to memfd_create. But it could make sense to have an shm module containing both of these things, and System V shared memory (#797). memfd_create certainly isn't a particularly "file system" related API.

@ids1024
Copy link
Contributor Author

ids1024 commented Sep 25, 2023

Of course, moving memfd_create would be a breaking API change.

@sunfishcode
Copy link
Member

Here's a rough sketch for how we might group all the things in this space that we may eventually add:

  • shm - POSIX shared memory
  • mq - POSIX message queues
  • sysvipc::sem, sysvipc::shm, and sysvipc::msg, for System V semaphores, shared memory, and message queues
  • fs - memfd_create

The unifying themes for the shm module are the namespace, and the fact that on some platforms, shm_open is not backed by plain files (even if it is plain files on Linux). memfd_create doesn't fit into either of those two themes. The unifying themes for the fs namespace are the filesystem namespace, and also operations on file objects, like fstat, lseek, ftruncate, and so on. memfd_create doesn't interact with the filesystem namespace, but it does produce a completely file-like file object, so it goes in fs.

Does that make sense?

@ids1024
Copy link
Contributor Author

ids1024 commented Sep 25, 2023

shm_open and shm_close seem more "fs" like than memfd_create, given they mirror open and close, while also returning file descriptors. Even if they don't necessarily exist in the filesystem.

But you're right that memfd_create is defined to return a "completely file-like object" (except for sealing support). I'd expect shm_open to as well, and on Linux it does, but the POSIX spec doesn't actually seem to say exactly what operations are supported, and macOS does in fact seem to be quite restrictive about what calls work with shm fds.

@sunfishcode
Copy link
Member

Ultimately, what we're talking about here is just the top-level API organization. You can always call fs functions on shm file descriptors, regardless of how we organize things. Just like you can always call many fs functions like fstat on socket fds from the net module. It's not a strict separation, just a general grouping.

Interestingly, POSIX did add a "future direction" note that shm_open's objects may have full file semantics in the future. If that ever happens, and macOS implements it, maybe that'd tip the scales and we should move shm_open into fs. But we don't need to predict that future today; we have tools like deprecation and semver for evolving APIs. So my sense is, until then, shm makes sense.

@ids1024 ids1024 force-pushed the shm_open branch 2 times, most recently from b4f2bf9 to acd6364 Compare September 26, 2023 02:16
@ids1024
Copy link
Contributor Author

ids1024 commented Sep 26, 2023

Moving it out of fs is a bit complicated with how things currently work, since it uses open and unlink from the fs module, as well as Mode and OFlags.

Though looking at the POSIX spec and shm_open(3), both note flags must contain "exactly one of" O_RDONLY and O_RDWR, and optionally O_CREAT, O_EXCL, and O_TRUNC. But no other flags. So possibly a custom flags type may be appropriate, instead of using the same one as open.

But Mode would probably be unchanged, so it makes sense to use the same type.

@sunfishcode
Copy link
Member

Moving it out of fs is a bit complicated with how things currently work, since it uses open and unlink from the fs module, as well as Mode and OFlags.

It'd be ok to have shm depend on fs, with shm = ["fs"] in Cargo.toml, and then the shm module can use functions and types in the fs module.

Though looking at the POSIX spec and shm_open(3), both note flags must contain "exactly one of" O_RDONLY and O_RDWR, and optionally O_CREAT, O_EXCL, and O_TRUNC. But no other flags. So possibly a custom flags type may be appropriate, instead of using the same one as open.

A custom flags type for this makes sense to me.

Also, looking at the shm_open docs, I just noticed "The FD_CLOEXEC file descriptor flag associated with the new file descriptor is set.", so we should have the code add OFlags::CLOEXEC when calling open in the linux_raw backend.

But Mode would probably be unchanged, so it makes sense to use the same type.

Makes sense. We can declare a type alias in the shm module for convenience.

@ids1024 ids1024 force-pushed the shm_open branch 5 times, most recently from eb57a17 to 9a3bd8d Compare September 28, 2023 22:01
@ids1024
Copy link
Contributor Author

ids1024 commented Sep 28, 2023

Ah, I should have thought about CLOEXEC.

This now has a passes that and has a separate flags type.

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@sunfishcode sunfishcode merged commit 5f0db52 into bytecodealliance:main Sep 29, 2023
43 checks passed
@sunfishcode
Copy link
Member

sunfishcode commented Sep 29, 2023

This is now released in #848 rustix 0.38.15.

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.

Missing shm_open?
2 participants