hoist "set default actions"

so that it's done in only one place

and then since we're cleaning things up, do a bit more work
on unlang_compile_children()

diff --git a/src/lib/unlang/catch.c b/src/lib/unlang/catch.c
index 2d73c9cfe146050f02ca2d63e1df7d8cb2565aac..70ef78fb7dd8c6b5878ad275e4acd2d4737c6714 100644 (file)
--- a/src/lib/unlang/catch.c
+++ b/src/lib/unlang/catch.c
@@ -190,7 +190,7 @@ static unlang_t *unlang_compile_catch(unlang_t *parent, unlang_compile_ctx_t *un
        /*
         *      @todo - Else parse and limit the things we catch
         */
-       return unlang_compile_children(g, unlang_ctx, true);
+       return unlang_compile_children(g, unlang_ctx);
 }
 
 void unlang_catch_init(void)

diff --git a/src/lib/unlang/compile.c b/src/lib/unlang/compile.c
index d05feda6393b92f8f177fa3ec700342217b2a660..fe1de147b83b46a662bfacd84bc7dcb5cd32e21d 100644 (file)
--- a/src/lib/unlang/compile.c
+++ b/src/lib/unlang/compile.c
@@ -484,7 +484,11 @@ unlang_group_t *unlang_group_allocate(unlang_t *parent, CONF_SECTION *cs, unlang
        return g;
 }
 
