You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ivan Zhakov <iv...@visualsvn.com> on 2015/09/08 11:21:19 UTC
Re: svn commit: r1701633 - /subversion/trunk/subversion/libsvn_subr/stream.c
On 7 September 2015 at 18:06, <ko...@apache.org> wrote:
> Author: kotkov
> Date: Mon Sep 7 15:06:57 2015
> New Revision: 1701633
>
> URL: http://svn.apache.org/r1701633
> Log:
> Fix svn_stream_for_stdin() and related functions for STDOUT and STDERR
> that were returning streams with mark() and seek() capabilities.
>
> STDIN, STDOUT and STDERR don't provide general support for positioning
> requests. This behavior is implementation-specific and depends on what's
> passed as the corresponding handle. For example, on Linux, apr_file_seek()
> that calls lseek() internally fails with ESPIPE [1] when the descriptor is
> associated with a terminal device. As we cannot safely advertise mark()
> and seek() support for these streams, don't do that.
>
> [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/lseek.html
>
> * subversion/libsvn_subr/stream.c
> (svn_stream_for_stdin, svn_stream_for_stdout, svn_stream_for_stderr):
> Don't install mark and seek handlers.
>
It may be worth to introduce local helper like
'make_stream_from_apr_file(svn_boolean_t supports_seek)' and use it in
svn_stream_from_apr_file2() and svn_stream_for_stdin() instead of
clearing mark/seek handlers after calling svn_stream_from_apr_file2().
Another problem that currently svn_stream_t for stdin/stderr/stdout
pretends to support native svn_stream_skip(), while I'm not sure that
all platforms support seek for stdin.
--
Ivan Zhakov
Re: svn commit: r1701633 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Sep 8, 2015 at 11:21 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 7 September 2015 at 18:06, <ko...@apache.org> wrote:
> > Author: kotkov
> > Date: Mon Sep 7 15:06:57 2015
> > New Revision: 1701633
> >
> > URL: http://svn.apache.org/r1701633
> > Log:
> > Fix svn_stream_for_stdin() and related functions for STDOUT and STDERR
> > that were returning streams with mark() and seek() capabilities.
> >
> > STDIN, STDOUT and STDERR don't provide general support for positioning
> > requests. This behavior is implementation-specific and depends on what's
> > passed as the corresponding handle. For example, on Linux,
> apr_file_seek()
> > that calls lseek() internally fails with ESPIPE [1] when the descriptor
> is
> > associated with a terminal device. As we cannot safely advertise mark()
> > and seek() support for these streams, don't do that.
>
Good find!
> > [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/lseek.html
> >
> > * subversion/libsvn_subr/stream.c
> > (svn_stream_for_stdin, svn_stream_for_stdout, svn_stream_for_stderr):
> > Don't install mark and seek handlers.
> >
> It may be worth to introduce local helper like
> 'make_stream_from_apr_file(svn_boolean_t supports_seek)' and use it in
> svn_stream_from_apr_file2() and svn_stream_for_stdin() instead of
> clearing mark/seek handlers after calling svn_stream_from_apr_file2().
>
> Another problem that currently svn_stream_t for stdin/stderr/stdout
> pretends to support native svn_stream_skip(), while I'm not sure that
> all platforms support seek for stdin.
>
One more thing we could do is not adding mark & seek
on stream based on write-only files. I haven't tried but
that's something we should be able to do generically in
svn_stream_from_apr_file already.
-- Stefan^2.
Re: svn commit: r1701633 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
>> Indeed, I missed that default skip_handler_apr() performs a seek that could
>> be unavailable or behave surprisingly when used with a handle like STDIN.
>> So, the fix is partial.
>>
>> What do you think about the attached patch?
>>
> Looks good for me.
I committed this change in r1701792, r1701797 and extended the corresponding
backport proposal.
Regards,
Evgeny Kotkov
Re: svn commit: r1701633 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 8 September 2015 at 14:10, Evgeny Kotkov <ev...@visualsvn.com> wrote:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
>> It may be worth to introduce local helper like
>> 'make_stream_from_apr_file(svn_boolean_t supports_seek)' and use it in
>> svn_stream_from_apr_file2() and svn_stream_for_stdin() instead of
>> clearing mark/seek handlers after calling svn_stream_from_apr_file2().
>
> Sounds good.
>
>> Another problem that currently svn_stream_t for stdin/stderr/stdout
>> pretends to support native svn_stream_skip(), while I'm not sure that
>> all platforms support seek for stdin.
>
> Indeed, I missed that default skip_handler_apr() performs a seek that could
> be unavailable or behave surprisingly when used with a handle like STDIN.
> So, the fix is partial.
>
> What do you think about the attached patch?
>
Looks good for me.
> As of other callbacks, I found out that data_available_handler_apr() calls
> PeekNamedPipe() on Windows, and if the STDIN handle is a tty, this call
> errors out with ERROR_INCORRECT_FUNCTION. We currently wrap all errors
> originating from PeekNamedPipe() into SVN_ERR_STREAM_NOT_SUPPORTED, so,
> technically, this behavior follows the contract. Still, I am thinking
> that this is something we could improve separately.
>
Agree that this can be improved separately.
--
Ivan Zhakov
Re: svn commit: r1701633 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> It may be worth to introduce local helper like
> 'make_stream_from_apr_file(svn_boolean_t supports_seek)' and use it in
> svn_stream_from_apr_file2() and svn_stream_for_stdin() instead of
> clearing mark/seek handlers after calling svn_stream_from_apr_file2().
Sounds good.
> Another problem that currently svn_stream_t for stdin/stderr/stdout
> pretends to support native svn_stream_skip(), while I'm not sure that
> all platforms support seek for stdin.
Indeed, I missed that default skip_handler_apr() performs a seek that could
be unavailable or behave surprisingly when used with a handle like STDIN.
So, the fix is partial.
What do you think about the attached patch?
As of other callbacks, I found out that data_available_handler_apr() calls
PeekNamedPipe() on Windows, and if the STDIN handle is a tty, this call
errors out with ERROR_INCORRECT_FUNCTION. We currently wrap all errors
originating from PeekNamedPipe() into SVN_ERR_STREAM_NOT_SUPPORTED, so,
technically, this behavior follows the contract. Still, I am thinking
that this is something we could improve separately.
Regards,
Evgeny Kotkov