From 5a7b51d7df3a23ced3fd979c26358b7c187accac Mon Sep 17 00:00:00 2001 From: "Alan T. DeKok" Date: Thu, 20 Jan 2022 16:23:21 -0500 Subject: [PATCH] groupify arguments to functions for now, we create the arguments and then groupify them if necessary. The alternative is to always create grouping nodes, and then remove / reparent them if they're not necessary. --- src/lib/unlang/xlat_expr.c | 62 ++++++++++++++++++++++++++++++++-- src/lib/unlang/xlat_tokenize.c | 8 +++-- src/tests/unit/xlat/expr.txt | 8 ++--- src/tests/unit/xlat/purify.txt | 13 +++++-- 4 files changed, 78 insertions(+), 13 deletions(-) diff --git a/src/lib/unlang/xlat_expr.c b/src/lib/unlang/xlat_expr.c index c90e1e8ffc..f10dce7a47 100644 --- a/src/lib/unlang/xlat_expr.c +++ b/src/lib/unlang/xlat_expr.c @@ -49,9 +49,12 @@ RCSID("$Id$") * @todo - short-circuit && / || need to be updated. This requires various magic in their instantiation * routines, which is not yet done. * - * @todo - Check for input flags->pure, and node->internal && node->token != T_INVALID && node->pure. If - * so, we call xlat_purify() to purify the results. This capability lets us write tests for parsing - * which use simple numbers, to verify that the parser is OK. + * @todo - all function arguments should be in groups, so we need to fix that. Right now, binary + * expressions are fixed. But unary ones are not. We did it via a hack, but it might be better to do it + * a different way in the future. The problem is that no matter which way we choose, we'll have to + * talloc_steal() something. + * + * @todo - run xlat_purify_expr() after creating the unary node. * * The purify function should also be smart enough to do things like remove redundant casts. * @@ -215,6 +218,52 @@ cleanup: return rcode; } +static xlat_exp_t *xlat_groupify_node(TALLOC_CTX *ctx, xlat_exp_t *node) +{ + xlat_exp_t *group; + + fr_assert(node->type != XLAT_GROUP); + + group = xlat_exp_alloc_null(ctx); + xlat_exp_set_type(group, XLAT_GROUP); + group->quote = T_BARE_WORD; + + group->child = talloc_steal(group, node); + group->flags = node->flags; + + if (node->next) { + group->next = xlat_groupify_node(ctx, node->next); + node->next = NULL; + } + + return group; +} + +/* + * Any function requires each argument to be in it's own XLAT_GROUP. But we can't create an XLAT_GROUP + * from the start of parsing, as we might need to return an XLAT_FUNC, or another type of xlat. Instead, + * we just work on the bare nodes, and then later groupify them. For now, it's just easier to do it this way. + */ +static void xlat_groupify_expr(xlat_exp_t *node) +{ + xlat_t const *func; + + if (node->type != XLAT_FUNC) return; + + func = node->call.func; + + if (!func->internal) return; + + if (func->token == T_INVALID) return; + + /* + * It's already been groupified, don't do anything. + */ + if (node->child->type == XLAT_GROUP) return; + + node->child = xlat_groupify_node(node, node->child); +} + static xlat_arg_parser_t const cast_xlat_args[] = { { .required = true, .type = FR_TYPE_INT32 }, { .required = true, .type = FR_TYPE_VOID }, @@ -948,6 +997,8 @@ done: cast->child->next = node; xlat_flags_merge(&cast->flags, &node->flags); node = cast; + + xlat_groupify_expr(node); } check_unary: @@ -1189,6 +1240,11 @@ redo: } } + /* + * Ensure that the various nodes are grouped properly. + */ + xlat_groupify_expr(node); + lhs = node; goto redo; } diff --git a/src/lib/unlang/xlat_tokenize.c b/src/lib/unlang/xlat_tokenize.c index bd2d00ffa0..443b1b0093 100644 --- a/src/lib/unlang/xlat_tokenize.c +++ b/src/lib/unlang/xlat_tokenize.c @@ -1136,10 +1136,13 @@ static ssize_t xlat_print_node(fr_sbuff_t *out, xlat_exp_t const *head, fr_sbuff if (node->quote != T_BARE_WORD) FR_SBUFF_IN_CHAR_RETURN(out, fr_token_quote[node->quote]); xlat_print(out, node->child, fr_value_escape_by_quote[node->quote]); if (node->quote != T_BARE_WORD) FR_SBUFF_IN_CHAR_RETURN(out, fr_token_quote[node->quote]); - if (node->next) FR_SBUFF_IN_CHAR_RETURN(out, ' '); /* Add ' ' between args */ + if (node->next) FR_SBUFF_IN_CHAR_RETURN(out, ' '); /* Add ' ' between args */ goto done; case XLAT_BOX: + /* + * @todo - respect node->quote here, too. Which also means updating the parser. + */ FR_SBUFF_RETURN(fr_value_box_print, out, &node->data, e_rules); goto done; @@ -1161,8 +1164,7 @@ static ssize_t xlat_print_node(fr_sbuff_t *out, xlat_exp_t const *head, fr_sbuff } else { FR_SBUFF_IN_STRCPY_LITERAL_RETURN(out, "("); - xlat_print_node(out, node->child, e_rules); - FR_SBUFF_IN_CHAR_RETURN(out, ' '); + xlat_print_node(out, node->child, e_rules); /* prints a space after the first argument */ /* * @todo - when things like "+" support more than 2 arguments, print them all out diff --git a/src/tests/unit/xlat/expr.txt b/src/tests/unit/xlat/expr.txt index fbf12bd94e..c03ccdc2bc 100644 --- a/src/tests/unit/xlat/expr.txt +++ b/src/tests/unit/xlat/expr.txt @@ -45,10 +45,10 @@ match (1 + (%{Service-Type} == Framed-User)) # Strings of various forms # xlat_expr &Filter-Id == "foo" -match (%{Filter-Id} == \"foo\") +match (%{Filter-Id} == "foo") xlat_expr "foo" == "bar" -match (\"foo\" == \"bar\") +match ("foo" == "bar") # note '/' is a prefix, not "divide by 24". # and a useless cast is removed @@ -82,11 +82,11 @@ xlat_expr &Filter-Id match %{Filter-Id} xlat_expr %{md5:foo} + "foo" -match (%{md5:foo} + \"foo\") +match (%{md5:foo} + "foo") # We can name the xlat's, tho we don't need to xlat_expr %(op_add:4 3) + 6 -match ((4 + 3) + 6) +match ((4 + 3) + 6) # diff --git a/src/tests/unit/xlat/purify.txt b/src/tests/unit/xlat/purify.txt index 0c5aa0f39f..78583efc1a 100644 --- a/src/tests/unit/xlat/purify.txt +++ b/src/tests/unit/xlat/purify.txt @@ -1,5 +1,8 @@ proto-dictionary radius +#xlat_purify %(op_add:4 3) + 6 +#match 13 + # # xlat_expr, but purified # @@ -48,7 +51,7 @@ match (1 + (%{Service-Type} == Framed-User)) # Strings of various forms # xlat_purify &Filter-Id == "foo" -match (%{Filter-Id} == \"foo\") +match (%{Filter-Id} == "foo") xlat_purify "foo" == "bar" match no @@ -85,11 +88,15 @@ xlat_purify &Filter-Id match %{Filter-Id} xlat_purify %{md5:foo} + "foo" -match (%{md5:foo} + \"foo\") +match (%{md5:foo} + "foo") # We can name the xlat's, tho we don't need to +# +# And naming the xlat's means that they don't set up +# with the magic token field, so optimizations don't work? +# xlat_purify %(op_add:4 3) + 6 -match ((4 + 3) + 6) +match ((4 + 3) + 6) # -- 2.47.2