subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Branko Čibej <br...@wandisco.com>
Subject Re: Possible incompatibility of svn_repos_verify_fs2() in 1.9.0-rc1
Date Sat, 23 May 2015 12:19:51 GMT
On 23.05.2015 13:07, Daniel Shahaf wrote:
> Evgeny Kotkov wrote on Thu, May 21, 2015 at 18:23:22 +0300:
>>   SVN_ERR(svn_repos_verify_fs2(repos, 1, 5, NULL, NULL, NULL, NULL, pool));
>>
>> ...would return two different errors depending on the binaries being used,
>> assuming that one of the revisions in the [r1:r5] range is corrupted:
>>
>>  (With 1.8.13)  E160004: Checksum mismatch in item at offset 0 of
>>     length 59 bytes in file path/asf/db/revs/0/2
>>
>>  (With 1.9.0-rc1)  E165011: Repository 'path/asf' failed to verify
> ...
>> Thoughts?
> I think the incumbent code doesn't use SVN_ERR_REPOS_CORRUPT (E165011)
> correctly.
>
> The keep-going implementation simply uses SVN_ERR_REPOS_CORRUPT to wrap
> whatever error the FS layer reported.  That's wrong, for two reasons:
>
> - The SVN_ERR_REPOS_CORRUPT code should be used to indicate corruption
>   of repos-layer on-disk data (such as $REPOS_DIR/db not existing or
>   $REPOS_DIR/format being a directory).  The code does not look for such
>   conditions.  In fact, since the repos layer got around to calling into
>   the FS layer, the repos layer itself is almost certainly integral.
>
> - The repos layer has no business second-guessing the FS layer's choice
>   of error code.  For example, a permission error on a rev file could
>   happen and does not indicate corruption, or a user might have pressed
>   ^C during svn_fs_verify()'s execution.  The incumbent code assumes
>   that any non-SVN_ERR_CANCELLED code indicates a corruption; that
>   assumption is wrong.
>
> So, the keep-going code should stop using SVN_ERR_REPOS_CORRUPT to
> indiscriminately wrap svn_fs_verify()'s error code.  That should make
> the E160004 (SVN_ERR_FS_CORRUPT) error visible, addressing Evgeny's
> issue.  (I am almost sure that's exactly what Evgeny's patch does.)
>
> Furthermore, I think the "if (found_corruption)" block at the end of
> svn_repos_verify_fs3() should also avoid using SVN_ERR_REPOS_CORRUPT,
> since that would be the wrong code to use in some circumstances (for
> example, if all revision files had permission errors).  The code should
> either say what it _does_ know for a fact — "N out of M revisions failed
> to verify" — or start inspecting the FS errors to determine whether they
> indicate corruption or transient errors (which isn't always possible,
> since the repos layer does not know the context the error was raised in).
>
> In fact, since the aforementioned two uses of SVN_ERR_REPOS_CORRUPT are
> the only two uses of that code, I conclude that I am of the opinion that
> that error code should be deleted entirely from the 1.9 codebase.  We
> can always revive it in 1.10 if needed.

I completely agree with your analysis and proposal.

-- Brane


Mime
View raw message