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

On Thu, 2007-04-05 at 20:47 +0530, Bhuvaneswaran Arumugam wrote:
> 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.

I've incorporated the above suggestion in the attached patch.

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

I've also incorporated the above suggestion in the attached patch.

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

Please find attached the revised patch. For now, I've moved the function
to check if the file system is already opened to libsvn_fs_util library.
I've not moved the function to get the version of file system
fs__version(), since I'm still unsure how it is behaving or should
behave when we move it to libsvn_fs_util library.

FWIW, I've run 'make davautocheck' with this patch and do not see any
issues. Please review and let me know your comments. Thank you.

Pre-commit log message:

[[
Move the function to check if the FS is already open,
check_already_open() from libsvn_fs_fs/fs.c and
libsvn_fs_base/fs.c to libsvn_fs_util/fs-util.c library.

The above function was earlier static, now it become non-static, since
it is moved to different library.

* subversion/libsvn_fs_base/fs.c
  Include "private/svn_fs_util.h" header file.
  (check_already_open): Remove.
  (open_databases): Rename check_already_open() function as
  svn_fs__check_already_open() function.

* subversion/libsvn_fs_fs/fs.c
  Include "private/svn_fs_util.h" header file.
  (check_already_open): Remove.
  (fs_create): Rename check_already_open() function as
  svn_fs__check_already_open() function.

* subversion/include/private/svn_fs_util.h
  Declare svn_fs__check_already_open() function to check if the
  filesystem is already opened.

* subversion/libsvn_fs_util/fs-util.c
  Include new dependent header files viz. "svn_fs.h",
  "svn_private_config.h" and "../libsvn_fs/fs-loader.h".
  svn_fs__check_already_open(): New function to check if the filesystem
  is already open.

Suggested by: malcolmr
]]

-- 
Regards,
Bhuvaneswaran