On Thu, 2011-06-02, Stefan Fuhrmann wrote:
> On 01.06.2011 16:40, Julian Foad wrote:
> >> 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.
Done in r1130500.
- Julian
> >> *count = (apr_size_t)(new_pos - current);
> >>
> >> return err;
> >> }
