Re: [PATCH] libsvn_fs_util: Move functions from fs.c to fs-util.c

On Thu, Apr 05, 2007 at 07:51:59PM +0530, Bhuvaneswaran Arumugam wrote:
> @@ -1182,7 +1163,7 @@
> 
> /* Base FS library vtable, used by the FS loader library. */
> static fs_library_vtable_t library_vtable = {
> - base_version,
> + svn_fs__version,

So previously the loader would check compatibility against the
version of the FS module, and now we're checking against the version
of libsvn_fs_util? Hmmm, I'm not too sure that makes sense.

> base_create,
> base_open,
> base_open_for_recovery,
> @@ -1211,7 +1192,7 @@
> return svn_error_createf(SVN_ERR_VERSION_MISMATCH, NULL,
> _("Unsupported FS loader version (%d) for bdb"),
> loader_version->major);
> - SVN_ERR(svn_ver_check_list(base_version(), checklist));
> + SVN_ERR(svn_ver_check_list(svn_fs__version(), checklist));

And when the FS module is initialised, we now check that the version of
libsvn_fs_util is compatible with the FS module's dependencies?

This definitely seems wrong to me - we aren't checking anything against
the FS module's version.

(Additionally, and this should probably go into a separate patch anyway,
shouldn't libsvn_fs_util be one of the dependencies listed in the FSFS
and BDB checklists? I think it probably should).

> +svn_error_t *
> +check_already_open(svn_fs_t *fs)

Needs to be svn_fs__check_already_open().

> +{
> + if (fs->fsap_data)
> + return svn_error_create(SVN_ERR_FS_ALREADY_OPEN, 0,
> + _("Filesystem object already open"));

If we're going to say that this is appropriate for a general filesystem,
I'd prefer that this checked fs->path (as the FSFS implementation used to
do). There's no requirement that a generic filesystem assign anything to
fs->fsap_data, whereas there is a requirement that they assign fs->path.

Regards,
Malcolm