Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 35.2k
gh-90102: Remove isatty call during regular open#124922
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
a1a1cb7b07dac7346517ecba68bc110c490de661d11f96a9f1b127231783237882867cFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Skip the ``isatty`` system call during open() when the file is known to not | ||
| be a character device. This provides a slight performance improvement when | ||
| reading whole files. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -12,9 +12,6 @@ | ||
| #ifdef HAVE_SYS_TYPES_H | ||
| # include <sys/types.h> | ||
| #endif | ||
| #ifdef HAVE_SYS_STAT_H | ||
vstinner marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| # include <sys/stat.h> | ||
| #endif | ||
| #ifdef HAVE_IO_H | ||
| # include <io.h> | ||
| #endif | ||
| @@ -1218,6 +1215,24 @@ _io_FileIO_isatty_impl(fileio *self) | ||
| return PyBool_FromLong(res); | ||
| } | ||
| /* Checks whether the file is a TTY using an open-only optimization. | ||
| TTYs are always character devices. If the interpreter knows a file is | ||
| not a character device when it would call ``isatty``, can skip that | ||
| call. Inside ``open()`` there is a fresh stat result that contains that | ||
| information. Use the stat result to skip a system call. Outside of that | ||
| context TOCTOU issues (the fd could be arbitrarily modified by | ||
| surrounding code). */ | ||
| static PyObject * | ||
| _io_FileIO_isatty_open_only(PyObject *op, PyObject *Py_UNUSED(ignored)) | ||
| { | ||
| fileio *self = _PyFileIO_CAST(op); | ||
| if (self->stat_atopen != NULL && !S_ISCHR(self->stat_atopen->st_mode)) { | ||
| Py_RETURN_FALSE; | ||
| } | ||
| return _io_FileIO_isatty_impl(self); | ||
| } | ||
| #include "clinic/fileio.c.h" | ||
| static PyMethodDef fileio_methods[] = { | ||
| @@ -1234,6 +1249,7 @@ static PyMethodDef fileio_methods[] = { | ||
| _IO_FILEIO_WRITABLE_METHODDEF | ||
| _IO_FILEIO_FILENO_METHODDEF | ||
| _IO_FILEIO_ISATTY_METHODDEF | ||
| {"_isatty_open_only", _io_FileIO_isatty_open_only, METH_NOARGS}, | ||
| {"_dealloc_warn", fileio_dealloc_warn, METH_O, NULL}, | ||
| {"__reduce__", _PyIOBase_cannot_pickle, METH_NOARGS}, | ||
| {"__reduce_ex__", _PyIOBase_cannot_pickle, METH_O}, | ||
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 think this should be:
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.
Yep,
Py_RETURN_FALSE;I got right in the C but not the pyio. Making a new PR to update....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.
#125089