You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2006/02/13 21:16:11 UTC
problem revealed by issue #2398 (server-side assertion)
Issue #2398 is about a server-side assertion that happens when merging
the diff between two mod_dav_svn URLs into a working copy, when (say)
the client system is case-insensitive and there is a case difference
between the wc's URL and the command's URLs.
But that's not what I'm here to discuss.
We think the issue #2398 bug may be gone these days, because of fixes
in the calling code. However, investigating it revealed a nasty
mis-assumption in our filesystem backend(s). As I point out in
http://subversion.tigris.org/issues/show_bug.cgi?id=2398#desc20
it's just wrong to use a pointer comparison to determine if two
svn_fs_t objects represent the same filesystem. Instead, we should be
comparing some underlying property, like their UUIDs.
The patch below does this. It passes 'make check', and I will be
running it through 'make davcheck' soon. I'd like to get people's
thoughts on it. I have no evidence yet that it causes a performance
hit, therefore have not done anything fancy like, say, modifying
svn_fs_get_uuid() to cache the uuid in the svn_fs_t object. (That
would raise lifetime issues, and I haven't yet looked into whether
copying the UUID into svn_fs_t->pool would solve them.)
Any thoughts about this patch's general approach?
-Karl
[[[
Fix issue #2398: user data could trigger assertions in server code.
################################################################
### ###
### This patch is not ready for commit yet. I'd like to ###
### discuss it on the dev@ list before applying. -kfogel ###
### ###
################################################################
* subversion/include/svn_fs.h
(svn_fs_same_p): New prototype.
* subversion/libsvn_fs/fs-loader.c
(svn_fs_same_p): Implement it.
* subversion/libsvn_fs_fs/tree.c
(copy_helper): Use above to check equality of the two filesystems,
instead of depending on pointer equality.
* subversion/libsvn_fs_base/tree.c
(copy_helper): Same.
* subversion/include/svn_error_codes.h
(SVN_ERR_FS_NOT_SAME): New error code.
]]]
Index: subversion/libsvn_fs_base/tree.c
===================================================================
--- subversion/libsvn_fs_base/tree.c (revision 18363)
+++ subversion/libsvn_fs_base/tree.c (working copy)
@@ -2902,8 +2902,14 @@
apr_pool_t *pool)
{
struct copy_args args;
+ svn_boolean_t same_p;
- assert (from_root->fs == to_root->fs);
+ SVN_ERR (svn_fs_same_p (&same_p, from_root->fs, to_root->fs, pool));
+ if (! same_p)
+ return svn_error_createf
+ (SVN_ERR_FS_NOT_SAME, NULL,
+ _("Cannot copy between two different filesystems ('%s' and '%s')."),
+ svn_fs_path (from_root->fs, pool), svn_fs_path (to_root->fs, pool));
if (! to_root->is_txn_root)
return not_txn (to_root);
Index: subversion/include/svn_error_codes.h
===================================================================
--- subversion/include/svn_error_codes.h (revision 18363)
+++ subversion/include/svn_error_codes.h (working copy)
@@ -585,6 +585,11 @@
SVN_ERR_FS_CATEGORY_START + 43,
"Unsupported FS format")
+ /** @since New in 1.4. */
+ SVN_ERRDEF (SVN_ERR_FS_NOT_SAME,
+ SVN_ERR_FS_CATEGORY_START + 44,
+ "Two FS objects do not refer to the same real FS")
+
/* repos errors */
SVN_ERRDEF (SVN_ERR_REPOS_LOCKED,
Index: subversion/include/svn_fs.h
===================================================================
--- subversion/include/svn_fs.h (revision 18363)
+++ subversion/include/svn_fs.h (working copy)
@@ -1549,6 +1549,12 @@
const char *uuid,
apr_pool_t *pool);
+/** Set @a same_p to @c TRUE iff @a fs1 and @a fs2 have the same UUID. */
+svn_error_t *svn_fs_same_p (svn_boolean_t *same_p,
+ svn_fs_t *fs1,
+ svn_fs_t *fs2,
+ apr_pool_t *pool);
+
/* Non-historical properties. */
Index: subversion/libsvn_fs/fs-loader.c
===================================================================
--- subversion/libsvn_fs/fs-loader.c (revision 18363)
+++ subversion/libsvn_fs/fs-loader.c (working copy)
@@ -879,6 +879,21 @@
return fs->vtable->set_uuid (fs, uuid, pool);
}
+svn_error_t *svn_fs_same_p (svn_boolean_t *same_p,
+ svn_fs_t *fs1,
+ svn_fs_t *fs2,
+ apr_pool_t *pool)
+{
+ const char *uuid1;
+ const char *uuid2;
+
+ SVN_ERR (svn_fs_get_uuid (fs1, &uuid1, pool));
+ SVN_ERR (svn_fs_get_uuid (fs2, &uuid2, pool));
+
+ *same_p = ! strcmp (uuid1, uuid2);
+ return SVN_NO_ERROR;
+}
+
svn_error_t *
svn_fs_lock (svn_lock_t **lock, svn_fs_t *fs, const char *path,
const char *token, const char *comment,
Index: subversion/libsvn_fs_fs/tree.c
===================================================================
--- subversion/libsvn_fs_fs/tree.c (revision 18363)
+++ subversion/libsvn_fs_fs/tree.c (working copy)
@@ -1928,8 +1928,14 @@
dag_node_t *from_node;
parent_path_t *to_parent_path;
const char *txn_id = to_root->txn;
+ svn_boolean_t same_p;
- assert (from_root->fs == to_root->fs);
+ SVN_ERR (svn_fs_same_p (&same_p, from_root->fs, to_root->fs, pool));
+ if (! same_p)
+ return svn_error_createf
+ (SVN_ERR_FS_NOT_SAME, NULL,
+ _("Cannot copy between two different filesystems ('%s' and '%s')."),
+ svn_fs_path (from_root->fs, pool), svn_fs_path (to_root->fs, pool));
if (from_root->is_txn_root)
return svn_error_create
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: problem revealed by issue #2398 (server-side assertion)
Posted by kf...@collab.net.
Branko Čibej <br...@xbc.nu> writes:
> Why did you make this a public function if it's only used within the
> FS back-ends? Making functions public without a real need for it is a
> bad idea. If avoiding code duplication matters, then declare it in
> fs-loader.h, with the double underscore.
+1. I'll change this before committing, thanks Brane.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: problem revealed by issue #2398 (server-side assertion)
Posted by Branko Čibej <br...@xbc.nu>.
kfogel@collab.net wrote:
> * subversion/include/svn_fs.h
> (svn_fs_same_p): New prototype.
>
> * subversion/libsvn_fs/fs-loader.c
> (svn_fs_same_p): Implement it.
>
> * subversion/libsvn_fs_fs/tree.c
> (copy_helper): Use above to check equality of the two filesystems,
> instead of depending on pointer equality.
>
> * subversion/libsvn_fs_base/tree.c
> (copy_helper): Same.
>
Why did you make this a public function if it's only used within the FS
back-ends? Making functions public without a real need for it is a bad
idea. If avoiding code duplication matters, then declare it in
fs-loader.h, with the double underscore.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: problem revealed by issue #2398 (server-side assertion)
Posted by Julian Foad <ju...@btopenworld.com>.
kfogel@collab.net wrote:
>
> Any thoughts about this patch's general approach?
> [[[
> Fix issue #2398: user data could trigger assertions in server code.
[...]
> ]]]
>
> Index: subversion/libsvn_fs_base/tree.c
> ===================================================================
>
> - assert (from_root->fs == to_root->fs);
> + SVN_ERR (svn_fs_same_p (&same_p, from_root->fs, to_root->fs, pool));
> + if (! same_p)
> + return svn_error_createf
> + (SVN_ERR_FS_NOT_SAME, NULL,
> + _("Cannot copy between two different filesystems ('%s' and '%s')."),
> + svn_fs_path (from_root->fs, pool), svn_fs_path (to_root->fs, pool));
My first thought was: do we need to change from an assertion to a run-time
error? Does or should the doc string say "FROM_ROOT and TO_ROOT must be in the
same repository"? A hard requirement might be appropriate if every
implementation of this interface would necessarily have that requirement. In
this case, it seems reasonable to return a "soft" error.
I wrote the above paragraph before reading Garrett's reply that it's
"fundamentally misusing the API to pass two different filesystems". If that's
the case, we should document that requirement and keep the assert, and the
requirement is then on callers to ensure that they comply with it. (If the
callers want a function like yours to help them check, that's fine, they can
have it.) It's potentially more efficient this way, as a caller making
multiple calls may only have to check once.
Off-topic:
Call me a stickler for specifications, but I went to look for the doc string of
this copy_helper() function which is effectively root_vtable_t::copy(), and the
cupboard was bare. However, the FS_FS versions of these vtable functions have
doc strings. Would it be good to move these doc strings to the vtable definition?
> Index: subversion/include/svn_error_codes.h
> ===================================================================
>
> + /** @since New in 1.4. */
> + SVN_ERRDEF (SVN_ERR_FS_NOT_SAME,
> + SVN_ERR_FS_CATEGORY_START + 44,
> + "Two FS objects do not refer to the same real FS")
Hmm. That eror code name and default description strikes me as too low level,
or something. Two FS objects not referring to the same FS isn't inherently an
error. What's an error in the function above is that a copy from one FS to
another isn't possible. Thus something like "invalid arguments" or the
existing SVN_ERR_UNSUPPORTED_FEATURE would be a more logical generic error code
to use, with your specific message ("Cannot copy between...") overriding the
default. But it's a bit subjective; never mind if you disagree.
The general approach of the patch seems fine, though I'm a novice in this area
and I have no idea about its efficiency.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: problem revealed by issue #2398 (server-side assertion)
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 2/13/06, Peter N. Lundblad <pe...@famlundblad.se> wrote:
> On Mon, 13 Feb 2006, Garrett Rooney wrote:
>
> > On 2/13/06, kfogel@collab.net <kf...@collab.net> wrote:
> >
> > > Any thoughts about this patch's general approach?
> >
> > Why change from an assert to a SVN_ERR? The assert was wrong because
> > of the pointer comparison, not because of what it's testing, it's
> > still fundamentally misusing the API to pass two different filesystems
> > to this function, so why not make it an assert?
> >
> In this particular case, I think changing to an error is appropriate
> because the fs uuid might change, either between the calls, or (if it is
> cached) between the two FS objects were opened. We can't let that scenario
> crash anything.
Ok, that's a justification I can buy.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: problem revealed by issue #2398 (server-side assertion)
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 13 Feb 2006, Garrett Rooney wrote:
> On 2/13/06, kfogel@collab.net <kf...@collab.net> wrote:
>
> > Any thoughts about this patch's general approach?
>
> Why change from an assert to a SVN_ERR? The assert was wrong because
> of the pointer comparison, not because of what it's testing, it's
> still fundamentally misusing the API to pass two different filesystems
> to this function, so why not make it an assert?
>
In this particular case, I think changing to an error is appropriate
because the fs uuid might change, either between the calls, or (if it is
cached) between the two FS objects were opened. We can't let that scenario
crash anything.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: problem revealed by issue #2398 (server-side assertion)
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 2/13/06, kfogel@collab.net <kf...@collab.net> wrote:
> Any thoughts about this patch's general approach?
Why change from an assert to a SVN_ERR? The assert was wrong because
of the pointer comparison, not because of what it's testing, it's
still fundamentally misusing the API to pass two different filesystems
to this function, so why not make it an assert?
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org