GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-85984: Add POSIX pseudo-terminal functions. by 8vasu · Pull Request #102413 · python/cpython

Conversation

@8vasu

@8vasu 8vasu commented Mar 4, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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() and pty.fork() return a pair pid, fd, where fd 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 reimplementing ptsname() 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 adds os.posix_openpt(), os.grantpt(), and os.unlockpt() since all of these POSIX functions are "companions" of each other.

Signed-off-by: Soumendra Ganguly [email protected]

  • Issue: Lib/pty.py major revision #85984

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@8vasu
8vasu requested review from corona10 and erlend-aasland as code owners March 4, 2023 07:35
@bedevere-bot bedevere-bot mentioned this pull request Mar 4, 2023
Lib/pty.py major revision #85984
Open
@bedevere-bot bedevere-bot added the awaiting review label Mar 4, 2023
@8vasu 8vasu changed the title gh-85984: Add posix pseudo-terminal functions. gh-85984: Add POSIX pseudo-terminal functions. Mar 4, 2023
This was referenced Mar 4, 2023
gh-85984: Major revision of the pty library. #101833
Closed
gh-85984: New additions and improvements to the tty library. #101832
Merged
@8vasu

8vasu commented Mar 4, 2023

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

8vasu
8vasu commented Mar 4, 2023
View reviewed changes
Comment thread Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@arhadthedev arhadthedev added interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-IO labels Mar 4, 2023
@arhadthedev

arhadthedev commented Mar 4, 2023

Copy link
Copy Markdown
Member

@gpshead as a reviewer of the preceeding gh-101831.

8vasu reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland
erlend-aasland reviewed Mar 6, 2023
View reviewed changes
Comment thread configure.ac Resolved

Uh oh!

There was an error while loading. Please reload this page.

@8vasu
8vasu requested review from erlend-aasland and removed request for corona10 March 8, 2023 07:23
@8vasu
8vasu force-pushed the posix_pt branch 4 times, most recently from 98516af to 2ab16ad Compare March 15, 2023 02:13
@8vasu
Add POSIX pseudo-terminal functions.
65eb913
Signed-off-by: Soumendra Ganguly <[email protected]>
@8vasu
8vasu force-pushed the posix_pt branch from 78ab582 to 65eb913 Compare March 15, 2023 03:20
@blurb-it
📜🤖 Added by blurb_it.
1e4a6ad
@8vasu

8vasu commented Mar 15, 2023

Copy link
Copy Markdown
Contributor Author

@erlend-aasland Thank you for being patient.

Turns out that unlike unlike openpty(), login_tty(), and forkpty(), these 4 POSIX functions are a part of the standard C library on *nix and not a part of libutil. Therefore, I just needed to add the function names in an AC_CHECK_FUNCS block in configure.ac.

However, maybe we need to make a separate PR to modify the openpty(), login_tty(), forkpty() block in configure.ac to use WITH_SAVE_ENV?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@8vasu
Merge branch 'main' into posix_pt
bb14261
@erlend-aasland

erlend-aasland commented Mar 22, 2023

Copy link
Copy Markdown
Contributor

[…] Therefore, I just needed to add the function names in an AC_CHECK_FUNCS block in configure.ac.

Much better, thanks! There is no need for WITH_SAVE_ENV now, AFAICS.

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland
erlend-aasland approved these changes Mar 22, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Mar 22, 2023
@8vasu

8vasu commented Mar 23, 2023

Copy link
Copy Markdown
Contributor Author

@erlend-aasland Thank you!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@8vasu

8vasu commented Apr 14, 2023

Copy link
Copy Markdown
Contributor Author

@gpshead Just a gentle reminder. No rush.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gpshead gpshead added 🔨 test-with-buildbots Test PR w/ buildbots; report in status section extension-modules C modules in the Modules dir labels May 19, 2023
@bedevere-bot

