-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
bpo-31368: Expose preadv (preadv2) and pwritev (pwritev2) in the os module #5239
Conversation
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.
Some comments below. Also: should we expose pwritev2
(and maybe pwritev
) at the same time? In fact half the constants that this patch adds are only usable with pwritev2
.
Modules/posixmodule.c
Outdated
if (PyModule_AddIntConstant(m, "RWF_HIPRI", RWF_HIPRI)) return -1; | ||
if (PyModule_AddIntConstant(m, "RWF_SYNC", RWF_SYNC)) return -1; | ||
if (PyModule_AddIntConstant(m, "RWF_NOWAIT", RWF_NOWAIT)) return -1; | ||
#endif |
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.
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
...
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.
Also EOPNOTSUPP flag might be important to add?
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.
That's already available as errno.EOPNOTSUPP
.
Modules/clinic/posixmodule.c.h
Outdated
@@ -3705,6 +3705,50 @@ os_pread(PyObject *module, PyObject *const *args, Py_ssize_t nargs) | |||
|
|||
#endif /* defined(HAVE_PREAD) */ | |||
|
|||
#if defined(HAVE_PREADV2) |
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 use HAVE_PREADV2
, but I don't see anything to actually set it. Did you forget to update configure.ac
?
Modules/clinic/posixmodule.c.h
Outdated
#if defined(HAVE_PREADV2) | ||
|
||
PyDoc_STRVAR(os_preadv2__doc__, | ||
"preadv2($module, fd, buffers, offset, flags, /)\n" |
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.
API question: preadv2
only exists because regular preadv
was missing a flags
argument, and in C the only way to add an argument is to make a new function. In python we don't have that problem. So an alternative would be to make this os.preadv(fd, buffers, offset[, flags])
, and have it call either preadv
or preadv2
as appropriate. (preadv2(..., flags=0)
is equivalent to preadv
.)
I kind of like this better, since (a) exposing preadv
is probably a good idea, even on systems that don't have preadv2
, (b) it reduces clutter in os
. OTOH the same thing happened for pipe
and pipe2
, and it looks like we exposed those as two independent functions in os
, so ¯\(ツ)/¯
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.
https://www.gnu.org/software/libc/manual/html_mono/libc.html#index-preadv2 seems to support @njsmith statement.
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.
@njsmith Thanks for the comment! CPython only exposes pread
and readv
and currently, it does not expose preadv
. Is your suggestion to implement preadv2
as preadv
and call the former if flags are provided and the later (preadv
) in ther case?
I am asking this because initially I thought you were saying that preadv
was available and we should reuse the function
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.
Right, I'm doing the classic pull request reviewer move of trying to trick you into doing extra work while you're there :-). We don't currently expose preadv
, but it's obviously a useful thing, so I'm suggesting you add a preadv
wrapper, and then add preadv2
support to that. If we add preadv2
now alone, then we'll probably end up adding preadv
later, and thus have a messier api in the end.
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.
BTW, it's probably simpler to structure the code like (forgive my mixed Python/c pseudocode):
#if defined(HAVE_PREADV) || defined (HAVE_PREADV2)
def preadv(fd, offset, buffers, flags=0):
#ifdef HAVE_PREADV2
return preadv2(...)
#else
if flags != 0: raise ...
return preadv(...)
#endif
Since preadv2
has a superset of the functionality of preadv
, when it's available you can use it for all flags values, including 0.
Wow what a great start by @pablogsal thank you for creating this patch :) |
@njsmith I am working on fixing your comments. I would prefer to implement |
a8ec27f
to
52c0bfb
Compare
I'm not a core dev so I can't give a definitive answer, but it seems fine to me. |
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.
Otherwise LGTM
Modules/posixmodule.c
Outdated
|
||
if (iov_setup(&iov, &buf, buffers, cnt, PyBUF_WRITABLE) < 0) | ||
return -1; | ||
#ifdef HAVE_PREADV2 |
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.
Style nit: I'm not sure why, but the convention in C is that preprocessor directives are never indented, so like:
if (iov_setup(&iov, &buf, buffers, cnt, PyBUF_WRITABLE) < 0)
return -1;
#ifdef HAVE_PREADV2
do {
(and similarly elsewhere).
Modules/posixmodule.c
Outdated
} while (n < 0 && errno == EINTR && !(async_err = PyErr_CheckSignals())); | ||
#endif | ||
|
||
iov_cleanup(iov, buf, cnt); |
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.
If we exit early (e.g. due to flags != 0
), then this cleanup function doesn't get called. Is that OK?
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 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 flags != 0
.
Does this need to be added to the |
In my opinion, yes. It needs to be for os.preadv() with a note mentioned it works for preadv2 as well. # Note-1 (function)os.preadv(fd, buffers, offset=0, flags=0) # Note-2 (flags)os.RWF_DSYNC You can read more about preadv & preadv2 here: https://www.gnu.org/software/libc/manual/html_node/I_002fO-Primitives.html |
@csabella Thanks for the comment! I was waiting to check that everyone was happy with the interface but I suppose there isn't much freedom given the current implementations of |
@YoSTEALTH Thanks for the links and comments! |
@pablogsal there are few places where preadv2 is mentioned including title "Expose preadv2 in the os module" shouldn't it be "Expose preadv (preadv2) in the os module" also under NEWS. Simple typo fixes, might lead to confusion. Also what about "RWF_DSYNC" & "RWF_SYNC" flag reference (docs) are they covered somewhere else? Thanks |
Doc/library/os.rst
Outdated
|
||
Combines the functionality of :func:`os.readv` and :func:`os.pread`. It | ||
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 |
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 put asterisks around flags below and I think the first offset should have the name because this is referring to the argument.
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.
Corrected in c7b7285
Doc/library/os.rst
Outdated
@@ -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) |
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=None
with 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?
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 None
was because a previous declaration before switching to the default suggested by @njsmith. It should be flags=0
. Corrected in c7b7285.
Doc/library/os.rst
Outdated
@@ -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 |
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 noticed you didn't duplicate the definition of fd
and buffers
from readv. I think most of docs explain all the parms, even if they are duplicated from another section.
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 has hessitant to duplicate because readv
is the function described inmediately before this one but I suppose that consistency makes sense. Thanks.
Corrected in c7b7285
Doc/library/os.rst
Outdated
|
||
The flags argument contains a bitwise OR of zero or more of the following | ||
flags: | ||
|
||
Availability: Unix (version 2.6.30), FreeBSD (version 6.0 and newer), |
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.
Unix: do you mean Linux?
"FreeBSD (version 6.0 and newer)" may be written "FreeBSD 6 and newer".
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.
Done in 47f65b5
Doc/library/os.rst
Outdated
:func:`~os.pwritev` will call *pwritev2* system call if available, which modifies | ||
the behavior on a per-call basis based on the value of the *flags* argument. If | ||
the *flags* argument is provided and different than zero, and *pwritev2* is not | ||
available, it will raise a :exc:`NotImplementedError`. | ||
|
||
The flags argument contains a bitwise OR of zero or more of the following | ||
flags: |
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.
of which flags? write an explicit list here.
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.
Done in 47f65b5
Modules/posixmodule.c
Outdated
|
||
if (iov_setup(&iov, &buf, buffers, cnt, PyBUF_WRITABLE) < 0) | ||
#ifndef HAVE_PREADV2 | ||
if(flags != 0){ |
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.
nitpick: add spaces (see PEP 7): "if (flags != 0) {".
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.
Done in 47f65b5
Modules/posixmodule.c
Outdated
if (iov_setup(&iov, &buf, buffers, cnt, PyBUF_SIMPLE) < 0) | ||
#ifndef HAVE_PWRITEV2 | ||
if(flags != 0){ | ||
argument_unavailable_error("pwritev2", "pwritev2() not available in this system"); |
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 misused the API. It should be something like: argument_unavailable_error("pwritev", "flags");
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.
Sorry, my fault. Is 2AM here and I clearly should be sleeping. 😪
Doc/library/os.rst
Outdated
:func:`~os.pwritev` will call *pwritev2* system call if available, which modifies | ||
the behavior on a per-call basis based on the value of the *flags* argument. If | ||
the *flags* argument is provided and different than zero, and *pwritev2* is not | ||
available, it will raise a :exc:`NotImplementedError`. |
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 would move this paragraph to the end, before Availability, and simplify it to one sentence, something like: "The :c:func:pwritev2
function is required to pass flags."
I would prefer to not mention NotImplementedError, it's not documlented for other functions.
Maybe mention in which Linux version is it was added (4.14?).
I have made the requested changes; please review again |
Thanks for making the requested changes! @vstinner: please review the changes made to this pull request. |
Doc/library/os.rst
Outdated
.. function:: pwritev(fd, buffers, offset, flags=0) | ||
|
||
Combines the functionality of :func:`os.writev` and :func:`os.pwrite`. It | ||
writes the contents of *buffers* to file descriptor *fd*. *buffers* must 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.
Hum, offset is not documented. You may just write: "writes the contents of buffers to file descriptor fd at offset offset."
Doc/library/os.rst
Outdated
:func:`~os.pwritev2` function is required to pass flags. | ||
|
||
*pwritev2* and *flags* different than zero are available since Linux version | ||
4.7 and newer. |
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.
nitpick: just "since Linux 4.7" (no "and newer") or "available on Linux 4.7 and newer".
Doc/library/os.rst
Outdated
*pwritev2* and *flags* different than zero are available since Linux version | ||
4.7 and newer. | ||
|
||
The :func:`~os.pwritev2` function is required to pass flags. |
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.
nitpick: I would move this sentence before describing on which Linux platforms version it is available.
Doc/library/os.rst
Outdated
data into each buffer until it is full and then move on to the next buffer in | ||
the sequence to hold the rest of the data. Its fourth argument, *offset*, | ||
specifies the file offset at which the input operation is to be performed. | ||
:func:`~os.preadv` return the total number of bytes read (which may be less than |
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.
may be => can be
Doc/library/os.rst
Outdated
|
||
:func:`~os.preadv` will call *preadv2* system call if available, which modifies | ||
the behavior on a per-call basis based on the value of the *flags* argument. The | ||
:func:`~os.preadv2` function is required to pass flags. |
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.
os.preadv2? I guess that you mean the C function preadv2, no? os.preadv2 doesn't exist.
I suggest "The C :c:func:preadv2
function is required ...".
Lib/test/test_posix.py
Outdated
finally: | ||
os.close(fd) | ||
|
||
@unittest.skipUnless(hasattr(posix, 'RWF_HIPRI'), "test needs posix.preadv2()") |
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.
posix.preadv2 doesn't exist. I suggest "test needs posix.RWF_HIPRI".
I also suggest to rename the test to test_preadv_flags() to be closer to the Python API than the C internals.
Lib/test/test_posix.py
Outdated
def test_pwritev(self): | ||
fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT) | ||
try: | ||
n = os.pwritev(fd, [b'test1tt2t3'],2) |
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 would prefer to test writing multiple buffers than a single buffer. Maybe write [b'test1', b'tt2', b't3']?
I would prefer to write explicitly the first 2 bytes, but then seek back at offset 0 before calling os.pwritev(). I don't want to test in Python the behaviour of pwritev() for non-offset offset with an empty file. Python should test its binding, not the exact behaviour of the OS.
For example:
write(b'XX')
seek(0)
pwrite(..., offset=2)
assert read() == b'XX...'
Lib/test/test_posix.py
Outdated
|
||
|
||
@unittest.skipUnless(hasattr(posix, 'os.RWF_SYNC'), "test needs posix.pwritev2()") | ||
def test_pwritev2(self): |
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.
ditto, rename to test_pwritev_flags() and change the skip message to not mention a function that doesn't exist.
Modules/posixmodule.c
Outdated
|
||
iov_cleanup(iov, buf, cnt); | ||
if (n < 0) { | ||
if (!async_err) |
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.
PEP 7 coding style: add { ... }
Modules/posixmodule.c
Outdated
|
||
iov_cleanup(iov, buf, cnt); | ||
if (result < 0) { | ||
if (!async_err) |
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.
ditto: add { ... }
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I have made the requested changes; please review again |
Thanks for making the requested changes! @vstinner: please review the changes made to this pull request. |
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.
Another review.
Most comments are on the documentation and coding style, so IMHO we are now very close to a ready PR ;-)
(sysconf() value SC_IOV_MAX) on the number of buffers that can be used. | ||
:func:`~os.pwritev` writes the contents of each object to the file descriptor | ||
and returns the total number of bytes written. | ||
|
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.
a single empty line is enough.
Doc/library/os.rst
Outdated
|
||
:func:`~os.pwritev` will call *pwritev2* system call if available, which modifies | ||
the behavior on a per-call basis based on the value of the *flags* argument. The | ||
:func:`~os.pwritev2` function is required to pass flags. |
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.
No, there is no :func:~os.pwritev2
function, stop creating links to it.
I suggested "The C function :func:pwritev2
", or you can just write pwrite2()
.
Doc/library/os.rst
Outdated
- RWF_SYNC | ||
|
||
:func:`~os.pwritev` will call *pwritev2* system call if available, which modifies | ||
the behavior on a per-call basis based on the value of the *flags* argument. The |
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'm not sure that ", which modifies the behavior on a per-call basis based on the value of the flags argument." is needed. Nobody expects a flag passed to a function to have an effect on following calls. And the usage of pwrite2() doesn't mean that the caller must pass a non-zero flags. This sentence is confusing.
Doc/library/os.rst
Outdated
the behavior on a per-call basis based on the value of the *flags* argument. The | ||
:func:`~os.pwritev2` function is required to pass flags. | ||
|
||
The :func:`~os.pwritev2` function is required to pass flags. |
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.
ditto, don't write ":func:~os.pwritev2
function" but "C :c:func:pwritev2
function" or just pwrite2()
.
Note the "c" prefix before "func".
Doc/library/os.rst
Outdated
The :func:`~os.pwritev2` function is required to pass flags. | ||
|
||
*pwritev2* and *flags* different than zero are available since Linux version | ||
4.7. |
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.
"The :func:~os.pwritev2
function is required to pass flags. pwritev2 and flags different than zero are available since Linux version 4.7."
I'm not sure that the Python has to mention pwritev2(). It's confusing and useless. I don't think that the documentation of other os function lists each internal called C functions.
I suggest something simpler like: "Using non-zero flags requires Linux 4.7 or newer."
Lib/test/test_posix.py
Outdated
finally: | ||
os.close(fd) | ||
|
||
|
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.
ditto: remove this empty line
Lib/test/test_posix.py
Outdated
def test_pwritev(self): | ||
fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT) | ||
try: | ||
os.write(fd,b"xx") |
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.
PEP 8: add a space after ","
@@ -0,0 +1,2 @@ | |||
Expose preadv (preadv2) and pwritev (pwritev2) system calls in the os module. |
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 suggest: Add os.preadv() and os.pwritev() functions.
Again, I don't think that it's needed to mention the underlying C functions.
Modules/posixmodule.c
Outdated
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. |
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.
Wait... is this description correct? it seems to be copied from two other functions?
Modules/posixmodule.c
Outdated
flags: int = 0 | ||
/ | ||
|
||
Write bytes to a file descriptor starting at a particular offset. |
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.
Ditto: this docstring seems wrong.
I have addressed all the feedback. The only thing I am not very sure is the description of the flags as that description is now the only place that explicitly mentions I have removed all other mentions of the C calls, please check that those paragraphs are clear enough and nothing is missed. |
I have made the requested changes; please review again |
Thanks for making the requested changes! @vstinner: please review the changes made to this pull request. |
@vstinner: Please replace |
I'm not 100% satisfied by the documentation but I prefer to merge this PR right now to get it before 3.7beta1, feature freeze of Python 3.7. I may propose a different PR to enhance the doc. Thanks Pablo for this addition! |
Thank you Pablo Galindo for your amazingly hard work. Also Victor Stinner for your guidance, thanks you :) |
https://bugs.python.org/issue31368