separate out && and || code, and clean up tests

diff --git a/src/lib/unlang/xlat_expr.c b/src/lib/unlang/xlat_expr.c
index 2a6f46f4c40034ca301c44e26e73d23951370ed4..f8126b28c042b7ad8d99d585128f0f16bfc9d96f 100644 (file)
--- a/src/lib/unlang/xlat_expr.c
+++ b/src/lib/unlang/xlat_expr.c
@@ -844,6 +844,7 @@ XLAT_REGEX_FUNC(reg_ne,  T_OP_REG_NE)
 
 typedef struct {
        bool            stop_on_match;
+       xlat_func_t     callback;
        int             argc;
        xlat_exp_head_t **argv;
 } xlat_logical_inst_t;
@@ -898,20 +899,6 @@ static fr_slen_t xlat_expr_print_nary(fr_sbuff_t *out, xlat_exp_t const *node, v
        return fr_sbuff_used_total(out) - at_in;
 }
 
-/*
- *     Each argument is it's own head, because we do NOT always want
- *     to go to the next argument.
- */
-static int xlat_instantiate_logical(xlat_inst_ctx_t const *xctx)
-{
-       xlat_logical_inst_t     *inst = talloc_get_type_abort(xctx->inst, xlat_logical_inst_t);
-
-       inst->argc = xlat_flatten_compiled_argv(inst, &inst->argv, xctx->ex->call.args);
-       inst->stop_on_match = (xctx->ex->call.func->token == T_LOR);
-
-       return 0;
-}
-
 /*
  *     This returns "false" for "ignore this argument"
  *
@@ -1113,21 +1100,51 @@ static int xlat_expr_logical_purify(xlat_exp_t *node, void *instance, request_t
        return 0;
 }
 
+/** Process one argument of a logical operation.
+ *
+ *  If we see a list in a truthy context, then we DON'T expand the list.  Instead, we return a bool which
+ *  indicates if the list was empty (or not).  This prevents us from returning a whole mess of value-boxes
+ *  when the user just wanted to see if the list existed.
+ *
+ *  Otherwise, we expand the xlat, and continue.
+ */
+static xlat_action_t xlat_logical_process_arg(UNUSED TALLOC_CTX *ctx, UNUSED fr_dcursor_t *out,
+                                             xlat_ctx_t const *xctx,
+                                             request_t *request, UNUSED fr_value_box_list_t *in)
+{
+       xlat_logical_inst_t const *inst = talloc_get_type_abort_const(xctx->inst, xlat_logical_inst_t);
+       xlat_logical_rctx_t     *rctx = talloc_get_type_abort(xctx->rctx, xlat_logical_rctx_t);
+
+       /*
+        *      Push the xlat onto the stack for expansion.
+        */
+       if (unlang_xlat_yield(request, inst->callback, NULL, 0, rctx) != XLAT_ACTION_YIELD) {
+       fail:
+               talloc_free(rctx->box);
+               talloc_free(rctx);
+               return XLAT_ACTION_FAIL;
+       }
+
+       if (unlang_xlat_push(rctx, &rctx->last_success, &rctx->list,
+                            request, inst->argv[rctx->current], UNLANG_SUB_FRAME) < 0) goto fail;
+
+       return XLAT_ACTION_PUSH_UNLANG;
+}
+
 /** See if the input is truthy or not.
  *
  *  @param[in]     ctx  talloc ctx
  *  @param[in,out] dst value-box containing the output box
  *  @param[in]     in   list of value-boxes to check
- *  @param[in] logical_or whether or not we're checking for ||.  false is &&
  *  @return
- *     - false if no match.
+ *     - false on failure
  *     - true for match, with dst updated to contain the relevant box.
  *
  *  Empty lists are not truthy.
  */
