You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2010/07/20 18:14:53 UTC
svn commit: r965892 - in /subversion/trunk: notes/dump-load-format.txt
subversion/include/svn_repos.h subversion/libsvn_repos/dump.c
Author: stsp
Date: Tue Jul 20 16:14:53 2010
New Revision: 965892
URL: http://svn.apache.org/viewvc?rev=965892&view=rev
Log:
Make svnadmin dump print headers containing MD5 and SHA1 checksums of
property content, as was already done for file content. Checksums are
printed for revision properties as well as versioned properties.
* notes/dump-load-format.txt: Document the new Prop-content-md5 and
Prop-content-sha1 headers. These headers are optional, and unknown
headers are ignored, so no format version number bump is needed.
* subversion/include/svn_repos.h
(SVN_REPOS_DUMPFILE_PROP_CONTENT_MD5,
SVN_REPOS_DUMPFILE_PROP_CONTENT_SHA1): New.
* subversion/libsvn_repos/dump.c
(write_prop_content_checksums): New. Writes Prop-content-md5 and
Prop-content-SHA1 headers for a given property hash.
(dump_nodem, write_revision_record): When dumping properties, generate
Prop-content-md5 and Prop-content-sha1 headers.
Modified:
subversion/trunk/notes/dump-load-format.txt
subversion/trunk/subversion/include/svn_repos.h
subversion/trunk/subversion/libsvn_repos/dump.c
Modified: subversion/trunk/notes/dump-load-format.txt
URL: http://svn.apache.org/viewvc/subversion/trunk/notes/dump-load-format.txt?rev=965892&r1=965891&r2=965892&view=diff
==============================================================================
--- subversion/trunk/notes/dump-load-format.txt (original)
+++ subversion/trunk/notes/dump-load-format.txt Tue Jul 20 16:14:53 2010
@@ -23,6 +23,8 @@ following:
[Text-delta-base-sha1: blob]
[Text-copy-source-sha1: blob]
[Text-content-sha1: blob]
+[Prop-content-md5: (<property name>) blob]
+[Prop-content-sha1: (<property name>) blob]
The default value for the boolean headers is "false". If the value is
set to "true", then the text and property contents will be treated
@@ -51,6 +53,9 @@ currently used by the loader. They are
Subversion so that future loaders can optionally choose which checksum to
use for checking for corruption.
+Prop-content-md5 and Prop-content-sha1 are written by 1.7-and-later versions,
+and can optionally be used to verify property content upon loading.
+
===== SVN DUMPFILE VERSION 2 FORMAT =====
(generated by SVN versions 0.18.0-present, by default)
Modified: subversion/trunk/subversion/include/svn_repos.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_repos.h?rev=965892&r1=965891&r2=965892&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_repos.h (original)
+++ subversion/trunk/subversion/include/svn_repos.h Tue Jul 20 16:14:53 2010
@@ -2323,6 +2323,10 @@ svn_repos_node_from_baton(void *edit_bat
/** @since New in 1.5. */
#define SVN_REPOS_DUMPFILE_TEXT_DELTA_BASE_CHECKSUM \
SVN_REPOS_DUMPFILE_TEXT_DELTA_BASE_MD5
+/** @since New in 1.7. */
+#define SVN_REPOS_DUMPFILE_PROP_CONTENT_MD5 "Prop-content-md5"
+/** @since New in 1.7. */
+#define SVN_REPOS_DUMPFILE_PROP_CONTENT_SHA1 "Prop-content-sha1"
/**
* Verify the contents of the file system in @a repos.
Modified: subversion/trunk/subversion/libsvn_repos/dump.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/dump.c?rev=965892&r1=965891&r2=965892&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_repos/dump.c (original)
+++ subversion/trunk/subversion/libsvn_repos/dump.c Tue Jul 20 16:14:53 2010
@@ -295,6 +295,53 @@ make_dir_baton(const char *path,
}
+/* For each property in PROPHASH, write headers containing MD5 and SHA1
+ * checksums of property content to STREAM.
+ * Do temporary allocations in SCRATCH_POOL. */
+static svn_error_t *
+write_prop_content_checksums(svn_stream_t *stream,
+ apr_hash_t *prophash,
+ apr_pool_t *scratch_pool)
+{
+ apr_hash_index_t *hi;
+ apr_pool_t *iterpool;
+
+ iterpool = svn_pool_create(scratch_pool);
+ for (hi = apr_hash_first(scratch_pool, prophash);
+ hi;
+ hi = apr_hash_next(hi))
+ {
+ svn_checksum_t *checksum;
+ const char *hex_digest;
+ const char *propname;
+ svn_string_t *propval;
+
+ svn_pool_clear(iterpool);
+
+ propname = svn__apr_hash_index_key(hi);
+ propval = svn__apr_hash_index_val(hi);
+
+ SVN_ERR(svn_checksum(&checksum, svn_checksum_md5,
+ propval->data, propval->len, iterpool));
+ hex_digest = svn_checksum_to_cstring(checksum, iterpool);
+ if (hex_digest)
+ SVN_ERR(svn_stream_printf(stream, iterpool,
+ SVN_REPOS_DUMPFILE_PROP_CONTENT_MD5
+ ": (%s) %s\n", propname, hex_digest));
+ SVN_ERR(svn_checksum(&checksum, svn_checksum_sha1,
+ propval->data, propval->len, iterpool));
+ hex_digest = svn_checksum_to_cstring(checksum, iterpool);
+ if (hex_digest)
+ SVN_ERR(svn_stream_printf(stream, iterpool,
+ SVN_REPOS_DUMPFILE_PROP_CONTENT_SHA1
+ ": (%s) %s\n", propname, hex_digest));
+ }
+ svn_pool_destroy(iterpool);
+
+ return SVN_NO_ERROR;
+}
+
+
/* This helper is the main "meat" of the editor -- it does all the
work of writing a node record.
@@ -565,6 +612,7 @@ dump_node(struct edit_baton *eb,
SVN_ERR(svn_stream_printf(eb->stream, pool,
SVN_REPOS_DUMPFILE_PROP_CONTENT_LENGTH
": %" APR_SIZE_T_FMT "\n", proplen));
+ SVN_ERR(write_prop_content_checksums(eb->stream, prophash, pool));
}
/* If we are supposed to dump text, write out a text length header
@@ -992,6 +1040,7 @@ write_revision_record(svn_stream_t *stre
SVN_REPOS_DUMPFILE_PROP_CONTENT_LENGTH
": %" APR_SIZE_T_FMT "\n",
encoded_prophash->len));
+ SVN_ERR(write_prop_content_checksums(stream, props, pool));
/* Write out a regular Content-length header for the benefit of
non-Subversion RFC-822 parsers. */
Re: svn commit: r965892 - in /subversion/trunk: notes/dump-load-format.txt
subversion/include/svn_repos.h subversion/libsvn_repos/dump.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 07/20/2010 12:14 PM, stsp@apache.org wrote:
> Author: stsp
> Date: Tue Jul 20 16:14:53 2010
> New Revision: 965892
[...]
> (dump_nodem, write_revision_record): When dumping properties, generate
^ Oops?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r965892 - in /subversion/trunk: notes/dump-load-format.txt
subversion/include/svn_repos.h subversion/libsvn_repos/dump.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 07/22/2010 11:33 AM, Stefan Sperling wrote:
> On Thu, Jul 22, 2010 at 10:56:11AM -0400, C. Michael Pilato wrote:
>> The error you show above happens deep inside the FS implementations' DAG
>> layer, as a sanity check on a text-base before apply a delta to it. It's
>> probably comparing the checksum stored in the FS for a given text-base
>> against the checksum coming across the wire as the delta-base checksum. The
>> FS layer doesn't accept untrusted checksums. That is, you can't tell the FS
>> what the checksum for a file's contents are -- you can only tell it the
>> file's contents, and you can tell it what you expect the checksum to be
>> after you store new file contents. I suspect the customer had actual
>> repository corruption that was only detectable by svnsync after the fact
>> because svnsync had nothing to do (directly, at least) with that corruption
>> in the first place.
>
> And what does the code do if the content stored doesn't match the
> expected checksum? As far as I understand, the sequence of events
> if everything works fine is something like this:
>
> - syncsync wants to commit r40
> [content: 'foo'
> expected checksum: 0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33 ]
- filesystem writes 'foo' into transaction, and calculates the content
checksum as it does so.
- filesystem verifies that calculated checksum matches the "expected
checksum" above. If it doesn't, the commit is failed.
- filesystem now has 'foo' stored in transaction, and checksum('foo')
stored in the transaction, too.
- all else works out for the commit, so the transaction is promoted
to a new revision. (In BDB this is a single table tweak. In FSFS,
doesn't this result in the transaction file being rewritten as a
new revision file with updated node-rev-ids? I don't know.)
- SOME BAD EVENT OCCURS
> - svnsync tries committing r41:
> [delta: s/^f/b/
> expected checksum: 78b371f0ea1410abc62ccb9b7f40c34288a72e1a
> expected base checksum: 0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33 ]
>
> Now, as r41 is committed, the filesystem sees the base checksum
> matches, the diff is applied, and the content becomes 'boo' in r41,
> again matching the expected checksum.
>
> It's probably more complicated than the above in reality.
> But if this is the gist of it, what went wrong at the customer site?
> Did the wrong content get stored in r40? Or was the expected base checksum
> transmitted wrongly when the commit of r41 was attempted? And how can we tell?
If wrong content was stored for r40, then that wrong content's checksum
matched the expected one for r40. Which seems unlikely.
Bogus expected base checksum transmitted for r41? That's possible.
> That's a good point. If comparing the content is sufficiently cheap,
> we don't need to bother with checksums. Should I try making the loader
> perform such checks on transactions before commit (and also make it do
> similar checks on the content if checksums were provided by the dump file)?
The loader already does checksum validation for text contents when the
checksums are present. It doesn't do so directly, but via the exact same
mechanisms I mentioned above that all code writing file contents to the FS
go through.
The problem we have is that the FS layer doesn't do any validation of
property contents *at all*, so the loader can't rely on the FS to do this
work for it. If you want to add such code to the loader, that's fine with
me. But do it with a comment that indicates that this validation really
ought to be happening in the FS someday, please.
>> (And I will further suggest that we fix the dump.c code to pass FALSE for
>> the 'force' parameter of svn_fs_file_checksum() calls, thus restoring the
>> pre-Hyrums-change behavior.)
>
> Would this mean that we will never output checksums for some files?
> How can we make sure checksums are stored for files which don't have
> them yet, so that the dumps will include checksums even if force == FALSE?
>
> If I understood correctly, for BDB we don't have to do anything special.
> But we'll need to shout curses and maybe hush some magic spells at FSFS
> until it generates checksums which are missing?
Yes, this means the loader will never *generate* a checksum (MD5, SHA1, or
otherwise). It will only provide the checksums that the FS have stored
already for that body of file contents. Remember, we're trusting our dump
stream here. If the file contents in the repository are wrong and don't
have a checksum, there's no value in generating a perfectly valid checksum
for those wrong contents just to make ourselves feel better about it.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r965892 - in /subversion/trunk:
notes/dump-load-format.txt subversion/include/svn_repos.h
subversion/libsvn_repos/dump.c
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jul 22, 2010 at 10:56:11AM -0400, C. Michael Pilato wrote:
> svnsync doesn't do any checksum verification. That work is done by deeper
> levels. But you provide no support for the implication that Subversion
> didn't attempt to verify checksums at all the reasonable times.
Looks like I need to spend some time in the backend code to get
a better understanding of it.
> The error you show above happens deep inside the FS implementations' DAG
> layer, as a sanity check on a text-base before apply a delta to it. It's
> probably comparing the checksum stored in the FS for a given text-base
> against the checksum coming across the wire as the delta-base checksum. The
> FS layer doesn't accept untrusted checksums. That is, you can't tell the FS
> what the checksum for a file's contents are -- you can only tell it the
> file's contents, and you can tell it what you expect the checksum to be
> after you store new file contents. I suspect the customer had actual
> repository corruption that was only detectable by svnsync after the fact
> because svnsync had nothing to do (directly, at least) with that corruption
> in the first place.
And what does the code do if the content stored doesn't match the
expected checksum? As far as I understand, the sequence of events
if everything works fine is something like this:
- syncsync wants to commit r40
[content: 'foo'
expected checksum: 0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33 ]
- filesystem now has 'foo' stored in transaction. Expected checksum
matches the content read back from disk.
Everything looks fine so we can commit the transaction.
(Are transactions being validated before they are committed?)
- svnsync tries committing r41:
[delta: s/^f/b/
expected checksum: 78b371f0ea1410abc62ccb9b7f40c34288a72e1a
expected base checksum: 0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33 ]
Now, as r41 is committed, the filesystem sees the base checksum
matches, the diff is applied, and the content becomes 'boo' in r41,
again matching the expected checksum.
It's probably more complicated than the above in reality.
But if this is the gist of it, what went wrong at the customer site?
Did the wrong content get stored in r40? Or was the expected base checksum
transmitted wrongly when the commit of r41 was attempted? And how can we tell?
Note again that removing r40 fixed the problem.
> Okay. It seems (if I'm reading you correctly) that we agree we shouldn't be
> using checksums to verify the integrity of the dump stream. That the value
> to be had here is in verifying that target repository, once loaded, has the
> right data.
Yes.
> So may I suggest that, when dealing with property lists, it's just a lot
> simpler (especially with our APIs) to compare the before-and-after property
> lists than to trade in the currency of checksums? I mean, for file contents
> it's arguably cheaper to do a checksum compare than a byte-for-byte
> comparison (which isn't even possible in --deltas mode), and that mostly
> because the checksum is pre-computed. But for properties? We don't have
> the checksums handy (so there's extra cost to compute them), and we hold
> those things en totale in memory all the time, and have handy routines for
> finding diffs. I dunno. The property checksums just seem unnecessary in
> the whole scheme of things to me.
That's a good point. If comparing the content is sufficiently cheap,
we don't need to bother with checksums. Should I try making the loader
perform such checks on transactions before commit (and also make it do
similar checks on the content if checksums were provided by the dump file)?
> (And I will further suggest that we fix the dump.c code to pass FALSE for
> the 'force' parameter of svn_fs_file_checksum() calls, thus restoring the
> pre-Hyrums-change behavior.)
Would this mean that we will never output checksums for some files?
How can we make sure checksums are stored for files which don't have
them yet, so that the dumps will include checksums even if force == FALSE?
If I understood correctly, for BDB we don't have to do anything special.
But we'll need to shout curses and maybe hush some magic spells at FSFS
until it generates checksums which are missing?
Stefan
Re: svn commit: r965892 - in /subversion/trunk:
notes/dump-load-format.txt subversion/include/svn_repos.h
subversion/libsvn_repos/dump.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Nov 03, 2010 at 08:13:54PM +0100, Stefan Sperling wrote:
> On Wed, Nov 03, 2010 at 02:34:11PM -0400, C. Michael Pilato wrote:
> > On 08/19/2010 04:42 PM, Stefan Sperling wrote:
> > > On Thu, Jul 22, 2010 at 10:56:11AM -0400, C. Michael Pilato wrote:
> > >> (And I will further suggest that we fix the dump.c code to pass FALSE for
> > >> the 'force' parameter of svn_fs_file_checksum() calls, thus restoring the
> > >> pre-Hyrums-change behavior.)
> > >
> > > Picking up on this old topic: I am planning to make the change Mike
> > > is proposing above "really soon now"(tm), unless I hear objections.
> >
> > I didn't hear objections, but I also don't see that this change was made.
>
> I had forgotten about it :)
> Thanks for reminding me.
r1030637
Re: svn commit: r965892 - in /subversion/trunk:
notes/dump-load-format.txt subversion/include/svn_repos.h
subversion/libsvn_repos/dump.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Nov 03, 2010 at 02:34:11PM -0400, C. Michael Pilato wrote:
> On 08/19/2010 04:42 PM, Stefan Sperling wrote:
> > On Thu, Jul 22, 2010 at 10:56:11AM -0400, C. Michael Pilato wrote:
> >> (And I will further suggest that we fix the dump.c code to pass FALSE for
> >> the 'force' parameter of svn_fs_file_checksum() calls, thus restoring the
> >> pre-Hyrums-change behavior.)
> >
> > Picking up on this old topic: I am planning to make the change Mike
> > is proposing above "really soon now"(tm), unless I hear objections.
>
> I didn't hear objections, but I also don't see that this change was made.
I had forgotten about it :)
Thanks for reminding me.
Re: svn commit: r965892 - in /subversion/trunk: notes/dump-load-format.txt
subversion/include/svn_repos.h subversion/libsvn_repos/dump.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 08/19/2010 04:42 PM, Stefan Sperling wrote:
> On Thu, Jul 22, 2010 at 10:56:11AM -0400, C. Michael Pilato wrote:
>> (And I will further suggest that we fix the dump.c code to pass FALSE for
>> the 'force' parameter of svn_fs_file_checksum() calls, thus restoring the
>> pre-Hyrums-change behavior.)
>
> Picking up on this old topic: I am planning to make the change Mike
> is proposing above "really soon now"(tm), unless I hear objections.
I didn't hear objections, but I also don't see that this change was made.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r965892 - in /subversion/trunk:
notes/dump-load-format.txt subversion/include/svn_repos.h
subversion/libsvn_repos/dump.c
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jul 22, 2010 at 10:56:11AM -0400, C. Michael Pilato wrote:
> (And I will further suggest that we fix the dump.c code to pass FALSE for
> the 'force' parameter of svn_fs_file_checksum() calls, thus restoring the
> pre-Hyrums-change behavior.)
Picking up on this old topic: I am planning to make the change Mike
is proposing above "really soon now"(tm), unless I hear objections.
Stefan
Re: svn commit: r965892 - in /subversion/trunk: notes/dump-load-format.txt
subversion/include/svn_repos.h subversion/libsvn_repos/dump.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 07/22/2010 10:11 AM, Stefan Sperling wrote:
> So we should not have added the additional checksums in the first place?
> You might as well point fingers at the old backends for not storing checksums,
> rather than at Hyrum trying to do his best with what he had.
You have to define "his best" here. Hyrum could have done his best to avoid
adding to the cost of dumpfile creation by passing FALSE for the 'force'
parameter to svn_fs_file_checksum(). Hyrum could have done his best to
carry self-validating information for all file contents by passing TRUE,
instead, and then changing the surrounding the code to expect a valid
checksum from those function calls. Given the state of the code today,
Hyrum didn't do his best at either of those approaches. :-)
>> (Some of those may reduce away if the
>> backend is able to store the SHA1's forcibly generated from previous requests.)
>
> Is this already implemented? So if people upgrade the repository format,
> over time the dump performance will get better again?
It probably is for BDB repositories.
It probably is not for FSFS repositories.
I probably won't use this opportunity to gripe about FSFS. Again. Or maybe
I will.
> Let me explain why I did this:
>
> The idea for this change came to me in a conversation with Michael Diers.
> He said he saw no way of verifying that svnsync created proper duplicates
> of a repository. He had seen svnsync breaking repositories at a customer
> site like this:
>
> Transmitting file data .svnsync: Base checksum mismatch on 'foobar':
> expected: a9c9eb4994b50f9667e0c1509782df26
> actual: ae3957e324c0ee2228d76732c9a89747
[...]
> Obviously svnsync did verification *after* something got broken.
> But why didn't svnsync detect the checksum mismatch before committing
> a broken transaction? I don't know. They could not reproduce the problem
> either. But they've seen it and they are afraid of it happening again.
svnsync doesn't do any checksum verification. That work is done by deeper
levels. But you provide no support for the implication that Subversion
didn't attempt to verify checksums at all the reasonable times.
The error you show above happens deep inside the FS implementations' DAG
layer, as a sanity check on a text-base before apply a delta to it. It's
probably comparing the checksum stored in the FS for a given text-base
against the checksum coming across the wire as the delta-base checksum. The
FS layer doesn't accept untrusted checksums. That is, you can't tell the FS
what the checksum for a file's contents are -- you can only tell it the
file's contents, and you can tell it what you expect the checksum to be
after you store new file contents. I suspect the customer had actual
repository corruption that was only detectable by svnsync after the fact
because svnsync had nothing to do (directly, at least) with that corruption
in the first place.
> They trust Subversion with their data, and such problems don't exactly
> encourage them to keep trusting it. So I think we shouldn't dismiss the
> value of internal integrity checks entirely just because most parts of
> Subversion don't care [yet]. Of course such checks need to be placed
> in the right spots.
That's fair. I certainly don't want to play down the importance of
Subversion's ability to reliably version folks' data! I see the error above
as proof that Subversion is doing exactly that, not that it has failed to do so.
> Though I agree that relying on content checksums to make sure data copied
> from one computer to another has been copied correctly is not that
> important anymore these days. We're not copying data across noisy serial
> links anymore, and have checksumming on many different levels (e.g.
> ethernet CRC and disk controller CRC).
> But what additional verification shields us from is subtle bugs in our
> own code that cause data corruption. These cannot be caught by lower
> layers and evidently seem to exist (or maybe the customer had a hardware
> problem, but I doubt that).
>
> So what's nice is that the content checksums allow verification of data
> read back from a freshly loaded repository, or even from a transaction
> before commit. I.e. the loader can make sure that the data that it wrote
> into the repository matches what the dump file expects. This can catch
> bugs in the loader code. My plan was to make svnsync do similar checks later.
>
> As for integrity of dump files themselves, we should tell paranoid
> people to sign dump files after they've been created, and verify the
> signatures independently of Subversion.
Okay. It seems (if I'm reading you correctly) that we agree we shouldn't be
using checksums to verify the integrity of the dump stream. That the value
to be had here is in verifying that target repository, once loaded, has the
right data.
So may I suggest that, when dealing with property lists, it's just a lot
simpler (especially with our APIs) to compare the before-and-after property
lists than to trade in the currency of checksums? I mean, for file contents
it's arguably cheaper to do a checksum compare than a byte-for-byte
comparison (which isn't even possible in --deltas mode), and that mostly
because the checksum is pre-computed. But for properties? We don't have
the checksums handy (so there's extra cost to compute them), and we hold
those things en totale in memory all the time, and have handy routines for
finding diffs. I dunno. The property checksums just seem unnecessary in
the whole scheme of things to me.
(And I will further suggest that we fix the dump.c code to pass FALSE for
the 'force' parameter of svn_fs_file_checksum() calls, thus restoring the
pre-Hyrums-change behavior.)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r965892 - in /subversion/trunk:
notes/dump-load-format.txt subversion/include/svn_repos.h
subversion/libsvn_repos/dump.c
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jul 22, 2010 at 09:00:25AM -0400, C. Michael Pilato wrote:
> On 07/22/2010 06:15 AM, Stefan Sperling wrote:
> > On Thu, Jul 22, 2010 at 09:53:35AM +0100, Philip Martin wrote:
> >> Perhaps we could just have one checksum that includes all the property
> >> names and values?
> >
> > I'd like loaders to be able to tell users which properties are
> > corrupted.
>
> This change feels obnoxious to me, adding still more cost to the dump
> process for little theoretical gain and zero practical gain (since our own
> loader does nothing with the information).
My plan was to make the loader do something with the information next.
> When the dump stream format and code was first designed, if the FS backend
> hadn't calculated and stored a checksum for the file contents, no checksum
> was placed into the dumpstream. Further, when in --deltas mode, the
> checksum you (might) get in the dumpstream is, again, the checksum stored in
> the FS backend for the fulltext of the file contents -- it doesn't represent
> the checksum of the svndiff-compressed delta in the stream. This was the
> overarching sentiment of the dump format: to carry information stored in
> the FS backend, as much as can be useful to the folks on the receiving end,
> but certainly without unnecessary cost to the dumpfile producer.
That reasoning makes sense. But there's no reason the FS backend
could not provide more information in the future.
> Back when Hyrum did his checksum API overhaul (in r872722, to be precise),
> he made a subtle change here that deviated from this sentiment. He told the
> dumpstream generator code to forcibly generate checksums for placement in
> the dump stream (he passed 'TRUE' for the 'force' parameter of the new
> svn_fs_file_checksum() function everywhere). The code today is left reading
> kinda weird as a result:
>
> checksum = svn_fs_file_checksum(FORCE)
> hex_digest = svn_checksum_to_string(checksum)
> if (hex_digest)
> ... #### Wait -- "*if*" hexdigest? You forced its creation above!
>
> Given the vast amount of Subversion data in existence today that probably
> doesn't have an FS-stored SHA1 checksum (since we introduced that only in
> the past couple of years), the immediate affect of these changes was that
> the dumper code was now running over every fulltext of every file an
> additional time just to generate SHA1's (since older repositories didn't
> store SHA1's). And then again for copyfrom sources. And then again for
> text bases (when in --deltas mode).
So we should not have added the additional checksums in the first place?
You might as well point fingers at the old backends for not storing checksums,
rather than at Hyrum trying to do his best with what he had.
I don't see a big problem. Just that it would be much nicer to have the
checksums in all repositories already.
> (Some of those may reduce away if the
> backend is able to store the SHA1's forcibly generated from previous requests.)
Is this already implemented? So if people upgrade the repository format,
over time the dump performance will get better again?
> Hyrum also changed the loader code so that it forcibly acquires a checksum
> for copyfrom sources for comparison against the copyfrom-checksum values
> from the stream.
>
> As if dump/load needed to run any slower.
Property values are usually not large, so the overhead introduced
by my change should be much lower than that of Hyrum's change.
> You've extended this behavioral change now. Tell me -- if the FS backend
> and the rest of Subversion don't care enough about the possibility of
> property corruption to store and transmit checksums for those things,
> where's the real value in teaching the dump format (alone) to do so?
Of course, there isn't much value in that alone.
But it's not as if the rest of Subversion could not get any more
paranoid in the future.
Let me explain why I did this:
The idea for this change came to me in a conversation with Michael Diers.
He said he saw no way of verifying that svnsync created proper duplicates
of a repository. He had seen svnsync breaking repositories at a customer
site like this:
Transmitting file data .svnsync: Base checksum mismatch on 'foobar':
expected: a9c9eb4994b50f9667e0c1509782df26
actual: ae3957e324c0ee2228d76732c9a89747
To fix replication they had to rollback the mirror to an earlier revision.
Now that replication was fixed, the customer asked what could be done to
make sure no such corruption had occurred earlier, but went unnoticed.
So you can dump the master and dump the clone, and then compare the
dump files. Then another question was raised: If dump files can easily
be verified with e.g. sha1sum, and incremental dumps exist, why use svnsync
in the first place if svnadmin dump/load, sha1sum and rsync can effectively
provide the same functionality, but with better verification?
create dump on master
note sha1 checksum
copy dump to clone
load dump on clone
dump revision just loaded
note sha1 checksum
compare checksums
Obviously svnsync did verification *after* something got broken.
But why didn't svnsync detect the checksum mismatch before committing
a broken transaction? I don't know. They could not reproduce the problem
either. But they've seen it and they are afraid of it happening again.
They trust Subversion with their data, and such problems don't exactly
encourage them to keep trusting it. So I think we shouldn't dismiss the
value of internal integrity checks entirely just because most parts of
Subversion don't care [yet]. Of course such checks need to be placed
in the right spots.
Though I agree that relying on content checksums to make sure data copied
from one computer to another has been copied correctly is not that
important anymore these days. We're not copying data across noisy serial
links anymore, and have checksumming on many different levels (e.g.
ethernet CRC and disk controller CRC).
But what additional verification shields us from is subtle bugs in our
own code that cause data corruption. These cannot be caught by lower
layers and evidently seem to exist (or maybe the customer had a hardware
problem, but I doubt that).
So what's nice is that the content checksums allow verification of data
read back from a freshly loaded repository, or even from a transaction
before commit. I.e. the loader can make sure that the data that it wrote
into the repository matches what the dump file expects. This can catch
bugs in the loader code. My plan was to make svnsync do similar checks later.
As for integrity of dump files themselves, we should tell paranoid
people to sign dump files after they've been created, and verify the
signatures independently of Subversion.
So there is some value in having additional verification steps built-in.
We can apply this idea to other subsystems. We now have sha1 checksums of
pristines in the working copy, for instance. This means that it's easy to
detect a corrupt pristine -- open it, read content and verify checksum.
I'm not saying we should do this all the time by default, but like
'svnadmin verify', would could have a command that verifies pristines
(possibly while removing ones that are no longer referenced).
We'll just have to figure out where such checks will be beneficial.
But I don't mind reverting this change if it really bothers you.
It's just a small step on a longer road anyway. I think we should go
down that road and try to catch corrupted data before committing to it,
but dump file property checksums aren't the most important part of that.
Assuming that we may some day get a new FS backend that stores checksums
of property content, could we re-add the feature?
> Now for some nit-picking. This format:
>
> Header-Name: (PROPNAME) CHECKSUM
>
> is unnecessarily complex and, I bet, will catch at least person offguard
> when their dumpstream contains a property name with a ')' character in it.
> Checksum strings have a constant length and a fixed character set, so you
> can simply do:
>
> Header-Name: CHECKSUM PROPNAME
>
> (which also has the nice property of looking similar to the output of
> 'md5sum' and 'sha1sum').
Well, those tools are linux-specific, and other unix systems use
similar output to what I used:
$ echo foo > /tmp/foo
$ sha1 /tmp/foo
SHA1 (/tmp/foo) = f1d2d2f924e986ac86fdf7b36c94bcdf32beec15
Though I like your suggestion better, because it will also align
property names nicely.
Stefan
Re: svn commit: r965892 - in /subversion/trunk: notes/dump-load-format.txt
subversion/include/svn_repos.h subversion/libsvn_repos/dump.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 07/22/2010 09:40 AM, Daniel Shahaf wrote:
> C. Michael Pilato wrote on Thu, Jul 22, 2010 at 09:00:25 -0400:
>> Now for some nit-picking. This format:
>>
>> Header-Name: (PROPNAME) CHECKSUM
>>
>> is unnecessarily complex and, I bet, will catch at least person offguard
>> when their dumpstream contains a property name with a ')' character in it.
>> Checksum strings have a constant length and a fixed character set, so you
>> can simply do:
>>
>> Header-Name: CHECKSUM PROPNAME
>>
>> (which also has the nice property of looking similar to the output of
>> 'md5sum' and 'sha1sum').
>>
>
> Are parentheses valid in property names? (svn_prop_name_is_valid() doesn't allow them.)
Ah! Okay. Then I reduce my nitpicking back to mere claims of unnecessary
complexity. :-)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r965892 - in /subversion/trunk:
notes/dump-load-format.txt subversion/include/svn_repos.h
subversion/libsvn_repos/dump.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
C. Michael Pilato wrote on Thu, Jul 22, 2010 at 09:00:25 -0400:
> Now for some nit-picking. This format:
>
> Header-Name: (PROPNAME) CHECKSUM
>
> is unnecessarily complex and, I bet, will catch at least person offguard
> when their dumpstream contains a property name with a ')' character in it.
> Checksum strings have a constant length and a fixed character set, so you
> can simply do:
>
> Header-Name: CHECKSUM PROPNAME
>
> (which also has the nice property of looking similar to the output of
> 'md5sum' and 'sha1sum').
>
Are parentheses valid in property names? (svn_prop_name_is_valid() doesn't allow them.)
Daniel
(that's the first thing I did when I saw the commit: make sure the (propname)
syntax would be unambiguously parsable)
> --
> C. Michael Pilato <cm...@collab.net>
> CollabNet <> www.collab.net <> Distributed Development On Demand
>
Re: svn commit: r965892 - in /subversion/trunk: notes/dump-load-format.txt
subversion/include/svn_repos.h subversion/libsvn_repos/dump.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 07/22/2010 06:15 AM, Stefan Sperling wrote:
> On Thu, Jul 22, 2010 at 09:53:35AM +0100, Philip Martin wrote:
>> Perhaps we could just have one checksum that includes all the property
>> names and values?
>
> I'd like loaders to be able to tell users which properties are
> corrupted.
This change feels obnoxious to me, adding still more cost to the dump
process for little theoretical gain and zero practical gain (since our own
loader does nothing with the information).
When the dump stream format and code was first designed, if the FS backend
hadn't calculated and stored a checksum for the file contents, no checksum
was placed into the dumpstream. Further, when in --deltas mode, the
checksum you (might) get in the dumpstream is, again, the checksum stored in
the FS backend for the fulltext of the file contents -- it doesn't represent
the checksum of the svndiff-compressed delta in the stream. This was the
overarching sentiment of the dump format: to carry information stored in
the FS backend, as much as can be useful to the folks on the receiving end,
but certainly without unnecessary cost to the dumpfile producer.
Back when Hyrum did his checksum API overhaul (in r872722, to be precise),
he made a subtle change here that deviated from this sentiment. He told the
dumpstream generator code to forcibly generate checksums for placement in
the dump stream (he passed 'TRUE' for the 'force' parameter of the new
svn_fs_file_checksum() function everywhere). The code today is left reading
kinda weird as a result:
checksum = svn_fs_file_checksum(FORCE)
hex_digest = svn_checksum_to_string(checksum)
if (hex_digest)
... #### Wait -- "*if*" hexdigest? You forced its creation above!
Given the vast amount of Subversion data in existence today that probably
doesn't have an FS-stored SHA1 checksum (since we introduced that only in
the past couple of years), the immediate affect of these changes was that
the dumper code was now running over every fulltext of every file an
additional time just to generate SHA1's (since older repositories didn't
store SHA1's). And then again for copyfrom sources. And then again for
text bases (when in --deltas mode). (Some of those may reduce away if the
backend is able to store the SHA1's forcibly generated from previous requests.)
Hyrum also changed the loader code so that it forcibly acquires a checksum
for copyfrom sources for comparison against the copyfrom-checksum values
from the stream.
As if dump/load needed to run any slower.
You've extended this behavioral change now. Tell me -- if the FS backend
and the rest of Subversion don't care enough about the possibility of
property corruption to store and transmit checksums for those things,
where's the real value in teaching the dump format (alone) to do so?
Now for some nit-picking. This format:
Header-Name: (PROPNAME) CHECKSUM
is unnecessarily complex and, I bet, will catch at least person offguard
when their dumpstream contains a property name with a ')' character in it.
Checksum strings have a constant length and a fixed character set, so you
can simply do:
Header-Name: CHECKSUM PROPNAME
(which also has the nice property of looking similar to the output of
'md5sum' and 'sha1sum').
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r965892 - in /subversion/trunk:
notes/dump-load-format.txt subversion/include/svn_repos.h
subversion/libsvn_repos/dump.c
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jul 22, 2010 at 09:53:35AM +0100, Philip Martin wrote:
> stsp@apache.org writes:
>
> > Author: stsp
> > Date: Tue Jul 20 16:14:53 2010
> > New Revision: 965892
> >
> > URL: http://svn.apache.org/viewvc?rev=965892&view=rev
> > Log:
> > Make svnadmin dump print headers containing MD5 and SHA1 checksums of
> > property content, as was already done for file content. Checksums are
> > printed for revision properties as well as versioned properties.
>
> Do we gain anything by having both MD5 and SHA1 checksums?
We print both kinds of checksum for file content, too, so it's just
for consistency.
> Do we need checksums per property?
Yes, because the idea is that the loader (or other tools handling dump
files) can identify properties that were corrupted, and let the user
know which properties were corrupted.
> Often the checksums will take up more space than the property.
Quite possibly. But the overhead is fixed in size, and it's all ASCII
so maybe it compresses quite well?
If you are concerned about space, there is already the --deltas option
which saves the bulk of the size of a regular v2 dump.
I haven't done any empirical analysis, but I'd suspect that deltas of
file content will proportionally save more space than the property hashes
can make up for.
> What about property names?
Hmmm... names aren't covered by checksums right now, that is true.
Maybe we should compute the hash over "key=value" strings, such as
"svn:eol-style=native"?
> Perhaps we could just have one checksum that includes all the property
> names and values?
I'd like loaders to be able to tell users which properties are
corrupted.
Note that I've added this in response to a complaint that dump files
carry very little information about their integrity (and that svnsync
does close to zero consistency checks, too, but that is a different site).
I've found out that we have checksums for content already in 1.6.x,
but then found out that properties don't have any checksums.
That's just unnecessary inconsistency. If we're going to have one,
we should have the other.
The checksums come before content, so the loader can easily be made to
verify content. It would make sense to make our own loader start verifying
checksums soon.
In general, having checksums built into the dump file itself does not
protect against purposeful corruption. It's just a safety net for accidental
corruption. For security purposes, people should cryptographically sign
the entire dump file.
Stefan
Re: svn commit: r965892 - in /subversion/trunk: notes/dump-load-format.txt subversion/include/svn_repos.h subversion/libsvn_repos/dump.c
Posted by Philip Martin <ph...@wandisco.com>.
stsp@apache.org writes:
> Author: stsp
> Date: Tue Jul 20 16:14:53 2010
> New Revision: 965892
>
> URL: http://svn.apache.org/viewvc?rev=965892&view=rev
> Log:
> Make svnadmin dump print headers containing MD5 and SHA1 checksums of
> property content, as was already done for file content. Checksums are
> printed for revision properties as well as versioned properties.
Do we gain anything by having both MD5 and SHA1 checksums? Do we need
checksums per property? Often the checksums will take up more space
than the property. What about property names?
Perhaps we could just have one checksum that includes all the property
names and values?
--
Philip
Re: svn commit: r965892 - in /subversion/trunk: notes/dump-load-format.txt
subversion/include/svn_repos.h subversion/libsvn_repos/dump.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 07/20/2010 12:14 PM, stsp@apache.org wrote:
> Author: stsp
> Date: Tue Jul 20 16:14:53 2010
> New Revision: 965892
[...]
> (dump_nodem, write_revision_record): When dumping properties, generate
^ Oops?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r965892 - in /subversion/trunk:
notes/dump-load-format.txt subversion/include/svn_repos.h
subversion/libsvn_repos/dump.c
Posted by Stefan Sperling <st...@apache.org>.
On Tue, Jul 20, 2010 at 04:14:53PM -0000, stsp@apache.org wrote:
> Author: stsp
> Date: Tue Jul 20 16:14:53 2010
> New Revision: 965892
>
> URL: http://svn.apache.org/viewvc?rev=965892&view=rev
> Log:
> Make svnadmin dump print headers containing MD5 and SHA1 checksums of
> property content, as was already done for file content. Checksums are
> printed for revision properties as well as versioned properties.
>
Ooops, this broke some tests. Will fix shortly.
Stefan