Make the delay module transparent for rcodes

diff --git a/raddb/mods-available/delay b/raddb/mods-available/delay
index 1164b2bff4fd27af5a7d6e49e3d0e8f4fb093e66..96bc3c8938e89c2063691b2b45a54f20edee7127 100644 (file)
--- a/raddb/mods-available/delay
+++ b/raddb/mods-available/delay
@@ -69,6 +69,24 @@ delay {
        #  The default is `no`, which means that.
        #
 #      relative = no
+
+       #
+       #  rcode:: Which rcode the delay module should return when it resumes
+       #  this can be useful for detecting whether a `timeout { ... }` section
+       #  expired, whilst the delay module was waiting.
+       #
+       #  The default is `notset`, which means that the delay module will be
+       #  transparent and not alter the section rcode.
+       #
+       # [NOTE]
+       # ====
+       # The default section `rcode` in many places is `fail`.  Care should be
+       # taken to set the `rcode` for the section a delay module appears in, if
+       # this configuration item is not set, and no other modules are present
+       # in the section.
+       # ====
+       #
+#      rcode = 'notset'
 }
 
 #

diff --git a/src/lib/server/rcode.c b/src/lib/server/rcode.c
index fabba4793def16bc082c162deaa587ace718b2d8..3293ed88cf7d348f4ad72f49f50c90011104c7a3 100644 (file)
--- a/src/lib/server/rcode.c
+++ b/src/lib/server/rcode.c
@@ -39,9 +39,10 @@ fr_table_num_sorted_t const rcode_table[] = {
        { L("invalid"),         RLM_MODULE_INVALID      },
        { L("noop"),            RLM_MODULE_NOOP         },
        { L("notfound"),        RLM_MODULE_NOTFOUND     },
+       { L("notset"),          RLM_MODULE_NOT_SET      },
        { L("ok"),              RLM_MODULE_OK           },
        { L("reject"),          RLM_MODULE_REJECT       },
        { L("timeout"),         RLM_MODULE_TIMEOUT      },
-       { L("updated"),         RLM_MODULE_UPDATED      },
+       { L("updated"),         RLM_MODULE_UPDATED      }
 };
 size_t rcode_table_len = NUM_ELEMENTS(rcode_table);

diff --git a/src/lib/unlang/module.c b/src/lib/unlang/module.c
index 3298346cfcd4fc7950cb94d7f07f4db425f11510..5b50d837676986d68ef0c2f42f5a6ee929177f59 100644 (file)
--- a/src/lib/unlang/module.c
+++ b/src/lib/unlang/module.c
@@ -677,9 +677,10 @@ static unlang_action_t unlang_module_resume(rlm_rcode_t *p_result, request_t *re
                request->module = state->previous_module;
                break;
 
-       case UNLANG_ACTION_EXECUTE_NEXT:        /* Not valid */
-               fr_assert(0);
-               *p_result = RLM_MODULE_FAIL;
+       /*
+        *      Module indicates we shouldn't process its rcode
+        */
+       case UNLANG_ACTION_EXECUTE_NEXT:
                break;
        }
 
@@ -941,9 +942,10 @@ static unlang_action_t unlang_module(rlm_rcode_t *p_result, request_t *request,
                *p_result = RLM_MODULE_FAIL;
                break;
 
+       /*
+        *      Module indicates we shouldn't process its rcode
+        */
        case UNLANG_ACTION_EXECUTE_NEXT:
-               fr_assert(0);
-               *p_result = RLM_MODULE_FAIL;
                break;
        }
 

diff --git a/src/lib/unlang/timeout.c b/src/lib/unlang/timeout.c
index 285d661597ef41df7d15f4a0acc2d29850e89ece..d9e6b6e2f65e4fcec1856e2c52e5d637acb6e677 100644 (file)
--- a/src/lib/unlang/timeout.c
+++ b/src/lib/unlang/timeout.c
@@ -112,7 +112,7 @@ static unlang_action_t unlang_timeout_resume_done(UNUSED rlm_rcode_t *p_result,
         *
         *      Unless the next instruction is a "catch timeout", in which case we skip it.
         */
-       if (!state->fired) return UNLANG_ACTION_CALCULATE_RESULT;
+       if (!state->fired) return UNLANG_ACTION_EXECUTE_NEXT;   /* Don't modify the return code*/
 
        RETURN_MODULE_TIMEOUT;
 }

