You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2011/03/30 11:45:01 UTC
Re: svn commit: r1086607 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_subr/cmdline.c libsvn_subr/io.c
libsvn_subr/opt.c svn/main.c svn/notify.c svn/obliterate-cmd.c
svnadmin/main.c
stsp@apache.org wrote on Tue, Mar 29, 2011 at 14:46:39 -0000:
> Author: stsp
> Date: Tue Mar 29 14:46:38 2011
> New Revision: 1086607
>
> URL: http://svn.apache.org/viewvc?rev=1086607&view=rev
> Log:
> Fix issue #3014, "svn log | head" should not print "Write error: Broken pipe".
>
> Make svn and other commands exit silently (with EXIT_FAILURE) if a pipe write
> error occured. The rationale being that the process closing the pipe is also
> responsible for printing a related error message, if necessary.
>
> * subversion/libsvn_subr/cmdline.c
> (svn_cmdline_fputs, svn_cmdline_fflush): Return
> SVN_ERR_IO_PIPE_WRITE_ERROR when writes fail due to EPIPE, so that
> callers can choose to ignore this kind of error.
> (svn_cmdline_handle_exit_error): Don't print message for pipe write errors.
>
> * subversion/libsvn_subr/io.c
> (do_io_file_wrapper_cleanup): Return SVN_ERR_IO_PIPE_WRITE_ERROR when writes
> fail due to EPIPE.
>
> * subversion/libsvn_subr/opt.c
> (svn_opt_print_generic_help2): Don't print message for pipe write errors.
>
> * subversion/svn/notify.c
> (notify): Same.
>
> * subversion/svn/main.c
> (main): Same.
>
> * subversion/svn/obliterate-cmd.c
> (notify): Same.
>
> * subversion/svnadmin/main.c
> (main): Same.
>
> * subversion/include/svn_error_codes.h
> (SVN_ERR_IO_PIPE_WRITE_ERROR): New error code.
>
> Modified: subversion/trunk/subversion/libsvn_subr/cmdline.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/cmdline.c?rev=1086607&r1=1086606&r2=1086607&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/cmdline.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/cmdline.c Tue Mar 29 14:46:38 2011
> @@ -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)?
> + return svn_error_create(SVN_ERR_IO_PIPE_WRITE_ERROR, NULL, NULL);
> + }
> + else
> + return svn_error_return(err);
> + }
> else
> return svn_error_create
> (SVN_ERR_IO_WRITE_ERROR, NULL, NULL);
> @@ -355,7 +367,19 @@ svn_cmdline_fflush(FILE *stream)
> if (fflush(stream) == EOF)
> {
> if (errno)
> - return svn_error_wrap_apr(errno, _("Write error"));
> + {
> + svn_error_t *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);
> + return svn_error_create(SVN_ERR_IO_PIPE_WRITE_ERROR, NULL, NULL);
> + }
> + else
> + return svn_error_return(err);
> + }
> else
> return svn_error_create(SVN_ERR_IO_WRITE_ERROR, NULL, NULL);
> }
> @@ -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?
> svn_error_clear(err);
> if (pool)
> svn_pool_destroy(pool);
>
> Modified: subversion/trunk/subversion/libsvn_subr/io.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/io.c?rev=1086607&r1=1086606&r2=1086607&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/io.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/io.c Tue Mar 29 14:46:38 2011
> @@ -3034,6 +3034,11 @@ do_io_file_wrapper_cleanup(apr_file_t *f
> name = NULL;
> svn_error_clear(err);
>
> + /* ### Issue #3014: Return a specific error for broken pipes,
> + * ### with a single element in the error chain. */
> + if (APR_STATUS_IS_EPIPE(status))
> + return svn_error_create(SVN_ERR_IO_PIPE_WRITE_ERROR, NULL, NULL);
> +
> if (name)
> return svn_error_wrap_apr(status, _(msg),
> svn_dirent_local_style(name, pool));
>
> Modified: subversion/trunk/subversion/libsvn_subr/opt.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/opt.c?rev=1086607&r1=1086606&r2=1086607&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/opt.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/opt.c Tue Mar 29 14:46:38 2011
> @@ -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?
Re: svn commit: r1086607 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_subr/cmdline.c libsvn_subr/io.c
libsvn_subr/opt.c svn/main.c svn/notify.c svn/obliterate-cmd.c
svnadmin/main.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Wed, Mar 30, 2011 at 15:06:35 +0200:
> On Wed, Mar 30, 2011 at 01:35:42PM +0200, Daniel Shahaf wrote:
> > > > 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.
>
> Fixed in r1086936.
>
Thanks.
> > > 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?"
>
> Well, that's what it's requiring. Hence the comments.
>
The comments require a chain that contains only one error, that's not
the same thing as requiring a chain whose outer-most error has error
code N.
> > > > > @@ -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.
>
> Though as I said on IRC, I can't come up with a good way of doing this.
>
> Do you want a macro like this? I think that's ugly.
>
> SVN_ERR_CALL_UNLESS(err->apr_err, SVN_ERR_IO_PIPE_WRITE_ERROR,
> svn_handle_error2(err, stderr, FALSE, "svn: "));
>
> #define SVN_ERR_CALL_UNLESS(error_code1, error_code2, expr) \
> if ((error_code1) != (error_code2) (expr)
>
> Something that hard-codes error_code2 to EPIPE would also be ugly.
>
How about the following, then? At least, it does avoid having
hard-coding any errors at the caller sites.
/** "%s\n\n%s\n" % (docstring, "see issue %d" % IforgotwhatNis) */
#define SVN_CMDLINE_IGNOREABLE_EXIT_ERROR(err) \
((err) == SVN_NO_ERROR \
|| (err)->apr_err == SVN_ERR_IO_PIPE_WRITE_ERROR \
|| (err)->apr_err == SVN_ERR_WHATEVER)
...
if (! SVN_CMDLINE_IGNOREABLE_EXIT_ERROR(err)):
svn_handle_error2(err);
Make sense?
> Note that this macro would need to be shared between several commands,
> not just 'svn'.
Re: svn commit: r1086607 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_subr/cmdline.c libsvn_subr/io.c
libsvn_subr/opt.c svn/main.c svn/notify.c svn/obliterate-cmd.c
svnadmin/main.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Mar 30, 2011 at 01:35:42PM +0200, Daniel Shahaf wrote:
> > > 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.
Fixed in r1086936.
> > 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?"
Well, that's what it's requiring. Hence the comments.
> > > > @@ -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.
Though as I said on IRC, I can't come up with a good way of doing this.
Do you want a macro like this? I think that's ugly.
SVN_ERR_CALL_UNLESS(err->apr_err, SVN_ERR_IO_PIPE_WRITE_ERROR,
svn_handle_error2(err, stderr, FALSE, "svn: "));
#define SVN_ERR_CALL_UNLESS(error_code1, error_code2, expr) \
if ((error_code1) != (error_code2) (expr)
Something that hard-codes error_code2 to EPIPE would also be ugly.
Note that this macro would need to be shared between several commands,
not just 'svn'.
Re: svn commit: r1086607 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_subr/cmdline.c libsvn_subr/io.c
libsvn_subr/opt.c svn/main.c svn/notify.c svn/obliterate-cmd.c
svnadmin/main.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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@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.
Re: svn commit: r1086607 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_subr/cmdline.c libsvn_subr/io.c
libsvn_subr/opt.c svn/main.c svn/notify.c svn/obliterate-cmd.c
svnadmin/main.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Mar 30, 2011 at 11:45:01AM +0200, Daniel Shahaf wrote:
> stsp@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.
> > @@ -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?
I don't want to add a loop...
> > @@ -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.