bedevere-bot commented May 19, 2023

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @gpshead for commit f6dcd54 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 19, 2023
encukou added 2 commits January 3, 2024 13:46
@encukou
Merge in the main branch
40bad66
@encukou
Update whatsnew version
4645976
encukou
encukou reviewed Jan 3, 2024
View reviewed changes
Comment thread Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/posixmodule.c
Comment on lines +8103 to +8105
sig_saved = PyOS_setsig(SIGCHLD, SIG_DFL);
ret = grantpt(fd);
PyOS_setsig(SIGCHLD, sig_saved);

@encukou encukou Jan 3, 2024

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@colesbury colesbury Jan 3, 2024

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

encukou reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@encukou encukou Jan 29, 2024

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@encukou
Add pseudoterminal tests
efc9482
@encukou

encukou commented Jan 8, 2024

Copy link
Copy Markdown
Member

Reading the manpages further, I see that Linux has a reentrant variant of ptsname:

ptsname_r() is a Linux extension, that is proposed for inclusion in the next major revision of POSIX.1 (Issue 8).

IMO, os.ptsname should call this on Linux if it's available (with a char buffer[MAXPATHLEN+1]), and the docs should note that the function isn't thread-safe on other systems.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou

encukou commented Jan 10, 2024

Copy link
Copy Markdown
Member

(If you prefer to leave ptsname_r to another PR, that's an option too.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@8vasu

8vasu commented Jan 10, 2024

Copy link
Copy Markdown
Contributor Author

@encukou I prefer to include ptsname_r in this PR; thank you for the suggestion and sorry about the late reply. This is on my TODO list; please give me 1 more day (or maybe 2).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou

encukou commented Jan 11, 2024

Copy link
Copy Markdown
Member

There's no rush, take your time :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@8vasu
Update os.ptsname() so that it uses the C function ptsname_r() if it …
3effe4b
…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 <[email protected]>
@8vasu

8vasu commented Jan 20, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@encukou

I have made the changes you requested and more; please take a look at them when you get time:

  1. Change os.grantpt() so that the correct errno is set before calling posix_error().
  2. Change os.ptsname() so that it uses os.ptsname_r() when available; reflect this in docs.
  3. Mention OS_CLOEXEC in the documentation for os.posix_openpt().
  4. Mention in the docs that these functions do not close the master fd upon failure.
  5. One of the tests that you added was using os.openpty() followed by os.grantpt() and os.unlockpt(). That did not make sense to me, since I think openpty() C implementations do this already? Please let me know if this is wrong. I added a new test that follows the standard sequence posix_openpt() -> grantpt() -> unlockpt() -> ptsname() that this new test subsumed test that you had written for ptsname().
  6. I used https://github.com/tiran/cpython_autoconf (autoconf version 2.71) written by @tiran to generate the configure script. The test Tests / Check if generated files are up to date (pull_request) failed; maybe I should use something else now? The default autoconf on my Debian system is also (GNU Autoconf) 2.71, so generating using that will not make a difference I think.
  7. About the pty pair terminology: I noticed that you used the naming scheme main, second. I used the alternative mother, son as a test to see if you like it. While I am not a CPython maintainer and hence do not have much say in deciding which directions such a prestigious project should take, I have severe clinical OCD/OCPD and would like to issue a warning here: using generic names like main_fd, second_fd, or parent_fd, child_fd, or server_fd, client_fd will lead to havoc very easily. Just take a look at the source of your very own https://github.com/python/cpython/blob/3.12/Lib/pty.py and try a search/replace involving terms parent/child (already used by processes) or a program like ssh(1) which uses the terms server/client in networking context and also sets up a pty pair. On the other hand one can easily have first_fd, second_fd, third_fd or even main_fd in their existing programs. The pair main, subordinate comes to mind, but again, main_fd might be taken and main, subordinate carry the same overtones as master, slave, which is what you are trying to avoid in the first place. For the most comfortable change, we must use something that is -
    • not already taken to almost absolutely eliminate any chances of collision while
    • maintaining that the initials m and s stay the same.

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 main_fd and second_fd. That is misleading: a file can be addressed via multiple fds and upon reopening the second file/second end, we should call the new fd third_fd or second_fd_of_second_end and not reopened_second_fd (since you open files and not file descriptors).

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.

encukou reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou

encukou commented Jan 22, 2024

Copy link
Copy Markdown
Member

1-4. Thank you!
5. Thanks. I'm not an expert here -- I'm learning about ptys and their C API as I go. I'm happy that the test I added is hackable :)
6. The command is make regen-configure. If that doesn't work for you, run the ubuntu:22.04 container (that is currently what the makefile uses, via /Tools/build/regen-configure.sh). If it's a problem, let me know and I'll regenerate this.
If you found @tiran's outdated container mentioned in some current documentation, please let me know -- we should fix that. (Sadly we can't fix all the docs, like personal notes or shell histories...)
7. For the tests, local variable names don't matter much; it's OK to be a bit creative. For the API, this adds fd -- no problem there! For docs, it's master/slave for now; changing that will be a different discussion (in which the Editorial board might well decide to keep following POSIX).

