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 fcntl_getlk for fetching information of a lock held by another process #1310

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

metent
Copy link

@metent metent commented Feb 5, 2025

Fixes #1076.

I added this inside the process module as Pid was inaccessible in the fs module, and it makes more sense to include it inside process as we can use it to access the pid, and lock data which is held by another process. I read the code for rustix::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 when l_type is set to F_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.

@metent metent changed the title Add fcntl_getfl for fetching information of a lock held by another process Add fcntl_getlk for fetching information of a lock held by another process Feb 5, 2025
@metent
Copy link
Author

metent commented Feb 5, 2025

Apparently someone else added a PR for this before(#1077). I couldn't find such issue or PR the last time I searched.

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!

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.

src/process/types.rs Outdated Show resolved Hide resolved
src/backend/libc/process/syscalls.rs Outdated Show resolved Hide resolved
src/backend/libc/process/syscalls.rs Outdated Show resolved Hide resolved
src/backend/linux_raw/c.rs Outdated Show resolved Hide resolved
@metent
Copy link
Author

metent commented Feb 8, 2025

I believe I've addressed everything, let me know if any other change is needed.

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.

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.

@metent
Copy link
Author

metent commented Feb 9, 2025

There are a lot of minor portability hazards and POSIX nonconformances that rustix doesn't hide

Makes sense if rustix is aiming for a relatively low-level API. I've removed the check and updated the docs and tests accordingly.

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.

Feature request: Add fcntl_getlk support
2 participants