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

Fixed close handles with Alpine and MUSL #376

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

dchansen
Copy link

The MUSL C library does not provide the close_range in unistd.h and Alpine linux therefore cannot compile boost::process::v2, at close_handles.ipp fails to compile.

This PR adds a check to only use close_range if we are using GLIBC.

@klemens-morgenstern
Copy link
Collaborator

Looks good, will merge if all tests pass.

@klemens-morgenstern
Copy link
Collaborator

Ok, I don't think this is correct, as it doesn't handle libc++. I'll need to add a MUSL test to CI.

@t43rr7
Copy link

t43rr7 commented Jun 18, 2024

Hello, @dchansen, I've encountered the same issue, but with glibc. The close_range checks here would be a bit more complicated. I can include your changes in my patch, so it could resolve 2 issues with single PR.

@dchansen
Copy link
Author

@t43rr7 That looks good to me. Could you make a PR for my branch? then I will merge it in.

@t43rr7
Copy link

t43rr7 commented Jun 20, 2024

@dchansen I've opened PR to your branch. Looks like your branch's HEAD a bit behind origin develop, so there're a bit more changes that expected. Anyway, the most interesting diffs here. I've also added raw system call support which should work even on MUSL library.

Please, verify this changes on your setup, because I have no possibility to do it fast (with MUSL). Thanks!

UPD:
Same changes can be verified in this PR

@t43rr7
Copy link

t43rr7 commented Jun 21, 2024

@klemens-morgenstern please, look at the new changes

@klemens-morgenstern
Copy link
Collaborator

Can't we just put a close_range wrapper around the syscall into a detail namespace and use using ::close_range otherwise? That way we don't need to reimpl close_all.

@t43rr7
Copy link

t43rr7 commented Jun 21, 2024

Do you mean something like "code below"?

int close_range_wrapper(...) {
#ifdef HAS_CLOSE_RANGE
    return ::close_range(...);
#elif defined(HAS_CLOSE_RANGE_SYSCALL) 
    return ::syscall(SYS_close_range, ...);
#endif
}

I like this idea :)
But this PR is not from my fork, I can do it in similar opened PR. How do you feel about this?

@t43rr7
Copy link

t43rr7 commented Jun 21, 2024

@klemens-morgenstern done in this commit

Please, see PR

@t43rr7
Copy link

t43rr7 commented Jun 25, 2024

@klemens-morgenstern are there any news?

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.

3 participants