[issue12196] add pipe2() to the os module
Charles-François Natali neolo...@free.fr added the comment: Hmm, thinking about it, I don't see any reason to make the flags argument optional. Here's a patch changing that (also, pipe2 is now declared as METH_O instead of METH_VARARGS). -- Added file: http://bugs.python.org/file22265/pipe2_arg.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Antoine Pitrou pit...@free.fr added the comment: Hmm, thinking about it, I don't see any reason to make the flags argument optional. Here's a patch changing that (also, pipe2 is now declared as METH_O instead of METH_VARARGS). Looks good to me. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Roundup Robot devnull@devnull added the comment: New changeset ba975c7c33d3 by Charles-François Natali in branch 'default': Issue #12196: Make os.pipe2() flags argument mandatory. http://hg.python.org/cpython/rev/ba975c7c33d3 -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Roundup Robot devnull@devnull added the comment: New changeset 277bbe6cae53 by Charles-François Natali in branch 'default': Issue #12196: Make test.support's requires_linux_version a decorator. http://hg.python.org/cpython/rev/277bbe6cae53 -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
STINNER Victor victor.stin...@haypocalc.com added the comment: support_linux_version.diff: cool, it's even better than the previous patch. You can commit it, except if you are motived for a last change: display the write also version in the SkipTest message (as it is done actually). I just added a very similar function for Mac OS X which is a decorator. Example: @support.requires_mac_ver(10, 5). requires_linux_version() should also be a decorator. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Roundup Robot devnull@devnull added the comment: New changeset 4124d1f75b93 by Charles-François Natali in branch 'default': Issue #12196: Add a note on os.pipe2() in the Whats' new in Python 3.3 http://hg.python.org/cpython/rev/4124d1f75b93 -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Charles-François Natali neolo...@free.fr added the comment:
requires_linux_version() should also be a decorator.
Patch attached.
--
Added file: http://bugs.python.org/file22219/support_linux_version.diff
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12196
___diff -r 22f077f82e74 Lib/test/support.py
--- a/Lib/test/support.py Wed Jun 01 20:44:40 2011 +0200
+++ b/Lib/test/support.py Wed Jun 01 23:26:57 2011 +0200
@@ -37,8 +37,8 @@
Error, TestFailed, ResourceDenied, import_module,
verbose, use_resources, max_memuse, record_original_stdout,
get_original_stdout, unload, unlink, rmtree, forget,
-is_resource_enabled, requires, linux_version, requires_mac_ver,
-find_unused_port, bind_port,
+is_resource_enabled, requires, requires_linux_version,
+requires_mac_ver, find_unused_port, bind_port,
IPV6_ENABLED, is_jython, TESTFN, HOST, SAVEDCWD, temp_cwd,
findfile, sortdict, check_syntax_error, open_urlresource,
check_warnings, CleanImport, EnvironmentVarGuard,
TransientResource,
@@ -292,13 +292,32 @@
msg = Use of the `%s' resource not enabled % resource
raise ResourceDenied(msg)
-def linux_version():
-try:
-# platform.release() is something like '2.6.33.7-desktop-2mnb'
-version_string = platform.release().split('-')[0]
-return tuple(map(int, version_string.split('.')))
-except ValueError:
-return 0, 0, 0
+def requires_linux_version(*min_version):
+Decorator raising SkipTest if the OS is Linux and the kernel version is
+less than min_version.
+
+For example, @requires_linux_version(2, 6, 35) raises SkipTest if the Linux
+kernel version is less than 2.6.35.
+
+def decorator(func):
+@functools.wraps(func)
+def wrapper(*args, **kw):
+if sys.platform.startswith('linux'):
+version_txt = platform.release().split('-', 1)[0]
+try:
+version = tuple(map(int, version_txt.split('.')))
+except ValueError:
+pass
+else:
+if version min_version:
+min_version_txt = '.'.join(map(str, min_version))
+raise unittest.SkipTest(
+Linux kernel %s or higher required, not %s
+% (min_version_txt, version_txt))
+return func(*args, **kw)
+wrapper.min_version = min_version
+return wrapper
+return decorator
def requires_mac_ver(*min_version):
Decorator raising SkipTest if the OS is Mac OS X and the OS X
diff -r 22f077f82e74 Lib/test/test_posix.py
--- a/Lib/test/test_posix.pyWed Jun 01 20:44:40 2011 +0200
+++ b/Lib/test/test_posix.pyWed Jun 01 23:26:57 2011 +0200
@@ -309,11 +309,8 @@
fp2.close()
@unittest.skipUnless(hasattr(os, 'O_CLOEXEC'), needs os.O_CLOEXEC)
+@support.requires_linux_version(2, 6, 23)
def test_oscloexec(self):
-version = support.linux_version()
-if sys.platform == 'linux2' and version (2, 6, 23):
-self.skipTest(Linux kernel 2.6.23 or higher required,
- not %s.%s.%s % version)
fd = os.open(support.TESTFN, os.O_RDONLY|os.O_CLOEXEC)
self.addCleanup(os.close, fd)
self.assertTrue(fcntl.fcntl(fd, fcntl.F_GETFD) fcntl.FD_CLOEXEC)
@@ -479,11 +476,8 @@
os.close(writer)
@unittest.skipUnless(hasattr(os, 'pipe2'), test needs os.pipe2())
+@support.requires_linux_version(2, 6, 27)
def test_pipe2(self):
-version = support.linux_version()
-if sys.platform == 'linux2' and version (2, 6, 27):
-self.skipTest(Linux kernel 2.6.27 or higher required,
- not %s.%s.%s % version)
self.assertRaises(TypeError, os.pipe2, 'DEADBEEF')
self.assertRaises(TypeError, os.pipe2, 0, 0)
diff -r 22f077f82e74 Lib/test/test_socket.py
--- a/Lib/test/test_socket.py Wed Jun 01 20:44:40 2011 +0200
+++ b/Lib/test/test_socket.py Wed Jun 01 23:26:57 2011 +0200
@@ -1023,11 +1023,8 @@
pass
if hasattr(socket, SOCK_NONBLOCK):
+@support.requires_linux_version(2, 6, 28)
def testInitNonBlocking(self):
-v = support.linux_version()
-if v (2, 6, 28):
-self.skipTest(Linux kernel 2.6.28 or higher required, not %s
- % ..join(map(str, v)))
# reinit server socket
self.serv.close()
self.serv = socket.socket(socket.AF_INET, socket.SOCK_STREAM |
@@ -2001,11 +1998,8 @@
SOCK_CLOEXEC not defined)
@unittest.skipUnless(fcntl, module fcntl not available)
class CloexecConstantTest(unittest.TestCase):
+@support.requires_linux_version(2, 6, 28)
def
[issue12196] add pipe2() to the os module
STINNER Victor victor.stin...@haypocalc.com added the comment: Your last support_linux_version.diff patch looks good. If you are motivated, this new function can also be added to test.support of Python 3.2 (test_socket.py has the original linux_version() function). -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Charles-François Natali neolo...@free.fr added the comment:
I like your patch, it removes many duplicate code :-)
Some comments:
Patch attached.
Doc/whatsnew/3.3.rst
Patch attached.
--
Added file: http://bugs.python.org/file22209/pipe2_whatsnew.diff
Added file: http://bugs.python.org/file22210/support_linux_version.diff
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12196
___diff -r 6fb2e5fb9082 Doc/whatsnew/3.3.rst
--- a/Doc/whatsnew/3.3.rst Tue May 31 12:15:42 2011 +0200
+++ b/Doc/whatsnew/3.3.rst Tue May 31 19:27:08 2011 +0200
@@ -106,6 +106,11 @@
os
--
+* The :mod:`os` module has a new :func:`~os.pipe2` function that makes it
+ possible to create a pipe with :data:`~os.O_CLOEXEC` or :data:`os.O_NONBLOCK`
+ flags set atomically. This is especially useful to avoid race conditions in
+ multi-threaded programs.
+
* The :mod:`os` module has a new :func:`~os.sendfile` function which provides
an efficent zero-copy way for copying data from one file (or socket)
descriptor to another. The phrase zero-copy refers to the fact that all of
diff -r 6fb2e5fb9082 Lib/test/support.py
--- a/Lib/test/support.py Tue May 31 12:15:42 2011 +0200
+++ b/Lib/test/support.py Tue May 31 19:20:57 2011 +0200
@@ -291,13 +291,24 @@
msg = Use of the `%s' resource not enabled % resource
raise ResourceDenied(msg)
-def linux_version():
+def requires_linux_version(*min_version):
+Raise SkipTest if the OS is Linux and the kernel version if less than
+min_version.
+
+For example, support.requires_linux_version(2, 6, 28) raises SkipTest if
the
+version is less than 2.6.28.
+
+if not sys.platform.startswith('linux'):
+return
try:
# platform.release() is something like '2.6.33.7-desktop-2mnb'
-version_string = platform.release().split('-')[0]
-return tuple(map(int, version_string.split('.')))
+version_string = platform.release().split('-', 1)[0]
+version = tuple(map(int, version_string.split('.')))
except ValueError:
-return 0, 0, 0
+return
+if version min_version:
+raise unittest.SkipTest(Linux kernel %s or higher required %
+..join(map(str, min_version)))
HOST = 'localhost'
diff -r 6fb2e5fb9082 Lib/test/test_posix.py
--- a/Lib/test/test_posix.pyTue May 31 12:15:42 2011 +0200
+++ b/Lib/test/test_posix.pyTue May 31 19:20:57 2011 +0200
@@ -310,10 +310,7 @@
@unittest.skipUnless(hasattr(os, 'O_CLOEXEC'), needs os.O_CLOEXEC)
def test_oscloexec(self):
-version = support.linux_version()
-if sys.platform == 'linux2' and version (2, 6, 23):
-self.skipTest(Linux kernel 2.6.23 or higher required,
- not %s.%s.%s % version)
+support.requires_linux_version(2, 6, 23)
fd = os.open(support.TESTFN, os.O_RDONLY|os.O_CLOEXEC)
self.addCleanup(os.close, fd)
self.assertTrue(fcntl.fcntl(fd, fcntl.F_GETFD) fcntl.FD_CLOEXEC)
@@ -480,10 +477,7 @@
@unittest.skipUnless(hasattr(os, 'pipe2'), test needs os.pipe2())
def test_pipe2(self):
-version = support.linux_version()
-if sys.platform == 'linux2' and version (2, 6, 27):
-self.skipTest(Linux kernel 2.6.27 or higher required,
- not %s.%s.%s % version)
+support.requires_linux_version(2, 6, 27)
self.assertRaises(TypeError, os.pipe2, 'DEADBEEF')
self.assertRaises(TypeError, os.pipe2, 0, 0)
diff -r 6fb2e5fb9082 Lib/test/test_socket.py
--- a/Lib/test/test_socket.py Tue May 31 12:15:42 2011 +0200
+++ b/Lib/test/test_socket.py Tue May 31 19:20:57 2011 +0200
@@ -1024,10 +1024,7 @@
if hasattr(socket, SOCK_NONBLOCK):
def testInitNonBlocking(self):
-v = support.linux_version()
-if v (2, 6, 28):
-self.skipTest(Linux kernel 2.6.28 or higher required, not %s
- % ..join(map(str, v)))
+support.requires_linux_version(2, 6, 28)
# reinit server socket
self.serv.close()
self.serv = socket.socket(socket.AF_INET, socket.SOCK_STREAM |
@@ -2002,10 +1999,7 @@
@unittest.skipUnless(fcntl, module fcntl not available)
class CloexecConstantTest(unittest.TestCase):
def test_SOCK_CLOEXEC(self):
-v = support.linux_version()
-if v (2, 6, 28):
-self.skipTest(Linux kernel 2.6.28 or higher required, not %s
- % ..join(map(str, v)))
+support.requires_linux_version(2, 6, 28)
with socket.socket(socket.AF_INET,
socket.SOCK_STREAM | socket.SOCK_CLOEXEC) as s:
self.assertTrue(s.type socket.SOCK_CLOEXEC)
@@ -2024,10 +2018,7 @@
self.assertEqual(s.gettimeout(), None)
def
[issue12196] add pipe2() to the os module
STINNER Victor victor.stin...@haypocalc.com added the comment: support_linux_version.diff: cool, it's even better than the previous patch. You can commit it, except if you are motived for a last change: display the write also version in the SkipTest message (as it is done actually). pipe2_whatsnew.diff: I don't have any special to say, except that you didn't use ~ for :data:`os.O_NONBLOCK`. Go ahead. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Charles-François Natali neolo...@free.fr added the comment:
support.linux_version() may be changed for requires_linux_version(2, 6, 27),
but linux_version() is always used in tests to check the Linux version.
requires_linux_version() would only raise a SkipTest if the OS is Linux and
if
the kernel is lesser than the specified version.
See the patch attached.
By the way, I like the new os.pipe2() function! You may want to document it
in
the What's new in Python 3.3 doc (just mention the new function, the
document will be rephrased later).
Sure, where is this document?
--
Added file: http://bugs.python.org/file22197/support_linux_version.diff
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12196
___diff -r f73d80d0ba44 Lib/test/support.py
--- a/Lib/test/support.py Mon May 30 11:15:05 2011 -0500
+++ b/Lib/test/support.py Mon May 30 19:21:14 2011 +0200
@@ -291,13 +291,17 @@
msg = Use of the `%s' resource not enabled % resource
raise ResourceDenied(msg)
-def linux_version():
-try:
-# platform.release() is something like '2.6.33.7-desktop-2mnb'
-version_string = platform.release().split('-')[0]
-return tuple(map(int, version_string.split('.')))
-except ValueError:
-return 0, 0, 0
+def requires_linux_version(*min_version):
+if sys.platform.startswith('linux'):
+try:
+# platform.release() is something like '2.6.33.7-desktop-2mnb'
+version_string = platform.release().split('-')[0]
+version = tuple(map(int, version_string.split('.')))
+if version min_version:
+raise unittest.SkipTest(Linux kernel %s or higher required %
+..join(map(str, min_version)))
+except ValueError:
+pass
HOST = 'localhost'
diff -r f73d80d0ba44 Lib/test/test_posix.py
--- a/Lib/test/test_posix.pyMon May 30 11:15:05 2011 -0500
+++ b/Lib/test/test_posix.pyMon May 30 19:21:14 2011 +0200
@@ -310,10 +310,7 @@
@unittest.skipUnless(hasattr(os, 'O_CLOEXEC'), needs os.O_CLOEXEC)
def test_oscloexec(self):
-version = support.linux_version()
-if sys.platform == 'linux2' and version (2, 6, 23):
-self.skipTest(Linux kernel 2.6.23 or higher required,
- not %s.%s.%s % version)
+support.requires_linux_version(2, 6, 23)
fd = os.open(support.TESTFN, os.O_RDONLY|os.O_CLOEXEC)
self.addCleanup(os.close, fd)
self.assertTrue(fcntl.fcntl(fd, fcntl.F_GETFD) fcntl.FD_CLOEXEC)
@@ -480,10 +477,7 @@
@unittest.skipUnless(hasattr(os, 'pipe2'), test needs os.pipe2())
def test_pipe2(self):
-version = support.linux_version()
-if sys.platform == 'linux2' and version (2, 6, 27):
-self.skipTest(Linux kernel 2.6.27 or higher required,
- not %s.%s.%s % version)
+support.requires_linux_version(2, 6, 27)
self.assertRaises(TypeError, os.pipe2, 'DEADBEEF')
self.assertRaises(TypeError, os.pipe2, 0, 0)
diff -r f73d80d0ba44 Lib/test/test_socket.py
--- a/Lib/test/test_socket.py Mon May 30 11:15:05 2011 -0500
+++ b/Lib/test/test_socket.py Mon May 30 19:21:14 2011 +0200
@@ -1024,10 +1024,7 @@
if hasattr(socket, SOCK_NONBLOCK):
def testInitNonBlocking(self):
-v = support.linux_version()
-if v (2, 6, 28):
-self.skipTest(Linux kernel 2.6.28 or higher required, not %s
- % ..join(map(str, v)))
+support.requires_linux_version(2, 6, 28)
# reinit server socket
self.serv.close()
self.serv = socket.socket(socket.AF_INET, socket.SOCK_STREAM |
@@ -2002,10 +1999,7 @@
@unittest.skipUnless(fcntl, module fcntl not available)
class CloexecConstantTest(unittest.TestCase):
def test_SOCK_CLOEXEC(self):
-v = support.linux_version()
-if v (2, 6, 28):
-self.skipTest(Linux kernel 2.6.28 or higher required, not %s
- % ..join(map(str, v)))
+support.requires_linux_version(2, 6, 28)
with socket.socket(socket.AF_INET,
socket.SOCK_STREAM | socket.SOCK_CLOEXEC) as s:
self.assertTrue(s.type socket.SOCK_CLOEXEC)
@@ -2024,10 +2018,7 @@
self.assertEqual(s.gettimeout(), None)
def test_SOCK_NONBLOCK(self):
-v = support.linux_version()
-if v (2, 6, 28):
-self.skipTest(Linux kernel 2.6.28 or higher required, not %s
- % ..join(map(str, v)))
+support.requires_linux_version(2, 6, 28)
# a lot of it seems silly and redundant, but I wanted to test that
# changing back and forth worked ok
with socket.socket(socket.AF_INET,
[issue12196] add pipe2() to the os module
STINNER Victor victor.stin...@haypocalc.com added the comment:
See the patch attached.
I like your patch, it removes many duplicate code :-)
Some comments:
- I don't like an if surrounding the whole function, I prefer if not ...:
return to avoid the (useless) indentation. Well, it's my coding style, do as
you want.
- You should add docstrings. Something like Raise a SkipTest if the OS is
Linux and the kernel version if lesser than min_version. For example,
support.requires_linux_version(2, 6, 28) raises a SkipTest if the version is
lesser than 2.6.28.
- You should move the if version ... outside the try/except ValueError
+# platform.release() is something like '2.6.33.7-desktop-2mnb'
+version_string = platform.release().split('-')[0]
Dummy micro-optimization: .split('-', 1)[0] or .partition('-')[0] is maybe
better than .split('-')[0].
By the way, I like the new os.pipe2() function! You may want to document
it in
the What's new in Python 3.3 doc (just mention the new function, the
document will be rephrased later).
Sure, where is this document?
Doc/whatsnew/3.3.rst
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12196
___
___
Python-bugs-list mailing list
Unsubscribe:
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Roundup Robot devnull@devnull added the comment: New changeset 866d959dea8e by Charles-François Natali in branch 'default': Issue #12196: Add PIPE_MAX_SIZE to test.support, constant larger than the http://hg.python.org/cpython/rev/866d959dea8e -- nosy: +python-dev ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Roundup Robot devnull@devnull added the comment: New changeset 293c398cd4cf by Charles-François Natali in branch 'default': Issue #12196: Add pipe2() to the os module. http://hg.python.org/cpython/rev/293c398cd4cf -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Charles-François Natali neolo...@free.fr added the comment: The Gentoo buildbot on which O_CLOEXEC test failed also chokes on test_pipe2: [ 10/355] test_posix test test_posix failed -- Traceback (most recent call last): File /var/lib/buildslave/3.x.murray-gentoo-wide/build/Lib/test/test_posix.py, line 487, in test_pipe2 r, w = os.pipe2() OSError: [Errno 38] Function not implemented If've added a test to skip this test on Linux kernels older than 2.6.27: like O_CLOEXEC, the problem is that the libc defines pipe2() while the kernel doesn't support it, so when syscall() is called it bails out with ENOSYS. (This buildbot should really have its libc/kernel upgraded...) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Charles-François Natali neolo...@free.fr added the comment: The test now runs fine on the buildbots, closing. -- resolution: - fixed stage: patch review - committed/rejected status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
STINNER Victor victor.stin...@haypocalc.com added the comment: If've added a test to skip this test on Linux kernels older than 2.6.27: like O_CLOEXEC, the problem is that the libc defines pipe2() while the kernel doesn't support it, so when syscall() is called it bails out with ENOSYS. (This buildbot should really have its libc/kernel upgraded...) You may add the issue number of your commit changelog, so a comment is generated directly in the issue. support.linux_version() may be changed for requires_linux_version(2, 6, 27), but linux_version() is always used in tests to check the Linux version. requires_linux_version() would only raise a SkipTest if the OS is Linux and if the kernel is lesser than the specified version. By the way, I like the new os.pipe2() function! You may want to document it in the What's new in Python 3.3 doc (just mention the new function, the document will be rephrased later). -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
R. David Murray rdmur...@bitdance.com added the comment: FYI that buildbot is likely to get a kernel upgrade in the not too distant future. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Ross Lagerwall rosslagerw...@gmail.com added the comment: Out of interest, is there any reason that the configure check for pipe2 is a special case near the bottom of configure.in instead of with all the other function checks in the AC_CHECK_FUNCS[] section in the middle? I know this patch didn't write the configure check but maybe it could be changed. Also, the pure python implementation of subprocess for posix can now be updated to use pipe2 if it exists (previously on _posixsubprocess.c used it). -- nosy: +rosslagerwall ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Charles-François Natali neolo...@free.fr added the comment: Out of interest, is there any reason that the configure check for pipe2 is a special case near the bottom of configure.in instead of with all the other function checks in the AC_CHECK_FUNCS[] section in the middle? No clue. I'll fix that. Also, the pure python implementation of subprocess for posix can now be updated to use pipe2 if it exists (previously on _posixsubprocess.c used it). I don't understand the last part :-) What do you suggest? Use os.pipe2 if available, otherwise use _posixsubprocess.c (because it has the advantage of running with the GIL held) if available, and use os.pipe + fcntl as last resort? By the way, what's the reason for having a both a Python and a C implementation? Are they some configurations where _posixsubprocess could not be available? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Changes by Charles-François Natali neolo...@free.fr: -- nosy: +gregory.p.smith ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Ross Lagerwall rosslagerw...@gmail.com added the comment: Also, the pure python implementation of subprocess for posix can now be updated to use pipe2 if it exists (previously on _posixsubprocess.c used it). I don't understand the last part :-) What do you suggest? Perhaps, os.pipe2 can be used if available, then a modified version of _posixsubprocess.cloexec_pipe without the check for pipe2 could be used as a fallback. I'm not sure why there exists both a python and c implementation. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Gregory P. Smith g...@krypto.org added the comment: I just nuked the pure Python POSIX subprocess implementation in 70467:75ca834df824. No need for both implementations. _posixsubprocess is now the only option. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Changes by Charles-François Natali neolo...@free.fr: Removed file: http://bugs.python.org/file22154/posix_pipe2.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Changes by Charles-François Natali neolo...@free.fr: Removed file: http://bugs.python.org/file22151/support_pipe_max.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Charles-François Natali neolo...@free.fr added the comment:
I just nuked the pure Python POSIX subprocess implementation
Nice.
I've updated both patches to address Victor's comments on test_io and
test_subprocess usage of PIPE_MAX_SIZE, and Ross' comment on pipe2's
configure tests.
I left subprocess use _posixsubprocess.cloexec_pipe, since it leads to
simpler code, especially now that the Python version has been removed.
--
Added file: http://bugs.python.org/file22166/posix_pipe2.diff
Added file: http://bugs.python.org/file22167/support_pipe_max.diff
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12196
___diff -r c5bd972391cd Doc/library/os.rst
--- a/Doc/library/os.rstSat May 28 10:00:14 2011 -0700
+++ b/Doc/library/os.rstSat May 28 19:07:31 2011 +0200
@@ -1019,6 +1019,17 @@
Availability: Unix, Windows.
+.. function:: pipe2(flags=0)
+
+ Create a pipe with *flags* set atomically.
+ *flags* is optional and can be constructed by ORing together zero or more of
+ these values: :data:`O_NONBLOCK`, :data:`O_CLOEXEC`.
+ Return a pair of file descriptors ``(r, w)`` usable for reading and writing,
+ respectively.
+
+ Availability: some flavors of Unix.
+
+
.. function:: posix_fallocate(fd, offset, len)
Ensures that enough disk space is allocated for the file specified by *fd*
diff -r c5bd972391cd Lib/test/test_posix.py
--- a/Lib/test/test_posix.pySat May 28 10:00:14 2011 -0700
+++ b/Lib/test/test_posix.pySat May 28 19:07:31 2011 +0200
@@ -477,6 +477,31 @@
reader, writer = posix.pipe()
os.close(reader)
os.close(writer)
+
+@unittest.skipUnless(hasattr(os, 'pipe2'), test needs os.pipe2())
+def test_pipe2(self):
+self.assertRaises(TypeError, os.pipe2, 'DEADBEEF')
+self.assertRaises(TypeError, os.pipe2, 0, 0)
+
+# try calling without flag, like os.pipe()
+r, w = os.pipe2()
+os.close(r)
+os.close(w)
+
+# test flags
+r, w = os.pipe2(os.O_CLOEXEC|os.O_NONBLOCK)
+self.addCleanup(os.close, r)
+self.addCleanup(os.close, w)
+self.assertTrue(fcntl.fcntl(r, fcntl.F_GETFD) fcntl.FD_CLOEXEC)
+self.assertTrue(fcntl.fcntl(w, fcntl.F_GETFD) fcntl.FD_CLOEXEC)
+# try reading from an empty pipe: this should fail, not block
+self.assertRaises(OSError, os.read, r, 1)
+# try a write big enough to fill-up the pipe (64K on most kernels):
this
+# should either fail or perform a partial write, not block
+try:
+os.write(w, b'x' * support.PIPE_MAX_SIZE)
+except OSError:
+pass
def test_utime(self):
if hasattr(posix, 'utime'):
diff -r c5bd972391cd Modules/posixmodule.c
--- a/Modules/posixmodule.c Sat May 28 10:00:14 2011 -0700
+++ b/Modules/posixmodule.c Sat May 28 19:07:31 2011 +0200
@@ -6547,6 +6547,31 @@
}
#endif /* HAVE_PIPE */
+#ifdef HAVE_PIPE2
+PyDoc_STRVAR(posix_pipe2__doc__,
+pipe2(flags=0) - (read_end, write_end)\n\n\
+Create a pipe with flags set atomically.\
+flags is optional and can be constructed by ORing together zero or more\n\
+of these values: O_NONBLOCK, O_CLOEXEC.\n\
+);
+
+static PyObject *
+posix_pipe2(PyObject *self, PyObject *args)
+{
+int flags = 0;
+int fds[2];
+int res;
+
+if (!PyArg_ParseTuple(args, |i:pipe2, flags))
+return NULL;
+
+res = pipe2(fds, flags);
+if (res != 0)
+return posix_error();
+return Py_BuildValue((ii), fds[0], fds[1]);
+}
+#endif /* HAVE_PIPE2 */
+
#ifdef HAVE_WRITEV
PyDoc_STRVAR(posix_writev__doc__,
writev(fd, buffers) - byteswritten\n\n\
@@ -9441,6 +9466,9 @@
#ifdef HAVE_PIPE
{pipe,posix_pipe, METH_NOARGS, posix_pipe__doc__},
#endif
+#ifdef HAVE_PIPE2
+{pipe2, posix_pipe2, METH_VARARGS, posix_pipe2__doc__},
+#endif
#ifdef HAVE_MKFIFO
{mkfifo, posix_mkfifo, METH_VARARGS, posix_mkfifo__doc__},
#endif
diff -r c5bd972391cd configure
--- a/configure Sat May 28 10:00:14 2011 -0700
+++ b/configure Sat May 28 19:07:31 2011 +0200
@@ -9307,7 +9307,7 @@
getpriority getresuid getresgid getpwent getspnam getspent getsid getwd \
if_nameindex \
initgroups kill killpg lchmod lchown lockf linkat lstat lutimes mbrtowc
mkdirat mkfifo \
- mkfifoat mknod mknodat mktime mremap nice openat pathconf pause plock poll \
+ mkfifoat mknod mknodat mktime mremap nice openat pathconf pause pipe2 plock
poll \
posix_fallocate posix_fadvise pread \
pthread_init pthread_kill putenv pwrite readlink readlinkat readv realpath
renameat \
select sem_open sem_timedwait sem_getvalue sem_unlink sendfile setegid
seteuid \
@@ -13824,14 +13824,6 @@
esac
-ac_fn_c_check_func $LINENO pipe2 ac_cv_func_pipe2
-if test x$ac_cv_func_pipe2 = xyes; then :
-
-$as_echo #define HAVE_PIPE2 1 confdefs.h
-
-fi
-
-
for h in
[issue12196] add pipe2() to the os module
Gregory P. Smith g...@krypto.org added the comment: Include an appropriate Version Added annotation in the pipe2 documentation. Otherwise the current patches look good to me. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
R. David Murray rdmur...@bitdance.com added the comment: For the record (for people reading this ticket later), the removal of the python version of the unix subprocess code was discussed on IRC, and it was clarified there that the reason for removing it is not that it duplicates the C code. We like having python implementations of C code where possible, but the key here is where possible. The python version of the Unix subprocess code cannot be made to work correctly, it is necessary to drop into C to get the semantics correct. -- nosy: +r.david.murray ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
New submission from Charles-François Natali neolo...@free.fr: pipe2() makes it possible to create a pipe O_CLOEXEC or O_NONBLOCK atomically, which can be quite useful, especially in multi-threaded code. It would be nice to expose it in the os module. Patch attached. -- components: Library (Lib) files: posix_pipe2.diff keywords: needs review, patch messages: 137054 nosy: charles-francois.natali, haypo, pitrou priority: low severity: normal stage: patch review status: open title: add pipe2() to the os module type: feature request versions: Python 3.4 Added file: http://bugs.python.org/file22147/posix_pipe2.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
STINNER Victor victor.stin...@haypocalc.com added the comment: +self.assertRaises(TypeError, os.pipe2, (0, 0)) Do you want to call the function with two arguments or one tuple with 2 items? You may test both :-) +# try a write big enough to fill-up the pipe (64K on most kernels): this +# should perform a partial write, not block +os.write(w, b'x' * 10) This constant should be moved to test.support. BaseTestCase.test_communicate_pipe_buf() on subprocess uses: x, y = os.pipe() if mswindows: pipe_buf = 512 else: pipe_buf = os.fpathconf(x, PC_PIPE_BUF) SignalsTest.check_interrupted_write() of test_io uses: # Fill the pipe enough that the write will be blocking. # It will be interrupted by the timer armed above. Since the # other thread has read one byte, the low-level write will # return with a successful (partial) result rather than an EINTR. # The buffered IO layer must check for pending signal # handlers, which in this case will invoke alarm_interrupt(). self.assertRaises(ZeroDivisionError, wio.write, item * (1024 * 1024)) Instead of a constant, it may be function taking a pipe end as argument and returning the size of its buffer. Something like: def pipe_buffer_size(fd): if hasattr(os, 'fpathconf'): # better than sys.platform == win32? pipe_buf = 512 else: pipe_buf = os.fpathconf(fd, PC_PIPE_BUF) +self.assertTrue((time.time() - start) 1.0, Test took too long for O_NONBLOCK.) Hum, I'm not sure that it's revelant to test the time: if the call blocks, it will block forever. If the system is busy, the read+write may takes longer than 1 second. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Charles-François Natali neolo...@free.fr added the comment:
Do you want to call the function with two arguments or one tuple with 2
items? You may test both :-)
The former. Fixed.
Hum, I'm not sure that it's revelant to test the time
I didn't like it either, I just reused what's done in test_socket. Fixed.
BaseTestCase.test_communicate_pipe_buf() on subprocess uses
Yeah, except that it doesn't work, POSIX's PIPE_BUF is the guaranteed limit for
the write to be atomic, not the buffer size (furthermore the libc doesn't have
any way to know the pipe's buffer size or max atomic write, it's just a defined
constant).
$ cat /tmp/test_pipe.py
import os
r, w = os.pipe()
max = os.fpathconf(w, 'PC_PIPE_BUF')
print(max)
os.write(w, 2 * max * b'x')
print('ok')
$ ./python /tmp/test_pipe.py
4096
ok
This constant should be moved to test.support.
Patch attached.
It uses a constant of 1MB, even on Windows. I hope it won't break there, you
never know with Windows...
--
Added file: http://bugs.python.org/file22151/support_pipe_max.diff
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12196
___
___
Python-bugs-list mailing list
Unsubscribe:
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Changes by Charles-François Natali neolo...@free.fr: Removed file: http://bugs.python.org/file22147/posix_pipe2.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Changes by Charles-François Natali neolo...@free.fr: Added file: http://bugs.python.org/file22152/posix_pipe2.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
STINNER Victor victor.stin...@haypocalc.com added the comment: +# A constant likely larger than the underlying OS pipe buffer size. +# Windows limit seems to be around 512B, and most Unix kernels have a 64K pipe +# buffer size: take 1MB to be sure. +PIPE_MAX_SIZE = 1024 * 1024 Hum, I am not sure that the comment is correct. If I understood correctly the usage of this constant: write PIPE_MAX_SIZE into a blocking pipe must block because PIPE_MAX_SIZE is greater than the size of the pipe buffer. I don't know what happen if you write PIPE_MAX_SIZE into a nonblocking pipe: the beginning of the buffer is written and write() returns something lesser than PIPE_MAX_SIZE? You may specify that if you should greater or equal to os.fpathconf(fd, PC_PIPE_BUF). --- 'sys.stderr.write(xyz*%d);' - 'sys.stdout.write(sys.stdin.read())' % pipe_buf], + 'sys.stdout.write(sys.stdin.read())' % + support.PIPE_MAX_SIZE], and +string_to_write = babc * support.PIPE_MAX_SIZE Here you use PIPE_MAX_SIZE*3, not PIPE_MAX_SIZE :-) You can use b'abc' * (support.PIPE_MAX_SIZE // 3), or b'a' * support.PIPE_MAX_SIZE. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Changes by Charles-François Natali neolo...@free.fr: Removed file: http://bugs.python.org/file22152/posix_pipe2.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Changes by Charles-François Natali neolo...@free.fr: Added file: http://bugs.python.org/file22154/posix_pipe2.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12196] add pipe2() to the os module
Charles-François Natali neolo...@free.fr added the comment: +# A constant likely larger than the underlying OS pipe buffer size. +# Windows limit seems to be around 512B, and most Unix kernels have a 64K pipe +# buffer size: take 1MB to be sure. +PIPE_MAX_SIZE = 1024 * 1024 Hum, I am not sure that the comment is correct. If I understood correctly the usage of this constant: write PIPE_MAX_SIZE into a blocking pipe must block because PIPE_MAX_SIZE is greater than the size of the pipe buffer. Correct. I don't know what happen if you write PIPE_MAX_SIZE into a nonblocking pipe: the beginning of the buffer is written and write() returns something lesser than PIPE_MAX_SIZE? Correct. Note that it could also fail with EAGAIN, that's why I added an except OSError clause to the write. You may specify that if you should greater or equal to os.fpathconf(fd, PC_PIPE_BUF). I don't understand what you mean. It will be greater than PIPE_BUF, which is around 4096 (I think 512 guaranteed by POSIX). Here you use PIPE_MAX_SIZE*3, not PIPE_MAX_SIZE :-) You can use b'abc' * (support.PIPE_MAX_SIZE // 3), or b'a' * support.PIPE_MAX_SIZE. I know, but that's what the original code did, so I kept it. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12196 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
