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-124405: Fix `NameError` in `openpty` by sobolevn · Pull Request #124406 · python/cpython

Conversation

@sobolevn

@sobolevn sobolevn commented Sep 24, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

@ambv how can I test this change? This looks very platform specific, but it seems that you have a reproducer :)

  • Issue: NameError in openpty after #118826 #124405

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sobolevn
pythongh-124405: Fix NameError in openpty
9517274
@sobolevn sobolevn added the skip news label Sep 24, 2024
@sobolevn
sobolevn requested review from ambv and hugovk September 24, 2024 02:38
@bedevere-app bedevere-app Bot added the awaiting core review label Sep 24, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Sep 24, 2024
NameError in openpty after #118826 #124405
Closed
@sobolevn sobolevn mentioned this pull request Sep 24, 2024
gh-118824: Remove deprecated master_open and slave_open from pty #118826
Merged
hauntsaninja
hauntsaninja approved these changes Sep 24, 2024
View reviewed changes

@hauntsaninja hauntsaninja left a comment •
edited
Loading

Uh oh!

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

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.

Thanks, this looks like the correct inlining of slave_open from #118826. Adding more test coverage sounds good though!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Sep 24, 2024
@sobolevn

sobolevn commented Sep 24, 2024

Copy link
Copy Markdown
Member Author

Adding more test coverage sounds good though!

Thanks for the review! I need help adding tests, since I cannot reproduce this on my machine :(
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
hauntsaninja
hauntsaninja reviewed Sep 24, 2024
View reviewed changes

@hauntsaninja hauntsaninja left a comment •
edited
Loading

Uh oh!

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

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.

I'm going to sleep now, will look tomorrow. Based on typeshed I_PUSH should exist on Linux?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@hauntsaninja

hauntsaninja commented Sep 24, 2024

Copy link
Copy Markdown
Contributor

"highly problematic" indeed ;-) https://github.com/python/typeshed/pull/6807/files#diff-f28c4d3b27ed19db8e4154e55b1ad119f6655aad863bd3e92d9aee2d481589d7R72
sobolevn reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

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

vstinner commented Sep 24, 2024

Copy link
Copy Markdown
Member

I tried to test this change by removing os.openpty():

import os
import pty
del os.openpty
master_fd, slave_fd = pty.openpty()
os.close(master_fd)
os.close(slave_fd)

But it fails on Linux with:

Traceback (most recent call last):
  File "/home/vstinner/python/main/x.py", line 4, in <module>
    master_fd, slave_fd = pty.openpty()
                          ~~~~~~~~~~~^^
  File "/home/vstinner/python/main/Lib/pty.py", line 34, in openpty
    master_fd, slave_name = _open_terminal()
                            ~~~~~~~~~~~~~~^^
  File "/home/vstinner/python/main/Lib/pty.py", line 58, in _open_terminal
    raise OSError('out of pty devices')
OSError: out of pty devices

On my Fedora 41, I have no /dev/pty* device, only /dev/tty* devices (97 files) that I cannot open (PermissionError) because my user is not in tty or dialout groups.

It seems like os.openpty() is implemented by communicating with /dev/ptmx device using ioctl(). strace:

openat(AT_FDCWD, "/dev/ptmx", O_RDWR)   = 3
ioctl(3, TIOCGPTN, [3])                 = 0
ioctl(3, TIOCSPTLCK, [0])               = 0
ioctl(3, TIOCGPTPEER, 0x102)            = 4
ioctl(3, FIOCLEX)                       = 0
ioctl(4, FIOCLEX)                       = 0
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Sep 24, 2024

Copy link
Copy Markdown
Member

On my Fedora 41, I have no /dev/pty* device, only /dev/tty* devices (97 files) that I cannot open (PermissionError) because my user is not in tty or dialout groups.

Same on FreeBSD. On FreeBSD, os.openpty() is implemented with (syscalls recorded by truss):

posix_openpt(O_RDWR|O_NOCTTY)                    = 3 (0x3) 
ioctl(3,TIOCPTMASTER,0x0)                        = 0 (0x0)
ioctl(3,TIOCPTMASTER,0x0)                        = 0 (0x0)
ioctl(3,TIOCPTMASTER,0x0)                        = 0 (0x0)
ioctl(3,FIODGNAME,0x820f8bb90)                   = 0 (0x0)
openat(AT_FDCWD,"/dev/pts/1",O_RDWR,00)          = 4 (0x4)
ioctl(3,FIOCLEX,0x0)                             = 0 (0x0)
ioctl(4,FIOCLEX,0x0)                             = 0 (0x0)
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Sep 24, 2024

Copy link
Copy Markdown
Member

pty._open_terminal() code is quite old, it was added in 1994 when the pty module was created by commit 23cb2a8. The code almost didn't change since 1994: it still tries to open a /dev/ptyXY device and then use /dev/ttyXY device.

I guess that os.openpty() is now commonly available, and nobody uses the _open_terminal() fallback code path anymore, which looks broken on most platforms.

We should maybe do something with this code. Deprecate it. Remove it. I don't know :-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner approved these changes Sep 24, 2024
View reviewed changes

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sobolevn

sobolevn commented Sep 24, 2024

Copy link
Copy Markdown
Member Author

@vstinner thanks a lot for your help! 👍
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ambv
ambv merged commit 17b3bc9 into python:main Sep 24, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hauntsaninja hauntsaninja hauntsaninja approved these changes
@vstinner vstinner vstinner approved these changes
@ambv ambv Awaiting requested review from ambv
@hugovk hugovk Awaiting requested review from hugovk

Uh oh!

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

Assignees

No one assigned

Labels

skip news

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.

4 participants

@sobolevn @hauntsaninja @vstinner @ambv
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.