MEDIUM: arg: make make_arg_list() stop after its own arguments
The main problem we're having with argument parsing is that at the
moment the caller looks for the first character looking like an end
of arguments (')') and calls make_arg_list() on the sub-string inside
the parenthesis.
Let's first change the way it works so that make_arg_list() also
consumes the parenthesis and returns the pointer to the first char not
consumed. This will later permit to refine each argument parsing.
For now there is no functional change.
diff --git a/include/proto/arg.h b/include/proto/arg.h
index db63b0e..884e5bb 100644
--- a/include/proto/arg.h
+++ b/include/proto/arg.h
@@ -80,7 +80,7 @@
struct arg_list *arg_list_clone(const struct arg_list *orig);
struct arg_list *arg_list_add(struct arg_list *orig, struct arg *arg, int pos);
int make_arg_list(const char *in, int len, uint64_t mask, struct arg **argp,
- char **err_msg, const char **err_ptr, int *err_arg,
+ char **err_msg, const char **end_ptr, int *err_arg,
struct arg_list *al);
#endif /* _PROTO_ARG_H */
diff --git a/src/acl.c b/src/acl.c
index bef3d4e..4b8a6d4 100644
--- a/src/acl.c
+++ b/src/acl.c
@@ -193,27 +193,12 @@
for (arg = args[0]; is_idchar(*arg); arg++)
;
- endt = arg;
- if (*endt == '(') {
- /* look for the end of this term and skip the opening parenthesis */
- endt = ++arg;
- while (*endt && *endt != ')')
- endt++;
- if (*endt != ')') {
- memprintf(err, "missing closing ')' after arguments to ACL keyword '%s'", aclkw->kw);
- goto out_free_smp;
- }
- }
-
/* At this point, we have :
* - args[0] : beginning of the keyword
* - arg : end of the keyword, first character not part of keyword
- * nor the opening parenthesis (so first character of args
- * if present).
- * - endt : end of the term (=arg or last parenthesis if args are present)
*/
- nbargs = make_arg_list(arg, endt - arg, smp->fetch->arg_mask, &smp->arg_p,
- err, NULL, NULL, al);
+ nbargs = make_arg_list(arg, -1, smp->fetch->arg_mask, &smp->arg_p,
+ err, &endt, NULL, al);
if (nbargs < 0) {
/* note that make_arg_list will have set <err> here */
memprintf(err, "ACL keyword '%s' : %s", aclkw->kw, *err);
@@ -241,9 +226,8 @@
while (*arg) {
struct sample_conv *conv;
struct sample_conv_expr *conv_expr;
-
- if (*arg == ')') /* skip last closing parenthesis */
- arg++;
+ int err_arg;
+ int argcnt;
if (*arg && *arg != ',') {
if (ckw)
@@ -255,6 +239,9 @@
goto out_free_smp;
}
+ /* FIXME: how long should we support such idiocies ? Maybe we
+ * should already warn ?
+ */
while (*arg == ',') /* then trailing commas */
arg++;
@@ -279,16 +266,6 @@
}
arg = endw;
- if (*arg == '(') {
- /* look for the end of this term */
- while (*arg && *arg != ')')
- arg++;
- if (*arg != ')') {
- memprintf(err, "ACL keyword '%s' : syntax error: missing ')' after converter '%s'.",
- aclkw->kw, ckw);
- goto out_free_smp;
- }
- }
if (conv->in_type >= SMP_TYPES || conv->out_type >= SMP_TYPES) {
memprintf(err, "ACL keyword '%s' : returns type of converter '%s' is unknown.",
@@ -312,35 +289,26 @@
conv_expr->conv = conv;
acl_conv_found = 1;
- if (arg != endw) {
- int err_arg;
-
- if (!conv->arg_mask) {
- memprintf(err, "ACL keyword '%s' : converter '%s' does not support any args.",
- aclkw->kw, ckw);
- goto out_free_smp;
- }
+ al->kw = smp->fetch->kw;
+ al->conv = conv_expr->conv->kw;
+ argcnt = make_arg_list(endw, -1, conv->arg_mask, &conv_expr->arg_p, err, &arg, &err_arg, al);
+ if (argcnt < 0) {
+ memprintf(err, "ACL keyword '%s' : invalid arg %d in converter '%s' : %s.",
+ aclkw->kw, err_arg+1, ckw, *err);
+ goto out_free_smp;
+ }
- al->kw = smp->fetch->kw;
- al->conv = conv_expr->conv->kw;
- if (make_arg_list(endw + 1, arg - endw - 1, conv->arg_mask, &conv_expr->arg_p, err, NULL, &err_arg, al) < 0) {
- memprintf(err, "ACL keyword '%s' : invalid arg %d in converter '%s' : %s.",
- aclkw->kw, err_arg+1, ckw, *err);
- goto out_free_smp;
- }
+ if (argcnt && !conv->arg_mask) {
+ memprintf(err, "converter '%s' does not support any args", ckw);
+ goto out_free_smp;
+ }
- if (!conv_expr->arg_p)
- conv_expr->arg_p = empty_arg_list;
+ if (!conv_expr->arg_p)
+ conv_expr->arg_p = empty_arg_list;
- if (conv->val_args && !conv->val_args(conv_expr->arg_p, conv, file, line, err)) {
- memprintf(err, "ACL keyword '%s' : invalid args in converter '%s' : %s.",
- aclkw->kw, ckw, *err);
- goto out_free_smp;
- }
- }
- else if (ARGM(conv->arg_mask)) {
- memprintf(err, "ACL keyword '%s' : missing args for converter '%s'.",
- aclkw->kw, ckw);
+ if (conv->val_args && !conv->val_args(conv_expr->arg_p, conv, file, line, err)) {
+ memprintf(err, "ACL keyword '%s' : invalid args in converter '%s' : %s.",
+ aclkw->kw, ckw, *err);
goto out_free_smp;
}
}
diff --git a/src/arg.c b/src/arg.c
index 2719b53..ce49046 100644
--- a/src/arg.c
+++ b/src/arg.c
@@ -81,21 +81,30 @@
return new;
}
-/* This function builds an argument list from a config line. It returns the
- * number of arguments found, or <0 in case of any error. Everything needed
- * it automatically allocated. A pointer to an error message might be returned
- * in err_msg if not NULL, in which case it would be allocated and the caller
- * will have to check it and free it. The output arg list is returned in argp
- * which must be valid. The returned array is always terminated by an arg of
- * type ARGT_STOP (0), unless the mask indicates that no argument is supported.
- * Unresolved arguments are appended to arg list <al>, which also serves as a
- * template to create new entries. The mask is composed of a number of
- * mandatory arguments in its lower ARGM_BITS bits, and a concatenation of each
- * argument type in each subsequent ARGT_BITS-bit sblock. If <err_msg> is not
- * NULL, it must point to a freeable or NULL pointer.
+/* This function builds an argument list from a config line, and stops at the
+ * first non-matching character, which is pointed to in <end_ptr>. A valid arg
+ * list starts with an opening parenthesis '(', contains a number of comma-
+ * delimited words, and ends with the closing parenthesis ')'. An empty list
+ * (with or without the parenthesis) will lead to a valid empty argument if the
+ * keyword has a mandatory one. The function returns the number of arguments
+ * emitted, or <0 in case of any error. Everything needed it automatically
+ * allocated. A pointer to an error message might be returned in err_msg if not
+ * NULL, in which case it would be allocated and the caller will have to check
+ * it and free it. The output arg list is returned in argp which must be valid.
+ * The returned array is always terminated by an arg of type ARGT_STOP (0),
+ * unless the mask indicates that no argument is supported. Unresolved arguments
+ * are appended to arg list <al>, which also serves as a template to create new
+ * entries. The mask is composed of a number of mandatory arguments in its lower
+ * ARGM_BITS bits, and a concatenation of each argument type in each subsequent
+ * ARGT_BITS-bit sblock. If <err_msg> is not NULL, it must point to a freeable
+ * or NULL pointer. The caller is expected to restart the parsing from the new
+ * pointer set in <end_ptr>, which is the first character considered as not
+ * being part of the arg list. The input string ends on the first between <len>
+ * characters (when len is positive) or the first NUL character. Placing -1 in
+ * <len> will make it virtually unbounded (~2GB long strings).
*/
int make_arg_list(const char *in, int len, uint64_t mask, struct arg **argp,
- char **err_msg, const char **err_ptr, int *err_arg,
+ char **err_msg, const char **end_ptr, int *err_arg,
struct arg_list *al)
{
int nbarg;
@@ -105,10 +114,22 @@
char *word = NULL;
const char *ptr_err = NULL;
int min_arg;
+ int empty;
struct arg_list *new_al = al;
*argp = NULL;
+ empty = 0;
+ if (!len || *in != '(') {
+ /* it's already not for us, stop here */
+ empty = 1;
+ len = 0;
+ } else {
+ /* skip opening parenthesis */
+ len--;
+ in++;
+ }
+
min_arg = mask & ARGM_MASK;
mask >>= ARGM_BITS;
@@ -122,7 +143,7 @@
/* Note: an empty input string contains an empty argument if this argument
* is marked mandatory. Otherwise we can ignore it.
*/
- if (!len && !min_arg)
+ if (empty && !min_arg)
goto end_parse;
arg = *argp = calloc(nbarg + 1, sizeof(*arg));
@@ -132,7 +153,7 @@
unsigned int uint;
beg = in;
- while (len && *in != ',') {
+ while (len && *in != ',' && *in && *in != ')') {
in++;
len--;
}
@@ -261,7 +282,7 @@
arg++;
/* don't go back to parsing if we reached end */
- if (!len || pos >= nbarg)
+ if (!len || !*in || *in == ')' || pos >= nbarg)
break;
/* skip comma */
@@ -279,16 +300,24 @@
goto err;
}
- if (len) {
- /* too many arguments, starting at <in> */
+ if (empty) {
+ /* nothing to do */
+ } else if (*in == ')') {
+ /* skip the expected closing parenthesis */
+ in++;
+ } else {
/* the caller is responsible for freeing this message */
- word = my_strndup(in, len);
- if (nbarg)
- memprintf(err_msg, "end of arguments expected at position %d, but got '%s'",
- pos + 1, word);
- else
- memprintf(err_msg, "no argument supported, but got '%s'", word);
- free(word); word = NULL;
+ word = (len > 0) ? my_strndup(in, len) : (char *)in;
+ memprintf(err_msg, "expected ')' before '%s'", word);
+ if (len > 0)
+ free(word);
+ word = NULL;
+ /* when we're missing a right paren, the empty part preceeding
+ * already created an empty arg, adding one to the position, so
+ * let's fix the reporting to avoid being confusing.
+ */
+ if (pos > 1)
+ pos--;
goto err;
}
@@ -297,8 +326,8 @@
*/
if (err_arg)
*err_arg = pos;
- if (err_ptr)
- *err_ptr = in;
+ if (end_ptr)
+ *end_ptr = in;
return pos;
err:
@@ -312,8 +341,8 @@
*argp = NULL;
if (err_arg)
*err_arg = pos;
- if (err_ptr)
- *err_ptr = in;
+ if (end_ptr)
+ *end_ptr = in;
return -1;
empty_err:
diff --git a/src/sample.c b/src/sample.c
index d9439fb..d82451f 100644
--- a/src/sample.c
+++ b/src/sample.c
@@ -856,24 +856,9 @@
goto out_error;
}
- endt = endw;
- if (*endt == '(') {
- /* look for the end of this term and skip the opening parenthesis */
- endt = ++endw;
- while (*endt && *endt != ')')
- endt++;
- if (*endt != ')') {
- memprintf(err_msg, "missing closing ')' after arguments to fetch keyword '%s'", fkw);
- goto out_error;
- }
- }
-
/* At this point, we have :
* - begw : beginning of the keyword
* - endw : end of the keyword, first character not part of keyword
- * nor the opening parenthesis (so first character of args
- * if present).
- * - endt : end of the term (=endw or last parenthesis if args are present)
*/
if (fetch->out_type >= SMP_TYPES) {
@@ -896,11 +881,16 @@
*/
al->kw = expr->fetch->kw;
al->conv = NULL;
- if (make_arg_list(endw, endt - endw, fetch->arg_mask, &expr->arg_p, err_msg, NULL, &err_arg, al) < 0) {
+ if (make_arg_list(endw, -1, fetch->arg_mask, &expr->arg_p, err_msg, &endt, &err_arg, al) < 0) {
memprintf(err_msg, "fetch method '%s' : %s", fkw, *err_msg);
goto out_error;
}
+ /* now endt is our first char not part of the arg list, typically the
+ * comma after the sample fetch name or after the closing parenthesis,
+ * or the NUL char.
+ */
+
if (!expr->arg_p) {
expr->arg_p = empty_arg_list;
}
@@ -926,9 +916,6 @@
int err_arg;
int argcnt;
- if (*endt == ')') /* skip last closing parenthesis */
- endt++;
-
if (*endt && *endt != ',') {
if (ckw)
memprintf(err_msg, "missing comma after converter '%s'", ckw);
@@ -937,6 +924,9 @@
goto out_error;
}
+ /* FIXME: how long should we support such idiocies ? Maybe we
+ * should already warn ?
+ */
while (*endt == ',') /* then trailing commas */
endt++;
@@ -965,18 +955,6 @@
goto out_error;
}
- endt = endw;
- if (*endt == '(') {
- /* look for the end of this term */
- endt = ++endw;
- while (*endt && *endt != ')')
- endt++;
- if (*endt != ')') {
- memprintf(err_msg, "syntax error: missing ')' after converter '%s'", ckw);
- goto out_error;
- }
- }
-
if (conv->in_type >= SMP_TYPES || conv->out_type >= SMP_TYPES) {
memprintf(err_msg, "returns type of converter '%s' is unknown", ckw);
goto out_error;
@@ -998,7 +976,7 @@
al->kw = expr->fetch->kw;
al->conv = conv_expr->conv->kw;
- argcnt = make_arg_list(endw, endt - endw, conv->arg_mask, &conv_expr->arg_p, err_msg, NULL, &err_arg, al);
+ argcnt = make_arg_list(endw, -1, conv->arg_mask, &conv_expr->arg_p, err_msg, &endt, &err_arg, al);
if (argcnt < 0) {
memprintf(err_msg, "invalid arg %d in converter '%s' : %s", err_arg+1, ckw, *err_msg);
goto out_error;