On Tue, Oct 7, 2014 at 1:27 PM, Stefan Sperling <s...@elego.de> wrote:
> On Tue, Oct 07, 2014 at 10:41:14AM -0000, stef...@apache.org wrote: > > Author: stefan2 > > Date: Tue Oct 7 10:41:14 2014 > > New Revision: 1629854 > > > > URL: http://svn.apache.org/r1629854 > > Log: > > In FSFS, always use the same function to read the 'current' file. > > > > Apart from the consistency aspect, this no longer lets atoi() mask > > 'current' file corruptions. Recovery must be adopted to this. > > Hi Stefan, > > Two questions below: > > > --- subversion/trunk/subversion/libsvn_fs_fs/fs.c (original) > > +++ subversion/trunk/subversion/libsvn_fs_fs/fs.c Tue Oct 7 10:41:14 > 2014 > > @@ -348,20 +349,47 @@ fs_open_for_recovery(svn_fs_t *fs, > > apr_pool_t *pool, > > apr_pool_t *common_pool) > > { > > + svn_error_t * err; > > + svn_revnum_t youngest_rev; > > + apr_pool_t * subpool = svn_pool_create(pool); > > + > > /* Recovery for FSFS is currently limited to recreating the 'current' > > file from the latest revision. */ > > > > /* The only thing we have to watch out for is that the 'current' file > > - might not exist. So we'll try to create it here unconditionally, > > - and just ignore any errors that might indicate that it's already > > - present. (We'll need it to exist later anyway as a source for the > > - new file's permissions). */ > > + might not exist or contain garbage. So we'll try to read it here > > + and provide or replace the existing file if we couldn't read it. > > + (We'll also need it to exist later anyway as a source for the new > > + file's permissions). */ > > > > - /* Use a partly-filled fs pointer first to create 'current'. This > will fail > > - if 'current' already exists, but we don't care about that. */ > > + /* Use a partly-filled fs pointer first to create 'current'. */ > > fs->path = apr_pstrdup(fs->pool, path); > > - svn_error_clear(svn_io_file_create(svn_fs_fs__path_current(fs, pool), > > - "0 1 1\n", pool)); > > + > > + SVN_ERR(initialize_fs_struct(fs)); > > The 'fs' struct is provided by the caller and is now initialised > and uninitialised within this function. Can't this function > use a local 'fs' variable? If not, why does it need to be uninitialised > again? This is a bit confusing -- though perhaps it's an idiom used in > the FS code that I'm not aware of? > The code would actually be nicer if it used a temporary FS. After all, we are only trying to fix / prepare the on-disk data before properly open the repo and trying to recover it. However, svn_fs_new() is deprecated and there is no nice alternative. We could use apr_pmemdup, write our own init code or so but all these approaches are slightly fragile and blur the lines between libsvn_fs and libsvn_fs_fs. > > + /* Figure out the repo format and check that we can even handle it. */ > > + SVN_ERR(svn_fs_fs__read_format_file(fs, subpool)); > > + > > + /* Now, read 'current' and try to patch it if necessary. */ > > + err = svn_fs_fs__youngest_rev(&youngest_rev, fs, subpool); > > + if (err) > > Can't we check for a specific error code here, and return the > error otherwise? This would make the intention of the error handling > code explicit and avoid masking of arbitrary error conditions. > If we wanted to enumerate all "unsurprising" error conditions, it might become quite a long list. After all, things are most likely broken when you run recover. To me, it seems best to try to get into a working state *despite* any previous errors. r1629879 tries to explain that. -- Stefan^2.