-static bool xlat_logical_match(TALLOC_CTX *ctx, fr_value_box_t **dst, fr_value_box_list_t const *in, bool logical_or)
+static bool xlat_logical_or(TALLOC_CTX *ctx, fr_value_box_t **dst, fr_value_box_list_t const *in)
 {
-       fr_value_box_t *last = NULL;
+       fr_value_box_t *found = NULL;
 
        /*
         *      Empty lists are !truthy.
@@ -1139,25 +1156,18 @@ static bool xlat_logical_match(TALLOC_CTX *ctx, fr_value_box_t **dst, fr_value_b
         */
        fr_value_box_list_foreach(in, box) {
                if (fr_box_is_group(box)) {
-                       if (!xlat_logical_match(ctx, dst, &box->vb_group, logical_or)) return false;
-                       continue;
-               }
-
-               if (logical_or) {
-                       if (fr_value_box_is_truthy(box)) {
-                               last = box; /* stop at the first matching one, and return it. */
-                               break;
-                       }
-
-                       continue;
+                       if (!xlat_logical_or(ctx, dst, &box->vb_group)) return false;
+                       continue;                       
                }
 
                /*
-                *      Must be logical &&
+                *      Remember the last box we found.
+                *
+                *      If it's truthy, then we stop immediately.
                 */
                if (fr_value_box_is_truthy(box)) {
-                       last = box;
-                       continue;
+                       found = box;
+                       break;
                }
 
                /*
@@ -1166,59 +1176,129 @@ static bool xlat_logical_match(TALLOC_CTX *ctx, fr_value_box_t **dst, fr_value_b
                return false;
        }
 
-       if (last) {
-               if (!*dst) {
-                       MEM(*dst = fr_value_box_alloc_null(ctx));
-               } else {
-                       fr_value_box_clear(*dst);
-               }
-               fr_value_box_copy(*dst, *dst, last);
+       if (!*dst) {
+               MEM(*dst = fr_value_box_alloc_null(ctx));
+       } else {
+               fr_value_box_clear(*dst);
        }
+       fr_value_box_copy(*dst, *dst, found);
 
        return true;
 }
 
-static xlat_action_t xlat_logical_resume(TALLOC_CTX *ctx, fr_dcursor_t *out,
-                                        xlat_ctx_t const *xctx,
-                                        request_t *request, fr_value_box_list_t *in);
-
-/** Process one argument of a logical operation.
- *
- *  If we see a list in a truthy context, then we DON'T expand the list.  Instead, we return a bool which
- *  indicates if the list was empty (or not).  This prevents us from returning a whole mess of value-boxes
- *  when the user just wanted to see if the list existed.
- *
- *  Otherwise, we expand the xlat, and continue.
+/*
+ *  We've evaluated an expression.  Let's see if we need to continue with ||
  */
-static xlat_action_t xlat_logical_process_arg(UNUSED TALLOC_CTX *ctx, UNUSED fr_dcursor_t *out,
-                                             xlat_ctx_t const *xctx,
-                                             request_t *request, UNUSED fr_value_box_list_t *in)
+static xlat_action_t xlat_logical_or_resume(TALLOC_CTX *ctx, fr_dcursor_t *out,
+                                           xlat_ctx_t const *xctx,
+                                           request_t *request, fr_value_box_list_t *in)
 {
        xlat_logical_inst_t const *inst = talloc_get_type_abort_const(xctx->inst, xlat_logical_inst_t);
        xlat_logical_rctx_t     *rctx = talloc_get_type_abort(xctx->rctx, xlat_logical_rctx_t);
+       bool                    match;
 
        /*
-        *      Push the xlat onto the stack for expansion.
+        *      If one of the expansions fails, then we fail the
+        *      entire thing.
         */
-       if (unlang_xlat_yield(request, xlat_logical_resume, NULL, 0, rctx) != XLAT_ACTION_YIELD) {
-       fail:
+       if (!rctx->last_success) {
                talloc_free(rctx->box);
                talloc_free(rctx);
                return XLAT_ACTION_FAIL;
        }
 
-       if (unlang_xlat_push(rctx, &rctx->last_success, &rctx->list,
-                            request, inst->argv[rctx->current], UNLANG_SUB_FRAME) < 0) goto fail;
+       /*
+        *      Recursively check groups.  i.e. we effectively flatten each list.
+        *
+        *      (a, b, c) || (d, e, f) == a || b || c || d || e || f
+        */
+       match = xlat_logical_or(rctx->ctx, &rctx->box, &rctx->list);
+       if (match) goto done;
 
-       return XLAT_ACTION_PUSH_UNLANG;
+       fr_value_box_list_talloc_free(&rctx->list);
+
+       rctx->current++;
+
+       /*
+        *      Nothing to expand, return the final value we saw.
+        */
+       if (rctx->current >= inst->argc) {
+       done:
+               /*
+                *      Otherwise we stop on failure, with the boolean
+                *      we just updated.
+                */
+               if (rctx->box) fr_dcursor_append(out, rctx->box);
+
+               talloc_free(rctx);
+               return XLAT_ACTION_DONE;
+       }
+
+       return xlat_logical_process_arg(ctx, out, xctx, request, in);
+}
+
+/** See if the input is truthy or not.
+ *
+ *  @param[in]     ctx  talloc ctx
+ *  @param[in,out] dst value-box containing the output box
+ *  @param[in]     in   list of value-boxes to check
+ *  @return
+ *     - false on failure
+ *     - true for match, with dst updated to contain the relevant box.
+ *
+ *  Empty lists are not truthy.
+ */
+static bool xlat_logical_and(TALLOC_CTX *ctx, fr_value_box_t **dst, fr_value_box_list_t const *in)
+{
+       fr_value_box_t *found = NULL;
+
+       /*
+        *      Empty lists are !truthy.
+        */
+       if (!fr_value_box_list_num_elements(in)) return false;
+
+       /*
+        *      Loop over the input list.  If the box is a group, then do this recursively.
+        */
+       fr_value_box_list_foreach(in, box) {
+               if (fr_box_is_group(box)) {
+                       if (!xlat_logical_or(ctx, dst, &box->vb_group)) return false;
+                       continue;                       
+               }
+
+               /*
+                *      Remember the last box we found.
+                *
+                *      If it's truthy, then we keep going either
+                *      until the end, or until we get a "false".
+                */
+               if (fr_value_box_is_truthy(box)) {
+                       found = box;
+                       continue;
+               }
+
+               /*
+                *      Stop on the first "false"
+                */
+               return false;
+       }
+
+       if (!*dst) {
+               MEM(*dst = fr_value_box_alloc_null(ctx));
+       } else {
+               fr_value_box_clear(*dst);
+       }
+       fr_value_box_copy(*dst, *dst, found);
+
+       return true;
 }
 
 /*
- *  We've evaluated an expression.  Let's see if we need to continue.
+ *  We've evaluated an expression.  Let's see if we need to continue with &&
  */
-static xlat_action_t xlat_logical_resume(TALLOC_CTX *ctx, fr_dcursor_t *out,
-                                        xlat_ctx_t const *xctx,
-                                        request_t *request, fr_value_box_list_t *in)
+static xlat_action_t xlat_logical_and_resume(TALLOC_CTX *ctx, fr_dcursor_t *out,
+                                            xlat_ctx_t const *xctx,
+                                            request_t *request, fr_value_box_list_t *in)
 {
        xlat_logical_inst_t const *inst = talloc_get_type_abort_const(xctx->inst, xlat_logical_inst_t);
        xlat_logical_rctx_t     *rctx = talloc_get_type_abort(xctx->rctx, xlat_logical_rctx_t);
@@ -1237,48 +1317,25 @@ static xlat_action_t xlat_logical_resume(TALLOC_CTX *ctx, fr_dcursor_t *out,
        /*
         *      Recursively check groups.  i.e. we effectively flatten each list.
         *
-        *      (a, b, c) || (d, e, f) == a || b || c || d || e || f
+        *      (a, b, c) && (d, e, f) == a && b && c && d && e && f
         */
-       match = xlat_logical_match(rctx->ctx, &rctx->box, &rctx->list, inst->stop_on_match);
-       if (!match) {
-               /*
-                *      we didn't match, (&&), so we're done.
-                */
-               if (!inst->stop_on_match) goto done;
-       }
+       match = xlat_logical_and(rctx->ctx, &rctx->box, &rctx->list);
+       if (!match) return XLAT_ACTION_FAIL;
 
        fr_value_box_list_talloc_free(&rctx->list);
 
-       /*
-        *      If we stop on the first match, and we got a "true" match we're done.
-        */
-       if (inst->stop_on_match && match && rctx->box && fr_value_box_is_truthy(rctx->box)) goto done;
-
        rctx->current++;
 
        /*
         *      Nothing to expand, return the final value we saw.
         */
        if (rctx->current >= inst->argc) {
-       done:
-               if (!inst->stop_on_match) {
-                       fr_assert(rctx->box);
-
-                       /*
-                        *      If nothing matches, we return a "false" box.
-                        */
-                       if (rctx->box->type != FR_TYPE_BOOL) {
-                               fr_value_box_clear(rctx->box);
-                               fr_value_box_init(rctx->box, FR_TYPE_BOOL, NULL, false);
-                       }
-                       rctx->box->vb_bool = false;
-               }
-
                /*
                 *      Otherwise we stop on failure, with the boolean
                 *      we just updated.
                 */
-               if (rctx->box) fr_dcursor_append(out, rctx->box);
+               fr_assert(rctx->box != NULL);
+               fr_dcursor_append(out, rctx->box);
 
                talloc_free(rctx);
                return XLAT_ACTION_DONE;
@@ -1287,6 +1344,27 @@ static xlat_action_t xlat_logical_resume(TALLOC_CTX *ctx, fr_dcursor_t *out,
        return xlat_logical_process_arg(ctx, out, xctx, request, in);
 }
 
+/*
+ *     Each argument is it's own head, because we do NOT always want
+ *     to go to the next argument.
+ */
+static int xlat_instantiate_logical(xlat_inst_ctx_t const *xctx)
+{
+       xlat_logical_inst_t     *inst = talloc_get_type_abort(xctx->inst, xlat_logical_inst_t);
+
+       inst->argc = xlat_flatten_compiled_argv(inst, &inst->argv, xctx->ex->call.args);
+       if (xctx->ex->call.func->token == T_LOR) {
+               inst->callback = xlat_logical_or_resume;
+               inst->stop_on_match = true;
+       } else {
+               inst->callback = xlat_logical_and_resume;
+               inst->stop_on_match = false;
+       }
+
+       return 0;
+}
+
+
 /** Process logical &&, ||
  *
  */

diff --git a/src/tests/keywords/logical-and b/src/tests/keywords/logical-and
index 0385140384ed212267ff86f1ec6bbe4924bbb82f..52ff61c0bdd7c8e02895b74c60a2971236b02ecb 100644 (file)
--- a/src/tests/keywords/logical-and
+++ b/src/tests/keywords/logical-and
@@ -7,14 +7,11 @@
 }
 
 #
-#  ! (true && true) --> false
+#  (true && true) --> true
 #
-#  But we don't want to complicate the condition, so
-#  instead we use an "else"
+#  But if it's false, then we have an issue.
 #
 if !((&Tmp-Integer-0 == 0) && (&Tmp-Integer-1 == 1)) {
-       # do nothing on "false"
-} else {
        test_fail
 }
 
@@ -32,4 +29,12 @@ if ((&Tmp-Integer-0 == 0) && (&Tmp-Integer-1 == 0)) {
        test_fail
 }
 
+&control.Tmp-String-0 := { "bob", "oof" }
+
+if ((&control.Tmp-String-0[0] == 'bob') && (&control.Tmp-String-0[1] == 'oof')) {
+       # OK
+} else {
+       test_fail
+}
+
 success

diff --git a/src/tests/keywords/logical-or b/src/tests/keywords/logical-or
index 1bda761780da4780460b85ba2c78b051c24dde95..653b664716074f30ab41c840a67ef4fd6a76dbf1 100644 (file)
--- a/src/tests/keywords/logical-or
+++ b/src/tests/keywords/logical-or
@@ -14,4 +14,13 @@ if !((&Tmp-Integer-1 == 1) || (&Tmp-Integer-0 == 1)) {
        test_fail
 }
 
+#
+#  Neither of these exists, so the resulting string should be empty
+#
+&Tmp-String-0 := "%{&Tmp-String-1 || &Tmp-String-1}"
+
+if !(&Tmp-String-0 == '') {
+       test_fail
+}
+
 success