On Fri, Sep 7, 2012 at 3:29 PM, Julian Foad <[email protected]> wrote:
> (Just forwarding this thread to Stefan.)
>
>
> - Julian
>
>
> I (Julian Foad) wrote:
>
>> Paul Burba wrote:
>>
>>> On Thu, Sep 6, 2012 at 7:32 PM, <[email protected]> wrote:
>>>> URL: http://svn.apache.org/viewvc?rev=1381800&view=rev
>>>> Log:
>>>> Re-implement svn_io_read_length_line as this is one of the
>>>> most-called functions in SVN. Instead of reading data a byte
>>>> at a time, read 128 byte chunks and scan those.
>> [...]
>>>> ==============================================================================
>>>> --- subversion/trunk/subversion/libsvn_subr/io.c (original)
>>>> +++ subversion/trunk/subversion/libsvn_subr/io.c Thu Sep 6
>>>> @@ -3427,30 +3427,60 @@ svn_error_t *
>>>> svn_io_read_length_line(apr_file_t *file, char *buf, apr_size_t *limit,
>>>> apr_pool_t *pool)
>>>> {
>>>> + /* variables */
>
> p.s. Redundant comment there.
>
>>>> + apr_size_t total_read = 0;
>>>> + svn_boolean_t eof = FALSE;
>>>> const char *name;
>>>> svn_error_t *err;
>>>> - apr_size_t i;
>>>> - char c;
>>>> + apr_size_t buf_size = *limit;
>>>>
>>>> - for (i = 0; i < *limit; i++)
>>>> + while (buf_size > 0)
>>>> {
>>>> - SVN_ERR(svn_io_file_getc(&c, file, pool));
>>>> - /* Note: this error could be APR_EOF, which
>>>> - is totally fine. The caller should be aware of
>>>> - this. */
>>>> -
>>>> - if (c == '\n')
>>>> + /* read a fair chunk of data at once. But don't get too ambitious
>>>> + * as that would result in too much waste. Also make sure we can
>>>> + * put a NUL after the last byte read.
>>>> + */
>>>> + apr_size_t to_read = buf_size < 129 ? buf_size - 1 : 128;
>>>> + apr_size_t bytes_read = 0;
>>>> + char *eol;
>>>> +
>>>> + /* read data block (or just a part of it) */
>>>> + SVN_ERR(svn_io_file_read_full2(file, buf, to_read,
>>>> + &bytes_read, &eof, pool));
>>>> +
>>>> + /* look or a newline char */
>>>> + buf[bytes_read] = 0;
>>>> + eol = strchr(buf, '\n');
>>>> + if (eol)
>>>> {
>>>> - buf[i] = '\0';
>>>> - *limit = i;
>>>> + apr_off_t offset = (eol + 1 - buf) - bytes_read;
Here is the problem: apr_off_t is unsigned and in some cases we are
trying to calculate a negative offset to pass to svn_io_file_seek.
The attached patch fixes this and all tests pass with it, but I'd
appreciate another set of eyes before committing.
[[[
Fix file seek breakage introduced in r1381800.
See also discussion at http://svn.haxx.se/dev/archive-2012-09/0114.shtml
* subversion/libsvn_subr/io.c
(svn_io_read_length_line): Move the read/write file offset to an
absolute offset rather than trying to applying a negative offset to
the current offset, thus preventing the mayhem that comes with
assigning a negative offset value to an unsigned apr_off_t.
]]]
--
Paul T. Burba
CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
Skype: ptburba
>>>> + *eol = 0;
>>>> + *limit = total_read + (eol - buf);
>>>> +
>>>> + /* correct the file pointer:
>>>> + * appear as though we just had read the newline char
>>>> + */
>>>> + SVN_ERR(svn_io_file_seek(file, APR_CUR, &offset, pool));
>>>> +
>>>> return SVN_NO_ERROR;
>>>> }
>>>> - else
>>>> + else if (eof)
>>>> {
>>>> - buf[i] = c;
>>>> + /* no EOL found but we hit the end of the file.
>>>> + * Generate a nice EOF error object and return it.
>>>> + */
>>>> + char dummy;
>>>> + SVN_ERR(svn_io_file_getc(&dummy, file, pool));
>>>> }
>>>> +
>>>> + /* next data chunk */
>>>> + buf_size -= bytes_read;
>>>> + buf += bytes_read;
>>>> + total_read += bytes_read;
>>>> }
>>>>
>>>> + /* buffer limit has been exceeded without finding the EOL */
>>>> err = svn_io_file_name_get(&name, file, pool);
>>>> if (err)
>>>> name = NULL;
>>>>
>>>>
>>>
>>> Anybody else seeing this?
>>
>> Yes, I'm seeing a failure with the same error message, on my Ubuntu Linux
>> system.
>>
>> - Julian
>>
>>
>>> I haven't figured out why yet, but r1381800 is causing failures on my
>>> Windows box, all similar to this:
>> [...]
>>> I: svn: E070014: Can't read file
>>>
>> 'C:\SVN\src-trunk\Debug\subversion\tests\cmdline\svn-test-work\local_tmp\repos\db\revs\0\0':
>>> End of file found
>> [...]
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 1382102)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -3443,7 +3443,11 @@
apr_size_t to_read = buf_size < 129 ? buf_size - 1 : 128;
apr_size_t bytes_read = 0;
char *eol;
+ apr_off_t starting_offset = 0;
+ /* Remember the starting offset. */
+ SVN_ERR(svn_io_file_seek(file, APR_CUR, &starting_offset, pool));
+
/* read data block (or just a part of it) */
SVN_ERR(svn_io_file_read_full2(file, buf, to_read,
&bytes_read, &eof, pool));
@@ -3453,16 +3457,15 @@
eol = strchr(buf, '\n');
if (eol)
{
- apr_off_t offset = (eol + 1 - buf) - bytes_read;
-
+ apr_off_t offset = starting_offset + eol + 1 - buf;
+
*eol = 0;
*limit = total_read + (eol - buf);
/* correct the file pointer:
* appear as though we just had read the newline char
*/
- SVN_ERR(svn_io_file_seek(file, APR_CUR, &offset, pool));
-
+ SVN_ERR(svn_io_file_seek(file, APR_SET, &offset, pool));
return SVN_NO_ERROR;
}
else if (eof)
