On 2/8/11 4:01 PM, stefan2_at_apache.org wrote:
> Author: stefan2
> Date: Wed Feb 9 00:01:04 2011
> New Revision: 1068695
>
> URL: http://svn.apache.org/viewvc?rev=1068695&view=rev
> Log:
> Introduce svn_stream_skip() and svn_stream_buffered() stream API functions
> and implement them for all stream types.
Hi Stefan,
I read through the public headers and it wasn't clear to me what skip did. I
assumed that the function takes the number of bytes to skip and seeks.
typedef svn_error_t *(*svn_skip_fn_t)(void *baton,
apr_size_t *count);
But in looking at the implementation, it actually consumes all the bytes. Maybe
it should be renamed to consume, because skip, to me, sounds like a cheap seek,
instead of a potentially more expensive read.
I suggest adding some more documentation here and/or renaming skip. Maybe consume?
> +
> +/* Skip data from any stream by reading and simply discarding it. */
> +static svn_error_t *
> +skip_default_handler(void *baton, apr_size_t *count, svn_read_fn_t read_fn)
> +{
> + apr_size_t total_bytes_read = 0;
> + apr_size_t bytes_read;
> + char buffer[4096];
> + svn_error_t *err = SVN_NO_ERROR;
> +
> + while ((total_bytes_read< *count)&& !err)
> + {
> + bytes_read = sizeof(buffer)< *count ? sizeof(buffer) : *count;
Since *count isn't being decremented, then you could read past the *count bytes
if *count isn't evenly divisible by 4096.
Sounds like a unit test for skipping is needed.
> static svn_error_t *
> +skip_handler_apr(void *baton, apr_size_t *count)
> +{
> + struct baton_apr *btn = baton;
> + apr_off_t offset = *count;
> +
> + SVN_ERR(svn_io_file_seek(btn->file, SEEK_CUR,&offset, btn->pool));
APR_CUR
> + *count = offset;
skip_default_handler() returns the number of bytes actually read while
skip_handler_apr() returns the current offset. If you were already N bytes into
the stream, then the two functions don't return the same conceptual value.
> +static svn_boolean_t
> +buffered_handler_apr(void *baton)
> +{
> + struct baton_apr *btn = baton;
> + return apr_file_buffer_size_get(btn->file) != 0;
We have systems with APR 1.2 and it doesn't provide apr_file_buffer_size_get().
> Modified: subversion/branches/integrate-stream-api-extensions/subversion/libsvn_subr/subst.c
> URL: http://svn.apache.org/viewvc/subversion/branches/integrate-stream-api-extensions/subversion/libsvn_subr/subst.c?rev=1068695&r1=1068694&r2=1068695&view=diff
> ==============================================================================
> --- subversion/branches/integrate-stream-api-extensions/subversion/libsvn_subr/subst.c (original)
> +++ subversion/branches/integrate-stream-api-extensions/subversion/libsvn_subr/subst.c Wed Feb 9 00:01:04 2011
> @@ -1249,6 +1249,27 @@ translated_stream_read(void *baton,
> return SVN_NO_ERROR;
> }
>
> +/* Implements svn_skip_fn_t. */
> +static svn_error_t *
> +translated_stream_skip(void *baton,
> + apr_size_t *count)
> +{
> + apr_size_t total_bytes_read = 0;
> + apr_size_t bytes_read;
> + char buffer[SVN__STREAM_CHUNK_SIZE];
> + svn_error_t *err = SVN_NO_ERROR;
> +
> + while ((total_bytes_read< *count)&& !err)
> + {
> + bytes_read = sizeof(buffer)< *count ? sizeof(buffer) : *count;
Same issue here with skipping.
Blair
Received on 2011-02-09 02:08:38 CET