From: Alan T. DeKok Date: Thu, 3 Jul 2025 19:22:53 +0000 (-0400) Subject: hoist "set default actions" X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=cbb0be7fc070339c9ad3601c53ed2b42c134a3ce;p=thirdparty%2Ffreeradius-server.git 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 2d73c9cfe14..70ef78fb7dd 100644 --- 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 d05feda6393..fe1de147b83 100644 --- 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 594b51663c6..3dbd687a6a3 100644 --- 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 a46ae3656fd..ee000c5b288 100644 --- 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 00b314fdf06..71c1fd0575b 100644 --- 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 8e4d7a38150..2673bafaf36 100644 --- 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 8c1779cb705..3e587370c68 100644 --- 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 65e07a869ef..83165a2ff6b 100644 --- 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 69c0b13b1e4..90ac6d1782e 100644 --- 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. */