subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Fuhrmann <stefan.fuhrm...@wandisco.com>
Subject Re: svn commit: r1702305 - /subversion/trunk/subversion/libsvn_subr/stream.c
Date Fri, 11 Sep 2015 09:05:55 GMT
On Fri, Sep 11, 2015 at 11:01 AM, Branko Čibej <brane@apache.org> wrote:

> On 10.09.2015 23:50, Evgeny Kotkov wrote:
> > Stefan Fuhrmann <stefan2@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.

Mime
View raw message