Re: [PATCH] Fix 2441

On Fri, 06 Jan 2006, Kamesh Jayachandran wrote:

> Hi All,
> Slightly better fix for #2441.

Thanks Kamesh! Last couple-few minor things.
 
> [[[
> Fix issue #2441, 'svnadmin load' malformed dumpfile errors are
> intolerably vague
>
> * subversion/libsvn_repos/load.c
> (contextual_error): will return a svn_error_t * indicating which
> header block
> problematic header occured with a header name and a reason for failure.

Usage of contextual_error() is missing from the change log message.

>
> ]]]
>

> Index: subversion/libsvn_repos/load.c
> ===================================================================
> --- subversion/libsvn_repos/load.c (revision 17993)
> +++ subversion/libsvn_repos/load.c (working copy)
> @@ -105,6 +105,56 @@
> }
>
>
> +/** This function gives a contextual clue of dump file malformed content. **/
> +static svn_error_t *
> +contextual_error (apr_hash_t *headers, char *key, char *reason)

Something like contextual_dumpfile_error() or
contextual_header_block_error() would provide a more explicit name.

> +{
> + char *block=NULL;
> + if ((block = (char *)apr_hash_get (headers,
                          ^
We usually put a space here (and below):

     if ((block = (char *) apr_hash_get (headers,

> + SVN_REPOS_DUMPFILE_REVISION_NUMBER,
> + APR_HASH_KEY_STRING)) != NULL)
                                        ^
Now they'll line up. Bonus.

> + {
> + 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);

Quote the key value '%s' to help the reader here and below.

> + }
> + else if ((block = (char *)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 = (char *)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 = (char *)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);
> + }
> +}
> +
> /*----------------------------------------------------------------------*/
> 
> /** The parser and related helper funcs **/
> @@ -153,9 +203,8 @@
> 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 contextual_error (*headers, header_str->data,
> + "with no : character");

Quote the ':' character here and below.

> i++;
> }
> /* Create a 'name' string and point to it. */
> @@ -165,9 +214,8 @@
> /* 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 contextual_error (*headers, header_str->data,
> + "with no value after a : character");
...