On 01.06.2011 16:40, Julian Foad wrote:
I (Julian Foad) wrote:
On Sun, 2011-02-20, [email protected] wrote:
Merge all changes (r1068695 - r1072516) from the
integrate-stream-api-extensions.
These patches add svn_stream_skip(), svn_stream_buffered()
and svn_stream_supports_mark().
A belated review.
And some more.
static svn_error_t *
skip_handler_apr(void *baton, apr_size_t *count)
{
struct baton_apr *btn = baton;
apr_off_t offset = *count;
apr_off_t new_pos = *count;
apr_off_t current = 0;
svn_error_t *err;
/* so far, we have not moved anything */
*count = 0;
SVN_ERR(svn_io_file_seek(btn->file, APR_CUR,¤t, btn->pool));
err = svn_io_file_seek(btn->file, APR_CUR,&new_pos, btn->pool);
/* Irrespective of errors, return the number of bytes we actually moved.
* If no new position has been returned from seek(), assume that no move
* happend and keep the *count==0 set earlier.
*/
if ((offset != new_pos) || (current == 0))
It doesn't make sense to compare 'offset' with 'new_pos', because the
former is a relative position and the latter an absolute position. And
comparing 'current' with 0 doesn't seem relevant: if the first seek
failed, we'd not still be in this function because an error would have
been raised.
No idea what went on there but this is clearly broken.
Luckily, nobody used the in/out parameter return value.
If the second seek returns an error you can't trust the value of
'new_pos', unless we're relying on undocumented behaviour. So I think
what you want here is simply:
if (! err)
and remove the doc-string promise that you'll return the current
position in the event of an error. And then the 'count' parameter can
be just an input parameter rather than in/out.
I agree. The "return the true position" was motivated by
the "seek to end-of-stream" use-case that we obviously
don't have anywhere in our code as of now.
Because our streams are often no immediate file streams,
such seeks won't be meaningful in the general case.
Hence, we could not use it without restricting the types
of streams a certain API accepts. That makes it unlikely
that we would use the 'count' out parameter.
*count = (apr_size_t)(new_pos - current);
return err;
}
-- Stefan^2.
