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

On Thu, 2007-04-05 at 15:55 +0100, Malcolm Rowe wrote:
> 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.

I'm unsure if svn_fs__version returns the version of the libsvn_fs_util
library or the version of the FS module. I'd expect it to return the
version of the FS module used. It will be ascertained if we know what
svn_fs__version would return in this context.

But I'm unaware of it.

> > 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).

I've no clue about this.

> > +svn_error_t *
> > +check_already_open(svn_fs_t *fs)
>
> Needs to be svn_fs__check_already_open().

I thought about it, but wished to do these kind of renaming activity
separately once we move all eligible functions to fs-util.c.

> > +{
> > + 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.

This check is meant to work with FSFS and BDB back ends. I'm unsure why
it would not work when it is called from generic library and why we
should change this check, in which case I'm unsure if would fit well in
BDB while it would fit for FSFS.

Finally, I understand, the svn_fs__version() *may* return the version of
libsvn_fs_util library instead of version of the FS module, in which
case, it is not eligible to be moved from fs.c.

I'd appreciate if you could clarify. Thank you.

-- 
Regards,
Bhuvaneswaran