classification
Title: os.read() must use Py_ssize_t for the size parameter
Type: enhancement Stage:
Components: Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: haypo, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2014-07-07 13:37 by haypo, last changed 2014-07-12 15:27 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
posix_read_py_ssize_t.patch haypo, 2014-07-07 13:37 review
test_posix_read_py_ssize_t.patch serhiy.storchaka, 2014-07-07 14:23 review
Messages (10)
msg222462 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-07-07 13:37
os.read() currently uses the int type for the size parameter, whereas the C function uses a C size_t parameter:

       ssize_t read(int fd, void *buf, size_t count);

The Python function must use the Py_ssize_t type, but truncate to INT_MAX on Windows, as done in FileIO.read(). It would help to implement FileIO in pure Python: issue #21859.

I don't think that this enhancement should be backported to Python 2.7 nor Python 3.4.
msg222465 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-07-07 14:23
Patch LGTM. Here is a test for it.
msg222756 - (view) Author: Roundup Robot (python-dev) Date: 2014-07-11 15:11
New changeset e9b401d46e20 by Victor Stinner in branch 'default':
Issue #21932: os.read() now uses a :c:func:`Py_ssize_t` type instead of
http://hg.python.org/cpython/rev/e9b401d46e20
msg222757 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-07-11 15:12
> Patch LGTM.

Thanks for the review.

> Here is a test for it.

Your test does not pass because Linux truncates the read() size to 2GB - 4096. I tested with /dev/zero device and with a regular file.

I wrote a simplified test to just check that size larger than INT_MAX does not emit an OverflowError.
msg222760 - (view) Author: Roundup Robot (python-dev) Date: 2014-07-11 15:35
New changeset 4a7fcd5273ce by Victor Stinner in branch 'default':
Issue #21932: Ooops, os.read(fd, size) allocates a buffer of size bytes, even
http://hg.python.org/cpython/rev/4a7fcd5273ce
msg222828 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-07-12 08:48
Failed on 32-bit:

======================================================================
ERROR: test_large_read (test.test_os.FileTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/serhiy/py/cpython/Lib/test/support/__init__.py", line 1592, in wrapper
    return f(self, maxsize)
  File "/home/serhiy/py/cpython/Lib/test/test_os.py", line 136, in test_large_read
    data = os.read(fp.fileno(), size)
OverflowError: Python int too large to convert to C ssize_t

----------------------------------------------------------------------

Re-add the @unittest.skipUnless(INT_MAX < PY_SSIZE_T_MAX, "needs INT_MAX < PY_SSIZE_T_MAX") decorator (between cpython_only and bigmemtest).
msg222831 - (view) Author: Roundup Robot (python-dev) Date: 2014-07-12 09:04
New changeset 880e2cdac8b1 by Victor Stinner in branch 'default':
Issue #21932: Skip test_os.test_large_read() on 32-bit system
http://hg.python.org/cpython/rev/880e2cdac8b1
msg222832 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-07-12 09:05
> Re-add the @unittest.skipUnless(INT_MAX < PY_SSIZE_T_MAX, "needs INT_MAX < PY_SSIZE_T_MAX") decorator (between cpython_only and bigmemtest).

Oh, I removed it because I thought that it was useless. I didn't understand the purpose of the test. I added a comment. Thanks for the report.
msg222836 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-07-12 11:45
Thank you for fixing os.read().
msg222845 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-07-12 15:27
> Serhiy Storchaka added the comment:
>
> Thank you for fixing os.read().
>

You're welcome, thanks for your help with the test. I hope that it would
help you to implement FileIO in Python.

FYI I saw EINVAL errors on test_large_read() on some buildbots, Mac OS X
and FreeBSD if I remember correctly, when I forgot the bigmem decorator.
And FreeBSD kills the process if there is not enough memory.
History
Date User Action Args
2014-07-12 15:27:41hayposetmessages: + msg222845
2014-07-12 11:45:36serhiy.storchakasetmessages: + msg222836
2014-07-12 09:05:58hayposetmessages: + msg222832
2014-07-12 09:04:58python-devsetmessages: + msg222831
2014-07-12 08:48:12serhiy.storchakasetmessages: + msg222828
2014-07-11 15:35:33python-devsetmessages: + msg222760
2014-07-11 15:12:57hayposetstatus: open -> closed
resolution: fixed
messages: + msg222757
2014-07-11 15:11:06python-devsetnosy: + python-dev
messages: + msg222756
2014-07-07 14:23:22serhiy.storchakasetfiles: + test_posix_read_py_ssize_t.patch

messages: + msg222465
2014-07-07 13:37:54haypocreate