Skip to content

Add fcntl_getlk for fetching information of a lock held by another process #1310

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

Merged
merged 12 commits into from
Feb 11, 2025

Conversation

metent
Copy link
Contributor

@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
Contributor 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.

@metent
Copy link
Contributor 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
Contributor 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.

@sunfishcode
Copy link
Member

Thanks!

@sunfishcode sunfishcode merged commit a876d83 into bytecodealliance:main Feb 11, 2025
45 checks passed
@metent metent deleted the fcntl-getfl branch February 11, 2025 12:39
@sunfishcode
Copy link
Member

This is now released in rustix 1.0.0.

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