Hi Julian,
Thanks for your feedback.
Attaching the new patch.
Limiting the header_data to 20 characters as it seems that maximum
header name that I see in my dump seems to be 20 characters in length.
In case some spurious binary line happen to be there in the stream we
limit to print only 20 characters. Don't think it is worth to loop
through the characters for the valid printable ascii range. Let me know.
With regards
Kamesh Jayachandran
[[[
Fix issue #2441, 'svnadmin load' malformed dumpfile errors are
intolerably vague
* subversion/libsvn_repos/load.c
(read_header_block): Error message with a hint.
]]]
Julian Foad wrote:
> Kamesh Jayachandran wrote:
>> Thanks Julian, DLR, Garrett.
>> Julian,
>> Not sure how to reduce it further eventhough I could understand your
>> points on stuffs like some spurious newline introduced by the program
>> that generates the dump file.
>
> See below.
>
>> [[[
>> Fix issue #2441, 'svnadmin load' malformed dumpfile errors are
>> intolerably vague
>>
>> * subversion/libsvn_repos/load.c
>> (contextual_header_block_error): New function to return a detailed
>> error message.
>> (read_header_block): Call contextual_header_block_error().
>>
>> ]]]
>
>> Index: subversion/libsvn_repos/load.c
>> ===================================================================
>> --- subversion/libsvn_repos/load.c (revision 17993)
>> +++ subversion/libsvn_repos/load.c (working copy)
>> @@ -105,6 +105,58 @@
>> }
>>
>>
>> +/* Compose and return an error describing the problem REASON with the
>> + * dump file header KEY, and the block where these header key is
>> located.
>> + */
>> +static svn_error_t *
>> +contextual_header_block_error (apr_hash_t *headers, const char *key,
>> const char *reason)
>> +{
>> + char *block;
>> + if ((block = apr_hash_get (headers, +
>> SVN_REPOS_DUMPFILE_REVISION_NUMBER,
>> + APR_HASH_KEY_STRING)) != NULL)
>> + {
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Found malformed revision '%s' header block "
>> + "near the line starting with a key '%s' %s "
>> + "in dumpfile stream"), block, key, reason);
>> + }
>> + else if ((block = apr_hash_get (headers, +
>> SVN_REPOS_DUMPFILE_NODE_PATH,
>> + APR_HASH_KEY_STRING)) != NULL)
>> + {
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Found malformed node '%s' header block "
>> + "near the line starting with a key '%s' %s "
>> + "in dumpfile stream"), block, key, reason);
>> + }
>> + else if ((block = apr_hash_get (headers, + SVN_REPOS_DUMPFILE_UUID,
>> + APR_HASH_KEY_STRING)) != NULL)
>> + {
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Found malformed UUID header block "
>> + "near the line starting with a key '%s' %s "
>> + "in dumpfile stream"), key, reason);
>> + }
>> + else if ((block = apr_hash_get (headers, +
>> SVN_REPOS_DUMPFILE_MAGIC_HEADER,
>> + APR_HASH_KEY_STRING)) != NULL)
>> + {
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Found malformed magic header block "
>> + "near the line starting with a key '%s' %s "
>> + "in dumpfile stream"), key, reason);
>> + }
>> + else
>> + {
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Found malformed unknown header block "
>> + "near the line starting with a key '%s' %s "
>> + "in dumpfile stream"), key, reason);
>> + }
>> +}
>
> There's a lot of duplication in that function. At the least I would
> like to see the common code (well, text, mainly) factored out.
> Additionally I would not bother trying to report the content of
> another header from the same block, unless there is evidence that
> doing so is really useful. As I said before, I suspect that it is not
> often really useful.
>
>
>> - return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> - _("Found malformed header block "
>> - "in dumpfile stream"));
>> + return contextual_header_block_error (*headers, header_str->data, +
>> _("with no ':' character"));
>
> Translating a sentence fragment doesn't work in all languages.
> Generally, text to be translated has to be in complete sentences.
>
>
> Here is what I am suggesting for the whole patch:
>
>> Index: subversion/libsvn_repos/load.c
>> ===================================================================
>> --- subversion/libsvn_repos/load.c (revision 18020)
>> +++ subversion/libsvn_repos/load.c (working copy)
>> @@ -153,9 +153,10 @@
>> while (header_str->data[i] != ':')
>> {
>> if (header_str->data[i] == '\0')
>> - return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> - _("Found malformed header block "
>> - "in dumpfile stream"));
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Dump stream contains a malformed "
>> + "header (with no colon): '%s'"),
>> + header_str->data);
>> i++;
>> }
>> /* Create a 'name' string and point to it. */
>> @@ -165,9 +166,10 @@
>> /* Skip over the NULL byte and the space following it. */
>> i += 2;
>> if (i > header_str->len)
>> - return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> - _("Found malformed header block "
>> - "in dumpfile stream"));
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Dump stream contains a malformed "
>> + "header (no value after colon): '%s'"),
>> + header_str->data);
>>
>> /* Point to the 'value' string. */
>> value = header_str->data + i;
>
>
>
> I can already see another problem: the supposed malformed header
> string is quite likely to be a block of arbitrary ("binary") data of
> arbitrary size. Therefore it would be polite not to print it raw but
> to restrict the length and character set of what we print.
>
>
> Further, I see that load.c also emits other vague dumpfile errors
> (e.g. in stream_malformed()) that could do with a bit more detail.
>
> - Julian
>
Index: subversion/libsvn_repos/load.c
===================================================================
--- subversion/libsvn_repos/load.c (revision 17993)
+++ subversion/libsvn_repos/load.c (working copy)
@@ -153,9 +153,11 @@
while (header_str->data[i] != ':')
{
if (header_str->data[i] == '\0')
- return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
- _("Found malformed header block "
- "in dumpfile stream"));
+ return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
+ _("Dump stream contains a malformed "
+ "header (with no colon) ':' "
+ "after '%.20s'"),
+ header_str->data);
i++;
}
/* Create a 'name' string and point to it. */
@@ -165,9 +167,11 @@
/* Skip over the NULL byte and the space following it. */
i += 2;
if (i > header_str->len)
- return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
- _("Found malformed header block "
- "in dumpfile stream"));
+ return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
+ _("Dump stream contains a malformed "
+ "header (no value after colon) ':' "
+ "after '%.20s'"),
+ header_str->data);
/* Point to the 'value' string. */
value = header_str->data + i;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jan 11 13:42:40 2006