Ivan Zhakov <i...@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
Index: subversion/libsvn_subr/stream.c
===================================================================
--- subversion/libsvn_subr/stream.c (revision 1701648)
+++ subversion/libsvn_subr/stream.c (working copy)
@@ -1055,10 +1055,12 @@ svn_stream_open_unique(svn_stream_t **stream,
}
-svn_stream_t *
-svn_stream_from_aprfile2(apr_file_t *file,
- svn_boolean_t disown,
- apr_pool_t *pool)
+/* Helper function that creates a stream from an APR file. */
+static svn_stream_t *
+make_stream_from_apr_file(apr_file_t *file,
+ svn_boolean_t disown,
+ svn_boolean_t supports_seek,
+ apr_pool_t *pool)
{
struct baton_apr *baton;
svn_stream_t *stream;
@@ -1072,9 +1074,14 @@ svn_stream_open_unique(svn_stream_t **stream,
stream = svn_stream_create(baton, pool);
svn_stream_set_read2(stream, read_handler_apr, read_full_handler_apr);
svn_stream_set_write(stream, write_handler_apr);
- svn_stream_set_skip(stream, skip_handler_apr);
- svn_stream_set_mark(stream, mark_handler_apr);
- svn_stream_set_seek(stream, seek_handler_apr);
+
+ if (supports_seek)
+ {
+ svn_stream_set_skip(stream, skip_handler_apr);
+ svn_stream_set_mark(stream, mark_handler_apr);
+ svn_stream_set_seek(stream, seek_handler_apr);
+ }
+
svn_stream_set_data_available(stream, data_available_handler_apr);
svn_stream__set_is_buffered(stream, is_buffered_handler_apr);
stream->file = file;
@@ -1085,6 +1092,14 @@ svn_stream_open_unique(svn_stream_t **stream,
return stream;
}
+svn_stream_t *
+svn_stream_from_aprfile2(apr_file_t *file,
+ svn_boolean_t disown,
+ apr_pool_t *pool)
+{
+ return svn_error_trace(make_stream_from_apr_file(file, disown, TRUE, pool));
+}
+
apr_file_t *
svn_stream__aprfile(svn_stream_t *stream)
{
@@ -1829,13 +1844,11 @@ svn_stream_for_stdin(svn_stream_t **in, apr_pool_t
if (apr_err)
return svn_error_wrap_apr(apr_err, "Can't open stdin");
- *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);
+ we cannot safely advertise mark(), seek() and non-default skip()
+ support. */
+ *in = make_stream_from_apr_file(stdin_file, TRUE, FALSE, pool);
return SVN_NO_ERROR;
}
@@ -1851,13 +1864,11 @@ svn_stream_for_stdout(svn_stream_t **out, apr_pool
if (apr_err)
return svn_error_wrap_apr(apr_err, "Can't open stdout");
- *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);
+ we cannot safely advertise mark(), seek() and non-default skip()
+ support. */
+ *out = make_stream_from_apr_file(stdout_file, TRUE, FALSE, pool);
return SVN_NO_ERROR;
}
@@ -1873,13 +1884,11 @@ svn_stream_for_stderr(svn_stream_t **err, apr_pool
if (apr_err)
return svn_error_wrap_apr(apr_err, "Can't open stderr");
- *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);
+ we cannot safely advertise mark(), seek() and non-default skip()
+ support. */
+ *err = make_stream_from_apr_file(stderr_file, TRUE, FALSE, pool);
return SVN_NO_ERROR;
}
