Stefan Sperling wrote on Wed, Mar 30, 2011 at 13:25:44 +0200:
> On Wed, Mar 30, 2011 at 11:45:01AM +0200, Daniel Shahaf wrote:
> > stsp_at_apache.org wrote on Tue, Mar 29, 2011 at 14:46:39 -0000:
> > > @@ -338,7 +338,19 @@ svn_cmdline_fputs(const char *string, FI
> > > if (fputs(out, stream) == EOF)
> > > {
> > > if (errno)
> > > - return svn_error_wrap_apr(errno, _("Write error"));
> > > + {
> > > + err = svn_error_wrap_apr(errno, _("Write error"));
> > > +
> > > + /* ### Issue #3014: Return a specific error for broken pipes,
> > > + * ### with a single element in the error chain. */
> > > + if (APR_STATUS_IS_EPIPE(err->apr_err))
> > > + {
> > > + svn_error_clear(err);
> >
> > So you're doing:
> >
> > * get error number
> > * construct an svn_error_t object (that's includes a few malloc()s)
> > * check error number
> > * destroy svn_error_t object
> >
> > Won't it be simpler to avoid constructing the svn_error_t object in the
> > first place (when errno is EPIPE)?
>
> Yes, that's what I had initially.
> Something like: if (errno == EPIPE) ...
> But I wasn't sure if that would need #ifdef on windows.
> And I didn't want to break the windows bots.
> So I decided to let APR figure out how to represent EPIPE.
>
Given the way your patch passes errno to apr_status_t-expecting
functions, I'd say that APR_STATUS_IS_EPIPE(errno) should work.
I'm not sure whether or not APR_FROM_OS_ERROR() is needed in the mix.
> > > @@ -376,7 +400,15 @@ svn_cmdline_handle_exit_error(svn_error_
> > > apr_pool_t *pool,
> > > const char *prefix)
> > > {
> > > - svn_handle_error2(err, stderr, FALSE, prefix);
> > > + /* Issue #3014:
> > > + * Don't print anything on broken pipes. The pipe was likely
> > > + * closed by the process at the other end. We expect that
> > > + * process to perform error reporting as necessary.
> > > + *
> > > + * ### This assumes that there is only one error in a chain for
> > > + * ### SVN_ERR_IO_PIPE_WRITE_ERROR. See svn_cmdline_fputs(). */
> > > + if (err->apr_err != SVN_ERR_IO_PIPE_WRITE_ERROR)
> > > + svn_handle_error2(err, stderr, FALSE, prefix);
> >
> > This ### seems easy enough to avoid, isn't it? i.e., why did you need
> > to assume a single-link chain?
>
> Well, if the pipe write error is somewhere within the chain, we'd
> have to iterate the entire chain. Is there an API for that?
Actually there is (svn_error_has_cause()), but what I had in mind while
writing my reply was "Hey, isn't it sufficient to require that
SVN_ERR_IO_PIPE_WRITE_ERROR be the outer-most code?"
> I don't want to add a loop...
>
svn_error_t *
svn_error_has_cause(svn_error_t *err, apr_status_t apr_err)
{
for (; err; err = err->child)
if (err->apr_err == apr_err)
break;
return err;
}
> > > @@ -383,7 +383,15 @@ svn_opt_print_generic_help2(const char *
> > > return;
> > >
> > > print_error:
> > > - svn_handle_error2(err, stderr, FALSE, "svn: ");
> > > + /* Issue #3014:
> > > + * Don't print anything on broken pipes. The pipe was likely
> > > + * closed by the process at the other end. We expect that
> > > + * process to perform error reporting as necessary.
> > > + *
> > > + * ### This assumes that there is only one error in a chain for
> > > + * ### SVN_ERR_IO_PIPE_WRITE_ERROR. See svn_cmdline_fputs(). */
> > > + if (err->apr_err != SVN_ERR_IO_PIPE_WRITE_ERROR)
> > > + svn_handle_error2(err, stderr, FALSE, "svn: ");
> >
> > This bit of code (including the comment, the ###, and the specific
> > condition) is repeated N times below.
> >
> > Abstract it into a private helper macro?
>
> Good idea.
Received on 2011-03-30 13:36:18 CEST