-
Notifications
You must be signed in to change notification settings - Fork 175
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 fcntl_getlk
for fetching information of a lock held by another process
#1310
base: main
Are you sure you want to change the base?
Conversation
fcntl_getfl
for fetching information of a lock held by another processfcntl_getlk
for fetching information of a lock held by another process
Apparently someone else added a PR for this before(#1077). I couldn't find such issue or PR the last time I searched. |
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.
Thanks!
I think putting this in rustix::process
makes sense, for this and for furture fcntl-style lock functions which expose the full fcntl-style process-associated lock API. The current rustix::fs::fcntl_getlk
doesn't expose the full fcntl-style lock functionality, and is mainly intended as an alternative for flock
on platforms which lack flock
. So we can evolve toward rustix::process::fcntl_*
eventually being the "full" fcntl-style lock API.
I believe I've addressed everything, let me know if any other change is needed. |
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 looks great! I just have the one comment above. While I understand the desire to add code to hide platform differences, rustix in general doesn't do that, especially for error conditions, an I'm reluctant to start doing it here.
Makes sense if rustix is aiming for a relatively low-level API. I've removed the check and updated the docs and tests accordingly. |
Fixes #1076.
I added this inside the
process
module asPid
was inaccessible in thefs
module, and it makes more sense to include it insideprocess
as we can use it to access the pid, and lock data which is held by another process. I read the code forrustix::fs::fcntl_lock
, and added this functionality in a similar manner, but let me know if the APIs/code organization need work.I still need to add a test for this though, but I'm not sure what would be the best way to do that. As a process will always be able to acquire it's own lock, we'll probably need to spawn another process and read the advisory lock set by the parent process over there. We may use
std::os::unix::process::CommandExt::before_exec
for forking but it might not be the best way, given that it's unsafe and we'll have to handle race conditions, so I need some guidance here.EDIT: Used
std::os::unix::process::CommandExt::pre_exec
for spawning child processes in tests for now.EDIT 2: After running the tests, I found that MacOS and FreeBSD are the only platform which don't return
EINVAL
whenl_type
is set toF_UNLCK
. For ensuring consistent behavior, I've added a check here. Let me know if I should respect platform-specific behavior or keep the status quo.