MEDIUM: acl/sample: unify sample conv parsing in a single function

Both sample_parse_expr() and parse_acl_expr() implement some code
logic to parse sample conv list after respective fetch or acl keyword.

(Seems like the acl one was inspired by the sample one historically)

But there is clearly code duplication between the two functions, making
them hard to maintain.
Hopefully, the parsing logic between them has stayed pretty much the
same, thus the sample conv parsing part may be moved in a dedicated
helper parsing function.

This is what's being done in this commit, we're adding the new function
sample_parse_expr_cnv() which does a single thing: parse the converters
that are listed right after a sample fetch keyword and inject them into
an already existing sample expression.

Both sample_parse_expr() and parse_acl_expr() were adapted to now make
use of this specific parsing function and duplicated code parts were
cleaned up.

Although sample_parse_expr() remains quite complicated (numerous function
arguments due to contextual parsing data) the first goal was to get rid of
code duplication without impacting the current behavior, with the added
benefit that it may allow further code cleanups / simplification in the
future.

diff --git a/include/haproxy/sample.h b/include/haproxy/sample.h
index 14d1a062314404c65d9ea6530ef01624ed49c281..74c206b25573e4ce39ad6c3fc5c219e831e4d473 100644 (file)
--- a/include/haproxy/sample.h
+++ b/include/haproxy/sample.h
@@ -33,6 +33,8 @@ extern const unsigned int fetch_cap[SMP_SRC_ENTRIES];
 extern const char *smp_to_type[SMP_TYPES];
 
 struct sample_expr *sample_parse_expr(char **str, int *idx, const char *file, int line, char **err, struct arg_list *al, char **endptr);