-void unlang_compile_action_defaults(unlang_t *c, unlang_compile_ctx_t *unlang_ctx)
+/**  Update a compiled unlang_t with the default actions.
+ *
+ *  Don't over-ride any actions which have been set.
+ */
+static void compile_set_default_actions(unlang_t *c, unlang_compile_ctx_t *unlang_ctx)
 {
        int i;
 
@@ -500,7 +504,8 @@ void unlang_compile_action_defaults(unlang_t *c, unlang_compile_ctx_t *unlang_ct
         *      have RETURN for all actions except fail.  But THEIR children are normal.
         */
        if (c->parent &&
-           ((c->parent->type == UNLANG_TYPE_REDUNDANT) || (c->parent->type == UNLANG_TYPE_REDUNDANT_LOAD_BALANCE))) {
+           ((c->parent->type == UNLANG_TYPE_REDUNDANT) ||
+            (c->parent->type == UNLANG_TYPE_REDUNDANT_LOAD_BALANCE))) {
                for (i = 0; i < RLM_MODULE_NUMCODES; i++) {
                        switch (i) {
                        case RLM_MODULE_FAIL:
@@ -520,7 +525,7 @@ void unlang_compile_action_defaults(unlang_t *c, unlang_compile_ctx_t *unlang_ct
        }
 
        /*
-        *      Set the default actions, if they haven't already been
+        *      Set the default actions if they haven't already been
         *      set.
         */
        for (i = 0; i < RLM_MODULE_NUMCODES; i++) {
@@ -681,8 +686,6 @@ static unlang_t *compile_edit_section(unlang_t *parent, unlang_compile_ctx_t *un
 
        map_list_init(&edit->maps);
 
-       unlang_compile_action_defaults(c, unlang_ctx);
-
        /*
         *      Allocate the map and initialize it.
         */
@@ -825,8 +828,6 @@ static unlang_t *compile_edit_pair(unlang_t *parent, unlang_compile_ctx_t *unlan
 
        map_list_init(&edit->maps);
 
-       unlang_compile_action_defaults(c, unlang_ctx);
-
        op = cf_pair_operator(cp);
        if ((op == T_OP_CMP_TRUE) || (op == T_OP_CMP_FALSE)) {
                cf_log_err(cp, "Invalid operator \"%s\".",
@@ -1229,7 +1230,7 @@ bool unlang_compile_actions(unlang_mod_actions_t *actions, CONF_SECTION *action_
        return true;
 }
 
-unlang_t *unlang_compile_empty(unlang_t *parent, unlang_compile_ctx_t *unlang_ctx, CONF_SECTION *cs, unlang_type_t type)
+unlang_t *unlang_compile_empty(unlang_t *parent, UNUSED unlang_compile_ctx_t *unlang_ctx, CONF_SECTION *cs, unlang_type_t type)
 {
        unlang_group_t *g;
        unlang_t *c;
@@ -1260,7 +1261,6 @@ unlang_t *unlang_compile_empty(unlang_t *parent, unlang_compile_ctx_t *unlang_ct
                }
        }
 
-       unlang_compile_action_defaults(c, unlang_ctx);
        return c;
 }
 
@@ -1321,7 +1321,7 @@ static bool compile_action_subsection(unlang_t *c, CONF_SECTION *cs, CONF_SECTIO
 }
 
 
-unlang_t *unlang_compile_children(unlang_group_t *g, unlang_compile_ctx_t *unlang_ctx_in, bool set_action_defaults)
+unlang_t *unlang_compile_children(unlang_group_t *g, unlang_compile_ctx_t *unlang_ctx_in)
 {
        CONF_ITEM       *ci = NULL;
        unlang_t        *c, *single;
@@ -1373,6 +1373,7 @@ unlang_t *unlang_compile_children(unlang_group_t *g, unlang_compile_ctx_t *unlan
                        if (fr_list_assignment_op[cf_section_name2_quote(subcs)]) {
                                single = compile_edit_section(c, unlang_ctx, subcs);
                                if (!single) {
+                               fail:
                                        talloc_free(c);
                                        return NULL;
                                }
@@ -1381,11 +1382,7 @@ unlang_t *unlang_compile_children(unlang_group_t *g, unlang_compile_ctx_t *unlan
                        }
 
                        if (strcmp(name, "actions") == 0) {
-                               if (!compile_action_subsection(c, g->cs, subcs)) {
-                                       talloc_free(c);
-                                       return NULL;
-                               }
-
+                               if (!compile_action_subsection(c, g->cs, subcs)) goto fail;
                                continue;
                        }
 
@@ -1400,8 +1397,7 @@ unlang_t *unlang_compile_children(unlang_group_t *g, unlang_compile_ctx_t *unlan
                                if (!was_if) {
                                        cf_log_err(ci, "Invalid location for '%s'.  There is no preceding "
                                                   "'if' or 'elsif' statement", name);
-                                       talloc_free(c);
-                                       return NULL;
+                                       goto fail;
                                }
 
                                /*
@@ -1432,8 +1428,7 @@ unlang_t *unlang_compile_children(unlang_group_t *g, unlang_compile_ctx_t *unlan
                        single = compile_item(c, unlang_ctx, ci);
                        if (!single) {
                                cf_log_err(ci, "Failed to parse \"%s\" subsection", cf_section_name1(subcs));
-                               talloc_free(c);
-                               return NULL;
+                               goto fail;
                        }
 
                        goto add_child;
@@ -1445,10 +1440,7 @@ unlang_t *unlang_compile_children(unlang_group_t *g, unlang_compile_ctx_t *unlan
                         *      Variable definition.
                         */
                        if (cf_pair_operator(cp) == T_OP_CMP_TRUE) {
-                               if (compile_variable(c, unlang_ctx, cp, &t_rules) < 0) {
-                                       talloc_free(c);
-                                       return NULL;
-                               }
+                               if (compile_variable(c, unlang_ctx, cp, &t_rules) < 0) goto fail;
 
                                single = UNLANG_IGNORE;
                                goto add_child;
@@ -1458,8 +1450,7 @@ unlang_t *unlang_compile_children(unlang_group_t *g, unlang_compile_ctx_t *unlan
                                single = compile_item(c, unlang_ctx, ci);
                                if (!single) {
                                        cf_log_err(ci, "Invalid keyword \"%s\".", cf_pair_attr(cp));
-                                       talloc_free(c);
-                                       return NULL;
+                                       goto fail;
                                }
 
                                goto add_child;
@@ -1470,16 +1461,12 @@ unlang_t *unlang_compile_children(unlang_group_t *g, unlang_compile_ctx_t *unlan
                         *      tells us what it is, and we don't really care if there's a leading '&'.
                         */
                        single = compile_edit_pair(c, unlang_ctx, cp);
-                       if (!single) {
-                               talloc_free(c);
-                               return NULL;
-                       }
+                       if (!single) goto fail;
 
                        goto add_child;
                } else {
                        cf_log_err(ci, "Asked to compile unknown conf type");
-                       talloc_free(c);
-                       return NULL;
+                       goto fail;
                }
 
        add_child:
@@ -1550,12 +1537,6 @@ unlang_t *unlang_compile_children(unlang_group_t *g, unlang_compile_ctx_t *unlan
                }
        }
 
-       /*
-        *      Set the default actions, if they haven't already been
-        *      set by an "actions" section above.
-        */
-       if (set_action_defaults) unlang_compile_action_defaults(c, unlang_ctx);
-
        return c;
 }
 
@@ -1601,7 +1582,7 @@ unlang_t *unlang_compile_section(unlang_t *parent, unlang_compile_ctx_t *unlang_
                MEM(c->debug_name = talloc_typed_asprintf(c, "%s %s", name1, name2));
        }
 
-       return unlang_compile_children(g, unlang_ctx, true);
+       return unlang_compile_children(g, unlang_ctx);
 }
 
 
@@ -1636,7 +1617,6 @@ static unlang_t *compile_tmpl(unlang_t *parent, unlang_compile_ctx_t *unlang_ctx
        }
        ut->tmpl = vpt; /* const issues */
 
-       unlang_compile_action_defaults(c, unlang_ctx);
        return c;
 }
 
@@ -1895,11 +1875,6 @@ static unlang_t *compile_module(unlang_t *parent, unlang_compile_ctx_t *unlang_c
         */
        c->actions = m->mmc.mi->actions;
 
-       /*
-        *      Add in the default actions for this section.
-        */
-       unlang_compile_action_defaults(c, unlang_ctx);
-
        /*
         *      Parse the method environment for this module / method
         */
@@ -2073,26 +2048,7 @@ static unlang_t *compile_item(unlang_t *parent, unlang_compile_ctx_t *unlang_ctx
                        }
 
                        c = op->compile(parent, unlang_ctx, ci);
-               allocate_number:
-                       if (!c) return NULL;
-                       if (c == UNLANG_IGNORE) return UNLANG_IGNORE;
-
-                       c->number = unlang_number++;
-
-                       /*
-                        *      Only insert the per-thread allocation && instantiation if it's used.
-                        */
-                       op = &unlang_ops[c->type];
-                       if (!op->thread_inst_size) return c;
-
-                       if (!fr_rb_insert(unlang_instruction_tree, c)) {
-                               cf_log_err(ci, "Instruction \"%s\" number %u has conflict with previous one.",
-                                          c->debug_name, c->number);
-                               talloc_free(c);
-                               return NULL;
-                       }
-
-                       return c;
+                       goto allocate_number;
                }
 
                /* else it's something like sql { fail = 1 ...} */
@@ -2106,25 +2062,30 @@ static unlang_t *compile_item(unlang_t *parent, unlang_compile_ctx_t *unlang_ctx
                 */
                CONF_PAIR *cp = cf_item_to_pair(ci);
 
-               name = cf_pair_attr(cp);
-               op = name_to_op(name);
-
-               /*
-                *      Forbid section keywords as pair names, e.g. bare "update"
-                */
-               if (op && ((op->flag & UNLANG_OP_FLAG_SINGLE_WORD) == 0)) {
-                       cf_log_err(ci, "Syntax error after keyword '%s' - missing '{'", name);
-                       return NULL;
-               }
-
                /*
                 *      We cannot have assignments or actions here.
                 */
                if (cf_pair_value(cp) != NULL) {
-                       cf_log_err(ci, "Entry is not a reference to a module");
+                       cf_log_err(ci, "Invalid assignment");
                        return NULL;
                }
 
+               name = cf_pair_attr(cp);
+               op = name_to_op(name);
+
+               if (op) {
+                       /*
+                        *      Forbid section keywords as pair names, e.g. bare "update"
+                        */
+                       if ((op->flag & UNLANG_OP_FLAG_SINGLE_WORD) == 0) {
+                               cf_log_err(ci, "Syntax error after keyword '%s' - missing '{'", name);
+                               return NULL;
+                       }
+
+                       c = op->compile(parent, unlang_ctx, ci);
+                       goto allocate_number;
+               }
+
                /*
                 *      In-place expansions.
                 *
@@ -2142,12 +2103,10 @@ static unlang_t *compile_item(unlang_t *parent, unlang_compile_ctx_t *unlang_ctx
                        goto allocate_number;
                }
 
-               if (!op) goto check_for_module;
+               goto check_for_module;
 
-               c = op->compile(parent, unlang_ctx, ci);
-               goto allocate_number;
        } else {
-               cf_log_err(ci, "Asked to compile unknown conf type");
+               cf_log_err(ci, "Asked to compile unknown configuration item");
                return NULL;    /* who knows what it is... */
        }
 
@@ -2237,15 +2196,37 @@ check_for_module:
         */
        UPDATE_CTX2;
        c = compile_module(parent, &unlang_ctx2, ci, realname);
-       if (c) goto allocate_number;
+       if (!c) {
+               if (ignore_notfound) {
+                       cf_log_warn(ci, "Ignoring \"%s\" as the \"%s\" module is not enabled, "
+                                   "or the method does not exist", name, realname);
+                       return UNLANG_IGNORE;
+               }
 
-       if (ignore_notfound) {
-               cf_log_warn(ci, "Ignoring \"%s\" as the \"%s\" module is not enabled, "
-                           "or the method does not exist", name, realname);
-               return UNLANG_IGNORE;
+               return NULL;
        }
 
-       return NULL;
+allocate_number:
+       if (!c) return NULL;
+       if (c == UNLANG_IGNORE) return UNLANG_IGNORE;
+
+       c->number = unlang_number++;
+       compile_set_default_actions(c, unlang_ctx);
+
+       /*
+        *      Only insert the per-thread allocation && instantiation if it's used.
+        */
+       op = &unlang_ops[c->type];
+       if (!op->thread_inst_size) return c;
+
+       if (!fr_rb_insert(unlang_instruction_tree, c)) {
+               cf_log_err(ci, "Instruction \"%s\" number %u has conflict with previous one.",
+                          c->debug_name, c->number);
+               talloc_free(c);
+               return NULL;
+       }
+
+       return c;
 }
 
 /** Compile an unlang section for a virtual server
@@ -2260,7 +2241,7 @@ check_for_module:
  *     - -1 on error.
  */
 int unlang_compile(virtual_server_t const *vs,
-                  CONF_SECTION *cs, unlang_mod_actions_t const * actions, tmpl_rules_t const *rules, void **instruction)
+                  CONF_SECTION *cs, unlang_mod_actions_t const *actions, tmpl_rules_t const *rules, void **instruction)
 {
        unlang_t                        *c;
        tmpl_rules_t                    my_rules;

diff --git a/src/lib/unlang/foreach.c b/src/lib/unlang/foreach.c
index 594b51663c6221e43be7394f59fe9fca676000c6..3dbd687a6a373ce0672dc87d74849dbc8f59bd6c 100644 (file)
--- a/src/lib/unlang/foreach.c
+++ b/src/lib/unlang/foreach.c
@@ -782,9 +782,7 @@ static unlang_t *unlang_compile_foreach(unlang_t *parent, unlang_compile_ctx_t *
                }
        }
 
-       if (!unlang_compile_children(g, &unlang_ctx2, true)) return NULL;
-
-       return c;
+       return unlang_compile_children(g, &unlang_ctx2);
 }
 
 

diff --git a/src/lib/unlang/group.c b/src/lib/unlang/group.c
index a46ae3656fdb5135c290309f51b0210f6d7ace0f..ee000c5b2885557a10899caa60e23720a148f551 100644 (file)
--- a/src/lib/unlang/group.c
+++ b/src/lib/unlang/group.c
@@ -49,7 +49,6 @@ static unlang_t *unlang_compile_group(unlang_t *parent, unlang_compile_ctx_t *un
 static unlang_t *unlang_compile_redundant(unlang_t *parent, unlang_compile_ctx_t *unlang_ctx, CONF_ITEM const *ci)
 {
        CONF_SECTION                    *cs = cf_item_to_section(ci);
-       unlang_t                        *c;
 
        if (!cf_item_next(cs, NULL)) return UNLANG_IGNORE;
 
@@ -57,14 +56,11 @@ static unlang_t *unlang_compile_redundant(unlang_t *parent, unlang_compile_ctx_t
                return NULL;
        }
 
-       c = unlang_compile_section(parent, unlang_ctx, cs, UNLANG_TYPE_REDUNDANT);
-       if (!c) return NULL;
-
-       /*
-        *      We no longer care if "redundant" sections have a name.  If they do, it's ignored.
-        */
+       if (cf_section_name2(cs) != NULL) {
+               cf_log_warn(cs, "Ignoring name for 'redundant' section");
+       }
 
-       return c;
+       return unlang_compile_section(parent, unlang_ctx, cs, UNLANG_TYPE_REDUNDANT);
 }
 
 

diff --git a/src/lib/unlang/map.c b/src/lib/unlang/map.c
index 00b314fdf06bf991d01e74fbe4cf79be2d7e4633..71c1fd0575b77d4e0ead29180cef29de2807ffd9 100644 (file)
--- a/src/lib/unlang/map.c
+++ b/src/lib/unlang/map.c
@@ -824,8 +824,6 @@ static unlang_t *unlang_compile_update(unlang_t *parent, unlang_compile_ctx_t *u
 
        if (!pass2_fixup_update(g, unlang_ctx->rules)) goto error;
 
-       unlang_compile_action_defaults(c, unlang_ctx);
-
        return c;
 }
 
@@ -1052,8 +1050,6 @@ static unlang_t *unlang_compile_map(unlang_t *parent, unlang_compile_ctx_t *unla
         */
        if (!pass2_fixup_map_rhs(g, unlang_ctx->rules)) goto error;
 
-       unlang_compile_action_defaults(c, unlang_ctx);
-
        return c;
 }
 

diff --git a/src/lib/unlang/switch.c b/src/lib/unlang/switch.c
index 8e4d7a381505ff8c0be982acfd0b597e6b68d067..2673bafaf36f9201a33d58830776fd49fdaee963 100644 (file)
--- a/src/lib/unlang/switch.c
+++ b/src/lib/unlang/switch.c
@@ -514,8 +514,6 @@ static unlang_t *unlang_compile_switch(unlang_t *parent, unlang_compile_ctx_t *u
                g->num_children++;
        }
 
-       unlang_compile_action_defaults(c, unlang_ctx); /* why is this here???? */
-
        return c;
 }
 

diff --git a/src/lib/unlang/transaction.c b/src/lib/unlang/transaction.c
index 8c1779cb7057f10e1af34651a187ac5d772e15c5..3e587370c688a1835d416a16b572e67c0b90e943 100644 (file)
--- a/src/lib/unlang/transaction.c
+++ b/src/lib/unlang/transaction.c
@@ -235,35 +235,32 @@ static unlang_t *unlang_compile_transaction(unlang_t *parent, unlang_compile_ctx
 
        if (!transaction_ok(cs)) return NULL;
 
-       /*
-        *      Any failure is return, not continue.
-        */
-       unlang_compile_ctx_copy(&unlang_ctx2, unlang_ctx);
-
-       unlang_ctx2.actions.actions[RLM_MODULE_REJECT] = MOD_ACTION_RETURN;
-       unlang_ctx2.actions.actions[RLM_MODULE_FAIL] = MOD_ACTION_RETURN;
-       unlang_ctx2.actions.actions[RLM_MODULE_INVALID] = MOD_ACTION_RETURN;
-       unlang_ctx2.actions.actions[RLM_MODULE_DISALLOW] = MOD_ACTION_RETURN;
-
        g = unlang_group_allocate(parent, cs, UNLANG_TYPE_TRANSACTION);
        if (!g) return NULL;
 
        c = unlang_group_to_generic(g);
        c->debug_name = c->name = cf_section_name1(cs);
 
-       if (!unlang_compile_children(g, &unlang_ctx2, false)) return NULL;
+       /*
+        *      The default for a failed transaction is to continue to
+        *      the next instruction on failure.
+        */
+       c->actions.actions[RLM_MODULE_FAIL] = 1;
+       c->actions.actions[RLM_MODULE_INVALID] = 1;
+       c->actions.actions[RLM_MODULE_DISALLOW] = 1;
 
        /*
-        *      The default for a failed transaction is to continue on
-        *      failure.
+        *      For the children of this keyword, any failure is
+        *      return, not continue.
         */
-       if (!c->actions.actions[RLM_MODULE_FAIL])     c->actions.actions[RLM_MODULE_FAIL] = 1;
-       if (!c->actions.actions[RLM_MODULE_INVALID])  c->actions.actions[RLM_MODULE_INVALID] = 1;
-       if (!c->actions.actions[RLM_MODULE_DISALLOW]) c->actions.actions[RLM_MODULE_DISALLOW] = 1;
+       unlang_compile_ctx_copy(&unlang_ctx2, unlang_ctx);
 
-       unlang_compile_action_defaults(c, unlang_ctx); /* why is this here???? */
+       unlang_ctx2.actions.actions[RLM_MODULE_REJECT] = MOD_ACTION_RETURN;
+       unlang_ctx2.actions.actions[RLM_MODULE_FAIL] = MOD_ACTION_RETURN;
+       unlang_ctx2.actions.actions[RLM_MODULE_INVALID] = MOD_ACTION_RETURN;
+       unlang_ctx2.actions.actions[RLM_MODULE_DISALLOW] = MOD_ACTION_RETURN;
 
-       return c;
+       return unlang_compile_children(g, &unlang_ctx2);
 }
 
 void unlang_transaction_init(void)

diff --git a/src/lib/unlang/try.c b/src/lib/unlang/try.c
index 65e07a869ef5668a78da1c39f678e1fcb4b78ac9..83165a2ff6bbd6f255532b77af0a7a5e69b697d9 100644 (file)
--- a/src/lib/unlang/try.c
+++ b/src/lib/unlang/try.c
@@ -77,7 +77,7 @@ static unlang_t *unlang_compile_try(unlang_t *parent, unlang_compile_ctx_t *unla
        c = unlang_group_to_generic(g);
        c->debug_name = c->name = cf_section_name1(cs);
 
-       return unlang_compile_children(g, unlang_ctx, true);
+       return unlang_compile_children(g, unlang_ctx);
 }
 
 

diff --git a/src/lib/unlang/unlang_priv.h b/src/lib/unlang/unlang_priv.h
index 69c0b13b1e42b4aba4df1f10aa112c2ffd4b71db..90ac6d1782e765e3c7a55a0bbbe3db1c1a03035a 100644 (file)
--- a/src/lib/unlang/unlang_priv.h
+++ b/src/lib/unlang/unlang_priv.h
@@ -221,7 +221,7 @@ unlang_t *unlang_compile_empty(unlang_t *parent, unlang_compile_ctx_t *unlang_ct
 
 unlang_t *unlang_compile_section(unlang_t *parent, unlang_compile_ctx_t *unlang_ctx, CONF_SECTION *cs, unlang_type_t type);
 
-unlang_t *unlang_compile_children(unlang_group_t *g, unlang_compile_ctx_t *unlang_ctx_in, bool set_action_defaults);
+unlang_t *unlang_compile_children(unlang_group_t *g, unlang_compile_ctx_t *unlang_ctx);
 
 unlang_group_t *unlang_group_allocate(unlang_t *parent, CONF_SECTION *cs, unlang_type_t type);
 
@@ -230,12 +230,6 @@ int unlang_define_local_variable(CONF_ITEM *ci, unlang_variable_t *var, tmpl_rul
 
 bool unlang_compile_limit_subsection(CONF_SECTION *cs, char const *name);
 
-/*
- *     @todo - arguably this should be part of the core compiler, and
- *     never called by any keyword.
- */
-void unlang_compile_action_defaults(unlang_t *c, unlang_compile_ctx_t *unlang_ctx);
-
 /*
  *     @todo - These functions should be made private once all of they keywords have been moved to foo(args) syntax.
  */