Re: svn_stream_copy3() in svn_io.h

On Tue, Apr 7, 2009 at 13:25, Stefan Sperling <stsp_at_elego.de> wrote:
> On Tue, Apr 07, 2009 at 07:15:10PM +0800, Edmund Wong wrote:
>> Hi,
>>
>> Again, reading svn_io.h, I came across a "TODO" for the
>> svn_stream_copy3() :
>>
>>   * ### TODO: should close the streams ALWAYS, even on error exit
>>
>>   Thinking that it might be my chance to do something for a
>> change, I looked into stream.c (this *is* the file, right?)
>> and came across the following:-
>>
>> svn_error_t *svn_stream_copy3(svn_stream_t *from, svn_stream_t *to,
>>                                svn_cancel_func_t cancel_func,
>>                                void *cancel_baton,
>>                                apr_pool_t *scratch_pool)
>> {
>>    char *buf = apr_palloc(scratch_pool, SVN__STREAM_CHUNK_SIZE);
>>
>>    /* 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
>>       associated error.) */
>>    while (1)
>>      {
>>        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;
>>      }
>>
>>
>>    return svn_error_compose_create(svn_stream_close(from),
>>                                    svn_stream_close(to));
>> }
>>
>> Does this mean that since svn_stream_copy3() calls
>> svn_stream_close() on both from and to, both from
>> and to are closed after the return?  (Though I'm not sure if
>> the resulting closure will result in an error.)
>>
>> If it does mean so, shouldn't the TODO be 'removed'?
>
> All the SVN_ERR(...) calls might return if an error is
> thrown from the function wrapped in SVN_ERR().
>
> So I'd say the TODO is still valid.

Right. I added to the TODO for specifically that reason: the streams
may not get closed if an error occurs. In some scenarios, we want to
ensure that svn_stream_close() is *always* called on certain streams.
Their logic needs/wants to rely on the fact that the streams will
always be closed after calling svn_stream_copy3().

Edmund: this should be a nice little bite-sized task to tweak that
function to ensure they're always closed.

Cheers,
-g

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1576194
Received on 2009-04-07 13:42:53 CEST