| Message ID | 20210316234039.15677-7-fw@strlen.de |
|---|---|
| State | Deferred, archived |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | arbirary table/chain names | expand |
Hi, On Wed, Mar 17, 2021 at 12:40:39AM +0100, Florian Westphal wrote: [...] > Another alternative is to deprecate implicit rule add altogether > so users would have to move to 'nft add rule ...'. Isn't this required for nested syntax? I didn't check, but does your arbitrary table/chain name support work also when restoring a ruleset in that nested syntax? Another interesting aspect might be arbitrary set names - 'set' is also a valid keyword used in rules, this fact killed my approach with start conditions. ;) Cheers, Phil
Phil Sutter <phil@nwl.cc> wrote: > > Another alternative is to deprecate implicit rule add altogether > > so users would have to move to 'nft add rule ...'. > > Isn't this required for nested syntax? I didn't check, but does your > arbitrary table/chain name support work also when restoring a ruleset in > that nested syntax? Whats 'nested syntax'? You mean "table bla { chain foo {"? > Another interesting aspect might be arbitrary set > names - 'set' is also a valid keyword used in rules, this fact killed my > approach with start conditions. ;) Right, arbitrary set names are needed as well, I forgot about them. It should be possible by using two "set" rules in flex. One in the INITIAL scope (to handle set bla {), and one in 'rule' or 'expression scope'. The former would switch to an exclusive start condition (expect STRING, close condition on '{', just like CHAIN is handled here. The latter would not change state and just return SET token.
Florian Westphal <fw@strlen.de> wrote: > Allow switch of the flex state from bison parser. > Note that this switch will happen too late to cover all cases: > > nft add ip dup fwd ip saddr ... # adds a rule to chain fwd in table dup > nft add dup fwd ... # syntax error (flex parses dup as expression keyword) > > to solve this, bison must carry a list of keywords that are allowed to > be used as table names. > > This adds FWD as an example. When new keywords are added, this can > then be extended as needed. > > Another alternative is to deprecate implicit rule add altogether > so users would have to move to 'nft add rule ...'. ... and another alternative is to not allow arbitrary table/chain/set names after all. We could just say that all future tokens that could break existing table/chain/set name need to be added to the 'identifier' in parser_bison.y. Provided new expressions with args use start conditionals the list of tokens would probably stay short. Given the 'set' complication Phil mentioned that might be the best way forward.
On Thu, Mar 18, 2021 at 01:37:24PM +0100, Florian Westphal wrote: > Phil Sutter <phil@nwl.cc> wrote: > > > Another alternative is to deprecate implicit rule add altogether > > > so users would have to move to 'nft add rule ...'. > > > > Isn't this required for nested syntax? I didn't check, but does your > > arbitrary table/chain name support work also when restoring a ruleset in > > that nested syntax? > > Whats 'nested syntax'? > > You mean "table bla { chain foo {"? Yes, exactly. > > Another interesting aspect might be arbitrary set > > names - 'set' is also a valid keyword used in rules, this fact killed my > > approach with start conditions. ;) > > Right, arbitrary set names are needed as well, I forgot about them. > > It should be possible by using two "set" rules in flex. > > One in the INITIAL scope (to handle set bla {), and one in > 'rule' or 'expression scope'. > > The former would switch to an exclusive start condition (expect > STRING, close condition on '{', just like CHAIN is handled here. > > The latter would not change state and just return SET token. Yes, that might work. Thanks, Phil
Florian Westphal <fw@strlen.de> wrote: > Florian Westphal <fw@strlen.de> wrote: > > Allow switch of the flex state from bison parser. > > Note that this switch will happen too late to cover all cases: > > > > nft add ip dup fwd ip saddr ... # adds a rule to chain fwd in table dup > > nft add dup fwd ... # syntax error (flex parses dup as expression keyword) > > > > to solve this, bison must carry a list of keywords that are allowed to > > be used as table names. > > > > This adds FWD as an example. When new keywords are added, this can > > then be extended as needed. > > > > Another alternative is to deprecate implicit rule add altogether > > so users would have to move to 'nft add rule ...'. > > ... and another alternative is to not allow arbitrary table/chain/set > names after all. > > We could just say that all future tokens that could break existing > table/chain/set name need to be added to the 'identifier' in > parser_bison.y. > > Provided new expressions with args use start conditionals the list > of tokens would probably stay short. > > Given the 'set' complication Phil mentioned that might be the best > way forward. I've pushed the first 3 patches and marked the last 3 as deferred -- lets first try conservative approach first before attempting to support arbitrary names.
diff --git a/include/parser.h b/include/parser.h index d6cf20729421..35117acc977f 100644 --- a/include/parser.h +++ b/include/parser.h @@ -77,5 +77,6 @@ extern void scanner_push_buffer(void *scanner, const char *buffer); extern void scanner_pop_start_cond(void *scanner, enum startcond_type sc); +extern void scanner_push_start_cond(void *scanner, enum startcond_type sc); #endif /* NFTABLES_PARSER_H */ diff --git a/src/parser_bison.y b/src/parser_bison.y index bbac85fd35ce..a910d813e637 100644 --- a/src/parser_bison.y +++ b/src/parser_bison.y @@ -568,8 +568,8 @@ int nft_lex(void *, void *, void *); %token IN "in" %token OUT "out" -%type <string> identifier type_identifier string comment_spec -%destructor { xfree($$); } identifier type_identifier string comment_spec +%type <string> identifier type_identifier string comment_spec implicit_table_name +%destructor { xfree($$); } identifier type_identifier string comment_spec implicit_table_name %type <val> time_spec quota_used @@ -582,13 +582,13 @@ int nft_lex(void *, void *, void *); %type <cmd> base_cmd add_cmd replace_cmd create_cmd insert_cmd delete_cmd get_cmd list_cmd reset_cmd flush_cmd rename_cmd export_cmd monitor_cmd describe_cmd import_cmd %destructor { cmd_free($$); } base_cmd add_cmd replace_cmd create_cmd insert_cmd delete_cmd get_cmd list_cmd reset_cmd flush_cmd rename_cmd export_cmd monitor_cmd describe_cmd import_cmd -%type <handle> table_spec tableid_spec table_or_id_spec -%destructor { handle_free(&$$); } table_spec tableid_spec table_or_id_spec -%type <handle> chain_spec chainid_spec chain_or_id_spec -%destructor { handle_free(&$$); } chain_spec chainid_spec chain_or_id_spec +%type <handle> table_spec tableid_spec table_or_id_spec implicit_table_spec +%destructor { handle_free(&$$); } table_spec tableid_spec table_or_id_spec implicit_table_spec +%type <handle> chain_spec chainid_spec chain_or_id_spec implicit_chain_spec +%destructor { handle_free(&$$); } chain_spec chainid_spec chain_or_id_spec implicit_chain_spec -%type <handle> flowtable_spec chain_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec index_spec -%destructor { handle_free(&$$); } flowtable_spec chain_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec index_spec +%type <handle> flowtable_spec chain_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec index_spec implicit_rule_position +%destructor { handle_free(&$$); } flowtable_spec chain_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec index_spec implicit_rule_position %type <handle> set_spec setid_spec set_or_id_spec %destructor { handle_free(&$$); } set_spec setid_spec set_or_id_spec %type <handle> obj_spec objid_spec obj_or_id_spec @@ -882,6 +882,7 @@ close_scope_socket : { scanner_pop_start_cond(nft->scanner, PARSER_SC_EXPR_SOCKE close_scope_log : { scanner_pop_start_cond(nft->scanner, PARSER_SC_STMT_LOG); } +open_scope_chain : { scanner_push_start_cond(nft->scanner, PARSER_SC_STRING_CHAIN); }; common_block : INCLUDE QUOTED_STRING stmt_separator { if (scanner_include_file(nft, scanner, $2, &@$) < 0) { @@ -998,7 +999,7 @@ add_cmd : TABLE table_spec { $$ = cmd_alloc(CMD_ADD, CMD_OBJ_RULE, &$2, &@$, $3); } - | /* empty */ rule_position rule + | /* empty */ implicit_rule_position rule { $$ = cmd_alloc(CMD_ADD, CMD_OBJ_RULE, &$1, &@$, $2); } @@ -2607,6 +2608,44 @@ rule_position : chain_spec } ; +implicit_table_name : FWD { $$ = xstrdup("fwd"); } + | DUP { $$ = xstrdup("dup"); } + ; + +implicit_table_spec : family_spec implicit_table_name + { + memset(&$$, 0, sizeof($$)); + $$.family = $1; + $$.table.location = @2; + $$.table.name = $2; + } + ; + +implicit_chain_spec : open_scope_chain implicit_table_spec identifier close_scope_chain + { + $$ = $2; + $$.chain.name = $3; + $$.chain.location = @3; + } + ; + +implicit_rule_position : open_scope_chain rule_position { $$ = $2; } + | implicit_chain_spec { $$ = $1; } + | implicit_chain_spec position_spec { handle_merge(&$1, &$2); $$ = $1; } + | implicit_chain_spec handle_spec { + $2.position.location = $2.handle.location; + $2.position.id = $2.handle.id; + $2.handle.id = 0; + handle_merge(&$1, &$2); + $$ = $1; + } + | implicit_chain_spec index_spec + { + handle_merge(&$1, &$2); + $$ = $1; + } + ; + ruleid_spec : chain_spec handle_spec { handle_merge(&$1, &$2); diff --git a/src/scanner.l b/src/scanner.l index a156accaa944..a4747b39b314 100644 --- a/src/scanner.l +++ b/src/scanner.l @@ -101,8 +101,6 @@ static void reset_pos(struct parser_state *state, struct location *loc) static int scanner_handle_tablename(void *scanner, const char *token); static int scanner_handle_chainname(void *scanner, const char *token); -static void scanner_push_start_cond(void *scanner, enum startcond_type type); - #define YY_USER_ACTION { \ update_pos(yyget_extra(yyscanner), yylloc, yyleng); \ update_offset(yyget_extra(yyscanner), yylloc, yyleng); \ @@ -1087,7 +1085,7 @@ void scanner_destroy(struct nft_ctx *nft) yylex_destroy(nft->scanner); } -static void scanner_push_start_cond(void *scanner, enum startcond_type type) +void scanner_push_start_cond(void *scanner, enum startcond_type type) { struct parser_state *state = yyget_extra(scanner);
Allow switch of the flex state from bison parser. Note that this switch will happen too late to cover all cases: nft add ip dup fwd ip saddr ... # adds a rule to chain fwd in table dup nft add dup fwd ... # syntax error (flex parses dup as expression keyword) to solve this, bison must carry a list of keywords that are allowed to be used as table names. This adds FWD as an example. When new keywords are added, this can then be extended as needed. Another alternative is to deprecate implicit rule add altogether so users would have to move to 'nft add rule ...'. Signed-off-by: Florian Westphal <fw@strlen.de> --- include/parser.h | 1 + src/parser_bison.y | 57 ++++++++++++++++++++++++++++++++++++++-------- src/scanner.l | 4 +--- 3 files changed, 50 insertions(+), 12 deletions(-)