From 6b6b9499a21983af0fbefdb688a6c3fa5c1c0c9b Mon Sep 17 00:00:00 2001 From: zxey Date: Tue, 30 Aug 2022 15:20:29 +0200 Subject: [PATCH] Allow fcntl functions to accept objects with fileno() function 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. --- Lib/test/test_fcntl.py | 6 ++---- stdlib/src/fcntl.rs | 16 +++------------- 2 files changed, 5 insertions(+), 17 deletions(-) diff --git a/Lib/test/test_fcntl.py b/Lib/test/test_fcntl.py index 4445028351..06a84118e1 100644 --- a/Lib/test/test_fcntl.py +++ b/Lib/test/test_fcntl.py @@ -141,8 +141,6 @@ def test_fcntl_64_bit(self): finally: os.close(fd) - # TODO: RUSTPYTHON, TypeError: 'BufferedRandom' object cannot be interpreted as an integer - @unittest.expectedFailure def test_flock(self): # Solaris needs readable file for shared lock self.f = open(TESTFN, 'wb+') @@ -157,7 +155,7 @@ def test_flock(self): self.assertRaises(ValueError, fcntl.flock, -1, fcntl.LOCK_SH) self.assertRaises(TypeError, fcntl.flock, 'spam', fcntl.LOCK_SH) - # TODO: RUSTPYTHON, TypeError: 'BufferedRandom' object cannot be interpreted as an integer + # TODO: RUSTPYTHON, AttributeError: module 'os' has no attribute 'fork' @unittest.expectedFailure @unittest.skipIf(platform.system() == "AIX", "AIX returns PermissionError") def test_lockf_exclusive(self): @@ -170,7 +168,7 @@ def test_lockf_exclusive(self): fcntl.lockf(self.f, fcntl.LOCK_UN) self.assertEqual(p.exitcode, 0) - # TODO: RUSTPYTHON, TypeError: 'BufferedRandom' object cannot be interpreted as an integer + # TODO: RUSTPYTHON, AttributeError: module 'os' has no attribute 'fork' @unittest.expectedFailure @unittest.skipIf(platform.system() == "AIX", "AIX returns PermissionError") def test_lockf_share(self): diff --git a/stdlib/src/fcntl.rs b/stdlib/src/fcntl.rs index d0548c84e8..795b570319 100644 --- a/stdlib/src/fcntl.rs +++ b/stdlib/src/fcntl.rs @@ -52,7 +52,6 @@ mod fcntl { #[cfg(any(target_os = "dragonfly", target_os = "netbsd", target_vendor = "apple"))] #[pyattr] use libc::F_GETPATH; - use rustpython_vm::PyObjectRef; #[pyfunction] fn fcntl( @@ -90,21 +89,12 @@ mod fcntl { #[pyfunction] fn ioctl( - obj: PyObjectRef, + io::Fildes(fd): io::Fildes, request: u32, arg: OptionalArg, i32>>, mutate_flag: OptionalArg, vm: &VirtualMachine, ) -> PyResult { - let fd = obj.try_to_value(vm).or_else(|_| { - let meth = vm.get_method_or_type_error( - obj.clone(), - vm.ctx.interned_str("fileno").unwrap(), - || "ioctl first arg must be an int or object with a fileno() method".to_owned(), - )?; - vm.invoke(&meth, ())?.try_into_value(vm) - })?; - let arg = arg.unwrap_or_else(|| Either::B(0)); match arg { Either::A(buf_kind) => { @@ -153,7 +143,7 @@ mod fcntl { // XXX: at the time of writing, wasi and redox don't have the necessary constants/function #[cfg(not(any(target_os = "wasi", target_os = "redox")))] #[pyfunction] - fn flock(fd: i32, operation: i32, vm: &VirtualMachine) -> PyResult { + fn flock(io::Fildes(fd): io::Fildes, operation: i32, vm: &VirtualMachine) -> PyResult { let ret = unsafe { libc::flock(fd, operation) }; // TODO: add support for platforms that don't have a builtin `flock` syscall if ret < 0 { @@ -166,7 +156,7 @@ mod fcntl { #[cfg(not(any(target_os = "wasi", target_os = "redox")))] #[pyfunction] fn lockf( - fd: i32, + io::Fildes(fd): io::Fildes, cmd: i32, len: OptionalArg, start: OptionalArg,