Re: CVS update: subversion/subversion/libsvn_fs fs.c

[Greg Stein <gs...@lyra.org>]

[ Jim: ]

This error cleanup/recovery isn't quite right. It gets me past the
txn_checkpoint() failure, though.

Specifically, I was calling svn_fs_open_berkeley() with a path to nowhere.
That would fail, but the svn_fs_new() had already registered a cleanup. When
the cleanup was run (at pool destruction), I'd get an abort().

So... there is some kind of fine-tuning that needs to happen in there to
leave the fs structure in a proper state on failure-to-open (so that a
cleanup will be mostly a no-op).

Note that I zeroed out fs->env in the cleanup. I put that *after* the
checkpoint, right before the close. It might need to go before the
checkpoint call, but I'm not intimately familar with the DB semantics to
determine where it made the most sense.

Cheers,
-g

On Sun, Nov 05, 2000 at 11:59:03AM -0000, gstein@tigris.org wrote:
>   User: gstein  
>   Date: 00/11/05 03:59:03
> 
>   Modified:    subversion/libsvn_fs fs.c
>   Log:
>   (cleanup_fs): cleanup fs->env only when it is non-NULL. set it to NULL after
>     cleaning it up.
>   (svn_fs_open_berkeley): cleanup_fs() isn't quite smart enough to clean up
>     after a failure during opening (it attempts to checkpoint an unopend db).
>     set fs->env to NULL if we don't get the thing opened properly.
>   
>   Revision  Changes    Path
>   1.20      +23 -15    subversion/subversion/libsvn_fs/fs.c
>   
>   Index: fs.c
>   ===================================================================
>   RCS file: /cvs/subversion/subversion/libsvn_fs/fs.c,v
>   retrieving revision 1.19
>   retrieving revision 1.20
>   diff -u -r1.19 -r1.20
>   --- fs.c	2000/11/03 23:33:57	1.19
>   +++ fs.c	2000/11/05 11:59:03	1.20
>   @@ -117,26 +117,30 @@
>    static svn_error_t *
>    cleanup_fs (svn_fs_t *fs)
>    {
>   -  int db_err;
>   -
>      /* Close the databases.  */
>      SVN_ERR (cleanup_fs_db (fs, &fs->revisions, "revisions"));
>      SVN_ERR (cleanup_fs_db (fs, &fs->nodes, "nodes"));
>      SVN_ERR (cleanup_fs_db (fs, &fs->transactions, "transactions"));
>    
>   -  /* Checkpoint any changes.  */
>   -  db_err = txn_checkpoint (fs->env, 0, 0, 0);
>   -  while (db_err == DB_INCOMPLETE)
>   +  if (fs->env != NULL)
>        {
>   -      apr_sleep (1000000L);
>   -      db_err = txn_checkpoint (fs->env, 0, 0, 0);
>   -    }
>   -  SVN_ERR (DB_WRAP (fs, "checkpointing environment", db_err));
>   +      DB_ENV *env = fs->env;
>   +      int db_err;
>   +
>   +      /* Checkpoint any changes.  */
>   +      db_err = txn_checkpoint (env, 0, 0, 0);
>   +      while (db_err == DB_INCOMPLETE)
>   +        {
>   +          apr_sleep (1000000L);
>   +          db_err = txn_checkpoint (env, 0, 0, 0);
>   +        }
>   +      SVN_ERR (DB_WRAP (fs, "checkpointing environment", db_err));
>          
>   -  /* Finally, close the environment.  */
>   -  if (fs->env)
>   -    SVN_ERR (DB_WRAP (fs, "closing environment",
>   -		      fs->env->close (fs->env, 0)));
>   +      /* Finally, close the environment.  */
>   +      fs->env = NULL;
>   +      SVN_ERR (DB_WRAP (fs, "closing environment",
>   +                        env->close (env, 0)));
>   +    }
>    
>      return 0;
>    }
>   @@ -315,7 +319,7 @@
>      SVN_ERR (check_already_open (fs));
>    
>      svn_err = allocate_env (fs);
>   -  if (svn_err) goto error;
>   +  if (svn_err) goto error_env;
>    
>      /* Open the Berkeley DB environment.  */
>      svn_err = DB_WRAP (fs, "opening environment",
>   @@ -325,7 +329,7 @@
>    				     | DB_INIT_MPOOL
>    				     | DB_INIT_TXN),
>    				    0666));
>   -  if (svn_err) goto error;
>   +  if (svn_err) goto error_env;
>    
>      /* Open the various databases.  */
>      svn_err = svn_fs__open_revisions (fs);
>   @@ -337,6 +341,10 @@
>    
>      return 0;
>      
>   + error_env:
>   +  fs->env = NULL;
>   +  /* FALLTHRU */
>   +
>     error:
>      cleanup_fs (fs);
>      return svn_err;
>   
>   
>   

-- 
Greg Stein, http://www.lyra.org/

Re: CVS update: subversion/subversion/libsvn_fs fs.c

[Jim Blandy <ji...@zwingli.cygnus.com>]

> This error cleanup/recovery isn't quite right. It gets me past the
> txn_checkpoint() failure, though.

Thanks for catching this.  I think I've got the logic right in my
sources now; I'd commit it immediately, but I've got other incomplete
changes in fs.c.

If allocate_env returns, we always need to call env->close, even when
the call to env->open fails.  So the jumps to error_env aren't right,
I think.

> Note that I zeroed out fs->env in the cleanup. I put that *after* the
> checkpoint, right before the close. It might need to go before the
> checkpoint call, but I'm not intimately familar with the DB semantics to
> determine where it made the most sense.

Checkpointing is just an ordinary operation that you could perform at
any time on a healthy env.  We shouldn't zero fs->env until we've
called fs->env->close, so I think your change is correct.