On Tue, Feb 18, 2014 at 12:10 AM, Evgeny Kotkov <evgeny.kotkov_at_visualsvn.com
> wrote:
> > Here is the fixup for the problem I was talking about in
> > http://svn.haxx.se/dev/archive-2014-01/0160.shtml and partially in
> > http://svn.haxx.se/dev/archive-2014-01/0089.shtml
>
> I had some time to think about this patch and so, here is a reroll.
>
> The recover_get_root_offset() function from the previous version of this
> patch
> has some problems in it. It was written the same way as the existing
> get_root_changes_offset() function from
> subversion/libsvn_fs_fs/cached_data.c,
> however, the get_root_changes_offset() itself has some problems. The
> problems
> I am talking about aren't really the "bang you're dead" kind of problems,
> but,
> as long as we are implementing a new helper, we might as well do it the
> best way
> possible. This V2 patch solves the following issues in V1:
>
> - It avoids seeking to a negative offset for the scenarios when the
> revision
> file is smaller than buffer (we're in the middle of the recovery
> procedure,
> so the repository might be in any possible state).
>
> - It avoids touching internal svn_stringbuf_t fields (such as DATA and LEN)
> and achieves the wanted behavior using the stringbuf API only.
>
> - Relying on the allocator-specific BLOCKSIZE when determining the buffer
> size
> for the trailer is not the best thing to do. In a theoretical case
> where the
> allocator allocates huge 2KiB blocks for every stringbuf, we would fail
> to
> parse the trailer for smaller (< 2KiB) revision files.
>
> - svn_io_file_read(), as opposed to svn_io_file_read_full2(), can read any
> number of bytes <= NBYTES before exiting. This essentially means we
> would fail to parse the revision trailer in all scenarios where this
> function
> returns without filling the buffer.
>
Mind applying equivalent changes to get_root_changes_offset()?
Index: subversion/tests/cmdline/svnadmin_tests.py
> ===================================================================
> --- subversion/tests/cmdline/svnadmin_tests.py (revision 1569069)
> +++ subversion/tests/cmdline/svnadmin_tests.py (working copy)
> @@ -828,10 +828,14 @@ _0.0.t1-1 add false false /A/B/E/bravo
>
> #----------------------------------------------------------------------
>
> -_at_SkipUnless(svntest.main.is_fs_type_fsfs)
> -def recover_fsfs(sbox):
> - "recover a repository (FSFS only)"
> - sbox.build()
> +# Helper for two test functions.
> +def corrupt_and_recover_db_current(sbox, minor_version=None):
> + """Build up a MINOR_VERSION sanbox and test different recovery
Typo: s/sanbox/sandbox/
scenarios
> + with missing, out-of-date or even corrupt db/current files. Recovery
> should
>
Except for the typo, you patch looks good.
I like that we get around the chicken/egg
problem with the revnum vs. HEAD check.
Please commit.
-- Stefan^2.
Received on 2014-02-25 22:38:24 CET