Skip to content

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.

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

Merged
vstinner merged 14 commits into from
Jan 27, 2018
Merged

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

Show file tree
Hide file tree
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
Implemented preadv and join implementation to preadv2
  • Loading branch information
@pablogsal
pablogsal committed Jan 20, 2018
commit c0c53dc91daabaa8d722607dd49c1b713f40ce64
17 changes: 14 additions & 3 deletions Lib/test/test_posix.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,18 +272,29 @@ def test_pread(self):
finally:
os.close(fd)

@unittest.skipUnless(hasattr(posix, 'preadv2'), "test needs posix.preadv2()")
def test_preadv2(self):
@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')

@vstinner vstinner Jan 26, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@vstinner vstinner Jan 26, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.preadv2(fd, buf, 0, os.RWF_SYNC), 10)
self.assertEqual(posix.preadv(fd, buf, 0), 10)
self.assertEqual([b'test1', b'tt2', b't3'], [bytes(i) for i in buf])

@vstinner vstinner Jan 26, 2018

Copy link
Copy Markdown
Member

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 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):
Expand Down
32 changes: 16 additions & 16 deletions Modules/clinic/posixmodule.c.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3705,39 +3705,39 @@ os_pread(PyObject *module, PyObject *const *args, Py_ssize_t nargs)

#endif /* defined(HAVE_PREAD) */

#if defined(HAVE_PREADV2)
#if (defined(HAVE_PREADV) || defined (HAVE_PREADV2))

