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 — bpo-41818: Add os.login_tty() for *nix. by 8vasu · Pull Request #29658 · python/cpython

Conversation

@8vasu

@8vasu 8vasu commented Nov 20, 2021 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

This follows #23536. Also, see #23546, #23686.

os.login_tty() calls the native login_tty() when it is available; otherwise, if setsid() is available and if TIOCSCTTY or ttyname() are available, it runs generic code to emulate login_tty().

Post #23686, #23546, #23740 goals:

  1. add test_winsize() to "Lib/test/test_pty.py";
  2. major revision of "Lib/pty.py", which has not happened since the beginning of the millennium.

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

https://bugs.python.org/issue41818

Sorry, something went wrong.

Uh oh!

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

All reactions
@8vasu
Add os.login_tty() for Unix.
576bc5a
Signed-off-by: Soumendra Ganguly <[email protected]>
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 20, 2021
@bedevere-bot bedevere-bot added the awaiting review label Nov 20, 2021
@blurb-it
📜🤖 Added by blurb_it.
d30e66b
@8vasu 8vasu mentioned this pull request Nov 20, 2021
bpo-41818: Add os.login_tty() for *nix. #23740
Closed
@8vasu

8vasu commented Nov 20, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

@asvetlov @gpshead I closed #23740 and created this because that one had several merge conflicts and unnecessary new content in configure script due to the lack of the "autoconf-archive" package on my system. The C code/documentation changes remain unchanged. Please take a look at this when you have time.

Edit: a gentle request for core review; no rush :)

All reactions

Sorry, something went wrong.

Uh oh!

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

tiran
tiran reviewed Dec 4, 2021
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 Resolved

Uh oh!

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

Comment thread configure.ac Resolved

Uh oh!

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

@tiran
Merge branch 'main' into login_tty
089cb17
@tiran

tiran commented Dec 4, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

I have resolved the conflicts for you. You have to pull the changes to your computer, regenerate the clinic and configure scripts and then commit the changes.

You also need to re-add utmp.h to the list of headers.

All reactions

Sorry, something went wrong.

Uh oh!

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

@8vasu

8vasu commented Dec 5, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

@tiran Thank you so much for your help. I will look into this very soon.

Edit: I have made the changes :)

All reactions

Sorry, something went wrong.

Uh oh!

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

arhadthedev
arhadthedev reviewed Dec 6, 2021
View reviewed changes
Comment thread aclocal.m4 Resolved

Uh oh!

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

tiran
tiran reviewed Dec 6, 2021
View reviewed changes
Comment thread configure.ac Resolved

Uh oh!

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

Comment thread aclocal.m4 Resolved

Uh oh!

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

@8vasu
Re-generate configure script using preferred version of autoconf.
1b02fd7
Signed-off-by: Soumendra Ganguly <[email protected]>
@8vasu
8vasu force-pushed the login_tty branch from 34bfc52 to 1b02fd7 Compare December 6, 2021 20:19
@8vasu
8vasu force-pushed the login_tty branch from e382b1c to 1b02fd7 Compare January 15, 2022 03:40
@8vasu
Use #ifdef instead of #if defined() whenever possible.
0f4d5b3
Signed-off-by: Soumendra Ganguly <[email protected]>
@8vasu
8vasu force-pushed the login_tty branch from 3bde312 to 2cbf457 Compare January 17, 2022 06:32
8vasu added 3 commits January 17, 2022 00:45
@8vasu
Copy posixmodule.c.h from branch 'main' to prepare to fix merge conflict
7b27681
Signed-off-by: Soumendra Ganguly <[email protected]>
@8vasu
Merge branch 'main' into login_tty
f5b28a0
@8vasu
Generate posixmodule.c.h using argument clinic
b8effd2
Signed-off-by: Soumendra Ganguly <[email protected]>
@8vasu
8vasu force-pushed the login_tty branch from 2cbf457 to b8effd2 Compare January 17, 2022 06:49
@8vasu
8vasu requested a review from tiran January 17, 2022 07:18
MaxwellDupre
MaxwellDupre reviewed Apr 17, 2022
View reviewed changes

@MaxwellDupre MaxwellDupre left a comment

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.

413 tests OK.
19 tests skipped:
Tests result: SUCCESS
Also test_pty

Sorry, something went wrong.

Uh oh!

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

All reactions
MaxwellDupre
MaxwellDupre approved these changes Apr 17, 2022
View reviewed changes

@MaxwellDupre MaxwellDupre left a comment

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.

Looks ok

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Apr 17, 2022
@8vasu

8vasu commented Apr 17, 2022

Copy link
Copy Markdown
Contributor Author

@MaxwellDupre Thank you for the review.

All reactions

Sorry, something went wrong.

Uh oh!

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

gpshead
gpshead requested changes Apr 25, 2022
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 Resolved

Uh oh!

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

@bedevere-bot bedevere-bot added awaiting changes and removed awaiting core review labels Apr 25, 2022
8vasu and others added 5 commits May 1, 2022 22:25
@8vasu
Copy posixmodule.c.h from branch 'main' to prepare to fix merge conflict
ec717f7
Signed-off-by: Soumendra Ganguly <[email protected]>
@8vasu
Merge branch 'main' into login_tty
995746e
@8vasu
Generate posixmodule.c.h using Argument Clinic
b2bcbc1
Signed-off-by: Soumendra Ganguly <[email protected]>
@8vasu
Remove the old news file.
e35e7a0
@blurb-it
📜🤖 Added by blurb_it.
6a033b8
@8vasu

