You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ko...@apache.org on 2015/09/07 17:06:57 UTC
svn commit: r1701633 - /subversion/trunk/subversion/libsvn_subr/stream.c
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.
Modified:
subversion/trunk/subversion/libsvn_subr/stream.c
Modified: subversion/trunk/subversion/libsvn_subr/stream.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/stream.c?rev=1701633&r1=1701632&r2=1701633&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/stream.c (original)
+++ subversion/trunk/subversion/libsvn_subr/stream.c Mon Sep 7 15:06:57 2015
@@ -1831,6 +1831,12 @@ svn_stream_for_stdin(svn_stream_t **in,
*in = svn_stream_from_aprfile2(stdin_file, TRUE, pool);
+ /* STDIN may or may not support positioning requests, but generally
+ it does not, or the behavior is implementation-specific. Hence,
+ we cannot safely advertise mark() and seek() support. */
+ svn_stream_set_mark(*in, NULL);
+ svn_stream_set_seek(*in, NULL);
+
return SVN_NO_ERROR;
}
@@ -1847,6 +1853,12 @@ svn_stream_for_stdout(svn_stream_t **out
*out = svn_stream_from_aprfile2(stdout_file, TRUE, pool);
+ /* STDOUT may or may not support positioning requests, but generally
+ it does not, or the behavior is implementation-specific. Hence,
+ we cannot safely advertise mark() and seek() support. */
+ svn_stream_set_mark(*out, NULL);
+ svn_stream_set_seek(*out, NULL);
+
return SVN_NO_ERROR;
}
@@ -1863,6 +1875,12 @@ svn_stream_for_stderr(svn_stream_t **err
*err = svn_stream_from_aprfile2(stderr_file, TRUE, pool);
+ /* STDERR may or may not support positioning requests, but generally
+ it does not, or the behavior is implementation-specific. Hence,
+ we cannot safely advertise mark() and seek() support. */
+ svn_stream_set_mark(*err, NULL);
+ svn_stream_set_seek(*err, NULL);
+
return SVN_NO_ERROR;
}
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
Re: svn commit: r1701633 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
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