PyDoc_STRVAR(os_preadv2__doc__,
"preadv2($module, fd, buffers, offset, flags, /)\n"
PyDoc_STRVAR(os_preadv__doc__,
"preadv($module, fd, buffers, offset, flags=0, /)\n"
"--\n"
"\n"
"Read a number of bytes from a file descriptor starting at a particular offset.\n"
"\n"
"Read length bytes from file descriptor fd, starting at offset bytes from\n"
"the beginning of the file. The file offset remains unchanged.");

#define OS_PREADV2_METHODDEF \
{"preadv2", (PyCFunction)os_preadv2, METH_FASTCALL, os_preadv2__doc__},
#define OS_PREADV_METHODDEF \
{"preadv", (PyCFunction)os_preadv, METH_FASTCALL, os_preadv__doc__},

static Py_ssize_t
os_preadv2_impl(PyObject *module, int fd, PyObject *buffers, Py_off_t offset,
int flags);
os_preadv_impl(PyObject *module, int fd, PyObject *buffers, Py_off_t offset,
int flags);

static PyObject *
os_preadv2(PyObject *module, PyObject *const *args, Py_ssize_t nargs)
os_preadv(PyObject *module, PyObject *const *args, Py_ssize_t nargs)
{
PyObject *return_value = NULL;
int fd;
PyObject *buffers;
Py_off_t offset;
int flags;
int flags = 0;
Py_ssize_t _return_value;

if (!_PyArg_ParseStack(args, nargs, "iOO&i:preadv2",
if (!_PyArg_ParseStack(args, nargs, "iOO&|i:preadv",
&fd, &buffers, Py_off_t_converter, &offset, &flags)) {
goto exit;
}
_return_value = os_preadv2_impl(module, fd, buffers, offset, flags);
_return_value = os_preadv_impl(module, fd, buffers, offset, flags);
if ((_return_value == -1) && PyErr_Occurred()) {
goto exit;
}
Expand All @@ -3747,7 +3747,7 @@ os_preadv2(PyObject *module, PyObject *const *args, Py_ssize_t nargs)
return return_value;
}

#endif /* defined(HAVE_PREADV2) */
#endif /* (defined(HAVE_PREADV) || defined (HAVE_PREADV2)) */

PyDoc_STRVAR(os_write__doc__,
"write($module, fd, data, /)\n"
Expand Down Expand Up @@ -6283,9 +6283,9 @@ os_getrandom(PyObject *module, PyObject *const *args, Py_ssize_t nargs, PyObject
#define OS_PREAD_METHODDEF
#endif /* !defined(OS_PREAD_METHODDEF) */

#ifndef OS_PREADV2_METHODDEF
#define OS_PREADV2_METHODDEF
#endif /* !defined(OS_PREADV2_METHODDEF) */
#ifndef OS_PREADV_METHODDEF
#define OS_PREADV_METHODDEF
#endif /* !defined(OS_PREADV_METHODDEF) */

#ifndef OS_PIPE_METHODDEF
#define OS_PIPE_METHODDEF
Expand Down Expand Up @@ -6458,4 +6458,4 @@ os_getrandom(PyObject *module, PyObject *const *args, Py_ssize_t nargs, PyObject
#ifndef OS_GETRANDOM_METHODDEF
#define OS_GETRANDOM_METHODDEF
#endif /* !defined(OS_GETRANDOM_METHODDEF) */
/*[clinic end generated code: output=efbe9448c1bba1a2 input=a9049054013a1b77]*/
/*[clinic end generated code: output=52cbdec1822e34f9 input=a9049054013a1b77]*/
31 changes: 20 additions & 11 deletions Modules/posixmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8163,14 +8163,14 @@ os_pread_impl(PyObject *module, int fd, int length, Py_off_t offset)
}
#endif /* HAVE_PREAD */

#ifdef HAVE_PREADV2
#if defined(HAVE_PREADV) || defined (HAVE_PREADV2)
/*[clinic input]
os.preadv2 -> Py_ssize_t
os.preadv -> Py_ssize_t

fd: int
buffers: object
offset: Py_off_t
flags: int
flags: int = 0
/

Read a number of bytes from a file descriptor starting at a particular offset.
Expand All @@ -8180,9 +8180,9 @@ the beginning of the file. The file offset remains unchanged.
[clinic start generated code]*/

static Py_ssize_t
os_preadv2_impl(PyObject *module, int fd, PyObject *buffers, Py_off_t offset,
int flags)
/*[clinic end generated code: output=fd040c20cf504631 input=f4a633d90005eb65]*/
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;
Expand All @@ -8201,12 +8201,23 @@ os_preadv2_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
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()));

@vstinner vstinner Jan 23, 2018

Copy link
Copy Markdown
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?

@pablogsal pablogsal Jan 23, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

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?

@vstinner vstinner Jan 23, 2018

Copy link
Copy Markdown
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.

@njsmith njsmith Jan 23, 2018

Copy link
Copy Markdown
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
if(flags != 0){
PyErr_SetString(PyExc_OSError, "preadv2() not available in this system");

@vstinner vstinner Jan 23, 2018

Copy link
Copy Markdown
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()?

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);

@njsmith njsmith Jan 20, 2018

Copy link
Copy Markdown
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?

@pablogsal pablogsal Jan 20, 2018

Copy link
Copy Markdown
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 @@ -8217,9 +8228,7 @@ os_preadv2_impl(PyObject *module, int fd, PyObject *buffers, Py_off_t offset,

return n;
}


#endif /* HAVE PREADV2 */
#endif /* HAVE_PREADV2 */


/*[clinic input]
Expand Down Expand Up @@ -12564,7 +12573,7 @@ static PyMethodDef posix_methods[] = {
OS_READ_METHODDEF
OS_READV_METHODDEF
OS_PREAD_METHODDEF
OS_PREADV2_METHODDEF
OS_PREADV_METHODDEF
OS_WRITE_METHODDEF
OS_WRITEV_METHODDEF
OS_PWRITE_METHODDEF
Expand Down
80 changes: 6 additions & 74 deletions aclocal.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

m4_ifndef([AC_CONFIG_MACRO_DIRS], [m4_defun([_AM_CONFIG_MACRO_DIRS], [])m4_defun([AC_CONFIG_MACRO_DIRS], [_AM_CONFIG_MACRO_DIRS($@)])])
# pkg.m4 - Macros to locate and utilise pkg-config. -*- Autoconf -*-
# serial 11 (pkg-config-0.29.1)
# serial 12 (pkg-config-0.29.2)

dnl Copyright © 2004 Scott James Remnant <scott@netsplit.com>.
dnl Copyright © 2012-2015 Dan Nicholson <dbn.lists@gmail.com>
Expand Down Expand Up @@ -55,7 +55,7 @@ dnl
dnl See the "Since" comment for each macro you use to see what version
dnl of the macros you require.
m4_defun([PKG_PREREQ],
[m4_define([PKG_MACROS_VERSION], [0.29.1])
[m4_define([PKG_MACROS_VERSION], [0.29.2])
m4_if(m4_version_compare(PKG_MACROS_VERSION, [$1]), -1,
[m4_fatal([pkg.m4 version $1 or higher is required but ]PKG_MACROS_VERSION[ found])])
])dnl PKG_PREREQ
Expand Down Expand Up @@ -156,7 +156,7 @@ AC_ARG_VAR([$1][_CFLAGS], [C compiler flags for $1, overriding pkg-config])dnl
AC_ARG_VAR([$1][_LIBS], [linker flags for $1, overriding pkg-config])dnl

pkg_failed=no
AC_MSG_CHECKING([for $1])
AC_MSG_CHECKING([for $2])

_PKG_CONFIG([$1][_CFLAGS], [cflags], [$2])
_PKG_CONFIG([$1][_LIBS], [libs], [$2])
Expand All @@ -166,11 +166,11 @@ and $1[]_LIBS to avoid the need to call pkg-config.
See the pkg-config man page for more details.])

if test $pkg_failed = yes; then
AC_MSG_RESULT([no])
AC_MSG_RESULT([no])
_PKG_SHORT_ERRORS_SUPPORTED
if test $_pkg_short_errors_supported = yes; then
$1[]_PKG_ERRORS=`$PKG_CONFIG --short-errors --print-errors --cflags --libs "$2" 2>&1`
else
else
$1[]_PKG_ERRORS=`$PKG_CONFIG --print-errors --cflags --libs "$2" 2>&1`
fi
# Put the nasty error message in config.log where it belongs
Expand All @@ -187,7 +187,7 @@ installed software in a non-standard prefix.
_PKG_TEXT])[]dnl
])
elif test $pkg_failed = untried; then
AC_MSG_RESULT([no])
AC_MSG_RESULT([no])
m4_default([$4], [AC_MSG_FAILURE(
[The pkg-config script could not be found or is too old. Make sure it
is in your PATH or set the PKG_CONFIG environment variable to the full
Expand Down Expand Up @@ -288,72 +288,4 @@ AS_VAR_COPY([$1], [pkg_cv_][$1])
AS_VAR_IF([$1], [""], [$5], [$4])dnl
])dnl PKG_CHECK_VAR

dnl PKG_WITH_MODULES(VARIABLE-PREFIX, MODULES,
dnl [ACTION-IF-FOUND],[ACTION-IF-NOT-FOUND],
dnl [DESCRIPTION], [DEFAULT])
dnl ------------------------------------------
dnl
dnl Prepare a "--with-" configure option using the lowercase
dnl [VARIABLE-PREFIX] name, merging the behaviour of AC_ARG_WITH and
dnl PKG_CHECK_MODULES in a single macro.
AC_DEFUN([PKG_WITH_MODULES],
[
m4_pushdef([with_arg], m4_tolower([$1]))

m4_pushdef([description],
[m4_default([$5], [build with ]with_arg[ support])])

m4_pushdef([def_arg], [m4_default([$6], [auto])])
m4_pushdef([def_action_if_found], [AS_TR_SH([with_]with_arg)=yes])
m4_pushdef([def_action_if_not_found], [AS_TR_SH([with_]with_arg)=no])

m4_case(def_arg,
[yes],[m4_pushdef([with_without], [--without-]with_arg)],
[m4_pushdef([with_without],[--with-]with_arg)])

AC_ARG_WITH(with_arg,
AS_HELP_STRING(with_without, description[ @<:@default=]def_arg[@:>@]),,
[AS_TR_SH([with_]with_arg)=def_arg])

AS_CASE([$AS_TR_SH([with_]with_arg)],
[yes],[PKG_CHECK_MODULES([$1],[$2],$3,$4)],
[auto],[PKG_CHECK_MODULES([$1],[$2],
[m4_n([def_action_if_found]) $3],
[m4_n([def_action_if_not_found]) $4])])

m4_popdef([with_arg])
m4_popdef([description])
m4_popdef([def_arg])

])dnl PKG_WITH_MODULES

dnl PKG_HAVE_WITH_MODULES(VARIABLE-PREFIX, MODULES,
dnl [DESCRIPTION], [DEFAULT])
dnl -----------------------------------------------
dnl
dnl Convenience macro to trigger AM_CONDITIONAL after PKG_WITH_MODULES
dnl check._[VARIABLE-PREFIX] is exported as make variable.
AC_DEFUN([PKG_HAVE_WITH_MODULES],
[
PKG_WITH_MODULES([$1],[$2],,,[$3],[$4])

AM_CONDITIONAL([HAVE_][$1],
[test "$AS_TR_SH([with_]m4_tolower([$1]))" = "yes"])
])dnl PKG_HAVE_WITH_MODULES

dnl PKG_HAVE_DEFINE_WITH_MODULES(VARIABLE-PREFIX, MODULES,
dnl [DESCRIPTION], [DEFAULT])
dnl ------------------------------------------------------
dnl
dnl Convenience macro to run AM_CONDITIONAL and AC_DEFINE after
dnl PKG_WITH_MODULES check. HAVE_[VARIABLE-PREFIX] is exported as make
dnl and preprocessor variable.
AC_DEFUN([PKG_HAVE_DEFINE_WITH_MODULES],
[
PKG_HAVE_WITH_MODULES([$1],[$2],[$3],[$4])

AS_IF([test "$AS_TR_SH([with_]m4_tolower([$1]))" = "yes"],
[AC_DEFINE([HAVE_][$1], 1, [Enable ]m4_tolower([$1])[ support])])
])dnl PKG_HAVE_DEFINE_WITH_MODULES

m4_include([m4/ax_check_openssl.m4])
2 changes: 1 addition & 1 deletion configure
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11201,7 +11201,7 @@ for ac_func in alarm accept4 setitimer getitimer bind_textdomain_codeset chown \
initgroups kill killpg lchmod lchown lockf linkat lstat lutimes mmap \
memrchr mbrtowc mkdirat mkfifo \
mkfifoat mknod mknodat mktime mremap nice openat pathconf pause pipe2 plock poll \
posix_fallocate posix_fadvise pread \
posix_fallocate posix_fadvise pread preadv preadv2 \
pthread_init pthread_kill putenv pwrite readlink readlinkat readv realpath renameat \
sem_open sem_timedwait sem_getvalue sem_unlink sendfile setegid seteuid \
setgid sethostname \
Expand Down
2 changes: 1 addition & 1 deletion configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3435,7 +3435,7 @@ AC_CHECK_FUNCS(alarm accept4 setitimer getitimer bind_textdomain_codeset chown \
initgroups kill killpg lchmod lchown lockf linkat lstat lutimes mmap \
memrchr mbrtowc mkdirat mkfifo \
mkfifoat mknod mknodat mktime mremap nice openat pathconf pause pipe2 plock poll \
posix_fallocate posix_fadvise pread \
posix_fallocate posix_fadvise pread preadv preadv2 \
pthread_init pthread_kill putenv pwrite readlink readlinkat readv realpath renameat \
sem_open sem_timedwait sem_getvalue sem_unlink sendfile setegid seteuid \
setgid sethostname \
Expand Down
6 changes: 6 additions & 0 deletions pyconfig.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -707,6 +707,12 @@
/* Define to 1 if you have the `pread' function. */
#undef HAVE_PREAD

/* Define to 1 if you have the `preadv' function. */
#undef HAVE_PREADV

/* Define to 1 if you have the `preadv2' function. */
#undef HAVE_PREADV2

/* Define if you have the 'prlimit' functions. */
#undef HAVE_PRLIMIT

Expand Down