diff --git a/src/lib/unlang/unlang_priv.h b/src/lib/unlang/unlang_priv.h
index 3812bc4deb5ceb592db9e7b524afb1f130479f62..1655e498bc3ce0ad7697b62425cc52b23f0cf47a 100644 (file)
--- a/src/lib/unlang/unlang_priv.h
+++ b/src/lib/unlang/unlang_priv.h
@@ -570,7 +570,7 @@ static inline void frame_next(unlang_stack_t *stack, unlang_stack_frame_t *frame
        frame_cleanup(frame);
        frame->instruction = frame->next;
 
-       if (!frame->instruction) return;
+       if (!frame->instruction) return;        /* No siblings, need to pop instead */
 
        frame->next = frame->instruction->next;
 

diff --git a/src/modules/rlm_delay/rlm_delay.c b/src/modules/rlm_delay/rlm_delay.c
index 679be93d1e158e2a0f648301a2c81e938e361deb..0e8bcd3fdee2703250b867378b78ca5234a32976 100644 (file)
--- a/src/modules/rlm_delay/rlm_delay.c
+++ b/src/modules/rlm_delay/rlm_delay.c
@@ -26,15 +26,18 @@ RCSID("$Id$")
 
 #include <freeradius-devel/server/base.h>
 #include <freeradius-devel/server/module_rlm.h>
+#include <freeradius-devel/server/rcode.h>
 #include <freeradius-devel/util/debug.h>
 #include <freeradius-devel/server/map_proc.h>
 #include <freeradius-devel/util/time.h>
 #include <freeradius-devel/unlang/xlat_func.h>
+#include <freeradius-devel/unlang/action.h>
 
 typedef struct {
        tmpl_t          *delay;                 //!< How long we delay for.
        bool            relative;               //!< Whether the delay is relative to the start of request processing.
        bool            force_reschedule;       //!< Whether we should force rescheduling of the request.
+       rlm_rcode_t     rcode;                  //!< The return code to use when the delay is complete.
 } rlm_delay_t;
 
 /*
@@ -44,6 +47,8 @@ static const conf_parser_t module_config[] = {
        { FR_CONF_OFFSET_HINT_TYPE("delay", FR_TYPE_TIME_DELTA, rlm_delay_t, delay) },
        { FR_CONF_OFFSET("relative", rlm_delay_t, relative), .dflt = "no" },
        { FR_CONF_OFFSET("force_reschedule", rlm_delay_t, force_reschedule), .dflt = "no" },
+       { FR_CONF_OFFSET("rcode", rlm_delay_t, rcode),
+                        .func = cf_table_parse_int, .uctx = &(cf_table_parse_ctx_t){ .table = rcode_table, .len = &rcode_table_len }, .dflt = "notset" },
        CONF_PARSER_TERMINATOR
 };
 
@@ -121,6 +126,7 @@ static int delay_add(request_t *request, fr_time_t *resume_at, fr_time_t now,
 static unlang_action_t mod_delay_return(rlm_rcode_t *p_result, module_ctx_t const *mctx, request_t *request)
 {
        rlm_delay_retry_t *yielded = talloc_get_type_abort(mctx->rctx, rlm_delay_retry_t);
+       rlm_delay_t *inst = talloc_get_type_abort_const(mctx->mi->data, rlm_delay_t);
 
        /*
         *      Print how long the delay *really* was.
@@ -128,7 +134,11 @@ static unlang_action_t mod_delay_return(rlm_rcode_t *p_result, module_ctx_t cons
        RDEBUG3("Request delayed by %pV", fr_box_time_delta(fr_time_sub(fr_time(), yielded->when)));
        talloc_free(yielded);
 
-       RETURN_MODULE_OK;
+       /*
+        *      Be transparent, don't alter the rcode
+        */
+       if (inst->rcode == RLM_MODULE_NOT_SET) return UNLANG_ACTION_EXECUTE_NEXT;
+       RETURN_MODULE_RCODE(inst->rcode);
 }
 
 static unlang_action_t CC_HINT(nonnull) mod_delay(rlm_rcode_t *p_result, module_ctx_t const *mctx, request_t *request)

