don't hoist things too far

do less hoisting during normalization.  Do one last set of
hoisting just before we return.

diff --git a/src/lib/server/cond_tokenize.c b/src/lib/server/cond_tokenize.c
index e4add0fccad68751e6b47399c9298394852624a0..3a82525e1f7f82d9a2582db554b0f0bb68b96b7e 100644 (file)
--- a/src/lib/server/cond_tokenize.c
+++ b/src/lib/server/cond_tokenize.c
@@ -528,10 +528,9 @@ static int cond_normalise(TALLOC_CTX *ctx, fr_token_t lhs_type, fr_cond_t **c_ou
        /*
         *      Normalize the condition before returning.
         *
-        *      We collapse multiple levels of braces to one.  Then
-        *      convert maps to literals.  Then literals to true/false
-        *      statements.  Then true/false ||/&& followed by other
-        *      conditions to just conditions.
+        *      We convert maps to literals.  Then literals to
+        *      true/false statements.  Then true/false ||/&& followed
+        *      by other conditions to just conditions.
         *
         *      Order is important.  The more complex cases are
         *      converted to simpler ones, from the most complex cases
@@ -539,59 +538,33 @@ static int cond_normalise(TALLOC_CTX *ctx, fr_token_t lhs_type, fr_cond_t **c_ou
         */
 
        /*
-        *      Loop because we might hoist a child to a child.
+        *      Do some limited normalizations here.
         */
-       while (c->type == COND_TYPE_CHILD) {
-               fr_cond_t *child;
-
-               child = c->data.child;
-
-               /*
-                *      (FOO)     --> FOO
-                *      (FOO) ... --> FOO ...
-                */
-               if (!child->next) {
-                       (void) talloc_steal(ctx, child);
-                       child->parent = c->parent;
-                       child->next = talloc_steal(child, c->next);
-
-                       /*
-                        *      !(!FOO) --> FOO, etc.
-                        */
-                       child->negate = (c->negate != child->negate);
-
-                       talloc_free(c);
-                       c = child;
-                       continue;
-               }
+       if (c->type == COND_TYPE_CHILD) {
+               fr_cond_t *child = c->data.child;
 
                /*
-                *      (FOO ...) --> FOO ...
-                *
-                *      But don't do !(FOO || BAR) --> !FOO || BAR
-                *      Because that's different.
+                *      ((FOO)) --> (FOO)
+                *      !(!(FOO)) --> FOO
                 */
-               if (!c->next && !c->negate) {
-                       (void) talloc_steal(ctx, child);
-                       child->parent = c->parent;
-
-                       talloc_free(c);
-                       c = child;
-                       continue;
+               if (!c->next && !child->next) {
+                       if ((child->type == COND_TYPE_CHILD) ||
+                           (child->type == COND_TYPE_TRUE) ||
+                           (child->type == COND_TYPE_FALSE)) {
+                               (void) talloc_steal(ctx, child);
+                               child->parent = c->parent;
+                               child->negate = (c->negate != child->negate);
+
+                               talloc_free(c);
+                               c = child;
+                       }
                }
 
                /*
-                *      Can't do anything else, stop looping.
+                *      No further optimizations are possible, so we
+                *      just check for and/or short-circuit.
                 */
-               break;
-       }
-
-       /*
-        *      No further optimizations are possible, so we just
-        *      return.
-        */
-       if (c->type == COND_TYPE_CHILD) {
-               goto check_short_circuit;
+                goto check_short_circuit;
        }
 
        /*
@@ -1610,7 +1583,27 @@ ssize_t fr_cond_tokenize(CONF_SECTION *cs, fr_cond_t **head, tmpl_rules_t const
        /*
         *      Now that everything has been normalized, reparent the children.
         */
-       if (*head) cond_reparent(*head, NULL);
+       if (*head) {
+               fr_cond_t *c = *head;
+
+               /*
+                *      (FOO) -> FOO
+                *      !(!(FOO)) -> FOO
+                */
+               if ((c->type == COND_TYPE_CHILD) && !c->next) {
+                       if (!c->negate || !c->data.child->next) {
+                               fr_cond_t *child = c->data.child;
+                               (void) talloc_steal(cs, child);
+                               child->parent = c->parent;
+                               child->negate = (c->negate != child->negate);
+
+                               talloc_free(c);
+                               *head = child;
+                       }
+               }
+
+               cond_reparent(*head, NULL);
+       }
 
        return slen + diff;
 }

diff --git a/src/tests/unit/condition/base.txt b/src/tests/unit/condition/base.txt
index 23981183bcaeb083638ea8c463bf925a9f2ab89d..990270169513206cde5441023aa6d49b76bf1655 100644 (file)
--- a/src/tests/unit/condition/base.txt
+++ b/src/tests/unit/condition/base.txt
@@ -1,4 +1,3 @@
-
 #
 #  Tests for parsing conditional expressions.
 #
@@ -52,7 +51,7 @@ match ERROR offset 4: Invalid operator
 
 # And now we have a bunch of VALID conditions we want to parse.
 
-# sillyness is OK
+# sillyness is OK, but cleaned up.
 condition ((((((ok))))))
 match ok
 
@@ -81,23 +80,23 @@ condition (&User-Name == &User-Password || &Filter-Id == &Reply-Message)
 match &User-Name == &User-Password || &Filter-Id == &Reply-Message
 
 condition ((&User-Name == &User-Password) || (&Filter-Id == &Reply-Message))
-match &User-Name == &User-Password || &Filter-Id == &Reply-Message
+match (&User-Name == &User-Password) || (&Filter-Id == &Reply-Message)
 
 condition (!(&User-Name == &User-Password) || (&Filter-Id == &Reply-Message))
-match !&User-Name == &User-Password || &Filter-Id == &Reply-Message
+match !(&User-Name == &User-Password) || (&Filter-Id == &Reply-Message)
 
 #  different from the previous ones.
 condition (!((&User-Name == &User-Password) || (&Filter-Id == &Reply-Message)))
-match !(&User-Name == &User-Password || &Filter-Id == &Reply-Message)
+match !((&User-Name == &User-Password) || (&Filter-Id == &Reply-Message))
 
 condition (!(&User-Name == &User-Password) || (&Filter-Id == &Reply-Message))
-match !&User-Name == &User-Password || &Filter-Id == &Reply-Message
+match !(&User-Name == &User-Password) || (&Filter-Id == &Reply-Message)
 
 condition ((a == b) || (c == d)))
 match ERROR offset 22: Unexpected closing brace
 
 condition (handled && (&Packet-Type == Access-Challenge))
-match handled && &Packet-Type == Access-Challenge
+match  handled && (&Packet-Type == Access-Challenge)
 
 # This is OK, without the braces
 condition handled && &Packet-Type == Access-Challenge
@@ -655,5 +654,14 @@ match false
 condition (&User-Name == "bob") || (true)
 match true
 
+#
+#  A && (B || C) is not the same as (A && B) || C, for 0/1/1
+#
+#      0 && (1 || 1) = 0 && 1 == 0
+#      (0 && 1) || 1 = 0 || 1 == 1
+#
+condition (&User-Name == "bob") && ((&User-Password == "bob") || &EAP-Message)
+match (&User-Name == "bob") && ((&User-Password == "bob") || &EAP-Message)
+
 count
-match 317
+match 319