subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Fuhrmann <stefan.fuhrm...@wandisco.com>
Subject Re: svn commit: r1629854 - in /subversion/trunk/subversion/libsvn_fs_fs: fs.c fs_fs.c
Date Tue, 07 Oct 2014 13:02:41 GMT
On Tue, Oct 7, 2014 at 1:27 PM, Stefan Sperling <stsp@elego.de> wrote:

> On Tue, Oct 07, 2014 at 10:41:14AM -0000, stefan2@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.

Mime
View raw message