diff --git a/src/tests/keywords/if-elsif-rcode-resume b/src/tests/keywords/if-elsif-rcode-resume
new file mode 100644 (file)
index 0000000..6954d48
--- /dev/null
+++ b/src/tests/keywords/if-elsif-rcode-resume
@@ -0,0 +1,26 @@
+# PRE: if if-else
+
+# This is a regression test.  We saw the request->rcode being lost after the first condition
+
+noop
+if (ok || notfound) {
+       test_fail
+} elsif (!noop) {
+       test_fail
+} elsif (noop) {
+       # Check nothing weird happens 
+       reschedule_notfound
+}
+
+if (!notfound) {
+       test_fail
+}
+
+notfound
+if (%expr.rcode('ok') || %expr.rcode('updated')) {
+       test_fail
+} elsif (!%expr.rcode('notfound')) {
+       test_fail
+}
+
+success

diff --git a/src/tests/keywords/if-rcode b/src/tests/keywords/if-rcode
new file mode 100644 (file)
index 0000000..1289b80
--- /dev/null
+++ b/src/tests/keywords/if-rcode
@@ -0,0 +1,18 @@
+noop {
+       # 0 gives greatest chance of overwrite
+       noop = 1
+}
+
+# Verify that checking the rcode doesn't _alter_ the rcode
+if (fail) {
+       test_fail
+}
+elsif (ok || updated) {
+       test_fail
+}
+# rcode should still be noop
+elsif (noop) {
+       success
+} else {
+       test_fail
+}

diff --git a/src/tests/keywords/parallel-yield b/src/tests/keywords/parallel-yield
index e31d95bf801da7b9a79eec919b43f7393a70bd1a..dfb95dc9068c67c8061b732ea7e6856c8f0da1b8 100644 (file)
--- a/src/tests/keywords/parallel-yield
+++ b/src/tests/keywords/parallel-yield
@@ -4,7 +4,10 @@
 
 # Ensure if one module yields, the rest execute
 parallel {
-       reschedule
+       group {
+               ok # Set the rcode for the section, else it defaults to fail
+               reschedule
+       }
        group {
                parent.request.Filter-Id := 'foo'
        }

diff --git a/src/tests/keywords/radius.conf b/src/tests/keywords/radius.conf
index 06949e70193fe40d0478c34999778686b89ed712..3adc4da6ccd7db907668a16379b4c48ec43285b0 100644 (file)
--- a/src/tests/keywords/radius.conf
+++ b/src/tests/keywords/radius.conf
@@ -9,6 +9,14 @@ modules {
                force_reschedule = yes
        }
 
+       #
+       #  Reschedule the request, then return notfound
+       #
+       delay reschedule_notfound {
+               force_reschedule = yes
+               rcode = "notfound"
+       }
+
        delay delay_10s {
                delay = 10
        }

diff --git a/src/tests/keywords/timeout-transparent-rcode b/src/tests/keywords/timeout-transparent-rcode
new file mode 100644 (file)
index 0000000..160008c
--- /dev/null
+++ b/src/tests/keywords/timeout-transparent-rcode
@@ -0,0 +1,20 @@
+#
+# PRE: timeout
+#
+
+#
+#  If the timeout doesn't fire, the rcode should not change, and the
+#  timeout block should be transparent.
+#
+timeout 1s {
+       if (ok) {
+               test_fail
+       } elsif (reject) {
+               notfound
+       }
+}
+if (!notfound) {
+       test_fail
+}
+
+success