You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2008/09/26 18:53:13 UTC
Re: svn commit: r33306 - in branches/fs-rep-sharing/subversion/libsvn_fs_base: . bdb
On Thu, Sep 25, 2008 at 9:35 PM, <hw...@tigris.org> wrote:
>...
> +++ branches/fs-rep-sharing/subversion/libsvn_fs_base/dag.c Thu Sep 25 21:35:28 2008 (r33306)
>...
> @@ -1245,8 +1245,10 @@ svn_fs_base__dag_finalize_edits(dag_node
> SVN_ERR(svn_fs_base__rep_contents_checksum(&rep_checksum, fs,
> noderev->edit_key, trail, pool));
>
> - /* If our caller provided a checksum to compare, do so. */
> - if (checksum && !svn_checksum_match(checksum, rep_checksum))
> + /* If our caller provided a checksum of the right kind to compare, do so. */
> + if (checksum
> + && checksum->kind == rep_checksum->kind
> + && !svn_checksum_match(checksum, rep_checksum))
Why check the kind? Doesn't svn_checksum_match() do that? And if not,
then it *should*.
>...
> +++ branches/fs-rep-sharing/subversion/libsvn_fs_base/reps-strings.c Thu Sep 25 21:35:28 2008 (r33306)
>...
> @@ -638,12 +638,19 @@ struct rep_read_baton
>...
> + /* MD5 checksum context. Initialized when the baton is created, updated as
> + we read data, and finalized when the stream is closed. */
> + svn_checksum_ctx_t *sha1_checksum_ctx;
Bad comment. That's the SHA1 checksum context.
And do we really need *both* forms? Why?
>...
> +++ branches/fs-rep-sharing/subversion/libsvn_fs_base/tree.c Thu Sep 25 21:35:28 2008 (r33306)
> @@ -3672,7 +3672,12 @@ txn_body_apply_textdelta(void *baton, tr
> contents, in other words, the base text. */
> SVN_ERR(svn_fs_base__dag_file_checksum(&checksum, tb->node,
> trail, trail->pool));
> - if (!svn_checksum_match(tb->base_checksum, checksum))
> + /* TODO: This only compares checksums if they are the same kind, but
> + we're calculating both SHA1 and MD5 checksums somewhere in
> + reps-strings.c. Could we keep them both around somehow so this
> + check could be more comprehensive? */
> + if (tb->base_checksum->kind == checksum->kind
> + && !svn_checksum_match(tb->base_checksum, checksum))
> return svn_error_createf
> (SVN_ERR_CHECKSUM_MISMATCH,
Again, why check the kind? Should be able to just call svn_checksum_match().
Note that you may want to consider something like:
svn_checksum_match_either(checksum, md5_desired, sha1_desired)
Then the checksum can be checked against the appropriate kind.
Cheers,
-g
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r33306 - in branches/fs-rep-sharing/subversion/libsvn_fs_base: . bdb
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Sep 30, 2008 at 7:28 AM, Hyrum K. Wright
<hy...@mail.utexas.edu> wrote:
> Greg Stein wrote:
>> On Thu, Sep 25, 2008 at 9:35 PM, <hw...@tigris.org> wrote:
>...
>>> + /* If our caller provided a checksum of the right kind to compare, do so. */
>>> + if (checksum
>>> + && checksum->kind == rep_checksum->kind
>>> + && !svn_checksum_match(checksum, rep_checksum))
>>
>> Why check the kind? Doesn't svn_checksum_match() do that? And if not,
>> then it *should*.
>
> If the kinds differ, svn_checksum_match() will return FALSE. In this case, we
> don't want that, we don't even want to check different kinds of checksums. The
> root of the problem here is that since we're storing sha1 checksums, but dump
> files and ra frontends are giving us md5 checksums, we don't want to fail just
> because a checksum mismatch.
Ah. Gotcha.
Sounds a bit dangerous though, as those checksums are looking for
(say) on-wire corruption, or on-disk corruption.
>...
>> And do we really need *both* forms? Why?
>
> See above. This is an attempt (albeit fruitless at this point), to compute
> *both* checksums, so that we can compare the same type in situations such as the
> above.
Fruitless? Hmm?
Cheers,
-g
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r33306 - in branches/fs-rep-sharing/subversion/libsvn_fs_base:
. bdb
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Greg Stein wrote:
> On Tue, Sep 30, 2008 at 7:28 AM, Hyrum K. Wright
> <hy...@mail.utexas.edu> wrote:
>> ...
>>>> +++ branches/fs-rep-sharing/subversion/libsvn_fs_base/reps-strings.c Thu Sep 25 21:35:28 2008 (r33306)
>>>> ...
>>>> @@ -638,12 +638,19 @@ struct rep_read_baton
>>>> ...
>>>> + /* MD5 checksum context. Initialized when the baton is created, updated as
>>>> + we read data, and finalized when the stream is closed. */
>>>> + svn_checksum_ctx_t *sha1_checksum_ctx;
>>> Bad comment. That's the SHA1 checksum context.
>
> Oh... and don't forget this one.
r33362
Thanks,
-Hyrum
Re: svn commit: r33306 - in branches/fs-rep-sharing/subversion/libsvn_fs_base: . bdb
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Sep 30, 2008 at 7:28 AM, Hyrum K. Wright
<hy...@mail.utexas.edu> wrote:
>...
>>> +++ branches/fs-rep-sharing/subversion/libsvn_fs_base/reps-strings.c Thu Sep 25 21:35:28 2008 (r33306)
>>> ...
>>> @@ -638,12 +638,19 @@ struct rep_read_baton
>>> ...
>>> + /* MD5 checksum context. Initialized when the baton is created, updated as
>>> + we read data, and finalized when the stream is closed. */
>>> + svn_checksum_ctx_t *sha1_checksum_ctx;
>>
>> Bad comment. That's the SHA1 checksum context.
Oh... and don't forget this one.
Cheers,
-g
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r33306 - in branches/fs-rep-sharing/subversion/libsvn_fs_base:
. bdb
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Greg Stein wrote:
> On Thu, Sep 25, 2008 at 9:35 PM, <hw...@tigris.org> wrote:
>> ...
>> +++ branches/fs-rep-sharing/subversion/libsvn_fs_base/dag.c Thu Sep 25 21:35:28 2008 (r33306)
>> ...
>> @@ -1245,8 +1245,10 @@ svn_fs_base__dag_finalize_edits(dag_node
>> SVN_ERR(svn_fs_base__rep_contents_checksum(&rep_checksum, fs,
>> noderev->edit_key, trail, pool));
>>
>> - /* If our caller provided a checksum to compare, do so. */
>> - if (checksum && !svn_checksum_match(checksum, rep_checksum))
>> + /* If our caller provided a checksum of the right kind to compare, do so. */
>> + if (checksum
>> + && checksum->kind == rep_checksum->kind
>> + && !svn_checksum_match(checksum, rep_checksum))
>
> Why check the kind? Doesn't svn_checksum_match() do that? And if not,
> then it *should*.
If the kinds differ, svn_checksum_match() will return FALSE. In this case, we
don't want that, we don't even want to check different kinds of checksums. The
root of the problem here is that since we're storing sha1 checksums, but dump
files and ra frontends are giving us md5 checksums, we don't want to fail just
because a checksum mismatch.
>> ...
>> +++ branches/fs-rep-sharing/subversion/libsvn_fs_base/reps-strings.c Thu Sep 25 21:35:28 2008 (r33306)
>> ...
>> @@ -638,12 +638,19 @@ struct rep_read_baton
>> ...
>> + /* MD5 checksum context. Initialized when the baton is created, updated as
>> + we read data, and finalized when the stream is closed. */
>> + svn_checksum_ctx_t *sha1_checksum_ctx;
>
> Bad comment. That's the SHA1 checksum context.
>
> And do we really need *both* forms? Why?
See above. This is an attempt (albeit fruitless at this point), to compute
*both* checksums, so that we can compare the same type in situations such as the
above.
>> ...
>> +++ branches/fs-rep-sharing/subversion/libsvn_fs_base/tree.c Thu Sep 25 21:35:28 2008 (r33306)
>> @@ -3672,7 +3672,12 @@ txn_body_apply_textdelta(void *baton, tr
>> contents, in other words, the base text. */
>> SVN_ERR(svn_fs_base__dag_file_checksum(&checksum, tb->node,
>> trail, trail->pool));
>> - if (!svn_checksum_match(tb->base_checksum, checksum))
>> + /* TODO: This only compares checksums if they are the same kind, but
>> + we're calculating both SHA1 and MD5 checksums somewhere in
>> + reps-strings.c. Could we keep them both around somehow so this
>> + check could be more comprehensive? */
>> + if (tb->base_checksum->kind == checksum->kind
>> + && !svn_checksum_match(tb->base_checksum, checksum))
>> return svn_error_createf
>> (SVN_ERR_CHECKSUM_MISMATCH,
>
> Again, why check the kind? Should be able to just call svn_checksum_match().
>
> Note that you may want to consider something like:
>
> svn_checksum_match_either(checksum, md5_desired, sha1_desired)
>
> Then the checksum can be checked against the appropriate kind.
Hmmm....I'll take a look at this.
-Hyrum