Re: svn commit: r1381800 - /subversion/trunk/subversion/libsvn_subr/io.c

On Fri, Sep 7, 2012 at 9:18 PM, Julian Foad <julianfoad_at_btopenworld.com>wrote:

> Paul Burba wrote:
>
> > On Thu, Sep 6, 2012 at 7:32 PM, <stefan2_at_apache.org> 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 23:32:11
> 2012
> >> @@ -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 */
> >> + 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;
> >> +
> >> + *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?
>

Thanks for the report, Paul!
Having no buildbots when one needs them is a nuisance.

> Yes, I'm seeing a failure with the same error message, on my Ubuntu Linux
> system.
>

Thanks for testing. I had no issues on 64 bit Ubuntu.
=> the issue must have been a 32 bit specific one.

>
> > 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
> [...]
>

Fixed in r1382196.

-- Stefan^2.

-- 
*
Join us this October at Subversion Live
2012<http://www.wandisco.com/svn-live-2012>
 for two days of best practice SVN training, networking, live demos,
committer meet and greet, and more! Space is limited, so get signed up
today<http://www.wandisco.com/svn-live-2012>
!
*