From: Alan T. DeKok <aland@freeradius.org>
Date: Tue, 24 May 2022 14:44:42 +0000 (-0400)
Subject: lots of fixes.
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b038e85fda4930ea003dd2f55c47993ce7f9c4a0;p=thirdparty%2Ffreeradius-server.git

lots of fixes.

Set flags better.  Do more resolution of binary operations.
We still need to add xlat_expr_resolve_unary()

update tests to show that most pure things can now be purified.
---

diff --git a/src/lib/unlang/xlat_expr.c b/src/lib/unlang/xlat_expr.c
index 12911b81d20..545b10a1c8c 100644
--- a/src/lib/unlang/xlat_expr.c
+++ b/src/lib/unlang/xlat_expr.c
@@ -101,17 +101,19 @@ static fr_slen_t xlat_expr_print_binary(fr_sbuff_t *out, xlat_exp_t const *node,
 	return fr_sbuff_used_total(out) - at_in;
 }
 
-static int xlat_expr_resolve_binary(xlat_exp_t *self, UNUSED void *inst, xlat_res_rules_t const *xr_rules)
+static int xlat_expr_resolve_binary(xlat_exp_t *node, UNUSED void *inst, xlat_res_rules_t const *xr_rules)
 {
 	xlat_exp_t *arg1, *arg2;
 	xlat_exp_t *a, *b;
-	tmpl_res_rules_t	my_tr_rules;
+	tmpl_res_rules_t my_tr_rules;
 
-	arg1 = xlat_exp_head(self->call.args);
+	XLAT_DEBUG("RESOLVE %s\n", node->fmt);
+
+	arg1 = xlat_exp_head(node->call.args);
 	fr_assert(arg1);
 	fr_assert(arg1->type == XLAT_GROUP);
 
-	arg2 = xlat_exp_next(self->call.args, arg1);
+	arg2 = xlat_exp_next(node->call.args, arg1);
 	fr_assert(arg2);
 	fr_assert(arg2->type == XLAT_GROUP);
 
@@ -121,16 +123,12 @@ static int xlat_expr_resolve_binary(xlat_exp_t *self, UNUSED void *inst, xlat_re
 	/*
 	 *	We have many things here, just call resolve recursively.
 	 */
-	if (xlat_exp_next(arg1->group, a) || (xlat_exp_next(arg2->group, b))) {
-		return xlat_resolve(self->call.args, xr_rules);
-	}
+	if (xlat_exp_next(arg1->group, a) || (xlat_exp_next(arg2->group, b))) goto resolve;
 
 	/*
 	 *	Anything else must get resolved at run time.
 	 */
-	if ((a->type != XLAT_TMPL) || (b->type != XLAT_TMPL)) {
-		return xlat_resolve(self->call.args, xr_rules);
-	}
+	if ((a->type != XLAT_TMPL) || (b->type != XLAT_TMPL)) goto resolve;
 
 	/*
 	 *	The tr_rules should always contain dict_def
@@ -146,24 +144,60 @@ static int xlat_expr_resolve_binary(xlat_exp_t *self, UNUSED void *inst, xlat_re
 	 *	The LHS attribute dictates the enumv for the RHS one.
 	 */
 	if (tmpl_contains_attr(a->vpt)) {
-		if (tmpl_resolve(a->vpt, &my_tr_rules) < 0) return -1;
+		XLAT_DEBUG("\ta - %s %s\n", a->fmt, b->fmt);
+
+		if (a->flags.needs_resolving) {
+			XLAT_DEBUG("\tresolve attr a\n");
+			if (tmpl_resolve(a->vpt, &my_tr_rules) < 0) return -1;
+			a->flags.needs_resolving = false;
+			arg1->flags = a->flags;
+		}
 
 		my_tr_rules.enumv = tmpl_da(a->vpt);
 
-		return tmpl_resolve(b->vpt, &my_tr_rules);
+		XLAT_DEBUG("\tresolve other b\n");
+		if (tmpl_resolve(b->vpt, &my_tr_rules) < 0) return -1;
+
+		b->flags.needs_resolving = false;
+		b->flags.pure = tmpl_is_data(b->vpt);
+		arg2->flags = b->flags;
+		goto flags;
 	}
 
 	if (tmpl_contains_attr(b->vpt)) {
-		if (tmpl_resolve(b->vpt, &my_tr_rules) < 0) return -1;
+		XLAT_DEBUG("\tb -  %s %s\n", a->fmt, b->fmt);
+
+		if (b->flags.needs_resolving) {
+			XLAT_DEBUG("\tresolve attr b\n");
+			if (tmpl_resolve(b->vpt, &my_tr_rules) < 0) return -1;
+
+			b->flags.needs_resolving = false;
+			arg2->flags = b->flags;
+		}
 
 		my_tr_rules.enumv = tmpl_da(b->vpt);
 
-		return tmpl_resolve(a->vpt, &my_tr_rules);
+		XLAT_DEBUG("\tresolve other a\n");
+		if (tmpl_resolve(a->vpt, &my_tr_rules) < 0) return -1;
+
+		a->flags.needs_resolving = false;
+		a->flags.pure = tmpl_is_data(b->vpt);
+		arg1->flags = a->flags;
+
+		goto flags;
 	}
 
-	if (tmpl_resolve(a->vpt, xr_rules->tr_rules) < 0) return -1;
+resolve:
+	if (xlat_resolve(node->call.args, xr_rules) < 0) return -1;
+
+flags:
+	node->flags = node->call.func->flags;
+	node->call.args->flags.needs_resolving = false;
+	xlat_flags_merge(&node->flags, &node->call.args->flags);
 
-	return tmpl_resolve(b->vpt, xr_rules->tr_rules);
+	node->flags.can_purify = (node->call.func->flags.pure && node->call.args->flags.pure) | node->call.args->flags.can_purify;
+
+	return 0;
 }
 
 
@@ -172,7 +206,12 @@ static void xlat_func_append_arg(xlat_exp_t *head, xlat_exp_t *node)
 	xlat_exp_t *group;
 
 	fr_assert(head->type == XLAT_FUNC);
-	fr_assert(node->type != XLAT_GROUP);
+
+	if (node->type == XLAT_GROUP) {
+		xlat_exp_insert_tail(head->call.args, node);
+		xlat_flags_merge(&head->flags, &head->call.args->flags);
+		return;
+	}
 
 	group = xlat_exp_alloc_null(head->call.args);
 	xlat_exp_set_type(group, XLAT_GROUP);
@@ -997,11 +1036,34 @@ static ssize_t tokenize_field(xlat_exp_head_t *head, xlat_exp_t **out, fr_sbuff_
 	error:
 		return -fr_sbuff_used(&our_in);
 	}
+
 	node->vpt = vpt;
 	node->quote = quote;
 	node->fmt = vpt->name;
+
 	node->flags.pure = tmpl_is_data(node->vpt);
 	node->flags.needs_resolving = tmpl_needs_resolving(node->vpt);
+
+	/*
+	 *	@todo - this doesn't quite work yet.  We really want
+	 *	to start out this node as "pure", and merge in the
+	 *	child flags.  But if we do that, then the xlat_eval()
+	 *	code dies, because we don't (yet) handle TMPL_XLAT or TMPL_EXEC
+	 */
+	if (tmpl_contains_xlat(node->vpt)) {
+		xlat_exp_head_t *xlat = tmpl_xlat(node->vpt);
+
+		talloc_steal(node, xlat);
+		node->fmt = talloc_typed_strdup(node, node->fmt);
+		talloc_free(node->vpt);
+
+		node->type = XLAT_GROUP;
+		node->group = xlat;
+
+		node->flags = xlat->flags;
+	}
+
+
 	/* don't merge flags.  That will happen when the node is added to the head */
 
 	/*
diff --git a/src/tests/unit/xlat/purify.txt b/src/tests/unit/xlat/purify.txt
index e758eba8dfc..51c79773cf1 100644
--- a/src/tests/unit/xlat/purify.txt
+++ b/src/tests/unit/xlat/purify.txt
@@ -42,9 +42,8 @@ xlat_purify 1 < 4
 match yes
 
 #
-#  There's a bunch of things to fix here:
+#  There's sopme bunch of things to fix here:
 #
-#  * Framed-User should be resolved via a custom callback, ala pass2_fixup
 #  * the tokenizer needs to track offsets, so it can return the offset which cause the error.
 #
 xlat_purify &Service-Type == Framed-User
@@ -57,15 +56,11 @@ match (%{Service-Type} == Framed-User)
 #  Strings of various forms
 #
 
-# this fails because the double-quoted string is unresolved?
-#
-#  @todo - call tmpl_resolve() on TMPL_UNRESOLVED for things other than T_BARE_WORD
-#
 xlat_purify &Filter-Id == "foo"
 match (%{Filter-Id} == "foo")
 
 xlat_purify "foo" == "bar"
-match ("foo" == "bar")
+match no
 
 # note '/' is a prefix, not "divide by 24".
 # and a useless cast is removed
@@ -92,26 +87,49 @@ match (192.168.0.0/24 > %{Framed-IP-Address})
 xlat_purify 1 < 2 || 4 > 3
 match yes
 
-#
-#  @todo - This should probably return "2"?
-#
 xlat_purify 2 || (1 > 4)
 match yes
 
 xlat_purify &Filter-Id
 match %{Filter-Id}
 
-xlat_purify %{md5:foo} + "foo"
-match  (%{md5:foo} + "foo")
+#
+#  @todo - the real answer is commented out, because
+#  %{md5:foo} while we added code to run the MD5
+#  function, it isn't called, and... who knows.
+#
+xlat_purify %{md5:foo} + "bar"
+match 0xacbd18db4cc2f85cedef654fccc4a4d8626172
 
 #  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)
 
+xlat_purify (4 + 3) + 6
+match 13
+
+#
+#  @todo - this gets parsed as "4" + "3", because the parsing of
+#  XLAT_BOX in xlat_tokenize_string() just makes everything into
+#  FR_TYPE_STRING.  The parsing in tmpl_tokenize() is different,
+#  as it tries to figure out what data type things are.
+#
+#  The solution is likely to have the expression code double-check
+#  it's arguments on instantiate, and if they are FR_TYPE_STRING with
+#  quote T_BARE_WORD, then go re-evaluate them.  Tho that seems
+#  terrible, TBH.  A cleaner way would be to add a parse flag which tells
+#  the xlat tokenizer "please try to figure out WTF this value is".
+#
+#  which then gets "43", and then "43" to int, and 43+6 -> 49
+#
+#  The rules for parsing tmpls at run-time are apparently different
+#  than parsing them at compile time?
+#
+xlat_purify %(op_add:4 3) + 6
+match 49
+#match ((4 + 3) + 6)
 
 #
 #  useless casts are omitted.
@@ -143,8 +161,18 @@ match (%{Service-Type} == 3)
 #
 #  This is so wrong...
 #
-xlat_purify &Reply-Message == "foo"
-match ERROR offset 1: Failed resolving attribute in expansion: Message
+#xlat_purify &Reply-Message == "foo"
+#match ERROR offset 1: Failed resolving attribute in expansion: Message
+
+#
+#  @todo - find some way quote the RHS?  Maybe just make it
+#  T_SINGLE_QUOTED_STRING for "string" types, and leave it T_BARE_WORD
+#  for everything else.  But this presumes that the RHS is always only
+#  one value-box, and perhaps it isn't?
+#
+xlat_purify &Filter-Id == ("foo" + "bar")
+match (%{Filter-Id} == foobar)
+
 
 count
-match 57
+match 59