You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2011/02/09 01:01:04 UTC
svn commit: r1068695 - in
/subversion/branches/integrate-stream-api-extensions: ./
subversion/include/svn_io.h subversion/libsvn_subr/stream.c
subversion/libsvn_subr/subst.c
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.
Merged revisions from /branches/performance:
985514, 986485, 986492 (partial), 987887, 1029055, 1067129 (partial)
Modified:
subversion/branches/integrate-stream-api-extensions/ (props changed)
subversion/branches/integrate-stream-api-extensions/subversion/include/svn_io.h
subversion/branches/integrate-stream-api-extensions/subversion/libsvn_subr/stream.c
subversion/branches/integrate-stream-api-extensions/subversion/libsvn_subr/subst.c
Propchange: subversion/branches/integrate-stream-api-extensions/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Wed Feb 9 00:01:04 2011
@@ -27,7 +27,7 @@
/subversion/branches/log-g-performance:870941-871032
/subversion/branches/merge-skips-obstructions:874525-874615
/subversion/branches/nfc-nfd-aware-client:870276,870376
-/subversion/branches/performance:979193,980118,981087,981684,982043,982355,983764,983766,984927,984973,984984,985014,985037,985046,985472,985477,985482,985500,985606,985669,986453,987888,987893,988319,990541,990568,990572,990600,990759,992899,992911,993127,993141,994956,995478,995507,995603,998858,999098,1001413,1001417,1004291,1022668,1022670,1022676,1022715,1022719,1025660,1025672,1027193,1027203,1027206,1027214,1027227,1028077,1028092,1028094,1028104,1028107,1028111,1028354,1029038,1029042-1029043,1029078,1029080,1029090,1029092-1029093,1029111,1029151,1029158,1029232,1029335,1029340,1029342,1029344,1030763,1030827,1031203,1031235,1032285,1032333,1033040,1033057,1033294,1035869,1039511,1043705,1053735,1056015,1066452,1067683
+/subversion/branches/performance:979193,980118,981087,981684,982043,982355,983764,983766,984927,984973,984984,985014,985037,985046,985472,985477,985482,985500,985514,985606,985669,986453,986485,986492,987887-987888,987893,988319,990541,990568,990572,990600,990759,992899,992911,993127,993141,994956,995478,995507,995603,998858,999098,1001413,1001417,1004291,1022668,1022670,1022676,1022715,1022719,1025660,1025672,1027193,1027203,1027206,1027214,1027227,1028077,1028092,1028094,1028104,1028107,1028111,1028354,1029038,1029042-1029043,1029055,1029078,1029080,1029090,1029092-1029093,1029111,1029151,1029158,1029232,1029335,1029340,1029342,1029344,1030763,1030827,1031203,1031235,1032285,1032333,1033040,1033057,1033294,1035869,1039511,1043705,1053735,1056015,1066452,1067683
/subversion/branches/py-tests-as-modules:956579-1033052
/subversion/branches/ra_serf-digest-authn:875693-876404
/subversion/branches/reintegrate-improvements:873853-874164
Modified: subversion/branches/integrate-stream-api-extensions/subversion/include/svn_io.h
URL: http://svn.apache.org/viewvc/subversion/branches/integrate-stream-api-extensions/subversion/include/svn_io.h?rev=1068695&r1=1068694&r2=1068695&view=diff
==============================================================================
--- subversion/branches/integrate-stream-api-extensions/subversion/include/svn_io.h (original)
+++ subversion/branches/integrate-stream-api-extensions/subversion/include/svn_io.h Wed Feb 9 00:01:04 2011
@@ -749,6 +749,12 @@ typedef svn_error_t *(*svn_read_fn_t)(vo
char *buffer,
apr_size_t *len);
+/** Skip data handler function for a generic stream. @see svn_stream_t.
+ * @since New in 1.7.
+ */
+typedef svn_error_t *(*svn_skip_fn_t)(void *baton,
+ apr_size_t *count);
+
/** Write handler function for a generic stream. @see svn_stream_t. */
typedef svn_error_t *(*svn_write_fn_t)(void *baton,
const char *data,
@@ -783,6 +789,13 @@ typedef svn_error_t *(*svn_io_mark_fn_t)
typedef svn_error_t *(*svn_io_seek_fn_t)(void *baton,
const svn_stream_mark_t *mark);
+/** Buffer test handler function for a generic stream. @see svn_stream_t
+ * and svn_stream_buffered().
+ *
+ * @since New in 1.7.
+ */
+typedef svn_boolean_t (*svn_io_buffered_fn_t)(void *baton);
+
/** Create a generic stream. @see svn_stream_t. */
svn_stream_t *
svn_stream_create(void *baton,
@@ -798,6 +811,14 @@ void
svn_stream_set_read(svn_stream_t *stream,
svn_read_fn_t read_fn);
+/** Set @a stream's skip function to @a skip_fn
+ *
+ * @since New in 1.7
+ */
+void
+svn_stream_set_skip(svn_stream_t *stream,
+ svn_skip_fn_t skip_fn);
+
/** Set @a stream's write function to @a write_fn */
void
svn_stream_set_write(svn_stream_t *stream,
@@ -824,6 +845,14 @@ void
svn_stream_set_seek(svn_stream_t *stream,
svn_io_seek_fn_t seek_fn);
+/** Set @a stream's buffer test function to @a buffered_fn
+ *
+ * @since New in 1.7.
+ */
+void
+svn_stream_set_buffered(svn_stream_t *stream,
+ svn_io_buffered_fn_t buffered_fn);
+
/** Create a stream that is empty for reading and infinite for writing. */
svn_stream_t *
svn_stream_empty(apr_pool_t *pool);
@@ -1018,6 +1047,11 @@ svn_stream_read(svn_stream_t *stream,
char *buffer,
apr_size_t *len);
+/** Skip data from a generic stream. @see svn_stream_t. */
+svn_error_t *
+svn_stream_skip(svn_stream_t *stream,
+ apr_size_t *count);
+
/** Write to a generic stream. @see svn_stream_t. */
svn_error_t *
svn_stream_write(svn_stream_t *stream,
@@ -1038,6 +1072,14 @@ svn_stream_close(svn_stream_t *stream);
svn_error_t *
svn_stream_reset(svn_stream_t *stream);
+/** Returns @c TRUE if the generic @a stream supports svn_stream_mark().
+ *
+ * @see svn_stream_mark()
+ * @since New in 1.7.
+ */
+svn_boolean_t
+svn_stream_supports_mark(svn_stream_t *stream);
+
/** Set a @a mark at the current position of a generic @a stream,
* which can later be sought back to using svn_stream_seek().
* The @a mark is allocated in @a pool.
@@ -1064,6 +1106,15 @@ svn_stream_mark(svn_stream_t *stream,
svn_error_t *
svn_stream_seek(svn_stream_t *stream, const svn_stream_mark_t *mark);
+/** Return whether this generic @a stream uses internal buffering.
+ * This may be used to work around subtle differences between buffered
+ * an non-buffered APR files.
+ *
+ * @since New in 1.7.
+ */
+svn_boolean_t
+svn_stream_buffered(svn_stream_t *stream);
+
/** Return a writable stream which, when written to, writes to both of the
* underlying streams. Both of these streams will be closed upon closure of
* the returned stream; use svn_stream_disown() if this is not the desired
Modified: subversion/branches/integrate-stream-api-extensions/subversion/libsvn_subr/stream.c
URL: http://svn.apache.org/viewvc/subversion/branches/integrate-stream-api-extensions/subversion/libsvn_subr/stream.c?rev=1068695&r1=1068694&r2=1068695&view=diff
==============================================================================
--- subversion/branches/integrate-stream-api-extensions/subversion/libsvn_subr/stream.c (original)
+++ subversion/branches/integrate-stream-api-extensions/subversion/libsvn_subr/stream.c Wed Feb 9 00:01:04 2011
@@ -49,9 +49,11 @@ struct svn_stream_t {
void *baton;
svn_read_fn_t read_fn;
svn_write_fn_t write_fn;
+ svn_skip_fn_t skip_fn;
svn_close_fn_t close_fn;
svn_io_mark_fn_t mark_fn;
svn_io_seek_fn_t seek_fn;
+ svn_io_buffered_fn_t buffered_fn;
};
@@ -65,10 +67,12 @@ svn_stream_create(void *baton, apr_pool_
stream = apr_palloc(pool, sizeof(*stream));
stream->baton = baton;
stream->read_fn = NULL;
+ stream->skip_fn = NULL;
stream->write_fn = NULL;
stream->close_fn = NULL;
stream->mark_fn = NULL;
stream->seek_fn = NULL;
+ stream->buffered_fn = NULL;
return stream;
}
@@ -86,6 +90,11 @@ svn_stream_set_read(svn_stream_t *stream
stream->read_fn = read_fn;
}
+void
+svn_stream_set_skip(svn_stream_t *stream, svn_skip_fn_t skip_fn)
+{
+ stream->skip_fn = skip_fn;
+}
void
svn_stream_set_write(svn_stream_t *stream, svn_write_fn_t write_fn)
@@ -111,6 +120,13 @@ svn_stream_set_seek(svn_stream_t *stream
stream->seek_fn = seek_fn;
}
+void
+svn_stream_set_buffered(svn_stream_t *stream,
+ svn_io_buffered_fn_t buffered_fn)
+{
+ stream->buffered_fn = buffered_fn;
+}
+
svn_error_t *
svn_stream_read(svn_stream_t *stream, char *buffer, apr_size_t *len)
{
@@ -120,6 +136,14 @@ svn_stream_read(svn_stream_t *stream, ch
svn_error_t *
+svn_stream_skip(svn_stream_t *stream, apr_size_t *count)
+{
+ SVN_ERR_ASSERT(stream->skip_fn != NULL);
+ return stream->skip_fn(stream->baton, count);
+}
+
+
+svn_error_t *
svn_stream_write(svn_stream_t *stream, const char *data, apr_size_t *len)
{
SVN_ERR_ASSERT(stream->write_fn != NULL);
@@ -134,6 +158,12 @@ svn_stream_reset(svn_stream_t *stream)
svn_stream_seek(stream, NULL));
}
+svn_boolean_t
+svn_stream_supports_mark(svn_stream_t *stream)
+{
+ return stream->mark_fn == NULL ? FALSE : TRUE;
+}
+
svn_error_t *
svn_stream_mark(svn_stream_t *stream, svn_stream_mark_t **mark,
apr_pool_t *pool)
@@ -153,6 +183,15 @@ svn_stream_seek(svn_stream_t *stream, co
return stream->seek_fn(stream->baton, mark);
}
+svn_boolean_t
+svn_stream_buffered(svn_stream_t *stream)
+{
+ if (stream->buffered_fn == NULL)
+ return FALSE;
+
+ return stream->buffered_fn(stream->baton);
+}
+
svn_error_t *
svn_stream_close(svn_stream_t *stream)
{
@@ -406,6 +445,29 @@ svn_stream_contents_same2(svn_boolean_t
svn_stream_close(stream2)));
}
+
+/*** Stream implementation utilities ***/
+
+/* 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;
+ err = read_fn(baton, buffer, &bytes_read);
+ total_bytes_read += bytes_read;
+ }
+
+ *count = total_bytes_read;
+ return err;
+}
+
/*** Generic readable empty stream ***/
@@ -417,6 +479,12 @@ read_handler_empty(void *baton, char *bu
return SVN_NO_ERROR;
}
+static svn_error_t *
+skip_handler_empty(void *baton, apr_size_t *count)
+{
+ *count = 0;
+ return SVN_NO_ERROR;
+}
static svn_error_t *
write_handler_empty(void *baton, const char *data, apr_size_t *len)
@@ -437,6 +505,12 @@ seek_handler_empty(void *baton, const sv
return SVN_NO_ERROR;
}
+static svn_boolean_t
+buffered_handler_empty(void *baton)
+{
+ return FALSE;
+}
+
svn_stream_t *
svn_stream_empty(apr_pool_t *pool)
@@ -445,9 +519,11 @@ svn_stream_empty(apr_pool_t *pool)
stream = svn_stream_create(NULL, pool);
svn_stream_set_read(stream, read_handler_empty);
+ svn_stream_set_skip(stream, skip_handler_empty);
svn_stream_set_write(stream, write_handler_empty);
svn_stream_set_mark(stream, mark_handler_empty);
svn_stream_set_seek(stream, seek_handler_empty);
+ svn_stream_set_buffered(stream, buffered_handler_empty);
return stream;
}
@@ -519,6 +595,12 @@ read_handler_disown(void *baton, char *b
}
static svn_error_t *
+skip_handler_disown(void *baton, apr_size_t *count)
+{
+ return svn_stream_skip(baton, count);
+}
+
+static svn_error_t *
write_handler_disown(void *baton, const char *buffer, apr_size_t *len)
{
return svn_stream_write(baton, buffer, len);
@@ -536,15 +618,23 @@ seek_handler_disown(void *baton, const s
return svn_stream_seek(baton, mark);
}
+static svn_boolean_t
+buffered_handler_disown(void *baton)
+{
+ return svn_stream_buffered(baton);
+}
+
svn_stream_t *
svn_stream_disown(svn_stream_t *stream, apr_pool_t *pool)
{
svn_stream_t *s = svn_stream_create(stream, pool);
svn_stream_set_read(s, read_handler_disown);
+ svn_stream_set_skip(s, skip_handler_disown);
svn_stream_set_write(s, write_handler_disown);
svn_stream_set_mark(s, mark_handler_disown);
svn_stream_set_seek(s, seek_handler_disown);
+ svn_stream_set_buffered(s, buffered_handler_disown);
return s;
}
@@ -579,6 +669,18 @@ read_handler_apr(void *baton, char *buff
}
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));
+ *count = offset;
+
+ return SVN_NO_ERROR;
+}
+
+static svn_error_t *
write_handler_apr(void *baton, const char *data, apr_size_t *len)
{
struct baton_apr *btn = baton;
@@ -618,6 +720,13 @@ seek_handler_apr(void *baton, const svn_
return SVN_NO_ERROR;
}
+static svn_boolean_t
+buffered_handler_apr(void *baton)
+{
+ struct baton_apr *btn = baton;
+ return apr_file_buffer_size_get(btn->file) != 0;
+}
+
svn_error_t *
svn_stream_open_readonly(svn_stream_t **stream,
const char *path,
@@ -690,8 +799,10 @@ svn_stream_from_aprfile2(apr_file_t *fil
stream = svn_stream_create(baton, pool);
svn_stream_set_read(stream, read_handler_apr);
svn_stream_set_write(stream, write_handler_apr);
+ svn_stream_set_skip(stream, skip_range_handler_apr);
svn_stream_set_mark(stream, mark_handler_apr);
svn_stream_set_seek(stream, seek_handler_apr);
+ svn_stream_set_buffered(stream, buffered_handler_apr);
if (! disown)
svn_stream_set_close(stream, close_handler_apr);
@@ -866,6 +977,13 @@ read_handler_gz(void *baton, char *buffe
return SVN_NO_ERROR;
}
+/* Skip data from a compressed stream by reading and discarding it. */
+static svn_error_t *
+skip_handler_gz(void *baton, apr_size_t *count)
+{
+ return skip_default_handler(baton, count, read_handler_gz);
+}
+
/* Compress data and write it to the substream */
static svn_error_t *
write_handler_gz(void *baton, const char *buffer, apr_size_t *len)
@@ -979,6 +1097,7 @@ svn_stream_compressed(svn_stream_t *stre
zstream = svn_stream_create(baton, pool);
svn_stream_set_read(zstream, read_handler_gz);
+ svn_stream_set_skip(zstream, skip_handler_gz);
svn_stream_set_write(zstream, write_handler_gz);
svn_stream_set_close(zstream, close_handler_gz);
@@ -1021,6 +1140,13 @@ read_handler_checksum(void *baton, char
static svn_error_t *
+skip_handler_checksum(void *baton, apr_size_t *count)
+{
+ return skip_default_handler(baton, count, read_handler_checksum);
+}
+
+
+static svn_error_t *
write_handler_checksum(void *baton, const char *buffer, apr_size_t *len)
{
struct checksum_stream_baton *btn = baton;
@@ -1094,6 +1220,7 @@ svn_stream_checksummed2(svn_stream_t *st
s = svn_stream_create(baton, pool);
svn_stream_set_read(s, read_handler_checksum);
+ svn_stream_set_skip(s, skip_handler_checksum);
svn_stream_set_write(s, write_handler_checksum);
svn_stream_set_close(s, close_handler_checksum);
return s;
@@ -1117,6 +1244,13 @@ read_handler_md5(void *baton, char *buff
}
static svn_error_t *
+skip_handler_md5(void *baton, apr_size_t *count)
+{
+ struct md5_stream_baton *btn = baton;
+ return svn_stream_skip(btn->proxy, count);
+}
+
+static svn_error_t *
write_handler_md5(void *baton, const char *buffer, apr_size_t *len)
{
struct md5_stream_baton *btn = baton;
@@ -1176,6 +1310,7 @@ svn_stream_checksummed(svn_stream_t *str
* want them) after it closes BATON->proxy. */
s = svn_stream_create(baton, pool);
svn_stream_set_read(s, read_handler_md5);
+ svn_stream_set_skip(s, skip_handler_md5);
svn_stream_set_write(s, write_handler_md5);
svn_stream_set_close(s, close_handler_md5);
return s;
@@ -1209,6 +1344,17 @@ read_handler_stringbuf(void *baton, char
}
static svn_error_t *
+skip_handler_stringbuf(void *baton, apr_size_t *count)
+{
+ struct stringbuf_stream_baton *btn = baton;
+ apr_size_t left_to_read = btn->str->len - btn->amt_read;
+
+ *count = (*count > left_to_read) ? left_to_read : *count;
+ btn->amt_read += *count;
+ return SVN_NO_ERROR;
+}
+
+static svn_error_t *
write_handler_stringbuf(void *baton, const char *data, apr_size_t *len)
{
struct stringbuf_stream_baton *btn = baton;
@@ -1249,6 +1395,12 @@ seek_handler_stringbuf(void *baton, cons
return SVN_NO_ERROR;
}
+static svn_boolean_t
+buffered_handler_stringbuf(void *baton)
+{
+ return TRUE;
+}
+
svn_stream_t *
svn_stream_from_stringbuf(svn_stringbuf_t *str,
apr_pool_t *pool)
@@ -1264,9 +1416,11 @@ svn_stream_from_stringbuf(svn_stringbuf_
baton->amt_read = 0;
stream = svn_stream_create(baton, pool);
svn_stream_set_read(stream, read_handler_stringbuf);
+ svn_stream_set_skip(stream, skip_handler_stringbuf);
svn_stream_set_write(stream, write_handler_stringbuf);
svn_stream_set_mark(stream, mark_handler_stringbuf);
svn_stream_set_seek(stream, seek_handler_stringbuf);
+ svn_stream_set_buffered(stream, buffered_handler_stringbuf);
return stream;
}
@@ -1325,6 +1479,23 @@ seek_handler_string(void *baton, const s
return SVN_NO_ERROR;
}
+static svn_error_t *
+skip_handler_string(void *baton, apr_size_t *count)
+{
+ struct string_stream_baton *btn = baton;
+ apr_size_t left_to_read = btn->str->len - btn->amt_read;
+
+ *count = (*count > left_to_read) ? left_to_read : *count;
+ btn->amt_read += *count;
+ return SVN_NO_ERROR;
+}
+
+static svn_boolean_t
+buffered_handler_string(void *baton)
+{
+ return TRUE;
+}
+
svn_stream_t *
svn_stream_from_string(const svn_string_t *str,
apr_pool_t *pool)
@@ -1342,6 +1513,8 @@ svn_stream_from_string(const svn_string_
svn_stream_set_read(stream, read_handler_string);
svn_stream_set_mark(stream, mark_handler_string);
svn_stream_set_seek(stream, seek_handler_string);
+ svn_stream_set_skip(stream, skip_handler_string);
+ svn_stream_set_buffered(stream, buffered_handler_string);
return stream;
}
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;
+ err = translated_stream_read(baton, buffer, &bytes_read);
+ total_bytes_read += bytes_read;
+ }
+
+ *count = total_bytes_read;
+ return err;
+}
+
/* Implements svn_write_fn_t. */
static svn_error_t *
translated_stream_write(void *baton,
@@ -1360,6 +1381,14 @@ translated_stream_seek(void *baton, cons
return SVN_NO_ERROR;
}
+/* Implements svn_io_buffered_fn_t. */
+static svn_boolean_t
+translated_stream_buffered(void *baton)
+{
+ struct translated_stream_baton *b = baton;
+ return svn_stream_buffered(b->stream);
+}
+
svn_error_t *
svn_subst_read_specialfile(svn_stream_t **stream,
const char *path,
@@ -1465,10 +1494,12 @@ stream_translated(svn_stream_t *stream,
/* Setup the stream methods */
svn_stream_set_read(s, translated_stream_read);
+ svn_stream_set_skip(s, translated_stream_skip);
svn_stream_set_write(s, translated_stream_write);
svn_stream_set_close(s, translated_stream_close);
svn_stream_set_mark(s, translated_stream_mark);
svn_stream_set_seek(s, translated_stream_seek);
+ svn_stream_set_buffered(s, translated_stream_buffered);
return s;
}
Re: svn commit: r1068695 - in /subversion/branches/integrate-stream-api-extensions:
./ subversion/include/svn_io.h subversion/libsvn_subr/stream.c subversion/libsvn_subr/subst.c
Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
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@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
>
Re: svn commit: r1068695 - in /subversion/branches/integrate-stream-api-extensions:
./ subversion/include/svn_io.h subversion/libsvn_subr/stream.c subversion/libsvn_subr/subst.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Thu, Feb 10, 2011 at 1:02 AM, Stefan Fuhrmann <eq...@web.de> wrote:
> On 09.02.2011 02:07, Blair Zajac wrote:
...
>> We have systems with APR 1.2 and it doesn't provide
>> apr_file_buffer_size_get().
>
> Is there anything in the 1.3 headers that would indicate
> in which version a specific function got introduced?
We currently maintain official compatibility with APR 0.9, so if it
ain't in those docs[1], we can't directly use it. (We can either
conditionally recreate a macro, or provide our own implementation
which calls the APR one if it exists.)
-Hyrum
[1] http://apr.apache.org/docs/apr/0.9/
Re: svn commit: r1068695 - in /subversion/branches/integrate-stream-api-extensions:
./ subversion/include/svn_io.h subversion/libsvn_subr/stream.c subversion/libsvn_subr/subst.c
Posted by Stefan Fuhrmann <eq...@web.de>.
On 09.02.2011 02:07, Blair Zajac wrote:
> On 2/8/11 4:01 PM, stefan2@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?
"consume" would be false as well since it is supposed to
use seek() or something similarly efficient when supported
by the respective stream. "advance" might be an alternative
but that would be more suitable for iterators / marks.
Therefore, I stick with "skip" for the time being and added
a detailed docstring to the function declaration.
>
>> +
>> +/* 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.
Yikes!
The problem is that I used this function for files and strings
only. Both provide specialized implementations.
>
> Sounds like a unit test for skipping is needed.
Definitely. Will provide tests tomorrow or so.
>
>> 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.
Fixed.
>
>> +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().
Is there anything in the 1.3 headers that would indicate
in which version a specific function got introduced?
>
>
>> 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.
Thanks again for the in-depth review.
-- Stefan^2.
Re: svn commit: r1068695 - in /subversion/branches/integrate-stream-api-extensions:
./ subversion/include/svn_io.h subversion/libsvn_subr/stream.c subversion/libsvn_subr/subst.c
Posted by Blair Zajac <bl...@orcaware.com>.
On 2/8/11 4:01 PM, stefan2@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