On Wed, 8 Apr 2009, Edmund Wong wrote:
> Hi,
>
> My bad. Here's a better patch. The previous patch
> bombed due to invalid use of the *. :) Sorry. Should've
> compiled it before sending it. My bad.
>
> Log:
>
> [[[
> Completed the TODO as stated in svn_io.h for
> svn_stream_copy3().
>
> * subversion/libsvn_subr/stream.c
> (svn_stream_copy3): Even on error exit, the two files
> from and to are closed.
>
> Patch by: Edmund Wong <edmund_at_belfordhk.com>
>
> ]]]
>
> ------------------------------------------------------
> Index: subversion/libsvn_subr/stream.c
> ===================================================================
> --- subversion/libsvn_subr/stream.c (revision 37093)
> +++ subversion/libsvn_subr/stream.c (working copy)
> @@ -210,6 +210,8 @@
> apr_pool_t *scratch_pool)
> {
> char *buf = apr_palloc(scratch_pool, SVN__STREAM_CHUNK_SIZE);
> + svn_error_t *svn_err__save_error;
> + svn_error_t *svn_err__save_error2;
Often the variable is just named 'err', or 'err2'.
The following looks very complicated. Why not just something like this:
err = somefunc();
if (err)
break;
As soon as there is an error you can leave the loop as has been done until
now.
> /* Read and write chunks until we get a short read, indicating the
> end of the stream. (We can't get a short write without an
> @@ -218,18 +220,54 @@
> {
> apr_size_t len = SVN__STREAM_CHUNK_SIZE;
>
> - if (cancel_func)
> - SVN_ERR(cancel_func(cancel_baton));
> -
> - SVN_ERR(svn_stream_read(from, buf, &len));
> - if (len > 0)
> - SVN_ERR(svn_stream_write(to, buf, &len));
> - if (len != SVN__STREAM_CHUNK_SIZE)
> - break;
> + if (cancel_func)
> + svn_err__save_error = cancel_func(cancel_baton);
> +
> + if (! svn_err__save_error)
> + {
> + svn_err__save_error = svn_stream_read(from, buf, &len);
> + if (! svn_err__save_error)
> + {
> + if (len > 0)
> + {
> + svn_err__save_error = svn_stream_write(to, buf, &len);
> + if (! svn_err__save_error)
> + {
> + if (len != SVN__STREAM_CHUNK_SIZE)
> + break;
> + }
> + else
> + {
> + break;
> + }
> + }
> + }
> + else
> + {
> + break;
> + }
> + }
> + else
> + {
> + break;
> + }
> }
>
> - return svn_error_compose_create(svn_stream_close(from),
> - svn_stream_close(to));
> + if (!svn_err__save_error)
> + {
> + svn_err__save_error = svn_error_compose_create(svn_stream_close(from),
> + svn_stream_close(to));
> + }
> + else
> + {
> + svn_err__save_error2 = svn_error_compose_create(svn_stream_close(from),
> + svn_stream_close(to));
> + svn_err__save_error = svn_error_compose_create(svn_err__save_error2,
> + svn_err__save_error);
> + }
> +
> +
> + return svn_err__save_error;
No if needed here, svn_error_compose_create takes care of that. You could
do something like this:
err2 = svn_error_compose_create(close(from), close(to);
return svn_error_compose_create(err, err2);
That way the error from the loop is the first in the chain, followed by
the error from closing 'from', then that from closing 'to'. If one of the
errors is SVN_NO_ERROR it's just not added to the chain.
(Don't forget to initialize 'err' to 'SVN_NO_ERROR')
> }
>
> svn_error_t *
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1606340
Received on 2009-04-09 04:13:15 CEST