On Thu, Sep 9, 2010 at 4:34 PM, Stefan Fuhrmann
<stefanfuhrmann_at_alice-dsl.de> wrote:
> Hyrum Wright wrote:
>>
>> On Thu, Sep 9, 2010 at 10:59 AM, <hwright_at_apache.org> wrote:
>>
>>>
>>> Author: hwright
>>> Date: Thu Sep 9 15:59:00 2010
>>> New Revision: 995478
>>>
>>> URL: http://svn.apache.org/viewvc?rev=995478&view=rev
>>> Log:
>>> On the performance branch:
>>> Bring up-to-date with trunk.
>>>
>>
>> ...
>>
>>>
>>> Modified: subversion/branches/performance/subversion/libsvn_subr/io.c
>>> URL:
>>> http://svn.apache.org/viewvc/subversion/branches/performance/subversion/libsvn_subr/io.c?rev=995478&r1=995477&r2=995478&view=diff
>>>
>>> ==============================================================================
>>> --- subversion/branches/performance/subversion/libsvn_subr/io.c
>>> (original)
>>> +++ subversion/branches/performance/subversion/libsvn_subr/io.c Thu Sep
>>> 9 15:59:00 2010
>>> @@ -1076,14 +1076,19 @@ svn_error_t *svn_io_file_create(const ch
>>> {
>>> apr_file_t *f;
>>> apr_size_t written;
>>> + svn_error_t *err;
>>>
>>> SVN_ERR(svn_io_file_open(&f, file,
>>> (APR_WRITE | APR_CREATE | APR_EXCL),
>>> APR_OS_DEFAULT,
>>> pool));
>>> - SVN_ERR(svn_io_file_write_full(f, contents, strlen(contents),
>>> - &written, pool));
>>> - return svn_io_file_close(f, pool);
>>> + err= svn_io_file_write_full(f, contents, strlen(contents),
>>> + &written, pool);
>>> +
>>> +
>>> + return svn_error_return(
>>> + svn_error_compose_create(err,
>>> + svn_io_file_close(f,
>>> pool)));
>>> }
>>>
>>> svn_error_t *svn_io_dir_file_copy(const char *src_path,
>>> @@ -2952,12 +2957,19 @@ svn_io_write_unique(const char **tmp_pat
>>> apr_pool_t *pool)
>>> {
>>> apr_file_t *new_file;
>>> + svn_error_t *err;
>>>
>>> SVN_ERR(svn_io_open_unique_file3(&new_file, tmp_path, dirpath,
>>> delete_when, pool, pool));
>>> - SVN_ERR(svn_io_file_write_full(new_file, buf, nbytes, NULL, pool));
>>> - SVN_ERR(svn_io_file_flush_to_disk(new_file, pool));
>>> - return svn_io_file_close(new_file, pool);
>>> +
>>> + err = svn_io_file_write_full(new_file, buf, nbytes, NULL, pool);
>>> +
>>> + if (!err)
>>> + err = svn_io_file_flush_to_disk(new_file, pool);
>>> +
>>> + return svn_error_return(
>>> + svn_error_compose_create(err,
>>> + svn_io_file_close(new_file,
>>> pool)));
>>> }
>>>
>>>
>>> @@ -3521,15 +3533,17 @@ svn_io_read_version_file(int *version,
>>> apr_file_t *format_file;
>>> char buf[80];
>>> apr_size_t len;
>>> + svn_error_t *err;
>>>
>>> /* Read a chunk of data from PATH */
>>> SVN_ERR(svn_io_file_open(&format_file, path, APR_READ,
>>> APR_OS_DEFAULT, pool));
>>> len = sizeof(buf);
>>> - SVN_ERR(svn_io_file_read(format_file, buf, &len, pool));
>>> + err = svn_io_file_read(format_file, buf, &len, pool);
>>>
>>> /* Close the file. */
>>> - SVN_ERR(svn_io_file_close(format_file, pool));
>>> + SVN_ERR(svn_error_compose_create(err,
>>> + svn_io_file_close(format_file,
>>> pool)));
>>>
>>> /* If there was no data in PATH, return an error. */
>>> if (len == 0)
>>> @@ -3556,7 +3570,7 @@ svn_io_read_version_file(int *version,
>>> }
>>>
>>> /* Convert to integer. */
>>> - *version = atoi(buf);
>>> + SVN_ERR(svn_cstring_atoi(version, buf));
>>>
>>> return SVN_NO_ERROR;
>>> }
>>> @@ -3570,40 +3584,65 @@ contents_identical_p(svn_boolean_t *iden
>>> const char *file2,
>>> apr_pool_t *pool)
>>> {
>>> + svn_error_t *err;
>>> apr_size_t bytes_read1, bytes_read2;
>>> char *buf1 = apr_palloc(pool, SVN__STREAM_CHUNK_SIZE);
>>> char *buf2 = apr_palloc(pool, SVN__STREAM_CHUNK_SIZE);
>>> apr_file_t *file1_h = NULL;
>>> apr_file_t *file2_h = NULL;
>>> + svn_boolean_t done1 = FALSE;
>>> + svn_boolean_t done2 = FALSE;
>>>
>>> SVN_ERR(svn_io_file_open(&file1_h, file1, APR_READ, APR_OS_DEFAULT,
>>> pool));
>>> - SVN_ERR(svn_io_file_open(&file2_h, file2, APR_READ, APR_OS_DEFAULT,
>>> - pool));
>>> +
>>> + err = svn_io_file_open(&file2_h, file2, APR_READ, APR_OS_DEFAULT,
>>> + pool);
>>> +
>>> + if (err)
>>> + return svn_error_return(
>>> + svn_error_compose_create(err,
>>> + svn_io_file_close(file1_h,
>>> pool)));
>>>
>>> *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. :)
Received on 2010-09-10 01:45:51 CEST