subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Fuhrmann <eq...@web.de>
Subject Re: svn commit: r1040832 - Port a fix for a FSFS packing race
Date Sun, 05 Dec 2010 22:48:07 GMT
On 02.12.2010 08:18, Daniel Shahaf wrote:
>> On Wed, 2010-12-01, stefan2@apache.org wrote:
>>> Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1040832&r1=1040831&r2=1040832&view=diff
>>> ==============================================================================
>>> --- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>>> +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Wed Dec  1 00:15:11 2010
>>> @@ -1903,21 +1903,42 @@ open_pack_or_rev_file(apr_file_t **file,
>>>   {
>>>     svn_error_t *err;
>>>     const char *path;
>>>     svn_boolean_t retry = FALSE;
>>>
> I agree the code below is correct, but I found it confusing:
>
>>>     do
>>>       {
>>>         err = svn_fs_fs__path_rev_absolute(&path, fs, rev, pool);
>>>
>>>         /* open the revision file in buffered r/o mode */
>>>         if (! err)
>>>           err = svn_io_file_open(file, path,
>>>                                 APR_READ | APR_BUFFERED, APR_OS_DEFAULT, pool);
>>>
>>>         if (err&&  APR_STATUS_IS_ENOENT(err->apr_err))
>>>           {
>>>             /* Could not open the file. This may happen if the
>>>              * file once existed but got packed later. */
>>>             svn_error_clear(err);
>>>
>>>             /* if that was our 2nd attempt, leave it at that. */
>>>             if (retry)
>>>               return svn_error_createf(SVN_ERR_FS_NO_SUCH_REVISION, NULL,
>>>                                       _("No such revision %ld"), rev);
>>>
>>>             /* we failed for the first time. Refresh cache&  retry. */
>>>             SVN_ERR(update_min_unpacked_rev(fs, pool));
>>>
> Philip noted that this call should be guarded by a format number check
> (otherwise we would assert on format-3 repositories that are missing
> a rev file).  I've fixed that.
>
Thanks.
>>>             retry = TRUE;
>>>           }
>>>         else
>>>           {
>>>             /* the file exists but something prevented us from opnening it */
>>>             return svn_error_return(err);
> The comment doesn't indicate that the else{} block is also entered in
> the rare case where ERR is SVN_NO_ERROR.
>
> In other words, the "success" case is handled not by the 'return SVN_NO_ERROR'
> below (which in fact is never reached), but by this else{} block.
Yup. The comment is just one of those "last minute improvements" ...
>>>           }
>>>       }
>>>     while (err);
>>>
>>>     return SVN_NO_ERROR;
>>>   }
> The error handling confused me here: it clears ERR but then checks that
> it's non-NULL, and right after that check (which normally means "there
> is an error") it overwrites ERR.  I think the loop would be clearer if
> were just 'while (1)'.
r1042478 should make the logic more obvious.

-- Stefan^2.

Mime
View raw message