+int sample_parse_expr_cnv(char **str, int *idx, char **endptr, char **err_msg, struct arg_list *al, const char *file, int line,
+                          struct sample_expr *expr, const char *start);
 struct sample_conv *find_sample_conv(const char *kw, int len);
 struct sample *sample_process(struct proxy *px, struct session *sess,
                               struct stream *strm, unsigned int opt,

diff --git a/src/acl.c b/src/acl.c
index 29b070bc13f2414ce3fe409586fb510f1993807f..198e1bfdddc2d2caf06ba0873f659f7076b6651d 100644 (file)
--- a/src/acl.c
+++ b/src/acl.c
@@ -138,8 +138,6 @@ struct acl_expr *parse_acl_expr(const char **args, char **err, struct arg_list *
        struct sample_expr *smp = NULL;
        int idx = 0;
        char *ckw = NULL;
-       const char *begw;
-       const char *endw;
        const char *endt;
        int cur_type;
        int nbargs;
@@ -209,106 +207,18 @@ struct acl_expr *parse_acl_expr(const char **args, char **err, struct arg_list *
                        memprintf(err, "in argument to '%s', %s", aclkw->kw, *err);
                        goto out_free_smp;
                }
-               arg = endt;
 
                /* look for the beginning of the converters list. Those directly attached
-                * to the ACL keyword are found just after <arg> which points to the comma.
+                * to the ACL keyword are found just after the comma.
                 * If we find any converter, then we don't use the ACL keyword's match
                 * anymore but the one related to the converter's output type.
                 */
-               cur_type = smp->fetch->out_type;
-               while (*arg) {
-                       struct sample_conv *conv;
-                       struct sample_conv_expr *conv_expr;
-                       int err_arg;
-                       int argcnt;
-
-                       if (*arg && *arg != ',') {
-                               if (ckw)
-                                       memprintf(err, "ACL keyword '%s' : missing comma after converter '%s'.",
-                                                 aclkw->kw, ckw);
-                               else
-                                       memprintf(err, "ACL keyword '%s' : missing comma after fetch keyword.",
-                                                 aclkw->kw);
-                               goto out_free_smp;
-                       }
-
-                       /* FIXME: how long should we support such idiocies ? Maybe we
-                        * should already warn ?
-                        */
-                       while (*arg == ',') /* then trailing commas */
-                               arg++;
-
-                       begw = arg; /* start of converter keyword */
-
-                       if (!*begw)
-                               /* none ? end of converters */
-                               break;
-
-                       for (endw = begw; is_idchar(*endw); endw++)
-                               ;
-
-                       free(ckw);
-                       ckw = my_strndup(begw, endw - begw);
-
-                       conv = find_sample_conv(begw, endw - begw);
-                       if (!conv) {
-                               /* Unknown converter method */
-                               memprintf(err, "ACL keyword '%s' : unknown converter '%s'.",
-                                         aclkw->kw, ckw);
-                               goto out_free_smp;
-                       }
-
-                       arg = endw;
-
-                       if (conv->in_type >= SMP_TYPES || conv->out_type >= SMP_TYPES) {
-                               memprintf(err, "ACL keyword '%s' : returns type of converter '%s' is unknown.",
-                                         aclkw->kw, ckw);
-                               goto out_free_smp;
-                       }
-
-                       /* If impossible type conversion */
-                       if (!sample_casts[cur_type][conv->in_type]) {
-                               memprintf(err, "ACL keyword '%s' : converter '%s' cannot be applied.",
-                                         aclkw->kw, ckw);
-                               goto out_free_smp;
-                       }
-
-                       cur_type = conv->out_type;
-                       conv_expr = calloc(1, sizeof(*conv_expr));
-                       if (!conv_expr)
-                               goto out_free_smp;
-
-                       LIST_APPEND(&(smp->conv_exprs), &(conv_expr->list));
-                       conv_expr->conv = conv;
-                       acl_conv_found = 1;
-
-                       if (al) {
-                               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;
-                       }
-
-                       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->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;
-                       }
+               if (!sample_parse_expr_cnv((char **)args, NULL, NULL, err, al, file, line, smp, endt)) {
+                       if (err)
+                               memprintf(err, "ACL keyword '%s' : %s", aclkw->kw, *err);
+                       goto out_free_smp;
                }
-               ha_free(&ckw);
+               acl_conv_found = !LIST_ISEMPTY(&smp->conv_exprs);
        }
        else {
                /* This is not an ACL keyword, so we hope this is a sample fetch
@@ -321,9 +231,11 @@ struct acl_expr *parse_acl_expr(const char **args, char **err, struct arg_list *
                        memprintf(err, "%s in ACL expression '%s'", *err, *args);
                        goto out_return;
                }
-               cur_type = smp_expr_output_type(smp);
        }
 
+       /* get last effective output type for smp */
+       cur_type = smp_expr_output_type(smp);
+
        expr = calloc(1, sizeof(*expr));
        if (!expr) {
                memprintf(err, "out of memory when parsing ACL expression");

diff --git a/src/sample.c b/src/sample.c
index ce01b0f8bb7346ea6977d296d2d7b4acf47cf2bf..d496ddbd940d6479fe830d83a012172f7faddbd5 100644 (file)
--- a/src/sample.c
+++ b/src/sample.c
@@ -989,104 +989,40 @@ sample_cast_fct sample_casts[SMP_TYPES][SMP_TYPES] = {
 /*       METH */ { c_none, NULL,      NULL,      NULL,       NULL,     NULL,       c_meth2str, c_meth2str, c_none,     }
 };
 
-/*
- * Parse a sample expression configuration:
- *        fetch keyword followed by format conversion keywords.
- * Returns a pointer on allocated sample expression structure.
- * <al> is an arg_list serving as a list head to report missing dependencies.
- * It may be NULL if such dependencies are not allowed. Otherwise, the caller
- * must have set al->ctx if al is set.
+/* Process the converters (if any) for a sample expr after the first fetch
+ * keyword. We have two supported syntaxes for the converters, which can be
+ * combined:
+ *  - comma-delimited list of converters just after the keyword and args ;
+ *  - one converter per keyword (if <idx> != NULL)
+ *    FIXME: should we continue to support this old syntax?
+ * The combination allows to have each keyword being a comma-delimited
+ * series of converters.
+ *
+ * We want to process the former first, then the latter. For this we start
+ * from the beginning of the supposed place in the exiting conv chain, which
+ * starts at the last comma (<start> which is then referred to as endt).
+ *
  * If <endptr> is non-nul, it will be set to the first unparsed character
  * (which may be the final '\0') on success. If it is nul, the expression
  * must be properly terminated by a '\0' otherwise an error is reported.
+ *
+ * <expr> should point the the sample expression that is already initialized
+ * with the sample fetch that precedes the converters chain.
+ *
+ * The function returns a positive value for success and 0 for failure, in which
+ * case <err_msg> will point to an allocated string that brings some info
+ * about the failure. It is the caller's responsibility to free it.
  */
-struct sample_expr *sample_parse_expr(char **str, int *idx, const char *file, int line, char **err_msg, struct arg_list *al, char **endptr)
+int sample_parse_expr_cnv(char **str, int *idx, char **endptr, char **err_msg, struct arg_list *al, const char *file, int line,
+                          struct sample_expr *expr, const char *start)
 {
-       const char *begw; /* beginning of word */
-       const char *endw; /* end of word */
-       const char *endt; /* end of term */
-       struct sample_expr *expr = NULL;
-       struct sample_fetch *fetch;
        struct sample_conv *conv;
-       unsigned long prev_type;
-       char *fkw = NULL;
+       const char *endt = start; /* end of term */
+       const char *begw;         /* beginning of word */
+       const char *endw;         /* end of word */
        char *ckw = NULL;
-       int err_arg;
-
-       begw = str[*idx];
-       for (endw = begw; is_idchar(*endw); endw++)
-               ;
-
-       if (endw == begw) {
-               memprintf(err_msg, "missing fetch method");
-               goto out_error;
-       }
-
-       /* keep a copy of the current fetch keyword for error reporting */
-       fkw = my_strndup(begw, endw - begw);
-
-       fetch = find_sample_fetch(begw, endw - begw);
-       if (!fetch) {
-               memprintf(err_msg, "unknown fetch method '%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
-        */
-
-       if (fetch->out_type >= SMP_TYPES) {
-               memprintf(err_msg, "returns type of fetch method '%s' is unknown", fkw);
-               goto out_error;
-       }
-       prev_type = fetch->out_type;
-
-       expr = calloc(1, sizeof(*expr));
-       if (!expr)
-               goto out_error;
-
-       LIST_INIT(&(expr->conv_exprs));
-       expr->fetch = fetch;
-       expr->arg_p = empty_arg_list;
-
-       /* Note that we call the argument parser even with an empty string,
-        * this allows it to automatically create entries for mandatory
-        * implicit arguments (eg: local proxy name).
-        */
-       if (al) {
-               al->kw = expr->fetch->kw;
-               al->conv = NULL;
-       }
-       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;
-       }
-       else if (fetch->val_args && !fetch->val_args(expr->arg_p, err_msg)) {
-               memprintf(err_msg, "invalid args in fetch method '%s' : %s", fkw, *err_msg);
-               goto out_error;
-       }
-
-       /* Now process the converters if any. We have two supported syntaxes
-        * for the converters, which can be combined :
-        *  - comma-delimited list of converters just after the keyword and args ;
-        *  - one converter per keyword
-        * The combination allows to have each keyword being a comma-delimited
-        * series of converters.
-        *
-        * We want to process the former first, then the latter. For this we start
-        * from the beginning of the supposed place in the exiting conv chain, which
-        * starts at the last comma (endt).
-        */
+       unsigned long prev_type = expr->fetch->out_type;
+       int success = 1;
 
        while (1) {
                struct sample_conv_expr *conv_expr;
@@ -1101,7 +1037,7 @@ struct sample_expr *sample_parse_expr(char **str, int *idx, const char *file, in
                        if (ckw)
                                memprintf(err_msg, "missing comma after converter '%s'", ckw);
                        else
-                               memprintf(err_msg, "missing comma after fetch keyword '%s'", fkw);
+                               memprintf(err_msg, "missing comma after fetch keyword");
                        goto out_error;
                }
 
@@ -1114,7 +1050,9 @@ struct sample_expr *sample_parse_expr(char **str, int *idx, const char *file, in
                begw = endt; /* start of converter */
 
                if (!*begw) {
-                       /* none ? skip to next string */
+                       /* none ? skip to next string if idx is set */
+                       if (!idx)
+                               break; /* end of converters */
                        (*idx)++;
                        begw = str[*idx];
                        if (!begw || !*begw)
@@ -1124,13 +1062,13 @@ struct sample_expr *sample_parse_expr(char **str, int *idx, const char *file, in
                for (endw = begw; is_idchar(*endw); endw++)
                        ;
 
-               free(ckw);
+               ha_free(&ckw);
                ckw = my_strndup(begw, endw - begw);
 
                conv = find_sample_conv(begw, endw - begw);
                if (!conv) {
                        /* we found an isolated keyword that we don't know, it's not ours */
-                       if (begw == str[*idx]) {
+                       if (idx && begw == str[*idx]) {
                                endt = begw;
                                break;
                        }
@@ -1139,7 +1077,7 @@ struct sample_expr *sample_parse_expr(char **str, int *idx, const char *file, in
                }
 
                if (conv->in_type >= SMP_TYPES || conv->out_type >= SMP_TYPES) {
-                       memprintf(err_msg, "returns type of converter '%s' is unknown", ckw);
+                       memprintf(err_msg, "return type of converter '%s' is unknown", ckw);
                        goto out_error;
                }
 
@@ -1185,10 +1123,105 @@ struct sample_expr *sample_parse_expr(char **str, int *idx, const char *file, in
                /* end found, let's stop here */
                *endptr = (char *)endt;
        }
+ out:
+       free(ckw);
+       return success;
+
+ out_error:
+       success = 0;
+       goto out;
+}
+
+/*
+ * Parse a sample expression configuration:
+ *        fetch keyword followed by format conversion keywords.
+ *
+ * <al> is an arg_list serving as a list head to report missing dependencies.
+ * It may be NULL if such dependencies are not allowed. Otherwise, the caller
+ * must have set al->ctx if al is set.
+ *
+ * Returns a pointer on allocated sample expression structure or NULL in case
+ * of error, in which case <err_msg> will point to an allocated string that
+ * brings some info about the failure. It is the caller's responsibility to
+ * free it.
+ */
+struct sample_expr *sample_parse_expr(char **str, int *idx, const char *file, int line, char **err_msg, struct arg_list *al, char **endptr)
+{
+       const char *begw; /* beginning of word */
+       const char *endw; /* end of word */
+       const char *endt; /* end of term */
+       struct sample_expr *expr = NULL;
+       struct sample_fetch *fetch;
+       char *fkw = NULL;
+       int err_arg;
+
+       begw = str[*idx];
+       for (endw = begw; is_idchar(*endw); endw++)
+               ;
+
+       if (endw == begw) {
+               memprintf(err_msg, "missing fetch method");
+               goto out_error;
+       }
+
+       /* keep a copy of the current fetch keyword for error reporting */
+       fkw = my_strndup(begw, endw - begw);
+
+       fetch = find_sample_fetch(begw, endw - begw);
+       if (!fetch) {
+               memprintf(err_msg, "unknown fetch method '%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
+        */
+
+       if (fetch->out_type >= SMP_TYPES) {
+               memprintf(err_msg, "returns type of fetch method '%s' is unknown", fkw);
+               goto out_error;
+       }
+
+       expr = calloc(1, sizeof(*expr));
+       if (!expr)
+               goto out_error;
+
+       LIST_INIT(&(expr->conv_exprs));
+       expr->fetch = fetch;
+       expr->arg_p = empty_arg_list;
+
+       /* Note that we call the argument parser even with an empty string,
+        * this allows it to automatically create entries for mandatory
+        * implicit arguments (eg: local proxy name).
+        */
+       if (al) {
+               al->kw = expr->fetch->kw;
+               al->conv = NULL;
+       }
+       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;
+       }
+       else if (fetch->val_args && !fetch->val_args(expr->arg_p, err_msg)) {
+               memprintf(err_msg, "invalid args in fetch method '%s' : %s", fkw, *err_msg);
+               goto out_error;
+       }
+
+       if (!sample_parse_expr_cnv(str, idx, endptr, err_msg, al, file, line, expr, endt))
+               goto out_error;
 
  out:
        free(fkw);
-       free(ckw);
        return expr;
 
 out_error: