Repository navigation
gh-129205: Add os.readinto API for reading data into a caller provided buffer - #129211
vstinner merged 23 commits into
Conversation
Sorry, something went wrong.
Add a new OS api which will read data directly into a caller provided writeable buffer protocol object.
Py_ssize_t can't get as big as Py_size_t, _Py_read checks max length
|
@vstinner ready for you to take a look |
Sorry, something went wrong.
picnixz
left a comment
There was a problem hiding this comment.
Some test nits
Sorry, something went wrong.
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
| /* Ensure negative is never returned without an error. Simplifies calling | ||
| code. _Py_read should succeed, possibly reading 0 bytes, _or_ set an | ||
| error. */ | ||
| assert(result >= 0 || (result == -1 && PyErr_Occurred())); |
There was a problem hiding this comment.
note: Clinic wrapping code takes -1 return + PyErr_Occurred to indicate error and will return a NULL PyObject* for the function. So should never get back a PyLong object which is negative.
Sorry, something went wrong.
Co-authored-by: Victor Stinner <vstinner@python.org>
vstinner
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the update, the implementation now looks better :-)
Sorry, something went wrong.
picnixz
left a comment
There was a problem hiding this comment.
Just some wording comments and LGTM.
Sorry, something went wrong.
| datas = [b"hello", b"world", b"spam"] | ||
| bufs = [bytearray(5), bytearray(5), bytearray(4)] | ||
|
|
||
| code = '\n'.join(( | ||
| 'import os, sys, time', | ||
| '', | ||
| 'wr = int(sys.argv[1])', | ||
| 'datas = %r' % datas, | ||
| 'sleep_time = %r' % self.sleep_time, | ||
| '', | ||
| 'for data in datas:', | ||
| ' # let the parent block on read()', | ||
| ' time.sleep(sleep_time)', | ||
| ' os.write(wr, data)', | ||
| )) |
There was a problem hiding this comment.
| datas = [b"hello", b"world", b"spam"] | |
| bufs = [bytearray(5), bytearray(5), bytearray(4)] | |
| code = '\n'.join(( | |
| 'import os, sys, time', | |
| '', | |
| 'wr = int(sys.argv[1])', | |
| 'datas = %r' % datas, | |
| 'sleep_time = %r' % self.sleep_time, | |
| '', | |
| 'for data in datas:', | |
| ' # let the parent block on read()', | |
| ' time.sleep(sleep_time)', | |
| ' os.write(wr, data)', | |
| )) | |
| data = [b"hello", b"world", b"spam"] | |
| bufs = [bytearray(5), bytearray(5), bytearray(4)] | |
| code = '\n'.join(( | |
| 'import os, sys, time', | |
| '', | |
| 'wr = int(sys.argv[1])', | |
| f'data = {data!r}', | |
| f'sleep_time = {self.sleep_time!r}', | |
| '', | |
| 'for datum in data:', | |
| ' # let the parent block on read()', | |
| ' time.sleep(sleep_time)', | |
| ' os.write(wr, datum)', | |
| )) |
Strictly speaking, data is already the plural form of datum.
Sorry, something went wrong.
There was a problem hiding this comment.
For data -> datum the code is copied from test_read just before with minor tweaks; I think the two being very close in code is useful for maintenance more than naming. I think I can get rid of bufs and zip though which will remove the ambiguities / make the code read better
Sorry, something went wrong.
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
|
Merged, thank you! |
Sorry, something went wrong.
|
As the initial author of test_eintr, I'm likely the one who wrote datas typo. So I wrote PR gh-129316 to fix grammar and modernize the code base (use f-strings) 😁 |
Sorry, something went wrong.
Adds os.readinto API as well as associated tests. Most functionality is provided by _Py_read which is fairly well exercised by os.read tests.
For testing I have been running:
make buildbottest python -bb -E -Wd -m test -M32g -uall test_os _test_eintr -vvvI skipped adding to test_largefile, that path should get exercised in part by test_os.FileTests.test_large_readinto, and will get exercised reading 2GB+ of bytes as move _pyio to use os.readinto (test_largefile.PyLargeFileTest).
Not sure how to make a negative length buffer for testing that case getting handled as expected (_Py_read takes a size_t and Py_buffer.len is a Py_ssize_t)
Should probably have buildbots run once reviewed. This should work across platforms, but I've only tested on Linux and things like large file tests aren't run by all bots.