Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-85984: Add POSIX pseudo-terminal functions.#102413

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
encukou merged 14 commits intopython:mainfrom8vasu:posix_pt
Jan 29, 2024
Merged

Conversation

8vasu
Copy link
Contributor

@8vasu8vasu commentedMar 4, 2023
edited
Loading

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 needsos.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 Gangulysoumendraganguly@gmail.com

@8vasu8vasu changed the titlegh-85984: Add posix pseudo-terminal functions.gh-85984: Add POSIX pseudo-terminal functions.Mar 4, 2023
@8vasu
Copy link
ContributorAuthor

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

@arhadthedevarhadthedev added interpreter-core(Objects, Python, Grammar, and Parser dirs) topic-IO labelsMar 4, 2023
@arhadthedev
Copy link
Member

@gpshead as a reviewer of the preceedinggh-101831.

8vasu reacted with thumbs up emoji

@8vasu8vasu requested review fromerlend-aasland and removed request forcorona10March 8, 2023 07:23
@8vasu8vasuforce-pushed theposix_pt branch 4 times, most recently from98516af to2ab16adCompareMarch 15, 2023 02:13
Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
@8vasu
Copy link
ContributorAuthor

@erlend-aasland Thank you for being patient.

Turns out that unlike unlikeopenpty(),login_tty(), andforkpty(), these 4 POSIX functions are a part of the standard C library on *nix and not a part oflibutil. Therefore, I just needed to add the function names in anAC_CHECK_FUNCS block inconfigure.ac.

However, maybe we need to make a separate PR to modify theopenpty(),login_tty(),forkpty() block inconfigure.ac to useWITH_SAVE_ENV?

@erlend-aasland
Copy link
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 forWITH_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.

@8vasu
Copy link
ContributorAuthor

@erlend-aasland Thank you!

@8vasu
Copy link
ContributorAuthor

@gpshead Just a gentle reminder. No rush.

@gpsheadgpshead added 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section extension-modulesC modules in the Modules dir labelsMay 19, 2023
@bedevere-bot
Copy link

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

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

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelMay 19, 2023
Comment on lines 8103 to 8105
sig_saved = PyOS_setsig(SIGCHLD, SIG_DFL);
ret = grantpt(fd);
PyOS_setsig(SIGCHLD, sig_saved);
Copy link
Member

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?

Copy link
Contributor

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

encukou reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

Filed here:#114727

@encukou
Copy link
Member

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

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 achar buffer[MAXPATHLEN+1]), and the docs should note that the function isn't thread-safe on other systems.

@encukou
Copy link
Member

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

@8vasu
Copy link
ContributorAuthor

@encukou I prefer to includeptsname_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).

@encukou
Copy link
Member

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>
@8vasu
Copy link
ContributorAuthor

8vasu commentedJan 20, 2024
edited
Loading

@encukou

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

  1. Changeos.grantpt() so that the correcterrno is set before callingposix_error().
  2. Changeos.ptsname() so that it usesos.ptsname_r() when available; reflect this in docs.
  3. MentionOS_CLOEXEC in the documentation foros.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 usingos.openpty() followed byos.grantpt() andos.unlockpt(). That did not make sense to me, since I thinkopenpty() C implementations do this already? Please let me know if this is wrong. I added a new test that follows the standard sequenceposix_openpt() -> grantpt() -> unlockpt() -> ptsname() that this new test subsumed test that you had written forptsname().
  6. I usedhttps://github.com/tiran/cpython_autoconf (autoconf version 2.71) written by@tiran to generate the configure script. The testTests / 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 schememain, second. I used the alternativemother, 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 likemain_fd, second_fd, orparent_fd, child_fd, orserver_fd, client_fdwill lead to havoc very easily. Just take a look at the source of your very ownhttps://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 likessh(1) which uses the terms server/client in networking context and also sets up a pty pair. On the other hand one can easily havefirst_fd, second_fd, third_fd or evenmain_fd in their existing programs. The pairmain, subordinate comes to mind, but again,main_fd might be taken andmain, subordinate carry the same overtones asmaster, 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 initialsm ands 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 offiles and not a pair offds. Therefore, if we call themmain file/main end andsecond file/second end, then in code the corresponding terms might bemain_fd andsecond_fd. That is misleading: a file can be addressed via multiple fds and upon reopening thesecond file/second end, we should call the new fdthird_fd orsecond_fd_of_second_end and notreopened_second_fd (since you open files and not file descriptors).

ON THE OTHER HAND:main end andsecondARY end might be better. I still think we should grep for all possible terms to avoid collision.

encukou reacted with heart emoji

@encukou
Copy link
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 ismake regen-configure. If that doesn't work for you, run theubuntu: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 somecurrent 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 addsfd -- no problem there! For docs, it'smaster/slave for now; changing that will be a different discussion (in which theEditorial board might well decide to keep following POSIX).

Let's pretendsecond_fd stands forfd of the second file, andreopened_second_fd stands forfd for the reopened second file, orreopened fd for the second file. Does that work better? If not, change this to whatever feels best :)

8vasu reacted with heart emoji

@8vasu
Copy link
ContributorAuthor

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

Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
@8vasu
Copy link
ContributorAuthor

@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

Copy link
Member

@encukouencukou left a 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?

@encukouencukouenabled auto-merge (squash)January 29, 2024 15:40
@encukouencukou merged commite351ca3 intopython:mainJan 29, 2024
@8vasu
Copy link
ContributorAuthor

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

@encukou
Copy link
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

@8vasu
Copy link
ContributorAuthor

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

encukou reacted with thumbs up emoji

aisk pushed a commit to aisk/cpython that referenced this pull requestFeb 11, 2024
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>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@encukouencukouencukou approved these changes

@colesburycolesburycolesbury left review comments

@erlend-aaslanderlend-aaslanderlend-aasland approved these changes

Assignees

@gpsheadgpshead

Labels
extension-modulesC modules in the Modules dirtopic-IOtype-featureA feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

7 participants
@8vasu@arhadthedev@erlend-aasland@bedevere-bot@encukou@colesbury@gpshead

[8]ページ先頭

©2009-2025 Movatter.jp