[PATCH] swig-py svn_stream_t read() glue

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] swig-py svn_stream_t read() glue

Daniel Shahaf-2
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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] swig-py svn_stream_t read() glue

Troy Curtis Jr


On Wed, Jan 3, 2018 at 10:55 PM Daniel Shahaf <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] swig-py svn_stream_t read() glue

Branko Čibej
On 07.01.2018 16:17, Troy Curtis Jr wrote:

> On Wed, Jan 3, 2018 at 10:55 PM Daniel Shahaf <[hidden email]>
> 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] swig-py svn_stream_t read() glue

Daniel Shahaf-2
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