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.

Sign up for GitHub

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

Jump to bottom

bpo-31368: Expose preadv (preadv2) and pwritev (pwritev2) in the os module #5239

Merged
merged 14 commits into from
Jan 27, 2018
Merged

bpo-31368: Expose preadv (preadv2) and pwritev (pwritev2) in the os module #5239

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ea21128
Expose preadv2 in os module
pablogsal Jan 19, 2018
4570976
Add News entry
pablogsal Jan 19, 2018
21a3004
Add correct guards for the presence of preadv2
pablogsal Jan 19, 2018
8a2a2a3
Implement check for individual constants
pablogsal Jan 19, 2018
c0c53dc
Implemented preadv and join implementation to preadv2
pablogsal Jan 19, 2018
4ddbca9
Clean iov on error
pablogsal Jan 20, 2018
cac62ac
Add documentation in the os module
pablogsal Jan 22, 2018
c7b7285
Add information about the rest of the parameters
pablogsal Jan 22, 2018
58910d0
Expose pwritev and pwritev2 in os module
pablogsal Jan 23, 2018
f7cdf22
Fix style issues and Docs
pablogsal Jan 23, 2018
47f65b5
Correct style, update docs, fix misused API
pablogsal Jan 23, 2018
87c6970
Improve tests, clean names and correct style
pablogsal Jan 26, 2018
cb74d38
Correct documentation
pablogsal Jan 26, 2018
5563b8d
Correct typos, docstring and improve Docs
pablogsal Jan 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Clean iov on error
  • Loading branch information
@pablogsal
pablogsal committed Jan 20, 2018
commit 4ddbca92005c9a3876cc54685083a4363fb2b020
9 changes: 5 additions & 4 deletions Modules/posixmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8201,23 +8201,24 @@ os_preadv_impl(PyObject *module, int fd, PyObject *buffers, Py_off_t offset,

if (iov_setup(&iov, &buf, buffers, cnt, PyBUF_WRITABLE) < 0)
return -1;
#ifdef HAVE_PREADV2
#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()));
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

@pablogsal pablogsal Jan 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The 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 preadv and pread and those aresafe to retry but I cannot point to some place that definitely corroborates this. What should we do?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that how literally every syscall except close defines EINTR? Also preadv2 doesn't even have any observable side-effects, it seems like pwritev2 is the one you should worry about if you're going to worry :-). (But if pwritev2 manages to only write some of the bytes and then gets interrupted by a signal, then it should report a successful partial write, not EINTR, the same way write etc. work.)

#else
#else
if(flags != 0){
PyErr_SetString(PyExc_OSError, "preadv2() not available in this system");
Copy link
Member

Choose a reason for hiding this comment

The 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);
Copy link
Member

Choose a reason for hiding this comment

The 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
#endif

iov_cleanup(iov, buf, cnt);
Copy link
Contributor

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?

Copy link
Member Author

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.

if (n < 0) {
Expand All @@ -8228,7 +8229,7 @@ os_preadv_impl(PyObject *module, int fd, PyObject *buffers, Py_off_t offset,

return n;
}
#endif /* HAVE_PREADV2 */
#endif /* HAVE_PREADV */


/*[clinic input]
Expand Down