-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
gh-85984: Add POSIX pseudo-terminal functions. #102413
Conversation
@gpshead This PR introduces 4 new functions which are simple wrappers. The rationale is explained in the main comment, and other details including testing instructions are provided as inline comments on the diffs. For now I have converted #101832 and #101833 to drafts since they require more work and are slightly more ambitious projects. As always, no rush. |
98516af
to
2ab16ad
Compare
Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
@erlend-aasland Thank you for being patient. Turns out that unlike unlike However, maybe we need to make a separate PR to modify the |
Much better, thanks! There is no need for The configure.ac changes look good to me. I'll leave the rest of the review for @zware and @gpshead who've been active on the issue and previous PRs. |
@erlend-aasland Thank you! |
@gpshead Just a gentle reminder. No rush. |
Modules/posixmodule.c
Outdated
sig_saved = PyOS_setsig(SIGCHLD, SIG_DFL); | ||
ret = grantpt(fd); | ||
PyOS_setsig(SIGCHLD, sig_saved); |
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 relies on the GIL. IMO, that shouldn't block this PR for 3.13, but I'd like to follow up.
@colesbury, I plan to do lots of reviews this year. Is there anything I should do when I spot a case like this?
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.
Let's track these in an issue with the "topic-free-threading" label
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.
Filed here: #114727
Reading the manpages further, I see that Linux has a reentrant variant of
IMO, |
(If you prefer to leave |
@encukou I prefer to include |
There's no rush, take your time :) |
…is available. Make os.grantpt() use saved errno upon failure. Update documentation related to the POSIX pty functions. Add a test for the POSIX pty functions. Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
I have made the changes you requested and more; please take a look at them when you get time:
Edit: while this is not the focus of this PR, about point 7 above, I have generated some data using grep on the cpython repository and added it here: #85984 (comment) (commenting again for better reach). Edit: Sorry about the obsessive spam; I cannot help myself, I have OCD. Before taking a break from this, I would like to finally point out that a pty is a pair of files and not a pair of fds. Therefore, if we call them main file/main end and second file/second end, then in code the corresponding terms might be ON THE OTHER HAND: main end and secondARY end might be better. I still think we should grep for all possible terms to avoid collision. |
1-4. Thank you! Let's pretend |
@encukou you are very kind. Let me work on the reconfigure (and everything else you suggested). |
Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
@encukou Looks like all checks have passed! Your |
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.
+1 from me!
@gpshead, do you want to take another look?
@encukou Thank you for merging this! Looks like finally after years I have all the prerequisites for #101833. That one now needs to be updated and maybe broken up into a few parts in case it becomes too difficult to review: one for signal handling, one for winsize handling, one for adjusting the tests, etc. I will do this sometime during the next few months. |
Thank you for perservering! If I happen to miss a notification (= I don't reply for about a week), please ping me again. |
The tests for that were my first contribution to CPython in 2020. However, I still need to update the tests to use the various robust functions that I have added to |
Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com> Co-authored-by: Gregory P. Smith <greg@krypto.org> Co-authored-by: Petr Viktorin <encukou@gmail.com>
This follows #101831. This is one in a series of PRs aimed at cleaning-up, fixing bugs in, introducing new features in, and updating the code in "Lib/pty.py".
This PR answers the following question:
os.forkpty()
andpty.fork()
return a pairpid, fd
, wherefd
is a file descriptor of the master end of a pseudo-terminal pair; if at a later stage one needs to make some modifications to the slave end (such as setting termios attributes), then how does one obtain access to it without reimplementingptsname()
in Python or loading it from a shared library like someone is doing here: https://stackoverflow.com/questions/52338062/calling-libc-select-from-python-from-pty-master-side?This is a dependency of #101833, which only needs
os.ptsname()
. However, this PR also addsos.posix_openpt()
,os.grantpt()
, andos.unlockpt()
since all of these POSIX functions are "companions" of each other.Signed-off-by: Soumendra Ganguly soumendraganguly@gmail.com