On Wed, Oct 29, 2008 at 1:24 PM, <hwright_at_tigris.org> wrote:
> Author: hwright
> Date: Wed Oct 29 13:24:02 2008
> New Revision: 33954
>
> Log:
> Use sha1 checksums in the FSFS backend.
>
> Note: there is a bit of consternation about whether or not we should
> be using sha1. However, we already do so for the BDB backend (see r33306
> on the fs-rep-sharing branch), so this at least consistifies the backends.
> This commit is more about making things consistent than attempting to
> enfore a particular point of view.
Hmm, wait, does this actually remove all md5 usage from FSFS (unlike
BDB where we do double calculation)? As in, we're suddenly losing all
md5s, other than the non-constant-time ones in svn_fs_file_checksum in
fs-loader? That seems misguided... either we'll end up losing
reliability by failing to transmit base checksums, or we'll end up
recalculating md5s all the time instead of just doing it once at
commit time (I'm not sure which of these situations happen in which
places, but they're both bad).
--dave
> For further information on the debate, see:
> * log message to r28976.
> * http://svn.haxx.se/dev/archive-2008-10/0933.shtml
> * http://svn.haxx.se/dev/archive-2008-10/0832.shtml
> * http://svn.haxx.se/dev/archive-2008-10/0932.shtml
>
> * subversion/libsvn_fs_fs/tree.c
> (apply_textdelta): Only compare like-kinded checksums.
>
> * subversion/libsvn_fs_fs/fs_fs.c
> (read_rep_offsets): Parse both sha1 and md5 checksums.
> (rep_read_get_baton): Initialize the checksum context to be the same
> kind of checksum we already have for the rep.
> (rep_read_contents): Update comments.
> (rep_write_get_baton): Create a sha1 context instead of a md5 one.
> (rep_write_contents_close): Avoid and error leak.
>
> * subversion/libsvn_fs_fs/rep-cache.c
> (svn_fs_fs__get_Rep_reference, svn_fs_fs__set_rep_reference): Make sure we're
> only using sha1 checksums in the rep cache.
>
> * subversion/libsvn_fs_fs/dag.c
> (svn_fs_fs__dag_finalize_edits): Only compare like-kinded checksums.
>
> Modified:
> trunk/subversion/libsvn_fs_fs/dag.c
> trunk/subversion/libsvn_fs_fs/fs_fs.c
> trunk/subversion/libsvn_fs_fs/rep-cache.c
> trunk/subversion/libsvn_fs_fs/tree.c
>
> Modified: trunk/subversion/libsvn_fs_fs/dag.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_fs_fs/dag.c?pathrev=33954&r1=33953&r2=33954
> ==============================================================================
> --- trunk/subversion/libsvn_fs_fs/dag.c Wed Oct 29 13:07:54 2008 (r33953)
> +++ trunk/subversion/libsvn_fs_fs/dag.c Wed Oct 29 13:24:02 2008 (r33954)
> @@ -1012,7 +1012,8 @@ svn_fs_fs__dag_finalize_edits(dag_node_t
> svn_checksum_t *file_checksum;
>
> SVN_ERR(svn_fs_fs__dag_file_checksum(&file_checksum, file, pool));
> - if (!svn_checksum_match(checksum, file_checksum))
> + if (checksum->kind == file_checksum->kind
> + && !svn_checksum_match(checksum, file_checksum))
> return svn_error_createf(SVN_ERR_CHECKSUM_MISMATCH, NULL,
> _("Checksum mismatch, file '%s':\n"
> " expected: %s\n"
>
> Modified: trunk/subversion/libsvn_fs_fs/fs_fs.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_fs_fs/fs_fs.c?pathrev=33954&r1=33953&r2=33954
> ==============================================================================
> --- trunk/subversion/libsvn_fs_fs/fs_fs.c Wed Oct 29 13:07:54 2008 (r33953)
> +++ trunk/subversion/libsvn_fs_fs/fs_fs.c Wed Oct 29 13:24:02 2008 (r33954)
> @@ -1588,6 +1588,7 @@ read_rep_offsets(representation_t **rep_
> {
> representation_t *rep;
> char *str, *last_str;
> + int len;
>
> rep = apr_pcalloc(pool, sizeof(*rep));
> *rep_p = rep;
> @@ -1627,13 +1628,22 @@ read_rep_offsets(representation_t **rep_
>
> rep->expanded_size = apr_atoi64(str);
>
> - /* Read in the MD5 hash. */
> + /* Read in the hash. */
> str = apr_strtok(NULL, " ", &last_str);
> - if ((str == NULL) || (strlen(str) != (APR_MD5_DIGESTSIZE * 2)))
> + if (str == NULL)
> return svn_error_create(SVN_ERR_FS_CORRUPT, NULL,
> _("Malformed text rep offset line in node-rev"));
>
> - SVN_ERR(svn_checksum_parse_hex(&rep->checksum, svn_checksum_md5, str, pool));
> + len = strlen(str);
> + if (len == (APR_MD5_DIGESTSIZE * 2) )
> + SVN_ERR(svn_checksum_parse_hex(&rep->checksum, svn_checksum_md5, str,
> + pool));
> + else if (len == (APR_SHA1_DIGESTSIZE * 2) )
> + SVN_ERR(svn_checksum_parse_hex(&rep->checksum, svn_checksum_sha1, str,
> + pool));
> + else
> + return svn_error_create(SVN_ERR_FS_CORRUPT, NULL,
> + _("Malformed text rep offset line in node-rev"));
>
> /* Read the reuse count. */
> str = apr_strtok(NULL, " ", &last_str);
> @@ -2474,7 +2484,7 @@ rep_read_get_baton(struct rep_read_baton
> b->fs = fs;
> b->chunk_index = 0;
> b->buf = NULL;
> - b->checksum_ctx = svn_checksum_ctx_create(svn_checksum_md5, pool);
> + b->checksum_ctx = svn_checksum_ctx_create(rep->checksum->kind, pool);
> b->checksum_finalized = FALSE;
> b->checksum = svn_checksum_dup(rep->checksum, pool);
> b->len = rep->expanded_size;
> @@ -2750,8 +2760,7 @@ rep_read_contents(void *baton,
>
> /* Perform checksumming. We want to check the checksum as soon as
> the last byte of data is read, in case the caller never performs
> - a short read, but we don't want to finalize the MD5 context
> - twice. */
> + a short read, but we don't want to finalize the context twice. */
> if (!rb->checksum_finalized)
> {
> SVN_ERR(svn_checksum_update(rb->checksum_ctx, buf, *len));
> @@ -4490,7 +4499,7 @@ rep_write_get_baton(struct rep_write_bat
>
> b = apr_pcalloc(pool, sizeof(*b));
>
> - b->checksum_ctx = svn_checksum_ctx_create(svn_checksum_md5, pool);
> + b->checksum_ctx = svn_checksum_ctx_create(svn_checksum_sha1, pool);
>
> b->fs = fs;
> b->parent_pool = pool;
> @@ -4572,7 +4581,8 @@ rep_write_contents_close(void *baton)
> rep->revision = SVN_INVALID_REVNUM;
>
> /* Finalize the checksum. */
> - svn_checksum_final(&rep->checksum, b->checksum_ctx, b->parent_pool);
> + SVN_ERR(svn_checksum_final(&rep->checksum, b->checksum_ctx,
> + b->parent_pool));
>
> /* Check and see if we already have a representation somewhere that's
> identical to the one we just wrote out. */
>
> Modified: trunk/subversion/libsvn_fs_fs/rep-cache.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_fs_fs/rep-cache.c?pathrev=33954&r1=33953&r2=33954
> ==============================================================================
> --- trunk/subversion/libsvn_fs_fs/rep-cache.c Wed Oct 29 13:07:54 2008 (r33953)
> +++ trunk/subversion/libsvn_fs_fs/rep-cache.c Wed Oct 29 13:24:02 2008 (r33954)
> @@ -108,6 +108,12 @@ svn_fs_fs__get_rep_reference(representat
> fs_fs_data_t *ffd = fs->fsap_data;
> svn_boolean_t have_row;
>
> + /* We only allow SHA1 checksums in this table. */
> + if (checksum->kind != svn_checksum_sha1)
> + return svn_error_create(SVN_ERR_BAD_CHECKSUM_KIND, NULL,
> + _("Only SHA1 checksums can be used as keys in the "
> + "rep_cache table.\n"));
> +
> if (!ffd->rep_cache.get_rep_stmt)
> SVN_ERR(svn_sqlite__prepare(&ffd->rep_cache.get_rep_stmt, ffd->rep_cache.db,
> "select revision, offset, size, expanded_size from rep_cache "
> @@ -144,6 +150,12 @@ svn_fs_fs__set_rep_reference(svn_fs_t *f
> svn_boolean_t have_row;
> representation_t *old_rep;
>
> + /* We only allow SHA1 checksums in this table. */
> + if (rep->checksum->kind != svn_checksum_sha1)
> + return svn_error_create(SVN_ERR_BAD_CHECKSUM_KIND, NULL,
> + _("Only SHA1 checksums can be used as keys in the "
> + "rep_cache table.\n"));
> +
> /* Check to see if we already have a mapping for REP->CHECKSUM. If so,
> and the value is the same one we were about to write, that's
> cool -- just do nothing. If, however, the value is *different*,
>
> Modified: trunk/subversion/libsvn_fs_fs/tree.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_fs_fs/tree.c?pathrev=33954&r1=33953&r2=33954
> ==============================================================================
> --- trunk/subversion/libsvn_fs_fs/tree.c Wed Oct 29 13:07:54 2008 (r33953)
> +++ trunk/subversion/libsvn_fs_fs/tree.c Wed Oct 29 13:24:02 2008 (r33954)
> @@ -2421,7 +2421,8 @@ apply_textdelta(void *baton, apr_pool_t
> /* Until we finalize the node, its data_key points to the old
> contents, in other words, the base text. */
> SVN_ERR(svn_fs_fs__dag_file_checksum(&checksum, tb->node, pool));
> - if (!svn_checksum_match(tb->base_checksum, checksum))
> + if (tb->base_checksum->kind == checksum->kind
> + && !svn_checksum_match(tb->base_checksum, checksum))
> return svn_error_createf
> (SVN_ERR_CHECKSUM_MISMATCH,
> NULL,
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe_at_subversion.tigris.org
> For additional commands, e-mail: svn-help_at_subversion.tigris.org
>
>
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-10-29 23:14:58 CET