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

[ 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/