Let's pretend second_fd stands for fd of the second file, and reopened_second_fd stands for fd for the reopened second file, or reopened fd for the second file. Does that work better? If not, change this to whatever feels best :)

8vasu reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@8vasu

8vasu commented Jan 23, 2024

Copy link
Copy Markdown
Contributor Author

@encukou you are very kind. Let me work on the reconfigure (and everything else you suggested).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@8vasu
Run /Tools/build/regen-configure.sh
2359d1b
Signed-off-by: Soumendra Ganguly <[email protected]>
@8vasu

8vasu commented Jan 24, 2024

Copy link
Copy Markdown
Contributor Author

@encukou Looks like all checks have passed! Your /Tools/build/regen-configure.sh suggestion worked like a charm... after I moved myself away from a directory that had colons (:) in its name as a time (hr, min, sec) separator; docker does not seem to like that.

encukou reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou
Docs: Add POSIX C functions to nitpick_ignore
57acdc3
encukou
encukou approved these changes Jan 24, 2024
View reviewed changes

@encukou encukou left a comment

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@encukou
Merge branch 'main' into posix_pt
dd6082c
@encukou
encukou enabled auto-merge (squash) January 29, 2024 15:40
@encukou
encukou merged commit e351ca3 into python:main Jan 29, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Jan 29, 2024
@8vasu

8vasu commented Jan 29, 2024

Copy link
Copy Markdown
Contributor Author

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

gpshead, encukou, and erlend-aasland reacted with heart emoji
All reactions
  • ❤️ 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou

encukou commented Jan 30, 2024

Copy link
Copy Markdown
Member

Thank you for perservering!
Looks like the main thing missing in 101833 is tests, so, a good strategy might be to send a PR whenever you get a test working :)

If I happen to miss a notification (= I don't reply for about a week), please ping me again.

8vasu reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@8vasu

8vasu commented Jan 30, 2024

Copy link
Copy Markdown
Contributor Author

@encukou

Looks like the main thing missing in 101833 is tests

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 os, termios, and tty over the years instead of the ad-hoc ones that I wrote in Lib/test/test_pty.py. If additional tests are required, I will definitely add them if you wish :)

encukou reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@8vasu 8vasu mentioned this pull request Feb 4, 2024
gh-85984: Add _POSIX_VDISABLE from unistd.h to termios module. #114985
Merged
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@8vasu @gpshead
@encukou @aisk
pythongh-85984: Add POSIX pseudo-terminal functions. (pythonGH-102413)
4dd996d
Signed-off-by: Soumendra Ganguly <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Petr Viktorin <[email protected]>
@zanieb zanieb mentioned this pull request Jun 6, 2024
WIP: Python 3.13 zanieb/python-build-standalone#5
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukou encukou encukou approved these changes
@colesbury colesbury colesbury left review comments
@erlend-aasland erlend-aasland erlend-aasland approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gpshead gpshead

Labels

extension-modules C modules in the Modules dir topic-IO type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@8vasu @arhadthedev @erlend-aasland @bedevere-bot @encukou @colesbury @gpshead
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.