You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@xbc.nu> on 2004/12/01 02:39:20 UTC
Re: svn commit: r12093 - branches/locking/subversion/mod_dav_svn
sussman@tigris.org wrote:
>Author: sussman
>Date: Mon Nov 29 22:03:52 2004
>New Revision: 12093
>
>
>+ /* Generate a UUID. */
>+ /* ### perhaps this should be a func in libsvn_fs.so, shared by
>+ mod_dav_svn and both fs back-ends?? */
>+ apr_uuid_get (&uuid);
>+ apr_uuid_format (uuid_str, &uuid);
>+ token->uuid_str = uuid_str;
>+ dlock->locktoken = token;
>
>
I think it's pretty much obvious that lock all lock token management
function should live in libsvn_fs, not in mod_dav_svn. What we have here
is a layering violation.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12093 - branches/locking/subversion/mod_dav_svn
Posted by Branko Čibej <br...@xbc.nu>.
Ben Collins-Sussman wrote:
> I think circular dependencies are the problem. Do we really need to
> create a whole new libsvn_fs_util library? We've been flirting with
> the idea for months. We keep coming to the edge, then backing off.
yah...
> Maybe there's a simpler solution here. We've already defined the
> transparent svn_lock_t structure in svn_types.h, since almost every
> library needs to use it. Perhaps we can create
> svn_generate_locktoken() in libsvn_subr. That already seems to be our
> universal 'utility' library.
Hmmm. Granted that svn_lock_t is used everywhere, but only the FS layer
should _create_ lock tokens.
> My only worry is that maybe someday, svn_generate_locktoken() will
> want to use fs-specific stuff, such as the repos UUID, to generate
> tokens. Then we'd be in the situation of libsvn_subr "depending on"
> libsvn_fs_* to grab the repos UUID. Circular dependency again.
Exactly. So what you do is, you add "gen_lock_token" to the FS vtable,
and let the FS backend take care of the details. This takes care of all
the issues, I think.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12093 - branches/locking/subversion/mod_dav_svn
Posted by Ben Collins-Sussman <su...@collab.net>.
On Dec 1, 2004, at 10:32 AM, Branko Čibej wrote:
>>
>> 1. libsvn_fs_base needs to generate lock-token UUIDs.
>> 2. libsvn_fs_fs needs to generate lock-token UUIDs.
>> 3. mod_dav_svn needs to generate lock-token UUIDs.
>
> All of the above are wrong, because these modules need lock tokens,
> not lock token UUIDs.
Yes, yes. They need tokens. SVN defines a token to be an opaque
(const char *). The fact that a token happens to be a stringified
apr_uuid_t is a secret implementation detail. It's good to stay
specific here.
>
>> At the moment, this is nothing more than calling apr_uuid_get(). But
>> maybe we'll want to change that someday, make it fancier, use the
>> repos UUID + counter, whatever. So a shared routine would be nice.
>>
>> We could certainly make a function in libsvn_fs to do this; but then
>> wouldn't it be a layering violation for fs_base or fs_fs to "call up"
>> into the fs dispatch library?
>
> No, why? They don't call up into the fs dispatch library, they call
> into a fs utilities library. Which doesn't have to be in a separate
> shared lib, of course, it can live in libsvn_fs, which happens to be
> the only part of the FS layer that implements a public API that other
> modules can use. It seems the logical place.
>
> (n.b.: I don't see the term "layering" to mean "calls go only from
> higher to lower layers"
That's how the rest of us define it. :-)
> -- callbacks do exactly the reverse.
Well, we have a project convention that callbacks are the only legal
way for a layer to "call up" in the hierarchy.
> And you can look at the fs_util module as a different logical entity
> that lives side by side with fs, fs_fs, fs_base and mod_dav_svn -- but
> just happens to physically sit in libsvn_fs.)
>
> Ah yes Unless circular dependencies are a problem, in which case we'd
> need a separate shared lib anyway. They might be on Windows, who
> knows...
>
I think circular dependencies are the problem. Do we really need to
create a whole new libsvn_fs_util library? We've been flirting with
the idea for months. We keep coming to the edge, then backing off.
Maybe there's a simpler solution here. We've already defined the
transparent svn_lock_t structure in svn_types.h, since almost every
library needs to use it. Perhaps we can create
svn_generate_locktoken() in libsvn_subr. That already seems to be our
universal 'utility' library.
My only worry is that maybe someday, svn_generate_locktoken() will want
to use fs-specific stuff, such as the repos UUID, to generate tokens.
Then we'd be in the situation of libsvn_subr "depending on" libsvn_fs_*
to grab the repos UUID. Circular dependency again.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12093 - branches/locking/subversion/mod_dav_svn
Posted by Branko Čibej <br...@xbc.nu>.
Ben Collins-Sussman wrote:
>
> On Nov 30, 2004, at 8:39 PM, Branko Čibej wrote:
>
>> sussman@tigris.org wrote:
>>
>>> Author: sussman
>>> Date: Mon Nov 29 22:03:52 2004
>>> New Revision: 12093
>>>
>>
>>> + /* Generate a UUID. */
>>> + /* ### perhaps this should be a func in libsvn_fs.so, shared by
>>> + mod_dav_svn and both fs back-ends?? */
>>> + apr_uuid_get (&uuid);
>>> + apr_uuid_format (uuid_str, &uuid);
>>> + token->uuid_str = uuid_str;
>>> + dlock->locktoken = token;
>>>
>> I think it's pretty much obvious that lock all lock token management
>> function should live in libsvn_fs, not in mod_dav_svn. What we have
>> here is a layering violation.
>>
>
> Well, here's the problem:
>
> 1. libsvn_fs_base needs to generate lock-token UUIDs.
> 2. libsvn_fs_fs needs to generate lock-token UUIDs.
> 3. mod_dav_svn needs to generate lock-token UUIDs.
All of the above are wrong, because these modules need lock tokens, not
lock token UUIDs.
> At the moment, this is nothing more than calling apr_uuid_get(). But
> maybe we'll want to change that someday, make it fancier, use the
> repos UUID + counter, whatever. So a shared routine would be nice.
>
> We could certainly make a function in libsvn_fs to do this; but then
> wouldn't it be a layering violation for fs_base or fs_fs to "call up"
> into the fs dispatch library?
No, why? They don't call up into the fs dispatch library, they call into
a fs utilities library. Which doesn't have to be in a separate shared
lib, of course, it can live in libsvn_fs, which happens to be the only
part of the FS layer that implements a public API that other modules can
use. It seems the logical place.
(n.b.: I don't see the term "layering" to mean "calls go only from
higher to lower layers" -- callbacks do exactly the reverse. And you can
look at the fs_util module as a different logical entity that lives side
by side with fs, fs_fs, fs_base and mod_dav_svn -- but just happens to
physically sit in libsvn_fs.)
Ah yes Unless circular dependencies are a problem, in which case we'd
need a separate shared lib anyway. They might be on Windows, who knows...
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12093 - branches/locking/subversion/mod_dav_svn
Posted by Ben Collins-Sussman <su...@collab.net>.
On Nov 30, 2004, at 8:39 PM, Branko Čibej wrote:
> sussman@tigris.org wrote:
>
>> Author: sussman
>> Date: Mon Nov 29 22:03:52 2004
>> New Revision: 12093
>>
>
>> + /* Generate a UUID. */
>> + /* ### perhaps this should be a func in libsvn_fs.so, shared by
>> + mod_dav_svn and both fs back-ends?? */
>> + apr_uuid_get (&uuid);
>> + apr_uuid_format (uuid_str, &uuid);
>> + token->uuid_str = uuid_str;
>> + dlock->locktoken = token;
>>
> I think it's pretty much obvious that lock all lock token management
> function should live in libsvn_fs, not in mod_dav_svn. What we have
> here is a layering violation.
>
Well, here's the problem:
1. libsvn_fs_base needs to generate lock-token UUIDs.
2. libsvn_fs_fs needs to generate lock-token UUIDs.
3. mod_dav_svn needs to generate lock-token UUIDs.
At the moment, this is nothing more than calling apr_uuid_get(). But
maybe we'll want to change that someday, make it fancier, use the repos
UUID + counter, whatever. So a shared routine would be nice.
We could certainly make a function in libsvn_fs to do this; but then
wouldn't it be a layering violation for fs_base or fs_fs to "call up"
into the fs dispatch library?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org