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

Allow fcntl functions to accept objects with fileno() function #4144

Merged
merged 1 commit into from
Sep 1, 2022

Conversation

richardhozak
Copy link
Contributor

As specified in Python official documentation
(https://docs.python.org/3.10/library/fcntl.html)

All functions in this module take a file descriptor fd as their first
argument. This can be an integer file descriptor, such as returned by
sys.stdin.fileno(), or an io.IOBase object, such as sys.stdin itself,
which provides a fileno() that returns a genuine file descriptor.

And clarified more in fcntl.fcntl function:

Perform the operation cmd on file descriptor fd (file objects
providing a fileno() method are accepted as well).

All function in fcntl modules should accept either int fd or object with
fileno() function which returns int fd.

This was already implemented with for fcntl.fcntl function with
io::Fildes newtype helper which extracted either int fd or called
fileno() function, and it was also implemented with duplicated ad-hoc
code for fcntl.ioctl.

This commit replaces the ad-hoc implementation with io::Fildes and
adds it to missing functions: fcntl.flock and fcntl.lockf.

For more information that this is implemented in the same way as
CPython, you can check the corresponding CPython module:
https://github.com/python/cpython/blob/3.10/Modules/clinic/fcntlmodule.c.h

The functions are: fcntl_fcntl, fcntl_ioctl, fcntl_flock and
fcntl_lockf, all of these functions use
_PyLong_FileDescriptor_Converter which does the same thing as
RustPython's io::Fildes, meaning it either extracts the argument as
int fd or calls fileno() function on passed object that returns the int
fd.

Here is the implementation for _PyLong_FileDescriptor_Converter:
https://github.com/python/cpython/blob/3.10/Objects/fileobject.c#L227
which in turns calls PyObject_AsFileDescriptor which is located here
https://github.com/python/cpython/blob/3.10/Objects/fileobject.c#L180 in
the same file and we can see that it tries to convert it to int or call
fileno() function.

@richardhozak
Copy link
Contributor Author

Note regarding the python unit tests: test_flock, test_lockf_exclusive, test_lockf_shared.

The tests no longer fail with TypeError: 'BufferedRandom' object cannot be interpreted as an integer which makes test_flock pass the test, but test_lockf_exclusive and test_lockf_exclusive still fail but not on fcntl calls, they fail on Process() constructor with AttributeError: module 'os' has no attribute 'fork' which seems unrelated with this change.

This unrelated error was probably never detected as fcntl calls failed earlier, before Process() could even be called.

@@ -90,21 +90,12 @@ mod fcntl {

#[pyfunction]
fn ioctl(
obj: PyObjectRef,
io::Fildes(fd): io::Fildes,
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch, thank you!
cc @tgsong827

Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

would you please check lint error? other parts looks good. thank you!

As specified in Python official documentation
(https://docs.python.org/3.10/library/fcntl.html)

> All functions in this module take a file descriptor fd as their first
> argument. This can be an integer file descriptor, such as returned by
> sys.stdin.fileno(), or an io.IOBase object, such as sys.stdin itself,
> which provides a fileno() that returns a genuine file descriptor.

And clarified more in fcntl.fcntl function:

> Perform the operation cmd on file descriptor fd (file objects
> providing a fileno() method are accepted as well).

All function in fcntl modules should accept either int fd or object with
fileno() function which returns int fd.

This was already implemented with for `fcntl.fcntl` function with
`io::Fildes` newtype helper which extracted either int fd or called
fileno() function, and it was also implemented with duplicated ad-hoc
code for `fcntl.ioctl`.

This commit replaces the ad-hoc implementation with `io::Fildes` and
adds it to missing functions: `fcntl.flock` and `fcntl.lockf`.

For more information that this is implemented in the same way as
CPython, you can check the corresponding CPython module:
https://github.com/python/cpython/blob/3.10/Modules/clinic/fcntlmodule.c.h

The functions are: `fcntl_fcntl`, `fcntl_ioctl`, `fcntl_flock` and
`fcntl_lockf`, all of these functions use
`_PyLong_FileDescriptor_Converter` which does the same thing as
RustPython's `io::Fildes`, meaning it either extracts the argument as
int fd or calls fileno() function on passed object that returns the int
fd.

Here is the implementation for `_PyLong_FileDescriptor_Converter`:
https://github.com/python/cpython/blob/3.10/Objects/fileobject.c#L227
which in turns calls `PyObject_AsFileDescriptor` which is located here
https://github.com/python/cpython/blob/3.10/Objects/fileobject.c#L180 in
the same file and we can see that it tries to convert it to int or call
fileno() function.

Note regarding the python unit tests `test_flock`,
`test_lockf_exclusive`, `test_lockf_shared`:

The tests no longer fail with `TypeError: 'BufferedRandom' object cannot
be interpreted as an integer` which makes `test_flock` pass the test,
but `test_lockf_exclusive` and `test_lockf_exclusive` still fail but not
on fcntl calls, they fail on `Process()` constructor with
`AttributeError: module 'os' has no attribute 'fork'` which seems
unrelated with this change.

This unrelated error was probably never detected as fcntl calls failed
earlier, before `Process()` could even be called.
@richardhozak
Copy link
Contributor Author

Fixed the lint error, side-effect of this MR is that PyObjectRef is no longer needed in stdlib/src/fcntl.rs and clippy warned about unused import.

@youknowone youknowone merged commit 31fdb20 into RustPython:main Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants