Re: svn commit: r924797 - /subversion/trunk/subversion/libsvn_subr/stream.c

[Julian Foad <ju...@wandisco.com>]

On Thu, 2010-03-18, rhuijben@apache.org wrote:
> Author: rhuijben
> Date: Thu Mar 18 14:21:26 2010
> New Revision: 924797
> 
> URL: http://svn.apache.org/viewvc?rev=924797&view=rev
> Log:
> Extract the range handling from the common apr read codepath in the streams
> api, as there is no need to complicate this common code path here just
> for being able to read from a specific range.

Good.

> * subversion/libsvn_subr/stream.c
>   (read_handler_apr): Extract the range code and move it to ...
>   (read_range_handler_apr): ... this new function wrapping read_handler_apr.
>   (svn_stream_from_aprfile_range_readonly): Hook read_range_handler_apr.
>      Add note about handling disown properly.
> 
> Modified:
>     subversion/trunk/subversion/libsvn_subr/stream.c
> 
> Modified: subversion/trunk/subversion/libsvn_subr/stream.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/stream.c?rev=924797&r1=924796&r2=924797&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/stream.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/stream.c Thu Mar 18 14:21:26 2010
> @@ -676,35 +676,6 @@ read_handler_apr(void *baton, char *buff
>    struct baton_apr *btn = baton;
>    svn_error_t *err;
>  
> -  /* Check for range restriction. */
> -  if (btn->start >= 0 && btn->end > 0)
> -    {
> -      /* Get the current file position and make sure it is in range. */
> -      apr_off_t pos;
> -
> -      pos = 0;
> -      SVN_ERR(svn_io_file_seek(btn->file, APR_CUR, &pos, btn->pool));
> -      if (pos < btn->start)
> -        {
> -          /* We're before the range, so forward the file cursor to
> -           * the start of the range. */
> -          pos = btn->start;
> -          SVN_ERR(svn_io_file_seek(btn->file, APR_SET, &pos, btn->pool));
> -        }
> -      else if (pos >= btn->end)
> -        {
> -          /* We're past the range, indicate that no bytes can be read. */
> -          *len = 0;
> -          return SVN_NO_ERROR;
> -        }
> -      else
> -        {
> -          /* We're in range, but don't read over the end of the range. */
> -          if (pos + *len > btn->end)
> -              *len = btn->end - pos;
> -        }
> -    }
> -
>    err = svn_io_file_read_full(btn->file, buffer, *len, len, btn->pool);
>    if (err && APR_STATUS_IS_EOF(err->apr_err))
>      {
[...]
> +static svn_error_t *
> +read_range_handler_apr(void *baton, char *buffer, apr_size_t *len)

Please can we all get into the habit of putting a doc string on EVERY
new function, even when it does not seem like there is much that needs
to be said about it.

>  {
> -  return svn_stream_from_aprfile2(file, TRUE, pool);
> +  struct baton_apr *btn = baton;
> +
> +  /* ### BH: I think this can be simplified/optimized by just keeping
> +             track of the current position. */
> +
> +  /* Check for range restriction. */
> +  if (btn->start >= 0 && btn->end > 0)
> +    {
> +      /* Get the current file position and make sure it is in range. */
> +      apr_off_t pos;
> +
> +      pos = 0;
> +      SVN_ERR(svn_io_file_seek(btn->file, APR_CUR, &pos, btn->pool));
> +      if (pos < btn->start)
> +        {
> +          /* We're before the range, so forward the file cursor to
> +           * the start of the range. */
> +          pos = btn->start;
> +          SVN_ERR(svn_io_file_seek(btn->file, APR_SET, &pos, btn->pool));
> +        }
> +      else if (pos >= btn->end)
> +        {
> +          /* We're past the range, indicate that no bytes can be read. */
> +          *len = 0;
> +          return SVN_NO_ERROR;
> +        }
> +      else
> +        {
> +          /* We're in range, but don't read over the end of the range. */
> +          if (pos + *len > btn->end)
> +              *len = btn->end - pos;
> +        }

I think that last block needs to take effect even if 'pos' was
previously before 'start'.  Otherwise there can be a too-long read.

(I know you only moved this code.)

- Julian


> +    }
> +
> +  return read_handler_apr(baton, buffer, len);
>  }
>  
> +
>  svn_stream_t *
>  svn_stream_from_aprfile_range_readonly(apr_file_t *file,
>                                         svn_boolean_t disown,
> @@ -863,6 +869,7 @@ svn_stream_from_aprfile_range_readonly(a
>    svn_stream_t *stream;
>    apr_off_t pos;
>  
> +  /* ### HACK: These shortcuts don't handle disown FALSE properly */
>    if (file == NULL || start < 0 || end <= 0 || start >= end)
>      return svn_stream_empty(pool);
>  
> @@ -877,7 +884,7 @@ svn_stream_from_aprfile_range_readonly(a
>    baton->start = start;
>    baton->end = end;
>    stream = svn_stream_create(baton, pool);
> -  svn_stream_set_read(stream, read_handler_apr);
> +  svn_stream_set_read(stream, read_range_handler_apr);
>    svn_stream_set_reset(stream, reset_handler_apr);
>    svn_stream_set_mark(stream, mark_handler_apr);
>    svn_stream_set_seek(stream, seek_handler_apr);
> 
>