On Tue, Feb 18, 2014 at 12:10 AM, Evgeny Kotkov <evgeny.kot...@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 > > #---------------------------------------------------------------------- > > -@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.
