You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2015/09/10 20:13:52 UTC
svn commit: r1702305 - /subversion/trunk/subversion/libsvn_subr/stream.c
Author: stefan2
Date: Thu Sep 10 18:13:52 2015
New Revision: 1702305
URL: http://svn.apache.org/r1702305
Log:
APR file based streams shall not superficially support read or write
functions when we know that the APR file itself does not. Instead,
they will now return a "not supported" stream error.
Note that this check is not perfect for arbitrary APR file handles
(may enable more functions than the handle actually supports) but
works correctly for our STD* streams and the files opened through
our svn_io_* API.
* subversion/libsvn_subr/stream.c
(make_stream_from_apr_file): Set read and write functions conditionally.
(svn_stream_from_aprfile2): Determine r/w capabilities to the degree
we can based on the APR API.
(svn_stream_for_stdin,
svn_stream_for_stdout,
svn_stream_for_stderr): Update constructor callers.
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=1702305&r1=1702304&r2=1702305&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/stream.c (original)
+++ subversion/trunk/subversion/libsvn_subr/stream.c Thu Sep 10 18:13:52 2015
@@ -1063,10 +1063,13 @@ static svn_stream_t *
make_stream_from_apr_file(apr_file_t *file,
svn_boolean_t disown,
svn_boolean_t supports_seek,
+ svn_boolean_t supports_read,
+ svn_boolean_t supports_write,
apr_pool_t *pool)
{
struct baton_apr *baton;
svn_stream_t *stream;
+ apr_int32_t flags;
if (file == NULL)
return svn_stream_empty(pool);
@@ -1075,8 +1078,11 @@ make_stream_from_apr_file(apr_file_t *fi
baton->file = file;
baton->pool = pool;
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);
+
+ if (supports_read)
+ svn_stream_set_read2(stream, read_handler_apr, read_full_handler_apr);
+ if (supports_write)
+ svn_stream_set_write(stream, write_handler_apr);
if (supports_seek)
{
@@ -1100,7 +1106,18 @@ svn_stream_from_aprfile2(apr_file_t *fil
svn_boolean_t disown,
apr_pool_t *pool)
{
- return make_stream_from_apr_file(file, disown, TRUE, pool);
+ /* Enable read and write only if the underlying file actually supports it.
+ *
+ * Special files like STDIN will have no flags set at all. In that case,
+ * we can't filter and must allow any operation - which may then fail at
+ * the APR level further down the stack.
+ */
+ apr_uint32_t flags = apr_file_flags_get(file);
+ svn_boolean_t supports_read = (flags == 0) || (flags & APR_READ);
+ svn_boolean_t supports_write = (flags == 0) || (flags & APR_WRITE);
+
+ return make_stream_from_apr_file(file, disown, TRUE, supports_read,
+ supports_write, pool);
}
apr_file_t *
@@ -1851,7 +1868,8 @@ svn_stream_for_stdin(svn_stream_t **in,
it does not, or the behavior is implementation-specific. Hence,
we cannot safely advertise mark(), seek() and non-default skip()
support. */
- *in = make_stream_from_apr_file(stdin_file, TRUE, FALSE, pool);
+ *in = make_stream_from_apr_file(stdin_file, TRUE, FALSE, TRUE, FALSE,
+ pool);
return SVN_NO_ERROR;
}
@@ -1871,7 +1889,8 @@ svn_stream_for_stdout(svn_stream_t **out
it does not, or the behavior is implementation-specific. Hence,
we cannot safely advertise mark(), seek() and non-default skip()
support. */
- *out = make_stream_from_apr_file(stdout_file, TRUE, FALSE, pool);
+ *out = make_stream_from_apr_file(stdout_file, TRUE, FALSE, FALSE, TRUE,
+ pool);
return SVN_NO_ERROR;
}
@@ -1891,7 +1910,8 @@ svn_stream_for_stderr(svn_stream_t **err
it does not, or the behavior is implementation-specific. Hence,
we cannot safely advertise mark(), seek() and non-default skip()
support. */
- *err = make_stream_from_apr_file(stderr_file, TRUE, FALSE, pool);
+ *err = make_stream_from_apr_file(stderr_file, TRUE, FALSE, FALSE, TRUE,
+ pool);
return SVN_NO_ERROR;
}
Re: svn commit: r1702305 -
/subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Branko Čibej <br...@apache.org>.
On 10.09.2015 21:40, Branko Čibej wrote:
> On 10.09.2015 21:06, Stefan Fuhrmann wrote:
>> On Thu, Sep 10, 2015 at 9:00 PM, Bert Huijben <be...@qqmail.nl> wrote:
>>
>>>> -----Original Message-----
>>>> From: stefan2@apache.org [mailto:stefan2@apache.org]
>>>> Sent: donderdag 10 september 2015 20:14
>>>> To: commits@subversion.apache.org
>>>> Subject: svn commit: r1702305 -
>>>> /subversion/trunk/subversion/libsvn_subr/stream.c
>>>>
>>>> Author: stefan2
>>>> Date: Thu Sep 10 18:13:52 2015
>>>> New Revision: 1702305
>>>>
>>>> URL: http://svn.apache.org/r1702305
>>>> Log:
>>>> APR file based streams shall not superficially support read or write
>>>> functions when we know that the APR file itself does not. Instead,
>>>> they will now return a "not supported" stream error.
>>>>
>>>> Note that this check is not perfect for arbitrary APR file handles
>>>> (may enable more functions than the handle actually supports) but
>>>> works correctly for our STD* streams and the files opened through
>>>> our svn_io_* API.
>>> This patch breaks JavaHL's tunnel tests on at least one buildbot.
>>>
>>>
>>> https://ci.apache.org/builders/svn-x64-centos-gcc/builds/277/steps/Test%20bindings%20fsfs%2Bra_serf/logs/stdio
>>>
>>> (The JavaHL tests PASS on Windows)
>>>
>> Ok. I'll wait for Brane to maybe have a look at it. Could well be
>> that we uncovered an inconsistency in the bindings.
> I noticed the failure; I'll take a look in the morning. The
> "inconsistency" is probably in APR, since AFAIK it uses anonymous pipes
> on Windows but just plain file handles on Unix in this particular case.
>
> I'm surprised the test passed on OSX but failed on CentOS/Linux, though.
Ah, it did not pass on OSX, I just didn't wait long enough. That's good
(I think ...)
-- Brane
Re: svn commit: r1702305 -
/subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Branko Čibej <br...@apache.org>.
On 10.09.2015 21:06, Stefan Fuhrmann wrote:
> On Thu, Sep 10, 2015 at 9:00 PM, Bert Huijben <be...@qqmail.nl> wrote:
>
>>
>>> -----Original Message-----
>>> From: stefan2@apache.org [mailto:stefan2@apache.org]
>>> Sent: donderdag 10 september 2015 20:14
>>> To: commits@subversion.apache.org
>>> Subject: svn commit: r1702305 -
>>> /subversion/trunk/subversion/libsvn_subr/stream.c
>>>
>>> Author: stefan2
>>> Date: Thu Sep 10 18:13:52 2015
>>> New Revision: 1702305
>>>
>>> URL: http://svn.apache.org/r1702305
>>> Log:
>>> APR file based streams shall not superficially support read or write
>>> functions when we know that the APR file itself does not. Instead,
>>> they will now return a "not supported" stream error.
>>>
>>> Note that this check is not perfect for arbitrary APR file handles
>>> (may enable more functions than the handle actually supports) but
>>> works correctly for our STD* streams and the files opened through
>>> our svn_io_* API.
>> This patch breaks JavaHL's tunnel tests on at least one buildbot.
>>
>>
>> https://ci.apache.org/builders/svn-x64-centos-gcc/builds/277/steps/Test%20bindings%20fsfs%2Bra_serf/logs/stdio
>>
>> (The JavaHL tests PASS on Windows)
>>
> Ok. I'll wait for Brane to maybe have a look at it. Could well be
> that we uncovered an inconsistency in the bindings.
I noticed the failure; I'll take a look in the morning. The
"inconsistency" is probably in APR, since AFAIK it uses anonymous pipes
on Windows but just plain file handles on Unix in this particular case.
I'm surprised the test passed on OSX but failed on CentOS/Linux, though.
-- Brane
Re: svn commit: r1702305 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Thu, Sep 10, 2015 at 9:00 PM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
> > -----Original Message-----
> > From: stefan2@apache.org [mailto:stefan2@apache.org]
> > Sent: donderdag 10 september 2015 20:14
> > To: commits@subversion.apache.org
> > Subject: svn commit: r1702305 -
> > /subversion/trunk/subversion/libsvn_subr/stream.c
> >
> > Author: stefan2
> > Date: Thu Sep 10 18:13:52 2015
> > New Revision: 1702305
> >
> > URL: http://svn.apache.org/r1702305
> > Log:
> > APR file based streams shall not superficially support read or write
> > functions when we know that the APR file itself does not. Instead,
> > they will now return a "not supported" stream error.
> >
> > Note that this check is not perfect for arbitrary APR file handles
> > (may enable more functions than the handle actually supports) but
> > works correctly for our STD* streams and the files opened through
> > our svn_io_* API.
>
> This patch breaks JavaHL's tunnel tests on at least one buildbot.
>
>
> https://ci.apache.org/builders/svn-x64-centos-gcc/builds/277/steps/Test%20bindings%20fsfs%2Bra_serf/logs/stdio
>
> (The JavaHL tests PASS on Windows)
>
Ok. I'll wait for Brane to maybe have a look at it. Could well be
that we uncovered an inconsistency in the bindings.
Otherwise, I'm happy to revert that change. It simply felt like
it belonged with similar changes that were made recently.
-- Stefan^2.
RE: svn commit: r1702305 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: stefan2@apache.org [mailto:stefan2@apache.org]
> Sent: donderdag 10 september 2015 20:14
> To: commits@subversion.apache.org
> Subject: svn commit: r1702305 -
> /subversion/trunk/subversion/libsvn_subr/stream.c
>
> Author: stefan2
> Date: Thu Sep 10 18:13:52 2015
> New Revision: 1702305
>
> URL: http://svn.apache.org/r1702305
> Log:
> APR file based streams shall not superficially support read or write
> functions when we know that the APR file itself does not. Instead,
> they will now return a "not supported" stream error.
>
> Note that this check is not perfect for arbitrary APR file handles
> (may enable more functions than the handle actually supports) but
> works correctly for our STD* streams and the files opened through
> our svn_io_* API.
This patch breaks JavaHL's tunnel tests on at least one buildbot.
https://ci.apache.org/builders/svn-x64-centos-gcc/builds/277/steps/Test%20bindings%20fsfs%2Bra_serf/logs/stdio
(The JavaHL tests PASS on Windows)
Bert
Re: svn commit: r1702305 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Sep 11, 2015 at 11:05 AM, Stefan Fuhrmann <
stefan.fuhrmann@wandisco.com> wrote:
> On Fri, Sep 11, 2015 at 11:01 AM, Branko Čibej <br...@apache.org> wrote:
>
>> On 10.09.2015 23:50, Evgeny Kotkov wrote:
>> > Stefan Fuhrmann <st...@apache.org> writes:
>> >
>> > + * Special files like STDIN will have no flags set at all. In that
>> case,
>> > + * we can't filter and must allow any operation - which may then
>> fail at
>> > + * the APR level further down the stack.
>> > + */
>> > + apr_uint32_t flags = apr_file_flags_get(file);
>> > + svn_boolean_t supports_read = (flags == 0) || (flags & APR_READ);
>> > + svn_boolean_t supports_write = (flags == 0) || (flags & APR_WRITE);
>> >
>> > Files corresponding to the standard I/O streams actually have the
>> appropriate
>> > APR_READ / APR_WRITE flags set starting from APR 1.3 — see, for example,
>> > apr_file_open_flags_stderr() in [1]. Hence, the (flags == 0) check is
>> going
>> > to return false for them.
>> >
>> >> Note that this check is not perfect for arbitrary APR file handles
>> >> (may enable more functions than the handle actually supports) but
>> >> works correctly for our STD* streams and the files opened through
>> >> our svn_io_* API.
>> > The actual problem, to my mind, is that relying on flags to determine
>> if the
>> > file allows reading or writing, is fragile. There are examples of
>> apr_file_t's
>> > that don't have the corresponding flags, but still allow reading and
>> writing,
>> > and svn_stream_from_aprfile2() is going to break for them.
>> >
>> > One example would be apr_file_pipe_create() on Unix that sets
>> APR_INHERIT
>> > flag on the created pipe [2]. Another example is creating a pipe on
>> Windows
>> > that currently initializes flags to zero [3].
>>
>> I've reviewed the JavaHL code again and it indeed appears that this is
>> caused by the difference in implementations of apr_file_pipe_create_ex()
>> on Windows and elsewhere. The bindings code itself is not
>> platform-specific; see TunnelContext in
>> subversion/bindings/javahl/native/OperationContext.cpp. Those pipes get
>> wrapped into streams deep in the RA layer.
>>
>> One could argue that apr_file_pipe_create is broken since it doesn't set
>> the appropriate flags on the input and output handles, but that doesn't
>> really help make this particular instance in the bindings code work.
>>
>
> O.k. Then I'll simply revert.
>
Done in r1702410.
-- Stefan^2.
Re: svn commit: r1702305 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Sep 11, 2015 at 11:01 AM, Branko Čibej <br...@apache.org> wrote:
> On 10.09.2015 23:50, Evgeny Kotkov wrote:
> > Stefan Fuhrmann <st...@apache.org> writes:
> >
> > + * Special files like STDIN will have no flags set at all. In that
> case,
> > + * we can't filter and must allow any operation - which may then fail
> at
> > + * the APR level further down the stack.
> > + */
> > + apr_uint32_t flags = apr_file_flags_get(file);
> > + svn_boolean_t supports_read = (flags == 0) || (flags & APR_READ);
> > + svn_boolean_t supports_write = (flags == 0) || (flags & APR_WRITE);
> >
> > Files corresponding to the standard I/O streams actually have the
> appropriate
> > APR_READ / APR_WRITE flags set starting from APR 1.3 — see, for example,
> > apr_file_open_flags_stderr() in [1]. Hence, the (flags == 0) check is
> going
> > to return false for them.
> >
> >> Note that this check is not perfect for arbitrary APR file handles
> >> (may enable more functions than the handle actually supports) but
> >> works correctly for our STD* streams and the files opened through
> >> our svn_io_* API.
> > The actual problem, to my mind, is that relying on flags to determine if
> the
> > file allows reading or writing, is fragile. There are examples of
> apr_file_t's
> > that don't have the corresponding flags, but still allow reading and
> writing,
> > and svn_stream_from_aprfile2() is going to break for them.
> >
> > One example would be apr_file_pipe_create() on Unix that sets APR_INHERIT
> > flag on the created pipe [2]. Another example is creating a pipe on
> Windows
> > that currently initializes flags to zero [3].
>
> I've reviewed the JavaHL code again and it indeed appears that this is
> caused by the difference in implementations of apr_file_pipe_create_ex()
> on Windows and elsewhere. The bindings code itself is not
> platform-specific; see TunnelContext in
> subversion/bindings/javahl/native/OperationContext.cpp. Those pipes get
> wrapped into streams deep in the RA layer.
>
> One could argue that apr_file_pipe_create is broken since it doesn't set
> the appropriate flags on the input and output handles, but that doesn't
> really help make this particular instance in the bindings code work.
>
O.k. Then I'll simply revert.
-- Stefan^2.
Re: svn commit: r1702305 -
/subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Branko Čibej <br...@apache.org>.
On 10.09.2015 23:50, Evgeny Kotkov wrote:
> Stefan Fuhrmann <st...@apache.org> writes:
>
> + * Special files like STDIN will have no flags set at all. In that case,
> + * we can't filter and must allow any operation - which may then fail at
> + * the APR level further down the stack.
> + */
> + apr_uint32_t flags = apr_file_flags_get(file);
> + svn_boolean_t supports_read = (flags == 0) || (flags & APR_READ);
> + svn_boolean_t supports_write = (flags == 0) || (flags & APR_WRITE);
>
> Files corresponding to the standard I/O streams actually have the appropriate
> APR_READ / APR_WRITE flags set starting from APR 1.3 — see, for example,
> apr_file_open_flags_stderr() in [1]. Hence, the (flags == 0) check is going
> to return false for them.
>
>> Note that this check is not perfect for arbitrary APR file handles
>> (may enable more functions than the handle actually supports) but
>> works correctly for our STD* streams and the files opened through
>> our svn_io_* API.
> The actual problem, to my mind, is that relying on flags to determine if the
> file allows reading or writing, is fragile. There are examples of apr_file_t's
> that don't have the corresponding flags, but still allow reading and writing,
> and svn_stream_from_aprfile2() is going to break for them.
>
> One example would be apr_file_pipe_create() on Unix that sets APR_INHERIT
> flag on the created pipe [2]. Another example is creating a pipe on Windows
> that currently initializes flags to zero [3].
I've reviewed the JavaHL code again and it indeed appears that this is
caused by the difference in implementations of apr_file_pipe_create_ex()
on Windows and elsewhere. The bindings code itself is not
platform-specific; see TunnelContext in
subversion/bindings/javahl/native/OperationContext.cpp. Those pipes get
wrapped into streams deep in the RA layer.
One could argue that apr_file_pipe_create is broken since it doesn't set
the appropriate flags on the input and output handles, but that doesn't
really help make this particular instance in the bindings code work.
-- Brane
Re: svn commit: r1702305 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Stefan Fuhrmann <st...@apache.org> writes:
+ * Special files like STDIN will have no flags set at all. In that case,
+ * we can't filter and must allow any operation - which may then fail at
+ * the APR level further down the stack.
+ */
+ apr_uint32_t flags = apr_file_flags_get(file);
+ svn_boolean_t supports_read = (flags == 0) || (flags & APR_READ);
+ svn_boolean_t supports_write = (flags == 0) || (flags & APR_WRITE);
Files corresponding to the standard I/O streams actually have the appropriate
APR_READ / APR_WRITE flags set starting from APR 1.3 — see, for example,
apr_file_open_flags_stderr() in [1]. Hence, the (flags == 0) check is going
to return false for them.
> Note that this check is not perfect for arbitrary APR file handles
> (may enable more functions than the handle actually supports) but
> works correctly for our STD* streams and the files opened through
> our svn_io_* API.
The actual problem, to my mind, is that relying on flags to determine if the
file allows reading or writing, is fragile. There are examples of apr_file_t's
that don't have the corresponding flags, but still allow reading and writing,
and svn_stream_from_aprfile2() is going to break for them.
One example would be apr_file_pipe_create() on Unix that sets APR_INHERIT
flag on the created pipe [2]. Another example is creating a pipe on Windows
that currently initializes flags to zero [3].
[1] https://svn.apache.org/repos/asf/apr/apr/branches/1.3.x/file_io/unix/open.c
[2] https://svn.apache.org/repos/asf/apr/apr/trunk/file_io/unix/pipe.c
[3] https://svn.apache.org/repos/asf/apr/apr/trunk/file_io/win32/pipe.c
Regards,
Evgeny Kotkov