Thanks for the feedback, Blair!
I will look into this tomorrow.
-- Stefan^2.
On 09.02.2011 02:07, Blair Zajac wrote:
> 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:59:11 CET