On Thu, Jun 21, 2018 at 10:42:25AM +0200, Phil Sutter wrote:
> Hi Máté,
>
> On Tue, Jun 19, 2018 at 11:50:24AM +0200, Máté Eckl wrote:
> [...]
> > diff --git a/src/parser_bison.y b/src/parser_bison.y
> > index 98bfeba..d753fd9 100644
> > --- a/src/parser_bison.y
> > +++ b/src/parser_bison.y
> > @@ -21,6 +21,7 @@
> > #include <linux/netfilter/nf_conntrack_tuple_common.h>
> > #include <linux/netfilter/nf_nat.h>
> > #include <linux/netfilter/nf_log.h>
> > +#include <linux/netfilter_ipv4.h>
> > #include <netinet/ip_icmp.h>
> > #include <netinet/icmp6.h>
> > #include <libnftnl/common.h>
> > @@ -182,6 +183,8 @@ int nft_lex(void *, void *, void *);
> > %token AT "@"
> > %token VMAP "vmap"
> >
> > +%token PLUS "+"
> > +
> > %token INCLUDE "include"
> > %token DEFINE "define"
> > %token REDEFINE "redefine"
> > @@ -521,7 +524,7 @@ int nft_lex(void *, void *, void *);
> > %destructor { handle_free(&$$); } table_spec tableid_spec chain_spec chainid_spec flowtable_spec chain_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec index_spec
> > %type <handle> set_spec setid_spec set_identifier flowtable_identifier obj_spec objid_spec obj_identifier
> > %destructor { handle_free(&$$); } set_spec setid_spec set_identifier obj_spec objid_spec obj_identifier
> > -%type <val> family_spec family_spec_explicit chain_policy prio_spec
> > +%type <val> family_spec family_spec_explicit chain_policy prio_spec standard_prio
> >
> > %type <string> dev_spec quota_unit
> > %destructor { xfree($$); } dev_spec quota_unit
> > @@ -1794,8 +1797,23 @@ hook_spec : TYPE STRING HOOK STRING dev_spec PRIORITY prio_spec
> > }
> > ;
> >
> > -prio_spec : NUM { $$ = $1; }
> > - | DASH NUM { $$ = -$2; }
> > +prio_spec : NUM { $$ = $1; }
> > + | DASH NUM { $$ = -$2; }
> > + | standard_prio
> > + | standard_prio PLUS NUM { $$ = $1 + $3; }
> > + | standard_prio DASH NUM { $$ = $1 - $3; }
> > + ;
> > +
> > +standard_prio : STRING
> > + {
> > + int tmp = chain_std_prio_lookup($1);
> > + if (tmp == NF_IP_PRI_LAST) {
> > + erec_queue(error(&@$, "priority name '%s' is invalid", $1),
> > + state->msgs);
> > + YYERROR;
> > + }
> > + $$ = tmp;
> > + }
> > ;
>
> I think you could simplify this by allowing for 'standard_prio' to
> consist of an empty string which evaluates to the value 0. Then
> 'prio_spec' would be just:
>
> | prio_spec : standard_prio
> | | standard_prio PLUS NUM { $$ = $1 + $3; }
> | | standard_prio DASH NUM { $$ = $1 - $3; }
>
Interesting point. I think only the `DASH NUM` line can be ommitted this way,
because this would make 'priority 5' miss the PLUS sign and thus fail.
The actual solution seems much more unambiguous to me, so I'd like to keep it
the way it is.
By the way, there's a question I haven't met yet. Prio spec is used by not only
hook_spec but also flowtable_block. Are these standard priorities applicable for
flowtable priorities? Or should I make it specific to chains?
> [...]
> > + for (i = 0; i < sizeof(chain_std_prios) / sizeof(struct chain_prio_tag); ++i) {
>
> Pablo already pointed this out, but one additional remark: There's no
> need to define ARRAY_SIZE macro, simply use 'array_size()' defined in
> include/utils.h.
Yes, I found it thanks.
>
> [...]
> > +static const char *chain_prio2str(int prio)
> > +{
> > + static char ret[256];
> > + char offstr[20];
> > + const int reach = 10;
> > + size_t i;
> > +
> > + ret[0] = 0;
> > + offstr[0] = 0;
> > + for (i = 0; i < sizeof(chain_std_prios) / sizeof(struct chain_prio_tag); ++i) {
>
> for (i = 0; i < array_size(chain_std_prios); i++) {
>
> > + const int sdt_prio = chain_std_prios[i].val;
> > + const char *sdt_prio_str = chain_std_prios[i].str;
> > + if (prio >= sdt_prio - reach && prio <= sdt_prio + reach) {
>
> if (abs(prio - sdt_prio) <= reach) {
>
> > + int offset = prio - sdt_prio;
> > + if (offset != 0) {
> > + snprintf(offstr, sizeof(offstr), " %c %d",
> > + offset > 0 ? '+' : '-', abs(offset));
> > + }
> > + sprintf(ret, "%s%s", sdt_prio_str, offstr);
>
> strcpy(ret, sdt_prio_str);
> if (offset > 0)
> snprintf(ret + strlen(ret), "+ %d", offset);
> else if (offset < 0)
> snprintf(ret + strlen(ret), "- %d", -offset);
>
> > + return ret;
> > + }
> > + }
> > + sprintf(ret, "%d", prio);
> > + return ret;
> > +}
> > +
> > static void chain_print_declaration(const struct chain *chain,
> > struct output_ctx *octx)
> > {
> > @@ -781,8 +832,8 @@ static void chain_print_declaration(const struct chain *chain,
> > hooknum2str(chain->handle.family, chain->hooknum));
> > if (chain->dev != NULL)
> > nft_print(octx, " device %s", chain->dev);
> > - nft_print(octx, " priority %d; policy %s;\n",
> > - chain->priority, chain_policy2str(chain->policy));
> > + nft_print(octx, " priority %s; policy %s;\n",
> > + chain_prio2str(chain->priority), chain_policy2str(chain->policy));
>
> Doesn't this exceed 80 column boundary?
>
> > }
> > }
> >
> > @@ -806,9 +857,9 @@ void chain_print_plain(const struct chain *chain, struct output_ctx *octx)
> > chain->handle.table.name, chain->handle.chain.name);
> >
> > if (chain->flags & CHAIN_F_BASECHAIN) {
> > - nft_print(octx, " { type %s hook %s priority %d; policy %s; }",
> > + nft_print(octx, " { type %s hook %s priority %s; policy %s; }",
> > chain->type, chain->hookstr,
> > - chain->priority, chain_policy2str(chain->policy));
> > + chain_prio2str(chain->priority), chain_policy2str(chain->policy));
>
> Column boundary again?
Probably by a few characters. I'll fix them in the next version.
>
> Apart from that, LGTM!
>
> Thanks, Phil
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