8vasu commented May 2, 2022

Copy link
Copy Markdown
Contributor Author

@gpshead I have made the requested changes; please review again. You can ignore the other comments that I made. The following are the answers to your questions/requests; sorry for the slightly long response; I just wanted to provide you with all the information that I gathered.

QUESTION1: aren't there potentially some successful dup2 calls to clean up from in this error case?
ANSWER1: I feel like the function should not close the input argument fd upon any sort of failure and let the user take care of what they want to do with fd (please let me know if that is not acceptable). The only other type of cleanup that comes to mind is, say, close()-ing 0 if dup2(fd, 0) succeeds and dup2(fd, 1) fails. However, are closed standard descriptors (0 or 1 or 2) any better than invalid ones? We could save the original standard descriptors using dup() calls and restore them if any of the dup2()s fail but that feels a little absurd because then we have to handle the dup() errors. I looked at a number of implementations (glibc, musl, *BSD, Illumos, util-linux, etc) and none of them actually bother with handling the dup2() errors at all; see justification in this musl mailing list archive (https://musl.openwall.narkive.com/seFOxlLL/add-login-tty#post2). In fact, currently cpython's os.forkpty() (and hence pty.fork() and pty.spawn()) just calls the system's forkpty(), which, in most cases that I saw, internally calls the system's login_tty(), where dup2() errors are ignored.

QUESTION2: add a comment as to why this is opened to check for an error and immediately closed.
ANSWER2: After putting a lot of thought into it, I decided to remove the "archaic code" completely with the following short conversation that I had ~1.5 years earlier as justification: https://marc.info/?l=glibc-help&m=164436296715753&w=3 This means that now the fallback code only uses TIOCSCTTY and avoids the System V way of obtaining a controlling terminal by open()-ing a tty device file. Also, according to https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap11.html,

if a session leader has no controlling terminal, and opens a terminal device file that is not already associated with a session without using the O_NOCTTY option (see open()), it is implementation-defined whether the terminal becomes the controlling terminal of the session leader

the bold part if a session leader has no controlling terminal is why the glibc implementation first attempts to close() 0,1,2 before opening the tty device; but that means that further errors cannot be reported until the dup2(fd, 2) succeeds (if at all) since stderr is closed.

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

@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels May 2, 2022
@bedevere-bot

bedevere-bot commented May 2, 2022

Copy link
Copy Markdown

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot
bedevere-bot requested a review from gpshead May 2, 2022 05:23
gpshead
gpshead approved these changes May 5, 2022
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels May 5, 2022
@gpshead gpshead self-assigned this May 5, 2022
@gpshead gpshead added type-feature A feature request or enhancement extension-modules C modules in the Modules dir labels May 5, 2022
gpshead
gpshead approved these changes May 5, 2022
View reviewed changes

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

It needs another merge and clinic output regeneration. otherwise this is good.

Sorry, something went wrong.

Uh oh!

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

All reactions
gpshead added 2 commits May 4, 2022 20:22
@gpshead
Merge branch 'main' into login_tty
dc69763
@gpshead
fixup the argument clinic footer (regen)
5ad23a2
@8vasu

8vasu commented May 5, 2022

Copy link
Copy Markdown
Contributor Author

@gpshead Thank you for the clinic output regeneration :)

All reactions

Sorry, something went wrong.

Uh oh!

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

@gpshead
gpshead merged commit ae553b3 into python:main May 5, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label May 5, 2022
@gpshead gpshead mentioned this pull request May 5, 2022
Lib/pty.py major revision #85984
Open
@8vasu

8vasu commented May 5, 2022

Copy link
Copy Markdown
Contributor Author

@gpshead Thank you for the merge.

All reactions

Sorry, something went wrong.

Uh oh!

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

@8vasu 8vasu mentioned this pull request May 6, 2022
gh-85984: Remove legacy Lib/pty.py code. #92365
Merged
@JelleZijlstra JelleZijlstra mentioned this pull request May 7, 2022
add os.login_tty for 3.11 python/typeshed#7800
Merged
JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this pull request May 7, 2022
@JelleZijlstra
add os.login_tty for 3.11
b496d22
python/cpython#29658
hauntsaninja pushed a commit to python/typeshed that referenced this pull request May 7, 2022
@JelleZijlstra
add os.login_tty for 3.11 (#7800)
963b8aa
python/cpython#29658
@reckenrode reckenrode mentioned this pull request Jun 14, 2022
python311: 3.11.0a7 -> 3.11.0b3 NixOS/nixpkgs#173998
Merged
1 of 13 tasks
@8vasu 8vasu mentioned this pull request Mar 7, 2023
gh-85984: Add POSIX pseudo-terminal functions. #102413
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arhadthedev arhadthedev arhadthedev left review comments
@gpshead gpshead gpshead approved these changes
@tiran tiran Awaiting requested review from tiran
+1 more reviewer
@MaxwellDupre MaxwellDupre MaxwellDupre approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

@gpshead gpshead

Labels

extension-modules C modules in the Modules dir 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 @tiran @bedevere-bot @gpshead @arhadthedev @MaxwellDupre @the-knights-who-say-ni
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.