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

Bhuvaneswaran Arumugam wrote:
> Any review comment for this?

I couldn't find any record of this being applied, so I've filed it as
issue 2885.

-Hyrum

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