You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2018/01/04 04:55:43 UTC
[PATCH] swig-py svn_stream_t read() glue
svn_swig_py_make_stream() is a function that wraps a PyObject in an
svn_stream_t. Its read implementation, read_handler_pyio(), just calls
the PyObject's .read() method.
According to <https://docs.python.org/2/library/io.html#io.RawIOBase.read>,
the read() method of "raw" file-like objects makes only one read(2)
syscall, and so may return fewer bytes than were requested. (The py3
docs are similar.)
However, svn_stream_t "full read" functions are obligated to return the
number of bytes requested, unless EOF.
Therefore, do we need the following patch?
[[[
swig-py: Support raw binary file-like objects for readable svn_stream_t*
parameters. [D:bindings]
* subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
(svn_swig_py_make_stream): Declare read_handler_pyio() as a non-full
svn_read_fn_t, in case PY_IO is a raw binary file object.
]]]
[[[
Index: subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
===================================================================
--- subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c (revision 1819751)
+++ subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c (working copy)
@@ -2578,8 +2578,7 @@ svn_swig_py_make_stream(PyObject *py_io, apr_pool_
svn_stream_t *stream;
stream = svn_stream_create(py_io, pool);
- svn_stream_set_read2(stream, NULL /* only full read support */,
- read_handler_pyio);
+ svn_stream_set_read2(stream, read_handler_pyio, NULL);
svn_stream_set_write(stream, write_handler_pyio);
svn_stream_set_close(stream, close_handler_pyio);
apr_pool_cleanup_register(pool, py_io, svn_swig_py_stream_destroy,
]]]
Cheers,
Daniel
Re: [PATCH] swig-py svn_stream_t read() glue
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Thanks for the reviews; committed in r1820518 and nominated for backport
for 1.10.x and for 1.9.x as well (although I can see that the risk/benefit balance
could be different for the two).
Daniel
Re: [PATCH] swig-py svn_stream_t read() glue
Posted by Branko Čibej <br...@apache.org>.
On 07.01.2018 16:17, Troy Curtis Jr wrote:
> On Wed, Jan 3, 2018 at 10:55 PM Daniel Shahaf <d....@daniel.shahaf.name>
> wrote:
>
>> svn_swig_py_make_stream() is a function that wraps a PyObject in an
>> svn_stream_t. Its read implementation, read_handler_pyio(), just calls
>> the PyObject's .read() method.
>>
>> According to <https://docs.python.org/2/library/io.html#io.RawIOBase.read
>>> ,
>> the read() method of "raw" file-like objects makes only one read(2)
>> syscall, and so may return fewer bytes than were requested. (The py3
>> docs are similar.)
>>
>>
> Probably in practice the passed in objects are one of the buffered types
> where the read() behaves as this code obviously expected, which is likely
> why this hasn't been noticed before. However, we can't actually *know*
> that, and anything with a read() method would actually be perfectly valid.
> So it seems like a good patch to me.
I agree. Though not buffered streams but files; reading from a file will
always behave as a full read whilst reading from a socket or pipe or
fifo will not.
Anyway +1 for the patch. The full-read functionality is simulated at the
stream API implementation level if there's no specific full-read function.
-- Brane
Re: [PATCH] swig-py svn_stream_t read() glue
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Wed, Jan 3, 2018 at 10:55 PM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> svn_swig_py_make_stream() is a function that wraps a PyObject in an
> svn_stream_t. Its read implementation, read_handler_pyio(), just calls
> the PyObject's .read() method.
>
> According to <https://docs.python.org/2/library/io.html#io.RawIOBase.read
> >,
> the read() method of "raw" file-like objects makes only one read(2)
> syscall, and so may return fewer bytes than were requested. (The py3
> docs are similar.)
>
>
Probably in practice the passed in objects are one of the buffered types
where the read() behaves as this code obviously expected, which is likely
why this hasn't been noticed before. However, we can't actually *know*
that, and anything with a read() method would actually be perfectly valid.
So it seems like a good patch to me.
However, svn_stream_t "full read" functions are obligated to return the
> number of bytes requested, unless EOF.
>
> Therefore, do we need the following patch?
>
> [[[
> swig-py: Support raw binary file-like objects for readable svn_stream_t*
> parameters. [D:bindings]
>
> * subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
> (svn_swig_py_make_stream): Declare read_handler_pyio() as a non-full
> svn_read_fn_t, in case PY_IO is a raw binary file object.
> ]]]
>
> [[[
> Index: subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
> ===================================================================
> --- subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
> (revision 1819751)
> +++ subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
> (working copy)
> @@ -2578,8 +2578,7 @@ svn_swig_py_make_stream(PyObject *py_io, apr_pool_
> svn_stream_t *stream;
>
> stream = svn_stream_create(py_io, pool);
> - svn_stream_set_read2(stream, NULL /* only full read support */,
> - read_handler_pyio);
> + svn_stream_set_read2(stream, read_handler_pyio, NULL);
> svn_stream_set_write(stream, write_handler_pyio);
> svn_stream_set_close(stream, close_handler_pyio);
> apr_pool_cleanup_register(pool, py_io, svn_swig_py_stream_destroy,
> ]]]
>
> Cheers,
>
> Daniel
>