-
-
Notifications
You must be signed in to change notification settings - Fork 35.2k
bpo-31368: Expose preadv (preadv2) and pwritev (pwritev2) in the os module #5239
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
Changes from 7 commits
ea21128
4570976
21a3004
8a2a2a3
c0c53dc
4ddbca9
cac62ac
c7b7285
58910d0
f7cdf22
47f65b5
87c6970
cb74d38
5563b8d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1195,6 +1195,32 @@ or `the MSDN <https://msdn.microsoft.com/en-us/library/z0kc8e3z.aspx>`_ on Windo | |
| .. versionadded:: 3.3 | ||
|
|
||
|
|
||
| .. function:: preadv(fd, buffers, offset, flags = None) | ||
|
|
||
| Combines the functionality of :func:`os.readv` and :func:`os.pread`. It | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed you didn't duplicate the definition of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I has hessitant to duplicate because Corrected in c7b7285 |
||
| performs the same task as :func:`os.readv`, but adds a fourth argument, | ||
| offset, which specifies the file offset at which the input operation is | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You put asterisks around flags below and I think the first offset should have the name because this is referring to the argument.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Corrected in c7b7285 |
||
| to be performed. | ||
|
|
||
| If the *flags* argument is provided it will invoke the *preadv2* system | ||
| call, which modifies the behavior on a per-call basis based on the value | ||
| of the *flags* argument. If this argument is ommited, the *preadv* system | ||
| call will be called instead. | ||
|
|
||
| The flags argument contains a bitwise OR of zero or more of the following | ||
| flags: | ||
|
|
||
| .. data:: RWF_HIPRI | ||
| RWF_NOWAIT | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hum, I suggest to have one ".. data" entry per constant and copy the documentation from the manual page. Extract of Fedora 27 manual pages: It would become simpler to describe the availability. |
||
|
|
||
| The above constants are available on Linux Kernel 4.11 and 4.6 (or newer) | ||
| respectively. | ||
|
|
||
| Availability: Unix. | ||
|
|
||
| .. versionadded:: 3.7 | ||
|
|
||
|
|
||
| .. function:: tcgetpgrp(fd) | ||
|
|
||
| Return the process group associated with the terminal given by *fd* (an open | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -272,6 +272,30 @@ def test_pread(self): | |
| finally: | ||
| os.close(fd) | ||
|
|
||
| @unittest.skipUnless(hasattr(posix, 'preadv'), "test needs posix.preadv()") | ||
| def test_preadv(self): | ||
| fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT) | ||
| try: | ||
| os.write(fd, b'test1tt2t3') | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please write more than 10 bytes, to make sure that preadv2() doesn't read more than expected. |
||
| os.lseek(fd, 0, os.SEEK_SET) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hum, is it really needed to seek back at offset 0? It would be more interesting to test preadv() with an offset different than the current offset. |
||
| buf = [bytearray(i) for i in [5, 3, 2]] | ||
| self.assertEqual(posix.preadv(fd, buf, 0), 10) | ||
| self.assertEqual([b'test1', b'tt2', b't3'], [bytes(i) for i in buf]) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure that bytes() cast is needed here. Just write list(buf) for the second argument? |
||
| finally: | ||
| os.close(fd) | ||
|
|
||
| @unittest.skipUnless(hasattr(posix, 'RWF_SYNC'), "test needs posix.preadv2()") | ||
| def test_preadv2(self): | ||
| fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT) | ||
| try: | ||
| os.write(fd, b'test1tt2t3') | ||
| os.lseek(fd, 0, os.SEEK_SET) | ||
| buf = [bytearray(i) for i in [5, 3, 2]] | ||
| self.assertEqual(posix.preadv(fd, buf, 0, os.RWF_SYNC), 10) | ||
| self.assertEqual([b'test1', b'tt2', b't3'], [bytes(i) for i in buf]) | ||
| finally: | ||
| os.close(fd) | ||
|
|
||
| @unittest.skipUnless(hasattr(posix, 'pwrite'), "test needs posix.pwrite()") | ||
| def test_pwrite(self): | ||
| fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Expose preadv2 system call in the os module. Patch by Pablo Galindo |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8163,6 +8163,74 @@ os_pread_impl(PyObject *module, int fd, int length, Py_off_t offset) | |
| } | ||
| #endif /* HAVE_PREAD */ | ||
|
|
||
| #if defined(HAVE_PREADV) || defined (HAVE_PREADV2) | ||
| /*[clinic input] | ||
| os.preadv -> Py_ssize_t | ||
|
|
||
| fd: int | ||
| buffers: object | ||
| offset: Py_off_t | ||
| flags: int = 0 | ||
| / | ||
|
|
||
| Read a number of bytes from a file descriptor starting at a particular offset. | ||
|
|
||
| Read length bytes from file descriptor fd, starting at offset bytes from | ||
| the beginning of the file. The file offset remains unchanged. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait... is this description correct? it seems to be copied from two other functions? |
||
| [clinic start generated code]*/ | ||
|
|
||
| static Py_ssize_t | ||
| os_preadv_impl(PyObject *module, int fd, PyObject *buffers, Py_off_t offset, | ||
| int flags) | ||
| /*[clinic end generated code: output=26fc9c6e58e7ada5 input=5b07b7e2d1825627]*/ | ||
| { | ||
| Py_ssize_t cnt, n; | ||
| int async_err = 0; | ||
| struct iovec *iov; | ||
| Py_buffer *buf; | ||
|
|
||
| if (!PySequence_Check(buffers)) { | ||
| PyErr_SetString(PyExc_TypeError, | ||
| "preadv2() arg 2 must be a sequence"); | ||
| return -1; | ||
| } | ||
|
|
||
| cnt = PySequence_Size(buffers); | ||
| if (cnt < 0) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please always add { ... } to if blocks: see PEP 7. Same comment for the whole PR. |
||
| return -1; | ||
|
|
||
| if (iov_setup(&iov, &buf, buffers, cnt, PyBUF_WRITABLE) < 0) | ||
| return -1; | ||
| #ifdef HAVE_PREADV2 | ||
| do { | ||
| Py_BEGIN_ALLOW_THREADS | ||
| n = preadv2(fd, iov, cnt, offset, flags); | ||
| Py_END_ALLOW_THREADS | ||
| } while (n < 0 && errno == EINTR && !(async_err = PyErr_CheckSignals())); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you know if it's safe to call preadv2() again if it fals with EINTR?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not completely sure. I assumed that it is the case because is implemented internally using
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't worry. It's not really our issue for make sure that preadv()/preadv2() can be called again with same arguments on EINTR. The kernel and libc must make that doable and safe.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't that how literally every syscall except |
||
| #else | ||
| if(flags != 0){ | ||
| PyErr_SetString(PyExc_OSError, "preadv2() not available in this system"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PyExc_NotImplementedError is preferred on such case: maybe reuse argument_unavailable_error()? |
||
| iov_cleanup(iov, buf, cnt); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please this if(flags) before iov_setup() to not have to cleanup iov? Use #ifndef HAVE_PREADV2. |
||
| return -1; | ||
| } | ||
| do { | ||
| Py_BEGIN_ALLOW_THREADS | ||
| n = preadv(fd, iov, cnt, offset); | ||
| Py_END_ALLOW_THREADS | ||
| } while (n < 0 && errno == EINTR && !(async_err = PyErr_CheckSignals())); | ||
| #endif | ||
|
|
||
| iov_cleanup(iov, buf, cnt); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we exit early (e.g. due to
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have corrected it in 4ddbca9. I could not use easily a goto label to have a clean exit section due to the check for async error in the signal handler so I just clean before exiting if |
||
| if (n < 0) { | ||
| if (!async_err) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PEP 7 coding style: add { ... } |
||
| posix_error(); | ||
| return -1; | ||
| } | ||
|
|
||
| return n; | ||
| } | ||
| #endif /* HAVE_PREADV */ | ||
|
|
||
|
|
||
| /*[clinic input] | ||
| os.write -> Py_ssize_t | ||
|
|
@@ -12506,6 +12574,7 @@ static PyMethodDef posix_methods[] = { | |
| OS_READ_METHODDEF | ||
| OS_READV_METHODDEF | ||
| OS_PREAD_METHODDEF | ||
| OS_PREADV_METHODDEF | ||
| OS_WRITE_METHODDEF | ||
| OS_WRITEV_METHODDEF | ||
| OS_PWRITE_METHODDEF | ||
|
|
@@ -12953,6 +13022,19 @@ all_ins(PyObject *m) | |
| if (PyModule_AddIntMacro(m, F_TEST)) return -1; | ||
| #endif | ||
|
|
||
| #ifdef RWF_DSYNC | ||
| if (PyModule_AddIntConstant(m, "RWF_DSYNC", RWF_DSYNC)) return -1; | ||
| #endif | ||
| #ifdef RWF_HIPRI | ||
| if (PyModule_AddIntConstant(m, "RWF_HIPRI", RWF_HIPRI)) return -1; | ||
| #endif | ||
| #ifdef RWF_SYNC | ||
| if (PyModule_AddIntConstant(m, "RWF_SYNC", RWF_SYNC)) return -1; | ||
| #endif | ||
| #ifdef RWF_NOWAIT | ||
| if (PyModule_AddIntConstant(m, "RWF_NOWAIT", RWF_NOWAIT)) return -1; | ||
| #endif | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These constants were added at different times, so they should be checked for individually, like: #ifdef RWF_DSYNC
if (PyModule_AddIntConstant(m, "RWF_DSYNC", RWF_DSYNC)) return -1;
#endif
#ifdef RWF_HIPRI
if (PyModule_AddIntConstant(m, "RWF_HIPRI", RWF_HIPRI)) return -1;
#endif
...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also EOPNOTSUPP flag might be important to add?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's already available as |
||
|
|
||
| #ifdef HAVE_SPAWNV | ||
| if (PyModule_AddIntConstant(m, "P_WAIT", _P_WAIT)) return -1; | ||
| if (PyModule_AddIntConstant(m, "P_NOWAIT", _P_NOWAIT)) return -1; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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 don't know the answer to this, but should it be
flags=Nonewith no spaces?Actually, I just realized this is implemented in C. I can't really comment on that code, but you have it as flags=0 in the C module. Is None and 0 the same thing?
Uh oh!
There was an error while loading. Please reload this page.
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.
You are right. The
Nonewas because a previous declaration before switching to the default suggested by @njsmith. It should beflags=0. Corrected in c7b7285.