Greg Stein wrote on Thu, Sep 22, 2011 at 06:12:58 -0400:
> On Wed, Sep 21, 2011 at 22:53, Daniel Shahaf <danielsh_at_elego.de> wrote:
> > [[[
> > Remove an edge case from svn_spillbuf__is_empty().
> >
> > * subversion/include/private/svn_subr_private.h
> > (svn_spillbuf_is_empty): Stop documenting this bug in the doc string.
> >
> > * subversion/libsvn_subr/spillbuf.c
> > (read_data): Also check for EOF in the non-error code path.
>
> I don't think that apr_file_eof() can predictively determine EOF
> unless/until you try and read off the end of the file. It is quite
> conceivable to arrange a situation where we read precisely the number
> of bytes in the file (ie. the remaining amount matches blocksize).
> Looking apr_file_eof(), it would return that data without setting
> thefile->eof_hit.
>
I see what you mean.
Two things here:
- Adding the check does not make the situation any worse than it already
is; it reduces the chance for a false negative, even though, as you
say, it does not eliminate it.
- I'm going to claim it's an APR bug :-). Your new API is specifically
documented as not detecting EOF if the reads aligned with the file's
size, but apr_file_eof() doesn't have that limitation documented.
Therefore, either it should DTRT or it should grow that limitation too
in its docstring.
> >...
> > +++ subversion/tests/libsvn_subr/spillbuf-test.c (working copy)
> > @@ -224,18 +224,22 @@ test_spillbuf_interleaving(apr_pool_t *pool)
> > SVN_ERR(svn_spillbuf__write(buf, "GHIJKL", 6, pool));
> > /* now: two blocks of 8 and 6 bytes, and 6 bytes spilled to a file */
>
> By writing "GHIJKLMN" here, that would drop 8 bytes into the spill file...
>
> >
> > + SVN_TEST_ASSERT(! svn_spillbuf__is_empty(buf));
> > SVN_ERR(svn_spillbuf__read(&readptr, &readlen, buf, pool));
> > SVN_TEST_ASSERT(readptr != NULL
> > && readlen == 8
> > && memcmp(readptr, "qrstuvwx", 8) == 0);
> > + SVN_TEST_ASSERT(! svn_spillbuf__is_empty(buf));
> > SVN_ERR(svn_spillbuf__read(&readptr, &readlen, buf, pool));
> > SVN_TEST_ASSERT(readptr != NULL
> > && readlen == 6
> > && memcmp(readptr, "ABCDEF", 6) == 0);
> > + SVN_TEST_ASSERT(! svn_spillbuf__is_empty(buf));
> > SVN_ERR(svn_spillbuf__read(&readptr, &readlen, buf, pool));
> > SVN_TEST_ASSERT(readptr != NULL
> > && readlen == 6
> > && memcmp(readptr, "GHIJKL", 6) == 0);
> > + SVN_TEST_ASSERT(svn_spillbuf__is_empty(buf));
>
> ... and I would guess this read would return 8 bytes and not be marked as empty.
>
For whatever reason the test still passes for me when I make that
change. I agree with your analysis, though.
> Cheers,
> -g
Received on 2011-09-22 14:22:29 CEST