You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <da...@elego.de> on 2011/09/22 04:53:03 UTC
[PATCH] svn_spillbuf__is_empty()
[[[
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.
* subversion/tests/libsvn_subr/spillbuf-test.c
(test_spillbuf_interleaving): Test svn_spillbuf_is_empty() a bit more.
(The last of the new calls failed before this patch.)
]]]
[[[
Index: subversion/include/private/svn_subr_private.h
===================================================================
--- subversion/include/private/svn_subr_private.h (revision 1173935)
+++ subversion/include/private/svn_subr_private.h (working copy)
@@ -94,13 +94,7 @@ svn_spillbuf__create(apr_size_t blocksize,
apr_pool_t *result_pool);
-/* Determine whether the spill buffer has any content.
-
- Note: there is an edge case for a false positive. If the spill file was
- read *just* to the end of the file, but not past... then the spill
- buffer will not realize that no further content exists in the spill file.
- In this situation, svn_spillbuf_is_empty() will return TRUE, but an
- attempt to read content will detect that it has been exhausted. */
+/* Determine whether the spill buffer has any content. */
svn_boolean_t
svn_spillbuf__is_empty(const svn_spillbuf_t *buf);
Index: subversion/libsvn_subr/spillbuf.c
===================================================================
--- subversion/libsvn_subr/spillbuf.c (revision 1173932)
+++ subversion/libsvn_subr/spillbuf.c (working copy)
@@ -285,7 +285,8 @@ read_data(struct memblock_t **mem,
/* Read some data from the spill file into the memblock. */
err = svn_io_file_read(buf->spill, (*mem)->data, &(*mem)->size,
scratch_pool);
- if (err != NULL && APR_STATUS_IS_EOF(err->apr_err))
+ if ((err != NULL && APR_STATUS_IS_EOF(err->apr_err))
+ || apr_file_eof(buf->spill) == APR_EOF)
{
/* We've exhausted the file. Close it, so any new content will go
into memory rather than the file. */
Index: subversion/tests/libsvn_subr/spillbuf-test.c
===================================================================
--- subversion/tests/libsvn_subr/spillbuf-test.c (revision 1173932)
+++ 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 */
+ 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));
return SVN_NO_ERROR;
}
]]]
Re: [PATCH] svn_spillbuf__is_empty()
Posted by Daniel Shahaf <da...@elego.de>.
If this is applied then it should probably get another hunk:
[[[
Index: subversion/libsvn_subr/spillbuf.c
===================================================================
--- subversion/libsvn_subr/spillbuf.c (revision 1173942)
+++ subversion/libsvn_subr/spillbuf.c (working copy)
@@ -299,15 +299,6 @@ read_data(struct memblock_t **mem,
return svn_error_trace(err);
}
- /* If we didn't read anything from the file, then avoid returning a
- memblock (ie. just like running out of content). */
- if ((*mem)->size == 0)
- {
- return_buffer(buf, *mem);
- *mem = NULL;
- return SVN_NO_ERROR;
- }
-
/* Mark the data that we consumed from the spill file. */
buf->spill_start += (*mem)->size;
]]]
but I haven't tested that yet.
Daniel Shahaf wrote on Thu, Sep 22, 2011 at 05:53:03 +0300:
> [[[
> 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.
>
> * subversion/tests/libsvn_subr/spillbuf-test.c
> (test_spillbuf_interleaving): Test svn_spillbuf_is_empty() a bit more.
> (The last of the new calls failed before this patch.)
> ]]]
>
> [[[
> Index: subversion/include/private/svn_subr_private.h
> ===================================================================
> --- subversion/include/private/svn_subr_private.h (revision 1173935)
> +++ subversion/include/private/svn_subr_private.h (working copy)
> @@ -94,13 +94,7 @@ svn_spillbuf__create(apr_size_t blocksize,
> apr_pool_t *result_pool);
>
>
> -/* Determine whether the spill buffer has any content.
> -
> - Note: there is an edge case for a false positive. If the spill file was
> - read *just* to the end of the file, but not past... then the spill
> - buffer will not realize that no further content exists in the spill file.
> - In this situation, svn_spillbuf_is_empty() will return TRUE, but an
> - attempt to read content will detect that it has been exhausted. */
> +/* Determine whether the spill buffer has any content. */
> svn_boolean_t
> svn_spillbuf__is_empty(const svn_spillbuf_t *buf);
>
> Index: subversion/libsvn_subr/spillbuf.c
> ===================================================================
> --- subversion/libsvn_subr/spillbuf.c (revision 1173932)
> +++ subversion/libsvn_subr/spillbuf.c (working copy)
> @@ -285,7 +285,8 @@ read_data(struct memblock_t **mem,
> /* Read some data from the spill file into the memblock. */
> err = svn_io_file_read(buf->spill, (*mem)->data, &(*mem)->size,
> scratch_pool);
> - if (err != NULL && APR_STATUS_IS_EOF(err->apr_err))
> + if ((err != NULL && APR_STATUS_IS_EOF(err->apr_err))
> + || apr_file_eof(buf->spill) == APR_EOF)
> {
> /* We've exhausted the file. Close it, so any new content will go
> into memory rather than the file. */
> Index: subversion/tests/libsvn_subr/spillbuf-test.c
> ===================================================================
> --- subversion/tests/libsvn_subr/spillbuf-test.c (revision 1173932)
> +++ 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 */
>
> + 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));
>
> return SVN_NO_ERROR;
> }
> ]]]
Re: [PATCH] svn_spillbuf__is_empty()
Posted by Daniel Shahaf <da...@elego.de>.
Greg Stein wrote on Thu, Sep 22, 2011 at 06:12:58 -0400:
> On Wed, Sep 21, 2011 at 22:53, Daniel Shahaf <da...@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
Re: [PATCH] svn_spillbuf__is_empty()
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Sep 21, 2011 at 22:53, Daniel Shahaf <da...@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.
>...
> +++ 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.
Cheers,
-g