From 7afa1438866fa94454ba133608b6877171f71d37 Mon Sep 17 00:00:00 2001 From: Jan Maria Matejka Date: Mon, 17 Dec 2018 13:51:11 +0100 Subject: [PATCH] Filter refactoring: Passing the resulting struct f_val as a pointer. This also drops the multiplexing of errors with the f_val itself together with the T_RETURN f_val type flag. --- conf/confbase.Y | 3 +-- filter/config.Y | 2 +- filter/f-inst.c | 26 ++++++++++------------ filter/filter.c | 52 ++++++++++++++++++++++++-------------------- filter/filter.h | 25 +++++++++++---------- filter/filter_test.c | 8 +++---- nest/cmds.c | 5 ++--- 7 files changed, 60 insertions(+), 61 deletions(-) diff --git a/conf/confbase.Y b/conf/confbase.Y index 3fd034db..3d573e10 100644 --- a/conf/confbase.Y +++ b/conf/confbase.Y @@ -124,8 +124,7 @@ conf: definition ; definition: DEFINE SYM '=' term ';' { struct f_val *val = cfg_alloc(sizeof(struct f_val)); - *val = f_eval($4, cfg_mem); - if (val->type == T_RETURN) cf_error("Runtime error"); + if (f_eval($4, cfg_mem, val) > F_RETURN) cf_error("Runtime error"); cf_define_symbol($2, SYM_CONSTANT | val->type, val); } ; diff --git a/filter/config.Y b/filter/config.Y index c1e74531..200c1f41 100644 --- a/filter/config.Y +++ b/filter/config.Y @@ -653,7 +653,7 @@ set_atom: | VPN_RD { $$.type = T_RD; $$.val.ec = $1; } | ENUM { $$.type = pair_a($1); $$.val.i = pair_b($1); } | '(' term ')' { - $$ = f_eval($2, cfg_mem); + if (f_eval($2, cfg_mem, &($$)) > F_RETURN) cf_error("Runtime error"); if (!f_valid_set_type($$.type)) cf_error("Set-incompatible type"); } | SYM { diff --git a/filter/f-inst.c b/filter/f-inst.c index 24908442..814e026d 100644 --- a/filter/f-inst.c +++ b/filter/f-inst.c @@ -121,15 +121,15 @@ while (tt) { *vv = lp_alloc(fs->pool, sizeof(struct f_path_mask)); if (tt->kind == PM_ASN_EXPR) { - struct f_val res; - INTERPRET(res, (struct f_inst *) tt->val); + struct f_val xres; + INTERPRET(xres, (struct f_inst *) tt->val); (*vv)->kind = PM_ASN; - if (res.type != T_INT) { + if (xres.type != T_INT) { runtime( "Error resolving path mask template: value not an integer" ); - return (struct f_val) { .type = T_VOID }; + return F_ERROR; } - (*vv)->val = res.val.i; + (*vv)->val = xres.val.i; } else { **vv = *tt; } @@ -280,9 +280,7 @@ /* Should take care about turning ACCEPT into MODIFY */ case F_ERROR: case F_REJECT: /* FIXME (noncritical) Should print complete route along with reason to reject route */ - res.type = T_RETURN; - res.val.i = what->a2.i; - return res; /* We have to return now, no more processing. */ + return what->a2.i; /* We have to return now, no more processing. */ case F_NONL: case F_NOP: break; @@ -658,14 +656,12 @@ case FI_RETURN: ARG_ANY(1); res = v1; - res.type |= T_RETURN; - return res; - case FI_CALL: /* CALL: this is special: if T_RETURN and returning some value, mask it out */ + return F_RETURN; + case FI_CALL: ARG_ANY(1); - res = interpret(fs, what->a2.p); - if (res.type == T_RETURN) - return res; - res.type &= ~T_RETURN; + fret = interpret(fs, what->a2.p, &res); + if (fret > F_RETURN) + return fret; break; case FI_CLEAR_LOCAL_VARS: /* Clear local variables */ for (sym = what->a1.p; sym != NULL; sym = sym->aux2) diff --git a/filter/filter.c b/filter/filter.c index 794e35e8..1e6fc003 100644 --- a/filter/filter.c +++ b/filter/filter.c @@ -615,24 +615,27 @@ static struct tbf rl_runtime_err = TBF_DEFAULT_LOG_LIMITS; * &f_val structures are copied around, so there are no problems with * memory managment. */ -static struct f_val -interpret(struct filter_state *fs, struct f_inst *what) +static enum filter_return +interpret(struct filter_state *fs, struct f_inst *what, struct f_val *resp) { struct symbol *sym; - struct f_val v1, v2, v3, res = { .type = T_VOID }, *vp; + struct f_val v1, v2, v3, *vp; unsigned u1, u2; + enum filter_return fret; int i; u32 as; + *resp = (struct f_val) { .type = T_VOID }; + for ( ; what; what = what->next) { - res.type = T_VOID; +#define res (*resp) + res = (struct f_val) { .type = T_VOID }; switch(what->fi_code) { + #define runtime(fmt, ...) do { \ if (!(fs->flags & FF_SILENT)) \ log_rl(&rl_runtime_err, L_ERR "filters, line %d: " fmt, what->lineno, ##__VA_ARGS__); \ - res.type = T_RETURN; \ - res.val.i = F_ERROR; \ - return res; \ + return F_ERROR; \ } while(0) #define ARG_ANY(n) INTERPRET(v##n, what->a##n.p) @@ -643,9 +646,9 @@ interpret(struct filter_state *fs, struct f_inst *what) n, f_instruction_name(what->fi_code), t, v##n.type); #define INTERPRET(val, what_) \ - val = interpret(fs, what_); \ - if (val.type & T_RETURN) \ - return val; + fret = interpret(fs, what_, &(val)); \ + if (fret >= F_RETURN) \ + return fret; #define ACCESS_RTE \ do { if (!fs->rte) runtime("No route to access"); } while (0) @@ -657,6 +660,7 @@ interpret(struct filter_state *fs, struct f_inst *what) #include "filter/f-inst.c" +#undef res #undef runtime #undef ARG_ANY #undef ARG @@ -664,7 +668,7 @@ interpret(struct filter_state *fs, struct f_inst *what) #undef ACCESS_RTE #undef ACCESS_EATTRS }} - return res; + return F_NOP; } @@ -837,7 +841,7 @@ i_same(struct f_inst *f1, struct f_inst *f2) * (and cached rta of read-only source rte is intact), if rte is * modified in place, old cached rta is possibly freed. */ -int +enum filter_return f_run(struct filter *filter, struct rte **rte, struct linpool *tmp_pool, int flags) { if (filter == FILTER_ACCEPT) @@ -857,7 +861,8 @@ f_run(struct filter *filter, struct rte **rte, struct linpool *tmp_pool, int fla LOG_BUFFER_INIT(fs.buf); - struct f_val res = interpret(&fs, filter->root); + struct f_val res; + enum filter_return fret = interpret(&fs, filter->root, &res); if (fs.old_rta) { /* @@ -879,18 +884,18 @@ f_run(struct filter *filter, struct rte **rte, struct linpool *tmp_pool, int fla } - if (res.type != T_RETURN) { + if (fret < F_ACCEPT) { if (!(fs.flags & FF_SILENT)) log_rl(&rl_runtime_err, L_ERR "Filter %s did not return accept nor reject. Make up your mind", filter->name); return F_ERROR; } DBG( "done (%u)\n", res.val.i ); - return res.val.i; + return fret; } /* TODO: perhaps we could integrate f_eval(), f_eval_rte() and f_run() */ -struct f_val +enum filter_return f_eval_rte(struct f_inst *expr, struct rte **rte, struct linpool *tmp_pool) { @@ -902,13 +907,12 @@ f_eval_rte(struct f_inst *expr, struct rte **rte, struct linpool *tmp_pool) LOG_BUFFER_INIT(fs.buf); /* Note that in this function we assume that rte->attrs is private / uncached */ - struct f_val res = interpret(&fs, expr); - - return res; + struct f_val res; + return interpret(&fs, expr, &res); } -struct f_val -f_eval(struct f_inst *expr, struct linpool *tmp_pool) +enum filter_return +f_eval(struct f_inst *expr, struct linpool *tmp_pool, struct f_val *pres) { struct filter_state fs = { .pool = tmp_pool, @@ -916,14 +920,16 @@ f_eval(struct f_inst *expr, struct linpool *tmp_pool) LOG_BUFFER_INIT(fs.buf); - return interpret(&fs, expr); + return interpret(&fs, expr, pres); } uint f_eval_int(struct f_inst *expr) { /* Called independently in parse-time to eval expressions */ - struct f_val res = f_eval(expr, cfg_mem); + struct f_val res; + if (f_eval(expr, cfg_mem, &res) > F_RETURN) + cf_error("Runtime error while evaluating expression"); if (res.type != T_INT) cf_error("Integer expression expected"); diff --git a/filter/filter.h b/filter/filter.h index a8c33287..8728f679 100644 --- a/filter/filter.h +++ b/filter/filter.h @@ -175,9 +175,19 @@ void trie_format(struct f_trie *t, buffer *buf); struct ea_list; struct rte; -int f_run(struct filter *filter, struct rte **rte, struct linpool *tmp_pool, int flags); -struct f_val f_eval_rte(struct f_inst *expr, struct rte **rte, struct linpool *tmp_pool); -struct f_val f_eval(struct f_inst *expr, struct linpool *tmp_pool); +enum filter_return { + F_NOP = 0, + F_NONL, + F_RETURN, + F_ACCEPT, /* Need to preserve ordering: accepts < rejects! */ + F_REJECT, + F_ERROR, + F_QUITBIRD, +}; + +enum filter_return f_run(struct filter *filter, struct rte **rte, struct linpool *tmp_pool, int flags); +enum filter_return f_eval_rte(struct f_inst *expr, struct rte **rte, struct linpool *tmp_pool); +enum filter_return f_eval(struct f_inst *expr, struct linpool *tmp_pool, struct f_val *pres); uint f_eval_int(struct f_inst *expr); char *filter_name(struct filter *filter); @@ -190,14 +200,6 @@ int val_same(struct f_val v1, struct f_val v2); void val_format(struct f_val v, buffer *buf); - -#define F_NOP 0 -#define F_NONL 1 -#define F_ACCEPT 2 /* Need to preserve ordering: accepts < rejects! */ -#define F_REJECT 3 -#define F_ERROR 4 -#define F_QUITBIRD 5 - #define FILTER_ACCEPT NULL #define FILTER_REJECT ((void *) 1) #define FILTER_UNDEF ((void *) 2) /* Used in BGP */ @@ -246,7 +248,6 @@ void val_format(struct f_val v, buffer *buf); #define T_LCLIST 0x29 /* Large community list */ #define T_RD 0x2a /* Route distinguisher for VPN addresses */ -#define T_RETURN 0x40 #define T_SET 0x80 #define T_PREFIX_SET 0x81 diff --git a/filter/filter_test.c b/filter/filter_test.c index be7fd521..b319225b 100644 --- a/filter/filter_test.c +++ b/filter/filter_test.c @@ -44,13 +44,11 @@ run_function(const void *parsed_fn_def) struct f_inst *f = (struct f_inst *) parsed_fn_def; linpool *tmp = lp_new_default(&root_pool); - struct f_val res = f_eval(f, tmp); + struct f_val res; + enum filter_return fret = f_eval(f, tmp, &res); rfree(tmp); - if (res.type == T_RETURN && res.val.i >= F_REJECT) - return 0; - - return 1; + return (fret < F_REJECT); } static void diff --git a/nest/cmds.c b/nest/cmds.c index ca601ef2..0c89d20f 100644 --- a/nest/cmds.c +++ b/nest/cmds.c @@ -97,9 +97,8 @@ cmd_show_memory(void) void cmd_eval(struct f_inst *expr) { - struct f_val v = f_eval(expr, this_cli->parser_pool); - - if (v.type == T_RETURN) + struct f_val v; + if (f_eval(expr, this_cli->parser_pool, &v) > F_RETURN) { cli_msg(8008, "runtime error"); return;