2010/9/10 Hyrum Wright <hwri...@apache.org>:
> On Thu, Sep 9, 2010 at 4:34 PM, Stefan Fuhrmann
> <stefanfuhrm...@alice-dsl.de> wrote:
>> Hyrum Wright wrote:
>>>> *identical_p = TRUE; /* assume TRUE, until disproved below */
>>>> - do
>>>> + while (! (done1 || done2))
>>>> {
>>>> - SVN_ERR(svn_io_file_read_full2(file1_h, buf1,
>>>> - SVN__STREAM_CHUNK_SIZE,
>>>> &bytes_read1,
>>>> - TRUE, pool));
>>>> - SVN_ERR(svn_io_file_read_full2(file2_h, buf2,
>>>> - SVN__STREAM_CHUNK_SIZE,
>>>> &bytes_read2,
>>>> - TRUE, pool));
>>>> + err = svn_io_file_read_full(file1_h, buf1,
>>>> + SVN__STREAM_CHUNK_SIZE, &bytes_read1,
>>>> pool);
>>>>
>>>
>>> Stefan,
>>> This call, and the one following, needed to revert back to
>>> svn_io_file_read_full(), since read_full2() was being stuck in an
>>> infinite loop. Perhaps you can investigate?
>>>
>>
>> Hm. That doesn't make any sense. As soon as it tries to read beyond EOF,
>> the while (bytes_read1==SVN__STREAM_CHUNK_SIZE) condition
>> should be triggered. Looking through the respective APR code for Windows
>> and Unix, they always seem to set the number of bytes read even if errors
>> were encountered.
>>
>> How did you trigger the infinite loop?
>
> Change those two function calls, and then run 'make check'. It
> stalled infinitely[1], with both cores pegged. Using gdb to attach to
> the running process showed that it was svn_io_file_read_full2(),
> invoked from that location.
>
> -Hyrum
>
> [1] Well, I've no way of knowing if it was infinite or just taking a
> long time, but I'll defer a solution to the Halting Problem as an
> exercise for the reader. :)
>
Hyrum,
you cannot just replace svn_io_file_read_full() with
svn_io_file_read_full2() in i...@995478 without adding additional code
there.
svn_io_file_read_full2() swallows end-of-file, thus done1 and done2 in
that loop will never become TRUE.
I think that it could be written like the following:
[[[
err = svn_io_file_read_full2(file1_h, buf1,
SVN__STREAM_CHUNK_SIZE, &bytes_read1, TRUE, pool);
if (err) {
break;
} else if (bytes_read1 != SVN__STREAM_CHUNK_SIZE) {
done1 = TRUE;
}
]]]
and the same for the second file.
My understanding is that
- you tried to backport rev.993308 from trunk
- the advantage of svn_io_file_read_full2() is that it does not waste
time creating detailed svn_error_t object when EOF is met.
I think that svn_io_file_read_full2() could be rewritten to accept
pointer to a boolean,
like the following: (warning: I did not try to compile or run this)
[[[
* /subversion/libsvn_subr/io.c
(svn_io_file_read_full2): Assume that eof_is_ok argument is always TRUE
and use a pointer to a boolean variable to pass back information about
detected EOF.
(contents_identical_p): use *_read_full2() to minimize EOF detection
overhead, like r985488 did but was reverted in r995478.
Index: io.c
===================================================================
--- io.c (revision 996063)
+++ io.c (working copy)
@@ -2875,12 +2875,14 @@
svn_error_t *
svn_io_file_read_full2(apr_file_t *file, void *buf,
apr_size_t nbytes, apr_size_t *bytes_read,
- svn_boolean_t eof_is_ok,
+ svn_boolean_t* eof_flag,
apr_pool_t *pool)
{
apr_status_t status = apr_file_read_full(file, buf, nbytes, bytes_read);
- if (APR_STATUS_IS_EOF(status) && eof_is_ok)
+ if (APR_STATUS_IS_EOF(status)) {
+ *eof_flag = TRUE;
return SVN_NO_ERROR;
+ }
return do_io_file_wrapper_cleanup
(file, status,
@@ -3607,26 +3609,14 @@
*identical_p = TRUE; /* assume TRUE, until disproved below */
while (! (done1 || done2))
{
- err = svn_io_file_read_full(file1_h, buf1,
- SVN__STREAM_CHUNK_SIZE, &bytes_read1, pool);
- if (err && APR_STATUS_IS_EOF(err->apr_err))
- {
- svn_error_clear(err);
- err = NULL;
- done1 = TRUE;
- }
- else if (err)
+ err = svn_io_file_read_full2(file1_h, buf1,
+ SVN__STREAM_CHUNK_SIZE,
&bytes_read1, &done1, pool);
+ if (err)
break;
- err = svn_io_file_read_full(file2_h, buf2,
- SVN__STREAM_CHUNK_SIZE, &bytes_read2, pool);
- if (err && APR_STATUS_IS_EOF(err->apr_err))
- {
- svn_error_clear(err);
- err = NULL;
- done2 = TRUE;
- }
- else if (err)
+ err = svn_io_file_read_full2(file2_h, buf2,
+ SVN__STREAM_CHUNK_SIZE,
&bytes_read2, &done2, pool);
+ if (err)
break;
if ((bytes_read1 != bytes_read2)
]]]
Best regards,
Konstantin Kolinko
* /subversion/libsvn_subr/io.c
(svn_io_file_read_full2): Assume that eof_is_ok argument is always TRUE
and use a pointer to a boolean variable to pass back information about
detected EOF.
(contents_identical_p): use *_read_full2() to minimize EOF detection
overhead, like r985488 did but was reverted in r995478.
Index: io.c
===================================================================
--- io.c (revision 996063)
+++ io.c (working copy)
@@ -2875,12 +2875,14 @@
svn_error_t *
svn_io_file_read_full2(apr_file_t *file, void *buf,
apr_size_t nbytes, apr_size_t *bytes_read,
- svn_boolean_t eof_is_ok,
+ svn_boolean_t* eof_flag,
apr_pool_t *pool)
{
apr_status_t status = apr_file_read_full(file, buf, nbytes, bytes_read);
- if (APR_STATUS_IS_EOF(status) && eof_is_ok)
+ if (APR_STATUS_IS_EOF(status)) {
+ *eof_flag = TRUE;
return SVN_NO_ERROR;
+ }
return do_io_file_wrapper_cleanup
(file, status,
@@ -3607,26 +3609,14 @@
*identical_p = TRUE; /* assume TRUE, until disproved below */
while (! (done1 || done2))
{
- err = svn_io_file_read_full(file1_h, buf1,
- SVN__STREAM_CHUNK_SIZE, &bytes_read1, pool);
- if (err && APR_STATUS_IS_EOF(err->apr_err))
- {
- svn_error_clear(err);
- err = NULL;
- done1 = TRUE;
- }
- else if (err)
+ err = svn_io_file_read_full2(file1_h, buf1,
+ SVN__STREAM_CHUNK_SIZE, &bytes_read1,
&done1, pool);
+ if (err)
break;
- err = svn_io_file_read_full(file2_h, buf2,
- SVN__STREAM_CHUNK_SIZE, &bytes_read2, pool);
- if (err && APR_STATUS_IS_EOF(err->apr_err))
- {
- svn_error_clear(err);
- err = NULL;
- done2 = TRUE;
- }
- else if (err)
+ err = svn_io_file_read_full2(file2_h, buf2,
+ SVN__STREAM_CHUNK_SIZE, &bytes_read2,
&done2, pool);
+ if (err)
break;
if ((bytes_read1 != bytes_read2)
