Re: svn commit: r25137 - in trunk/subversion: include include/private libsvn_fs libsvn_fs_base libsvn_fs_base/bdb libsvn_fs_fs libsvn_subr

On Thu, May 24, 2007 at 05:09:42PM -0700, djh@tigris.org wrote:
> Fix the undefined behavior invoked when the fs module is dynamically
> unloaded and the bdb cache tries to clean itself up.

Nice! (though I didn't try understanding the BDB crash problem -- pool
lifetime is hard, let's go shopping!). Two comments/questions:

> --- trunk/subversion/libsvn_fs/fs-loader.c (original)
> +++ trunk/subversion/libsvn_fs/fs-loader.c Thu May 24 17:09:42 2007
> @@ -131,7 +154,34 @@
> _("Failed to load module for FS type '%s'"),
> fst->fs_type);
>
> - SVN_ERR(initfunc(my_version, vtable));
> + {
> + apr_status_t status;
> + svn_error_t *err;
> + svn_error_t *err2;
> +
> + /* Per our API compatibility rules, we cannot ensure that
> + svn_fs_initialize is called by the application. If not, we
> + cannot create the common pool and lock in a thread-safe fashion,
> + nor can we clean up the common pool if libsvn_fs is dynamically
> + unloaded. This function makes a best effort by creating the
> + common pool as a child of the global pool; the window of failure
> + due to thread collision is small. */
> + if (!common_pool)
> + SVN_ERR(svn_fs_initialize(NULL));

Weird indentation.

> @@ -352,20 +370,36 @@
>
> /* Perform the actual creation. */
> *fs_p = svn_fs_new(fs_config, pool);
> - SVN_ERR(vtable->create(*fs_p, path, pool));
> - return serialized_init(*fs_p, pool);
> + SVN_ERR(acquire_fs_mutex());
> + err = vtable->create(*fs_p, path, pool, common_pool);
> + err2 = release_fs_mutex();
> + if (err2)
> + {
> + svn_error_clear(err);
> + return err2;
> + }
> + return err;
> }

Here and elsewhere you prioritise reporting the later mutex error over
the earlier operation error - I think it might be better the other way
round.

Regards,
Malcolm