You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Max Bowsher <ma...@ukf.net> on 2005/09/22 16:07:46 UTC
Problem with new svn_fs_type() API
There is a problem with the current implementation of svn_fs_type().
I justified it's addition with the following two use-cases:
1. For libsvn_repos to perform the extra locking required for BDB FSes only
for FSes which are actually BDB.
For this use-case, the current API is no use at all, because you need an
open svn_fs_t to call svn_fs_type(), but libsvn_repos needs the information
*before* it opens the FS.
2. For administrative programs which need to perform backend specific
maintenance operations - e.g., backup, bdb log pruning.
For this use-case, the current API is not very helpful, since this kind of
application probably won't want a svn_fs_t for any other purpose, and it is
unnecessarily expensive to open the fs just to get its type.
Therefore I propose changing the interface from:
const char *svn_fs_type (svn_fs_t *fs);
to:
svn_error_t *svn_fs_type (const char **fs_type, const char *path,
apr_pool_t *pool);
and replacing the current implementation with one based on the 'read the
fs-type file' code in fs-loader.c:fs_library_vtable(), as I suggested in my
previous patch.
Max.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem with new svn_fs_type() API
Posted by "Daniel L. Rall" <dl...@finemaltcoding.com>.
On Fri, 23 Sep 2005, Greg Hudson wrote:
> On Fri, 2005-09-23 at 15:43 -0500, kfogel@collab.net wrote:
> > At first I thought: "Obviously, it should be the repository path,
> > because theoretically, if you don't know the type of the fs, you can't
> > know where inside the repos to find it. Sure, currently all fs's live
> > in the "db/" subdir, but maybe that won't always be true."
>
> That will always be true, or at least, that's an assumption with more
> permanance than the current libsvn_fs API.
Indeed, this is the location where each FS backend stores its
implementation-specific data.
> (Yeah, "db" is a bad name for it, but we're stuck with it.)
Ah, it could be worse -- conceptually fsfs is a database of sorts.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem with new svn_fs_type() API
Posted by Greg Hudson <gh...@MIT.EDU>.
On Fri, 2005-09-23 at 15:43 -0500, kfogel@collab.net wrote:
> At first I thought: "Obviously, it should be the repository path,
> because theoretically, if you don't know the type of the fs, you can't
> know where inside the repos to find it. Sure, currently all fs's live
> in the "db/" subdir, but maybe that won't always be true."
That will always be true, or at least, that's an assumption with more
permanance than the current libsvn_fs API.
(Yeah, "db" is a bad name for it, but we're stuck with it.)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem with new svn_fs_type() API
Posted by Max Bowsher <ma...@ukf.net>.
kfogel@collab.net wrote:
> "Max Bowsher" <ma...@ukf.net> writes:
>> Therefore I propose changing the interface from:
>> const char *svn_fs_type (svn_fs_t *fs);
>> to:
>> svn_error_t *svn_fs_type (const char **fs_type, const char *path,
>> apr_pool_t *pool);
>>
>> and replacing the current implementation with one based on the 'read
>> the fs-type file' code in fs-loader.c:fs_library_vtable(), as I
>> suggested in my previous patch.
>
> I started on this, and right away ran into a question:
>
> Should the 'path' parameter be path to repos, or path to the fs subdir
> inside the repos?
>
> At first I thought: "Obviously, it should be the repository path,
> because theoretically, if you don't know the type of the fs, you can't
> know where inside the repos to find it. Sure, currently all fs's live
> in the "db/" subdir, but maybe that won't always be true."
>
> But then I looked at some of our existing svn_fs_*() APIs that take a
> path, and they actually want the path to repos/db, not to repos. They
> don't document this clearly, but an examination of callers shows it to
> be true. See svn_fs_create() and svn_fs_open(), for example.
>
> Now I'm not sure what to do. My API design instincts say repos path,
> but my consistency instincts say fs-inside-repos path.
It has to be the path including "db/". At least theoretically, libsvn_fs
could be used free from any libsvn_repos wrapper. For convenience of the API
consumers, there could be a svn_repos_fs_type() wrapper.
Here is my patch (the docstring could use further tweaking):
Index: subversion/include/svn_fs.h
===================================================================
--- subversion/include/svn_fs.h (revision 16201)
+++ subversion/include/svn_fs.h (working copy)
@@ -170,9 +170,9 @@
apr_hash_t *config, apr_pool_t *pool);
/**
- * Return a constant string describing the back-end type of @a fs,
- * e.g. "fsfs", "bdb". If the type is not yet known, for example
- * because @a fs has been created but not opened, just return NULL.
+ * Return a string describing the back-end type of Subversion
+ * filesystem located in @a path.
+ * e.g. "fsfs", "bdb".
*
* For filesystem back ends that ship with Subversion, the string
* returned here is the same as the corresponding @c SVN_FS_TYPE_*
@@ -184,7 +184,8 @@
*
* @since New in 1.3.
*/
-const char *svn_fs_type (svn_fs_t *fs);
+svn_error_t *svn_fs_type (const char **fs_type, const char *path,
+ apr_pool_t *pool);
/**
* Return the path to @a fs's repository, allocated in @a pool.
Index: subversion/libsvn_fs/fs-loader.c
===================================================================
--- subversion/libsvn_fs/fs-loader.c (revision 16201)
+++ subversion/libsvn_fs/fs-loader.c (working copy)
@@ -163,12 +163,10 @@
_("Unknown FS type '%s'"), fs_type);
}
-/* Fetch the library vtable for an existing FS. */
-static svn_error_t *
-fs_library_vtable (fs_library_vtable_t **vtable, const char *path,
- apr_pool_t *pool)
+svn_error_t *
+svn_fs_type (const char **fs_type, const char *path, apr_pool_t *pool)
{
- const char *filename, *fs_type;
+ const char *filename;
char buf[128];
svn_error_t *err;
apr_file_t *file;
@@ -180,18 +178,29 @@
if (err && APR_STATUS_IS_ENOENT (err->apr_err))
{
svn_error_clear (err);
- fs_type = SVN_FS_TYPE_BDB;
+ *fs_type = apr_pstrdup (pool, SVN_FS_TYPE_BDB);
+ return SVN_NO_ERROR;
}
else if (err)
return err;
- else
- {
- len = sizeof(buf);
- SVN_ERR (svn_io_read_length_line (file, buf, &len, pool));
- SVN_ERR (svn_io_file_close (file, pool));
- fs_type = buf;
- }
+ len = sizeof(buf);
+ SVN_ERR (svn_io_read_length_line (file, buf, &len, pool));
+ SVN_ERR (svn_io_file_close (file, pool));
+ *fs_type = apr_pstrdup (pool, buf);
+
+ return SVN_NO_ERROR;
+}
+
+/* Fetch the library vtable for an existing FS. */
+static svn_error_t *
+fs_library_vtable (fs_library_vtable_t **vtable, const char *path,
+ apr_pool_t *pool)
+{
+ const char *fs_type;
+
+ SVN_ERR (svn_fs_type (&fs_type, path, pool));
+
/* Fetch the library vtable by name, now that we've chosen one. */
return get_library_vtable (vtable, fs_type, pool);
}
@@ -361,12 +370,6 @@
}
const char *
-svn_fs_type (svn_fs_t *fs)
-{
- return fs->type;
-}
-
-const char *
svn_fs_path (svn_fs_t *fs, apr_pool_t *pool)
{
return apr_pstrdup (pool, fs->path);
Max.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem with new svn_fs_type() API
Posted by kf...@collab.net.
"Max Bowsher" <ma...@ukf.net> writes:
> Therefore I propose changing the interface from:
> const char *svn_fs_type (svn_fs_t *fs);
> to:
> svn_error_t *svn_fs_type (const char **fs_type, const char *path,
> apr_pool_t *pool);
>
> and replacing the current implementation with one based on the 'read
> the fs-type file' code in fs-loader.c:fs_library_vtable(), as I
> suggested in my previous patch.
I started on this, and right away ran into a question:
Should the 'path' parameter be path to repos, or path to the fs subdir
inside the repos?
At first I thought: "Obviously, it should be the repository path,
because theoretically, if you don't know the type of the fs, you can't
know where inside the repos to find it. Sure, currently all fs's live
in the "db/" subdir, but maybe that won't always be true."
But then I looked at some of our existing svn_fs_*() APIs that take a
path, and they actually want the path to repos/db, not to repos. They
don't document this clearly, but an examination of callers shows it to
be true. See svn_fs_create() and svn_fs_open(), for example.
Now I'm not sure what to do. My API design instincts say repos path,
but my consistency instincts say fs-inside-repos path.
Thoughts?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem with new svn_fs_type() API
Posted by Julian Foad <ju...@btopenworld.com>.
Karl Fogel wrote:
> Would you be +1 on getting rid of the enum type and just returning a
> constant string [...]
Max Bowsher wrote:
> For this use-case, the current API is no use at all, [...]
> For this use-case, the current API is not very helpful, [...]
> Therefore I propose changing the interface [...]
:-)
Gosh, isn't it infuriating how difficult even an apparently simple little bit
of software design can be!
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem with new svn_fs_type() API
Posted by kf...@collab.net.
"Max Bowsher" <ma...@ukf.net> writes:
> There is a problem with the current implementation of svn_fs_type().
>
> I justified it's addition with the following two use-cases:
>
> 1. For libsvn_repos to perform the extra locking required for BDB FSes
> only for FSes which are actually BDB.
> For this use-case, the current API is no use at all, because you need
> an open svn_fs_t to call svn_fs_type(), but libsvn_repos needs the
> information *before* it opens the FS.
>
> 2. For administrative programs which need to perform backend specific
> maintenance operations - e.g., backup, bdb log pruning.
> For this use-case, the current API is not very helpful, since this
> kind of application probably won't want a svn_fs_t for any other
> purpose, and it is unnecessarily expensive to open the fs just to get
> its type.
>
> Therefore I propose changing the interface from:
> const char *svn_fs_type (svn_fs_t *fs);
> to:
> svn_error_t *svn_fs_type (const char **fs_type, const char *path,
> apr_pool_t *pool);
>
> and replacing the current implementation with one based on the 'read
> the fs-type file' code in fs-loader.c:fs_library_vtable(), as I
> suggested in my previous patch.
+1
Would you like me to make the change, Max? Say the word and it shall
be done..
-Karl
--
www.collab.net <> CollabNet | Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem with new svn_fs_type() API
Posted by "C. Michael Pilato" <cm...@collab.net>.
"Max Bowsher" <ma...@ukf.net> writes:
> There is a problem with the current implementation of svn_fs_type().
[...]
> Therefore I propose changing the interface from:
> const char *svn_fs_type (svn_fs_t *fs);
> to:
> svn_error_t *svn_fs_type (const char **fs_type, const char *path,
> apr_pool_t *pool);
>
> and replacing the current implementation with one based on the 'read
> the fs-type file' code in fs-loader.c:fs_library_vtable(), as I
> suggested in my previous patch.
+1.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org