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

Any review comment for this?

On Sat, 2007-04-07 at 22:19 +0530, Bhuvaneswaran Arumugam wrote:
> 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