Re: svn commit: r1100213 - in /subversion/trunk/subversion/libsvn_fs_fs: fs_fs.c rep-cache.c

[Daniel Shahaf <da...@elego.de>]

danielsh@apache.org wrote on Fri, May 06, 2011 at 13:43:11 -0000:
> Author: danielsh
> Date: Fri May  6 13:43:10 2011
> New Revision: 1100213
> 
> URL: http://svn.apache.org/viewvc?rev=1100213&view=rev
> Log:
> Two changes: identify one particular FSFS corruption, and don't mask either
> that particular corruption or any other corruption reported at the same spot.
> 
> * subversion/libsvn_fs_fs/rep-cache.c
>   (fs_fs.h): Include.
>   (svn_fs_fs__get_rep_reference):
>     Before returning the reference, check that it's not over youngest.
> 
> * subversion/libsvn_fs_fs/fs_fs.c
>   (rep_write_contents_close):
>     Don't mask SVN_ERR_FS_CORRUPT errors (including the error raised above).
> 
> Found by: pipern on #svn-dev
> 
> Modified:
>     subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>     subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c
> 
> Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1100213&r1=1100212&r2=1100213&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Fri May  6 13:43:10 2011
> @@ -5639,7 +5639,8 @@ rep_write_contents_close(void *baton)
>        svn_error_t *err;
>        err = svn_fs_fs__get_rep_reference(&old_rep, b->fs, rep->sha1_checksum,
>                                           b->parent_pool);
> -      if (err)
> +      /* ### Other error codes that we shouldn't mask out? */

Just bringing attention to this --- are there any other error codes that
we shouldn't be silently ignoring?

> +      if (err && err->apr_err != SVN_ERR_FS_CORRUPT)
>          {
>            /* Something's wrong with the rep-sharing index.  We can continue
>               without rep-sharing, but warn.
> @@ -5648,6 +5649,17 @@ rep_write_contents_close(void *baton)
>            svn_error_clear(err);
>            old_rep = NULL;
>          }
> +      else
> +        {
> +          /* Fatal error; don't mask it.
> +           
> +             In particular, this block is triggered when the rep-cache refers
> +             to revisions in the future.  We signal that as a corruption situation
> +             since, once those revisions are less than youngest (because of more
> +             commits), the rep-cache would be invalid.
> +           */
> +          SVN_ERR(err);
> +        }
>      }
>    else
>      old_rep = NULL;
> 
> Modified: subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c?rev=1100213&r1=1100212&r2=1100213&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/rep-cache.c Fri May  6 13:43:10 2011
> @@ -22,6 +22,7 @@
>  
>  #include "svn_private_config.h"
>  
> +#include "fs_fs.h"
>  #include "fs.h"
>  #include "rep-cache.h"
>  #include "../libsvn_fs/fs-loader.h"
> @@ -114,6 +115,26 @@ svn_fs_fs__get_rep_reference(representat
>    else
>      *rep = NULL;
>  
> +  /* Sanity check. */
> +  if (*rep)
> +    {
> +      svn_revnum_t youngest;
> +      
> +      youngest = ffd->youngest_rev_cache;
> +      if (youngest < (*rep)->revision)
> +      {
> +        /* Stale cache. */
> +        SVN_ERR(svn_fs_fs__youngest_rev(&youngest, fs, pool));
> +
> +        /* Fresh cache. */
> +        if (youngest < (*rep)->revision)
> +          return svn_error_createf(SVN_ERR_FS_CORRUPT, NULL,
> +                                   _("Youngest revision is r%ld, but "
> +                                     "rep-cache contains r%ld"),
> +                                   youngest, (*rep)->revision);
> +      }
> +    }
> +
>    return svn_sqlite__reset(stmt);
>  }
>  
> 
>