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

bpo-41818: Add os.login_tty() for *nix. #29658

Merged
merged 17 commits into from
May 5, 2022
Merged

bpo-41818: Add os.login_tty() for *nix. #29658

merged 17 commits into from
May 5, 2022

Conversation

8vasu
Copy link
Contributor

@8vasu 8vasu commented Nov 20, 2021

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

https://bugs.python.org/issue41818

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

8vasu commented Nov 20, 2021

@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 :)

Modules/posixmodule.c Outdated Show resolved Hide resolved
Modules/posixmodule.c Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
@tiran
Copy link
Member

tiran commented Dec 4, 2021

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.

@8vasu
Copy link
Contributor Author

8vasu commented Dec 5, 2021

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

Edit: I have made the changes :)

aclocal.m4 Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
aclocal.m4 Outdated Show resolved Hide resolved
Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
Signed-off-by: Soumendra Ganguly <soumendraganguly@gmail.com>
Copy link
Contributor

@MaxwellDupre MaxwellDupre left a 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

Copy link
Contributor

@MaxwellDupre MaxwellDupre left a comment

Choose a reason for hiding this comment

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

Looks ok

@8vasu
Copy link
Contributor Author

8vasu commented Apr 17, 2022

@MaxwellDupre Thank you for the review.

Modules/posixmodule.c Show resolved Hide resolved
Modules/posixmodule.c Outdated Show resolved Hide resolved
@8vasu
Copy link
Contributor Author

8vasu commented May 2, 2022

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

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@bedevere-bot bedevere-bot requested a review from gpshead May 2, 2022 05:23
@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
Copy link
Member

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

@8vasu
Copy link
Contributor Author

8vasu commented May 5, 2022

@gpshead Thank you for the clinic output regeneration :)

@gpshead gpshead merged commit ae553b3 into python:main May 5, 2022
@8vasu
Copy link
Contributor Author

8vasu commented May 5, 2022

@gpshead Thank you for the merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants