[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] svn_spillbuf__is_empty()

From: Daniel Shahaf <danielsh_at_elego.de>
Date: Thu, 22 Sep 2011 15:21:37 +0300

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

This is an archived mail posted to the Subversion Dev mailing list.