Skip to content

Rewrite the fcntl module #132742

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

Open
serhiy-storchaka opened this issue Apr 20, 2025 · 4 comments
Open

Rewrite the fcntl module #132742

serhiy-storchaka opened this issue Apr 20, 2025 · 4 comments
Assignees
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Apr 20, 2025

The current code of the fcntl module is a little mess. This was caused particularly by the nature of the underlying C API (fcntl() and ioctl() take an argument as a C int or a pointer to a structure, and the type of the argument, as well as the size of the structure is not explicitly specified, but depends on opcode and is platform depending), and particularly by the decades of incremental changes.

At Python level, fcntl() and ioctl() take an optional argument which can be an integer, a string, or a mutable or immutable bytes-like object. The result can be an integer or a bytes object. The input mutable bytes-like object can be modified in-place in ioctl(). Issues, differences and inconsistencies between fcntl() and ioctl():

  • ioctl() first try to interpret the argument as a mutable bytes-like object, then as a string or immutable bytes-like object, then as an integer. fcntl() does the same except trying a mutable bytes-like object. Raising and silencing exceptions is not particularly efficient, and silencing arbitrary exceptions is not good.
  • fcntl() and ioctl() accept string argument. This looks like an unintentional side effect in Python 2 which was not fixed in Python 3. I do not know any operation code which takes a string argument -- they take binary structures. Even if there is an operation which takes a char array, most likely it should be encoded using filesystem encoding instead of UTF-8, so bytes is more preferable than str. String argument should be deprecated.
  • ioctl() makes the temporary buffer null-terminated, but fcntl() does not.
  • ioctl() has the mutate_arg parameter, but fcntl() does not, even if some operations mutate argument.
  • ioctl() copies the content of the mutable bytes-like object to a temporary buffer if mutate_arg is true and it fits in a buffer, and then copies it back. It was needed when the parsing code used "w#", because the object could be resized after releasing the GIL. And it was wrong, because copying back will not work after resizing. But now "w*" is used ("w#" no longer supported), so no copying to temporary buffer is needed.
  • ioctl() does not release the GIL when call the C ioctl() if the mutable bytes-like object does not fit in the temporary buffer. It can, for reasons described above.
  • ioctl() accepts opcode as unsigned int unsigned long, fcntl() accepts only int. fcntl() accepts the integer argument as unsigned int, ioctl() accepts only int. Using an unsigned format makes the code more error-prone (Deprecate accepting out of range values for unsigned integers in PyArg_Parse #132629 should mitigate this), but makes it more usable if some opcode or value constants in C has the MSB set. I think that both function should accept unsigned int (unsigned long?) for both arguments.
  • fcntl() automatically retries on EINTR, ioctl() raises an OSError.

Linked PRs

@serhiy-storchaka serhiy-storchaka self-assigned this Apr 20, 2025
@serhiy-storchaka
Copy link
Member Author

@vstinner, was there a reason to not retry ioctl() after EINTR, or it was an omission?

@serhiy-storchaka serhiy-storchaka added type-feature A feature request or enhancement 3.14 new features, bugs and security fixes labels Apr 20, 2025
@serhiy-storchaka
Copy link
Member Author

It seems that ioctl() on BSD platforms uses unsigned long for opcode (see #69214), so there is a reason for the difference in this case.

@serhiy-storchaka
Copy link
Member Author

It seems that string argument can be used with ioctl() (see #42917). Although bytes would work as well, but the existing code may use str. So let's leave it at that.

@picnixz picnixz added stdlib Python modules in the Lib dir extension-modules C modules in the Modules dir and removed 3.14 new features, bugs and security fixes stdlib Python modules in the Lib dir labels Apr 20, 2025
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Apr 20, 2025
Test with different types of argument: integer, mutable and immutable
buffers, immutable buffer with mutable_flag set to false.
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Apr 21, 2025
serhiy-storchaka added a commit that referenced this issue Apr 21, 2025
Test with different types of argument: integer, mutable and immutable
buffers, immutable buffer with mutable_flag set to false.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Apr 21, 2025
Test with different types of argument: integer, mutable and immutable
buffers, immutable buffer with mutable_flag set to false.
(cherry picked from commit a04390b)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Apr 21, 2025
serhiy-storchaka added a commit that referenced this issue Apr 21, 2025
…2764)

Test with different types of argument: integer, mutable and immutable
buffers, immutable buffer with mutable_flag set to false.
(cherry picked from commit a04390b)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@collinfunk
Copy link
Contributor

POSIX required (until removed in POSIX.1-2024) the ioctl function to use int for the second argument. However, 4.4BSD used unsigned long and since many systems were derived/inspired by 4.4BSD that is mostly what is used. There is a decade old bug report in glibc for this, so I doubt it will ever change [1].

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=14362

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Apr 21, 2025
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Apr 21, 2025
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Apr 21, 2025
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Apr 22, 2025
* Do not skip ALL ioctl() tests when /dev/tty is not available.
* Use better tests for integer argument.
* Add also parallel tests for tcflush() and tcflow().
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Apr 23, 2025
…132765)

(cherry picked from commit 5f50541)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Apr 23, 2025
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Apr 23, 2025
serhiy-storchaka added a commit that referenced this issue Apr 23, 2025
… (GH-132832)

(cherry picked from commit 5f50541)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit that referenced this issue Apr 24, 2025
* Support arbitrary bytes-like objects, not only bytes, in fcntl().
* The fcntl() buffer argument is now null-terminated.
* Automatically retry an ioctl() system calls failing with EINTR.
* Release the GIL for an ioctl() system call even for large bytes-like object.
* Do not silence arbitrary errors whet try to get a buffer.
* Optimize argument parsing, check the argument type before trying to get
  a buffer or convert it to integer.
* Fix some error messages.
serhiy-storchaka added a commit that referenced this issue Apr 28, 2025
* Use better tests for integer argument.
* Add also parallel tests for tcflush() and tcflow().
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Apr 28, 2025
)

* Use better tests for integer argument.
* Add also parallel tests for tcflush() and tcflow().
(cherry picked from commit ed8e886)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit that referenced this issue Apr 28, 2025
)

* Use better tests for integer argument.
* Add also parallel tests for tcflush() and tcflow().
(cherry picked from commit ed8e886)
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Apr 28, 2025
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Apr 28, 2025
…-133070)

(cherry picked from commit 25186c2)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit that referenced this issue Apr 28, 2025
…) (GH-133104)

(cherry picked from commit 25186c2)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
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

No branches or pull requests

3 participants