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, stef...@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
