Has anyone had time to look at the patch yet? Please tell me if this
was not the right place for submission, or if I should do something
else to make it easier to be reviewed and accepted.
7 dec 2013 kl. 21.21 skrev Mattias Engdegård:
The error messages of getopt are tricky to localise properly, since
they are served as fprintf-like function invocations only. Since
there is only very few distinct messages, the best solution is to
introduce some sort of codes that the application can do whatever it
wants with.
[...]
Please review, and tell me if I should supply a commit message,
CHANGES addition and so on. The patch is against trunk.
Index: include/apr_getopt.h
===================================================================
--- include/apr_getopt.h (revision 1552978)
+++ include/apr_getopt.h (arbetskopia)
@@ -92,6 +92,22 @@
};
/**
+ * Error codes returned from apr_getopt_errcode().
+ *
+ * The associated values returned from apr_getopt_errparam() are given
+ * below for each error code.
+ */
+typedef enum {
+ APR_GETOPT_INVALID_OPTION, /**< Invalid option; error parameter
+ is the option. */
+ APR_GETOPT_MISSING_ARGUMENT, /**< Missing argument; error parameter is
+ the option whose argument is
+ missing. */
+ APR_GETOPT_INVALID_ARGUMENT /**< Invalid or malformed argument; error
+ parameter is the invalid argument. */
+} apr_getopt_error_e;
+
+/**
* Initialize the arguments for parsing by apr_getopt().
* @param os The options structure created for apr_getopt()
* @param cont The pool to operate on
@@ -151,6 +167,30 @@
const apr_getopt_option_t *opts,
int *option_ch,
const char **option_arg);
+
+/**
+ * Retrieve a code describing the current getopt error. This function
+ * may only be called from the getopt error function, @c errfn
+ * in @c apr_getopt_t.
+ * @param os The apr_getopt_t structure created by apr_getopt_init().
+ * @return The error code.
+ * Error codes may have an associated string parameter;
+ * use apr_getopt_errparam() to retrieve it.
+ */
+APR_DECLARE(apr_getopt_error_e) apr_getopt_errcode(apr_getopt_t *os);
+
+/**
+ * Retrieve a string parameter pertaining to the current getopt error.
+ * This function may only be called from the getopt error function, @c errfn
+ * in @c apr_getopt_t.
+ * @param os The apr_getopt_t structure created by apr_getopt_init().
+ * @return The error parameter string. It is only valid during the error
+ * function call.
+ * The meaning of the returned string depends on the current getopt error code;
+ * use apr_getopt_errcode() to retrieve it.
+ */
+APR_DECLARE(const char *) apr_getopt_errparam(apr_getopt_t *os);
+
/** @} */
#ifdef __cplusplus
Index: misc/unix/getopt.c
===================================================================
--- misc/unix/getopt.c (revision 1552978)
+++ misc/unix/getopt.c (arbetskopia)
@@ -68,6 +68,43 @@
return APR_SUCCESS;
}
+struct error_info_t {
+ apr_getopt_error_e code;
+ const char *param;
+};
+
+APR_DECLARE(apr_getopt_error_e) apr_getopt_errcode(apr_getopt_t *os)
+{
+ struct error_info_t *e = os->errarg;
+ return e->code;
+}
+
+APR_DECLARE(const char *) apr_getopt_errparam(apr_getopt_t *os)
+{
+ struct error_info_t *e = os->errarg;
+ return e->param;
+}
+
+/* Print out an error by calling the user-defined error callback, if any. */
+static void perr(apr_getopt_t *os, apr_getopt_error_e err,
+ const char *msg, const char *par)
+{
+ if (os->errfn) {
+ /* Hack: we temporarily re-purpose the errarg member for
+ holding a reference to the error code and string parameter
+ during the call to the error function. */
+ void *errarg = os->errarg;
+ struct error_info_t e;
+ e.code = err;
+ e.param = par;
+ os->errarg = &e;
+ (os->errfn)(errarg, "%s: %s: %s\n",
+ apr_filepath_name_get(*os->argv), msg, par);
+ if (os->errarg == &e)
+ os->errarg = errarg;
+ }
+}
+
APR_DECLARE(apr_status_t) apr_getopt(apr_getopt_t *os, const char *opts,
char *optch, const char **optarg)
{
@@ -99,9 +136,11 @@
}
if (!*os->place)
++os->ind;
- if (os->errfn && *opts != ':') {
- (os->errfn)(os->errarg, "%s: illegal option -- %c\n",
- apr_filepath_name_get(*os->argv), os->opt);
+ if (*opts != ':') {
+ char par[2] = "x";
+ par[0] = os->opt;
+ perr(os, APR_GETOPT_INVALID_OPTION,
+ "illegal option", par);
}
*optch = os->opt;
return (APR_BADCH);
@@ -115,16 +154,15 @@
if (*os->place) /* no white space */
*optarg = os->place;
else if (os->argc <= ++os->ind) { /* no arg */
+ char par[2] = "x";
os->place = EMSG;
if (*opts == ':') {
*optch = os->opt;
return (APR_BADARG);
}
- if (os->errfn) {
- (os->errfn)(os->errarg,
- "%s: option requires an argument -- %c\n",
- apr_filepath_name_get(*os->argv), os->opt);
- }
+ par[0] = os->opt;
+ perr(os, APR_GETOPT_MISSING_ARGUMENT,
+ "option requires an argument", par);
*optch = os->opt;
return (APR_BADCH);
}
@@ -177,26 +215,6 @@
os->skip_end += len2;
}
-/* Helper function to print out an error involving a long option */
-static apr_status_t serr(apr_getopt_t *os, const char *err, const char *str,
- apr_status_t status)
-{
- if (os->errfn)
- (os->errfn)(os->errarg, "%s: %s: %s\n",
- apr_filepath_name_get(*os->argv), err, str);
- return status;
-}
-
-/* Helper function to print out an error involving a short option */
-static apr_status_t cerr(apr_getopt_t *os, const char *err, int ch,
- apr_status_t status)
-{
- if (os->errfn)
- (os->errfn)(os->errarg, "%s: %s: %c\n",
- apr_filepath_name_get(*os->argv), err, ch);
- return status;
-}
-
APR_DECLARE(apr_status_t) apr_getopt_long(apr_getopt_t *os,
const apr_getopt_option_t *opts,
int *optch, const char **optarg)
@@ -236,8 +254,11 @@
p++;
for (i = 0; ; i++) {
- if (opts[i].optch == 0) /* No match */
- return serr(os, "invalid option", p - 2, APR_BADCH);
+ if (opts[i].optch == 0) { /* No match */
+ perr(os, APR_GETOPT_INVALID_OPTION,
+ "invalid option", p - 2);
+ return APR_BADCH;
+ }
if (opts[i].name) {
len = strlen(opts[i].name);
@@ -252,15 +273,20 @@
if (p[len] == '=') /* Argument inline */
*optarg = p + len + 1;
else {
- if (os->ind >= os->argc) /* Argument missing */
- return serr(os, "missing argument", p - 2, APR_BADARG);
- else /* Argument in next arg */
+ if (os->ind >= os->argc) { /* Argument missing */
+ perr(os, APR_GETOPT_MISSING_ARGUMENT,
+ "missing argument", p - 2);
+ return APR_BADARG;
+ } else /* Argument in next arg */
*optarg = os->argv[os->ind++];
}
} else {
*optarg = NULL;
- if (p[len] == '=')
- return serr(os, "erroneous argument", p - 2, APR_BADARG);
+ if (p[len] == '=') {
+ perr(os, APR_GETOPT_INVALID_ARGUMENT,
+ "erroneous argument", p - 2);
+ return APR_BADARG;
+ }
}
permute(os);
return APR_SUCCESS;
@@ -271,8 +297,11 @@
return APR_EOF;
}
else
- if (*p == '\0') /* Bare "-" is illegal */
- return serr(os, "invalid option", p, APR_BADCH);
+ if (*p == '\0') { /* Bare "-" is illegal */
+ perr(os, APR_GETOPT_INVALID_OPTION,
+ "invalid option", p - 1);
+ return APR_BADCH;
+ }
}
}
@@ -281,8 +310,13 @@
* Look for it in the caller's table.
*/
for (i = 0; ; i++) {
- if (opts[i].optch == 0) /* No match */
- return cerr(os, "invalid option character", *p, APR_BADCH);
+ if (opts[i].optch == 0) { /* No match */
+ char arg[2] = "x";
+ arg[0] = *p;
+ perr(os, APR_GETOPT_INVALID_OPTION,
+ "invalid option", arg);
+ return APR_BADCH;
+ }
if (*p == opts[i].optch)
break;
@@ -293,9 +327,13 @@
if (*p != '\0') /* Argument inline */
*optarg = p;
else {
- if (os->ind >= os->argc) /* Argument missing */
- return cerr(os, "missing argument", *optch, APR_BADARG);
- else /* Argument in next arg */
+ if (os->ind >= os->argc) { /* Argument missing */
+ char arg[2] = "x";
+ arg[0] = *optch;
+ perr(os, APR_GETOPT_MISSING_ARGUMENT,
+ "missing argument", arg);
+ return APR_BADARG;
+ } else /* Argument in next arg */
*optarg = os->argv[os->ind++];
}
os->place = EMSG;
Index: test/testargs.c
===================================================================
--- test/testargs.c (revision 1552978)
+++ test/testargs.c (arbetskopia)
@@ -30,13 +30,25 @@
}
}
-static void unknown_arg(void *str, const char *err, ...)
+typedef struct {
+ apr_getopt_t *opt;
+ char *formatted;
+ char *param;
+ int errcode; /* Actually an apr_getopt_error_e, but declared
+ int so that it can contain an OOB value. */
+} error_data_t;
+
+static void unknown_arg(void *arg, const char *err, ...)
{
+ error_data_t *ed = arg;
va_list va;
va_start(va, err);
- apr_vsnprintf(str, 8196, err, va);
+ apr_vsnprintf(ed->formatted, 8196, err, va);
va_end(va);
+
+ ed->errcode = apr_getopt_errcode(ed->opt);
+ strncpy(ed->param, apr_getopt_errparam(ed->opt), 128);
}
static void no_options_found(abts_case *tc, void *data)
@@ -78,13 +90,21 @@
char ch;
const char *opt_arg;
char str[8196];
+ char par[128];
+ error_data_t ed;
str[0] = '\0';
+ par[0] = '\0';
rv = apr_getopt_init(&opt, p, largc, largv);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+ ed.opt = opt;
+ ed.formatted = str;
+ ed.param = par;
+ ed.errcode = -1;
+
opt->errfn = unknown_arg;
- opt->errarg = str;
+ opt->errarg = &ed;
while (apr_getopt(opt, "efgh", &ch, &opt_arg) == APR_SUCCESS) {
switch (ch) {
@@ -98,7 +118,9 @@
break;
}
}
- ABTS_STR_EQUAL(tc, "testprog: illegal option -- a\n", str);
+ ABTS_STR_EQUAL(tc, "testprog: illegal option: a\n", str);
+ ABTS_STR_EQUAL(tc, "a", par);
+ ABTS_INT_EQUAL(tc, APR_GETOPT_INVALID_OPTION, ed.errcode);
}
static void required_option(abts_case *tc, void *data)
@@ -110,13 +132,19 @@
char ch;
const char *opt_arg;
char str[8196];
+ error_data_t ed;
str[0] = '\0';
rv = apr_getopt_init(&opt, p, largc, largv);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+ ed.opt = opt;
+ ed.formatted = str;
+ ed.param = NULL;
+ ed.errcode = -1;
+
opt->errfn = unknown_arg;
- opt->errarg = str;
+ opt->errarg = &ed;
while (apr_getopt(opt, "a:", &ch, &opt_arg) == APR_SUCCESS) {
switch (ch) {
@@ -128,6 +156,7 @@
}
}
ABTS_STR_EQUAL(tc, "option: a with foo\n", str);
+ ABTS_INT_EQUAL(tc, -1, ed.errcode);
}
static void required_option_notgiven(abts_case *tc, void *data)
@@ -139,13 +168,21 @@
char ch;
const char *opt_arg;
char str[8196];
+ char par[128];
+ error_data_t ed;
str[0] = '\0';
+ par[0] = '\0';
rv = apr_getopt_init(&opt, p, largc, largv);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+ ed.opt = opt;
+ ed.formatted = str;
+ ed.param = par;
+ ed.errcode = -1;
+
opt->errfn = unknown_arg;
- opt->errarg = str;
+ opt->errarg = &ed;
while (apr_getopt(opt, "a:", &ch, &opt_arg) == APR_SUCCESS) {
switch (ch) {
@@ -156,7 +193,9 @@
break;
}
}
- ABTS_STR_EQUAL(tc, "testprog: option requires an argument -- a\n", str);
+ ABTS_STR_EQUAL(tc, "testprog: option requires an argument: a\n", str);
+ ABTS_STR_EQUAL(tc, "a", par);
+ ABTS_INT_EQUAL(tc, APR_GETOPT_MISSING_ARGUMENT, ed.errcode);
}
static void optional_option(abts_case *tc, void *data)
@@ -168,13 +207,19 @@
char ch;
const char *opt_arg;
char str[8196];
+ error_data_t ed;
str[0] = '\0';
rv = apr_getopt_init(&opt, p, largc, largv);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+ ed.opt = opt;
+ ed.formatted = str;
+ ed.param = NULL;
+ ed.errcode = -1;
+
opt->errfn = unknown_arg;
- opt->errarg = str;
+ opt->errarg = &ed;
while (apr_getopt(opt, "a::", &ch, &opt_arg) == APR_SUCCESS) {
switch (ch) {
@@ -186,6 +231,7 @@
}
}
ABTS_STR_EQUAL(tc, "option: a with foo\n", str);
+ ABTS_INT_EQUAL(tc, -1, ed.errcode);
}
static void optional_option_notgiven(abts_case *tc, void *data)
@@ -197,13 +243,21 @@
char ch;
const char *opt_arg;
char str[8196];
+ char par[128];
+ error_data_t ed;
str[0] = '\0';
+ par[0] = '\0';
rv = apr_getopt_init(&opt, p, largc, largv);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+ ed.opt = opt;
+ ed.formatted = str;
+ ed.param = par;
+ ed.errcode = -1;
+
opt->errfn = unknown_arg;
- opt->errarg = str;
+ opt->errarg = &ed;
while (apr_getopt(opt, "a::", &ch, &opt_arg) == APR_SUCCESS) {
switch (ch) {
@@ -218,7 +272,9 @@
/* Our version of getopt doesn't allow for optional arguments. */
ABTS_STR_EQUAL(tc, "option: a\n", str);
#endif
- ABTS_STR_EQUAL(tc, "testprog: option requires an argument -- a\n", str);
+ ABTS_STR_EQUAL(tc, "testprog: option requires an argument: a\n", str);
+ ABTS_STR_EQUAL(tc, "a", par);
+ ABTS_INT_EQUAL(tc, APR_GETOPT_MISSING_ARGUMENT, ed.errcode);
}
abts_suite *testgetopt(abts_suite *suite)
