Hi,
Currently the svnrdump command line tool does something quite different compared
to other programs like svn, svnadmin and svnauthz. By "quite different" I mean
that svnrdump does exactly the following:
- Inconsistently uses (svn_cmdline_handle_exit_error) and SVNRDUMP_ERR,
which actually is (svn_handle_error2 + svn_error_clear) to handle errors.
- Sometimes destroys the top-level pool and sometimes does not destroy it, e.g.:
* Running 'svnrdump --non-interactive --force-interactive' explicitly destroys
the pool.
* Running 'svnrdump dump INVALID-URL' does not destroy the pool thus leaving
it to be destroyed during apr_terminate.
- Prefixes every possible exit path with the svn_pool_destroy call instead of
making that call in a single place.
- Inconsistently uses SVN_INT_ERR instead of SVNRDUMP_ERR in one place:
[[[
if (subcommand->name[0] == '-')
SVN_INT_ERR(help_cmd(NULL, NULL, pool));
]]]
We can avoid all these inconsistencies by utilizing the (main / submain /
SVN_INT_ERR / EXIT_ERROR / single svn_pool_destroy call) pattern. That is the
way things are done in in svn / svnadmin and svnauthz.
See the attached patch.
Another thing is worth mentioning here. This patch prevents svnrdump from
crashing in the SSL + error-out case, which I recently reported to serf-dev [1].
That is *just a lucky coincidence*, because the problem is in serf, not in
svnrdump. So, this patch should in no way be considered a workaround for the
serf issue, it is simply about making things more consistent and better in
general.
[1]: https://groups.google.com/d/msg/serf-dev/gOn9HTUN98U/pz0_AqdrmJYJ
Thanks and regards,
Evgeny Kotkov
Index: subversion/svnrdump/svnrdump.c
===================================================================
--- subversion/svnrdump/svnrdump.c (revision 1514184)
+++ subversion/svnrdump/svnrdump.c (working copy)
@@ -666,23 +666,21 @@ version(const char *progname,
}
-/* A statement macro, similar to @c SVN_ERR, but returns an integer.
- * Evaluate @a expr. If it yields an error, handle that error and
- * return @c EXIT_FAILURE.
- */
-#define SVNRDUMP_ERR(expr) \
- do \
- { \
- svn_error_t *svn_err__temp = (expr); \
- if (svn_err__temp) \
- { \
- svn_handle_error2(svn_err__temp, stderr, FALSE, "svnrdump: "); \
- svn_error_clear(svn_err__temp); \
- return EXIT_FAILURE; \
- } \
- } \
- while (0)
+/* Report and clear the error ERR, and return EXIT_FAILURE. */
+#define EXIT_ERROR(err) \
+ svn_cmdline_handle_exit_error(err, NULL, "svnrdump: ")
+/* A redefinition of the public SVN_INT_ERR macro, that suppresses the
+ * error message if it is SVN_ERR_IO_PIPE_WRITE_ERROR, and with the
+ * program name 'svnrdump' instead of 'svn'. */
+#undef SVN_INT_ERR
+#define SVN_INT_ERR(expr) \
+ do { \
+ svn_error_t *svn_err__temp = (expr); \
+ if (svn_err__temp) \
+ return EXIT_ERROR(svn_err__temp); \
+ } while (0)
+
/* Handle the "dump" subcommand. Implements `svn_opt_subcommand_t'. */
static svn_error_t *
dump_cmd(apr_getopt_t *os,
@@ -830,14 +828,13 @@ validate_and_resolve_revisions(opt_baton_t *opt_ba
return SVN_NO_ERROR;
}
-int
-main(int argc, const char **argv)
+static int
+sub_main(int argc, const char *argv[], apr_pool_t *pool)
{
svn_error_t *err = SVN_NO_ERROR;
const svn_opt_subcommand_desc2_t *subcommand = NULL;
opt_baton_t *opt_baton;
svn_revnum_t latest_revision = SVN_INVALID_REVNUM;
- apr_pool_t *pool = NULL;
const char *config_dir = NULL;
const char *username = NULL;
const char *password = NULL;
@@ -851,20 +848,12 @@ validate_and_resolve_revisions(opt_baton_t *opt_ba
apr_array_header_t *received_opts;
int i;
- if (svn_cmdline_init ("svnrdump", stderr) != EXIT_SUCCESS)
- return EXIT_FAILURE;
-
- /* Create our top-level pool. Use a separate mutexless allocator,
- * given this application is single threaded.
- */
- pool = apr_allocator_owner_get(svn_pool_create_allocator(FALSE));
-
opt_baton = apr_pcalloc(pool, sizeof(*opt_baton));
opt_baton->start_revision.kind = svn_opt_revision_unspecified;
opt_baton->end_revision.kind = svn_opt_revision_unspecified;
opt_baton->url = NULL;
- SVNRDUMP_ERR(svn_cmdline__getopt_init(&os, argc, argv, pool));
+ SVN_INT_ERR(svn_cmdline__getopt_init(&os, argc, argv, pool));
os->interleave = TRUE; /* Options and arguments can be interleaved */
@@ -904,8 +893,8 @@ validate_and_resolve_revisions(opt_baton_t *opt_ba
break;
if (status != APR_SUCCESS)
{
- SVNRDUMP_ERR(usage(argv[0], pool));
- exit(EXIT_FAILURE);
+ SVN_INT_ERR(usage(argv[0], pool));
+ return EXIT_FAILURE;
}
/* Stash the option code in an array before parsing it. */
@@ -922,7 +911,7 @@ validate_and_resolve_revisions(opt_baton_t *opt_ba
_("Multiple revision arguments "
"encountered; try '-r N:M' instead "
"of '-r N -r M'"));
- return svn_cmdline_handle_exit_error(err, pool, "svnrdump: ");
+ return EXIT_ERROR(err);
}
/* Parse the -r argument. */
if (svn_opt_parse_revision(&(opt_baton->start_revision),
@@ -935,7 +924,7 @@ validate_and_resolve_revisions(opt_baton_t *opt_ba
err = svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
_("Syntax error in revision "
"argument '%s'"), utf8_opt_arg);
- return svn_cmdline_handle_exit_error(err, pool, "svnrdump: ");
+ return EXIT_ERROR(err);
}
}
break;
@@ -952,10 +941,10 @@ validate_and_resolve_revisions(opt_baton_t *opt_ba
opt_baton->help = TRUE;
break;
case opt_auth_username:
- SVNRDUMP_ERR(svn_utf_cstring_to_utf8(&username, opt_arg, pool));
+ SVN_INT_ERR(svn_utf_cstring_to_utf8(&username, opt_arg, pool));
break;
case opt_auth_password:
- SVNRDUMP_ERR(svn_utf_cstring_to_utf8(&password, opt_arg, pool));
+ SVN_INT_ERR(svn_utf_cstring_to_utf8(&password, opt_arg, pool));
break;
case opt_auth_nocache:
no_auth_cache = TRUE;
@@ -978,9 +967,9 @@ validate_and_resolve_revisions(opt_baton_t *opt_ba
apr_array_make(pool, 1,
sizeof(svn_cmdline__config_argument_t*));
- SVNRDUMP_ERR(svn_utf_cstring_to_utf8(&opt_arg, opt_arg, pool));
- SVNRDUMP_ERR(svn_cmdline__parse_config_option(config_options,
- opt_arg, pool));
+ SVN_INT_ERR(svn_utf_cstring_to_utf8(&opt_arg, opt_arg, pool));
+ SVN_INT_ERR(svn_cmdline__parse_config_option(config_options,
+ opt_arg, pool));
}
}
@@ -991,7 +980,7 @@ validate_and_resolve_revisions(opt_baton_t *opt_ba
err = svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
_("--non-interactive and --force-interactive "
"are mutually exclusive"));
- return svn_cmdline_handle_exit_error(err, pool, "svnrdump: ");
+ return EXIT_ERROR(err);
}
if (opt_baton->help)
@@ -1016,9 +1005,8 @@ validate_and_resolve_revisions(opt_baton_t *opt_ba
else
{
- SVNRDUMP_ERR(help_cmd(NULL, NULL, pool));
- svn_pool_destroy(pool);
- exit(EXIT_FAILURE);
+ SVN_INT_ERR(help_cmd(NULL, NULL, pool));
+ return EXIT_FAILURE;
}
}
else
@@ -1032,14 +1020,13 @@ validate_and_resolve_revisions(opt_baton_t *opt_ba
const char *first_arg_utf8;
err = svn_utf_cstring_to_utf8(&first_arg_utf8, first_arg, pool);
if (err)
- return svn_cmdline_handle_exit_error(err, pool, "svnrdump: ");
+ return EXIT_ERROR(err);
svn_error_clear(
svn_cmdline_fprintf(stderr, pool,
_("Unknown subcommand: '%s'\n"),
first_arg_utf8));
- SVNRDUMP_ERR(help_cmd(NULL, NULL, pool));
- svn_pool_destroy(pool);
- exit(EXIT_FAILURE);
+ SVN_INT_ERR(help_cmd(NULL, NULL, pool));
+ return EXIT_FAILURE;
}
}
}
@@ -1071,7 +1058,6 @@ validate_and_resolve_revisions(opt_baton_t *opt_ba
_("Subcommand '%s' doesn't accept option
'%s'\n"
"Type 'svnrdump help %s' for usage.\n"),
subcommand->name, optstr, subcommand->name));
- svn_pool_destroy(pool);
return EXIT_FAILURE;
}
}
@@ -1078,16 +1064,14 @@ validate_and_resolve_revisions(opt_baton_t *opt_ba
if (strcmp(subcommand->name, "--version") == 0)
{
- SVNRDUMP_ERR(version(argv[0], opt_baton->quiet, pool));
- svn_pool_destroy(pool);
- exit(EXIT_SUCCESS);
+ SVN_INT_ERR(version(argv[0], opt_baton->quiet, pool));
+ return EXIT_SUCCESS;
}
if (strcmp(subcommand->name, "help") == 0)
{
- SVNRDUMP_ERR(help_cmd(os, opt_baton, pool));
- svn_pool_destroy(pool);
- exit(EXIT_SUCCESS);
+ SVN_INT_ERR(help_cmd(os, opt_baton, pool));
+ return EXIT_SUCCESS;
}
/* --trust-server-cert can only be used with --non-interactive */
@@ -1096,30 +1080,27 @@ validate_and_resolve_revisions(opt_baton_t *opt_ba
err = svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
_("--trust-server-cert requires "
"--non-interactive"));
- return svn_cmdline_handle_exit_error(err, pool, "svnrdump: ");
+ return EXIT_ERROR(err);
}
/* Expect one more non-option argument: the repository URL. */
if (os->ind != os->argc - 1)
{
- SVNRDUMP_ERR(usage(argv[0], pool));
- svn_pool_destroy(pool);
- exit(EXIT_FAILURE);
+ SVN_INT_ERR(usage(argv[0], pool));
+ return EXIT_FAILURE;
}
else
{
const char *repos_url;
- SVNRDUMP_ERR(svn_utf_cstring_to_utf8(&repos_url,
- os->argv[os->ind], pool));
+ SVN_INT_ERR(svn_utf_cstring_to_utf8(&repos_url,
+ os->argv[os->ind], pool));
if (! svn_path_is_url(repos_url))
{
err = svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, 0,
"Target '%s' is not a URL",
repos_url);
- SVNRDUMP_ERR(err);
- svn_pool_destroy(pool);
- exit(EXIT_FAILURE);
+ return EXIT_ERROR(err);
}
opt_baton->url = svn_uri_canonicalize(repos_url, pool);
}
@@ -1140,16 +1121,16 @@ validate_and_resolve_revisions(opt_baton_t *opt_ba
non_interactive = !svn_cmdline__be_interactive(non_interactive,
force_interactive);
- SVNRDUMP_ERR(init_client_context(&(opt_baton->ctx),
- non_interactive,
- username,
- password,
- config_dir,
- opt_baton->url,
- no_auth_cache,
- trust_server_cert,
- config_options,
- pool));
+ SVN_INT_ERR(init_client_context(&(opt_baton->ctx),
+ non_interactive,
+ username,
+ password,
+ config_dir,
+ opt_baton->url,
+ no_auth_cache,
+ trust_server_cert,
+ config_options,
+ pool));
err = svn_client_open_ra_session2(&(opt_baton->session),
opt_baton->url, NULL,
@@ -1174,11 +1155,31 @@ validate_and_resolve_revisions(opt_baton_t *opt_ba
_("Authentication failed and interactive"
" prompting is disabled; see the"
" --force-interactive option"));
+ return EXIT_ERROR(err);
}
+ else if (err)
+ return EXIT_ERROR(err);
+ else
+ return EXIT_SUCCESS;
+}
- SVNRDUMP_ERR(err);
+int
+main(int argc, const char *argv[])
+{
+ apr_pool_t *pool;
+ int exit_code;
+ /* Initialize the app. */
+ if (svn_cmdline_init("svnrdump", stderr) != EXIT_SUCCESS)
+ return EXIT_FAILURE;
+
+ /* Create our top-level pool. Use a separate mutexless allocator,
+ * given this application is single threaded.
+ */
+ pool = apr_allocator_owner_get(svn_pool_create_allocator(FALSE));
+
+ exit_code = sub_main(argc, argv, pool);
+
svn_pool_destroy(pool);
-
- return EXIT_SUCCESS;
+ return exit_code;
}
svnrdump: Handle errors the svn / svnadmin / svnauthz way.
* subversion/svnrdump/svnrdump.c
(SVNRDUMP_ERR): Remove this macro.
(EXIT_ERROR, SVN_INT_ERR): New macros, same as those defined in
subversion/svn/svn.c, but for svnrdump.
(submain): Extract most of 'main' into this new function. Use SVN_INT_ERR /
EXIT_ERROR to handle errors. Remove all svn_pool_destroy calls, because
the top-level pool is now always destroyed in 'main'.
(main): Now just a wrapper, that creates the top-level pool, calls 'submain'
and cleans everything up.
Patch by: Evgeny Kotkov <evgeny.kotkov{_AT_}visualsvn.com>
