subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Sperling <s...@elego.de>
Subject Re: svn commit: r1629854 - in /subversion/trunk/subversion/libsvn_fs_fs: fs.c fs_fs.c
Date Tue, 07 Oct 2014 11:27:48 GMT
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?

> +
> +  /* 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.

> +    {
> +      const char *file_path;
> +
> +      /* 'current' file is missing or contains garbage.
> +       * Start with HEAD = 0 in that case. */
> +      svn_error_clear(err);
> +      file_path = svn_fs_fs__path_current(fs, subpool);
> +
> +      /* Best effort to ensure the file exists and is valid.
> +       * This may fail for r/o filesystems etc. */
> +      SVN_ERR(svn_io_remove_file2(file_path, TRUE, subpool));
> +      SVN_ERR(svn_io_file_create_empty(file_path, subpool));
> +      SVN_ERR(svn_fs_fs__write_current(fs, 0, 1, 1, subpool));
> +    }
> +
> +  uninitialize_fs_struct(fs);
> +  svn_pool_destroy(subpool);
>  
>    /* Now open the filesystem properly by calling the vtable method directly. */
>    return fs_open(fs, path, common_pool_lock, pool, common_pool);
> 

Mime
View raw message