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

Fds accessor #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fds accessor #2

wants to merge 2 commits into from

Conversation

cgull
Copy link

@cgull cgull commented Sep 30, 2017

We're using pstreams in a threaded program and running into problems that can be solved with FD_CLOEXEC. We can get at the FILE*s with fopen() and then the file descriptors with fileno(FILE*). But this invokes unnecessary action, and our platform doesn't have an fdclose() call to clean up.

So I wrote a fds() accessor that returns the existing file descriptors, rather than new stdio files. This arguably is a better way to provide access to the underlying pipes, because it is easy to call fdopen() on these file descriptors.

Would you consider this? I left the code outside REDI_EVISCERATE_PSTREAMS because the interaction with raw I/O is simpler, and setsockopt/ioctl are valid, non-hacky reasons to access the file descriptors, but that is really your call. There's a couple of other small fixes as well. Tested on Ubuntu 17.04, OS X/Xcode 10.13, FreeBSD 11.1.

I've submitted this here because I don't have an SF account handy and GH is superior for submitting multiple commits, but I can submit on SF if that's better for you.

@jwakely
Copy link
Owner

jwakely commented Sep 30, 2017

Submitting it here is fine - makes it easier for me to fetch the change and merge it locally.

I haven't looked at the patch but it sounds sane, I'll take a proper look ASAP. Thanks.

test_pstreams.cc Outdated
explicit pguard(ipstream& in, int signal=SIGKILL)
: buf_(*in.rdbuf()), signal_(signal) { }
explicit pguard(ipstream& is, int signal=SIGKILL)
: buf_(*is.rdbuf()), signal_(signal) { }
Copy link
Owner

Choose a reason for hiding this comment

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

Sigh, this is a stupid warning from Clang. The code is fine. I'll merge this though, just to shut the warning up.

@cgull
Copy link
Author

cgull commented Oct 2, 2017

Over the weekend I realized that the better way to handle our problem would be to open all the pipes with O_CLOEXEC in the first place, and then reset that flag appropriately before executing the pipe child. Unfortunately, there's no portable way to do that: pipe() does not have a flags parameter, and so this would require the non-standard pipe2(), or maybe sockets, or named pipes, or something.

That however would be a more drastic change for a portable header-only library-- it would probably require using autoconf or nasty #ifdefs or something.

Our case is adequately handled by adding a little locking and setting FD_CLOEXEC later, so I'm not asking for anything-- just mentioning this issue for future reference.

@jwakely
Copy link
Owner

jwakely commented Oct 2, 2017

I know I've considered this problem in the past (many years ago), but I don't remember if I came to any conclusion. One option would be additional flags passed to the constructors and open functions that would set the close-on-exec flag.

(My long-term plan has always been to separate the creation of the child process and pipes into a separate process-management class, which would explicitly support such things, and then build the streambuf functionality around that ... but it's currently vapourware).

@cgull
Copy link
Author

cgull commented Nov 16, 2017

ping

I haven't seen anything for a while...

@jwakely jwakely self-assigned this Jun 10, 2020
@cgull
Copy link
Author

cgull commented Jun 22, 2021

ping again...

Would it be better to submit this on SF?

@cgull
Copy link
Author

cgull commented Jun 22, 2021

rebased and cleaned up.

@jwakely
Copy link
Owner

jwakely commented Jun 24, 2021

Hi - no, I actually prefer using GH to SF, so this is fine, I'm just really bad at getting through my TODO list.

I'll re-review this tomorrow. Thanks for the ping.

@jwakely
Copy link
Owner

jwakely commented Jul 30, 2021

Why does the new function return a size_t and not a pmode ?

Other than that, it looks fine.

@cgull
Copy link
Author

cgull commented Aug 14, 2021

I don't know why it's size_t, my best guess is that I originally had it returning a count of file descriptors. I agree it should be pmode.

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.

2 participants