Hey Edmund,
Thanks for stretching your svn coding legs with this one. I've got
some detailed feedback for you below:
On Fri, Apr 10, 2009 at 09:55, Edmund Wong <ed_at_kdtc.net> wrote:
>...
> If len == 0, it doesn't break out of the loop; though
> it should break out the next time it tries to read.
I wouldn't exactly trust a stream that has returned len=0 to be readable.
Now... you said the test suite passed, so apparently none of our
streams have a problem with that. But I wouldn't want to rely on that
:-P ... in my feedback below, I've got a suggestion for this.
> I'll need to read up on the stream_read() function.
> What happens if it reads past? It should throw
> an error, no? And thusly break.
A stream cannot "read past".
>...
> +++ 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 *err = SVN_NO_ERROR;
> + svn_error_t *err2 = SVN_NO_ERROR;
>
> /* 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,46 @@
> {
> apr_size_t len = SVN__STREAM_CHUNK_SIZE;
>
> - if (cancel_func)
> - SVN_ERR(cancel_func(cancel_baton));
> + if (cancel_func)
> + err = cancel_func(cancel_baton);
> +
> + if (err)
> + break;
You cancel_func == NULL, then you can avoid the second if, since there
is no way err could get set:
if (cancel_func)
{
err = cancel_func(cancel_baton);
if (err)
break;
}
> + err = svn_stream_read(from, buf, &len);
> + if (err)
> + break;
> +
> + if (len > 0)
> + {
> + err = svn_stream_write(to, buf, &len);
>
> - 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 (len != SVN__STREAM_CHUNK_SIZE)
> + break;
I'd suggest moving this outside of the len>0 block.
> + if (err)
> + break;
> + }
> + else
> + {
> + break;
> + }
Skip this else branch, and just do the len test here.
> }
> +
> + if (err)
> + {
> + err2 = svn_error_compose_create(svn_stream_close(from),
> + svn_stream_close(to));
> + }
> + else
> + {
> + err2 = SVN_NO_ERROR;
> + err = svn_error_compose_create(svn_stream_close(from),
> + svn_stream_close(to));
> + }
You could skip the if/else blocks altogether, and just do:
err2 = svn_error_compose_create(...);
> - return svn_error_compose_create(svn_stream_close(from),
> - svn_stream_close(to));
> + return svn_error_compose_create(err,err2);
If err==NULL, then it will return whatever err2 is... in this case,
any potential problem with closing the streams.
Style note: there should be a space after that comma.
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1627618
Received on 2009-04-10 11:35:12 CEST