diff mbox series

[v3,nft] Set/print standard chain priorities with textual names

Message ID 20180619095024.32630-1-ecklm94@gmail.com
State Changes Requested
Delegated to: Pablo Neira
Headers show
Series [v3,nft] Set/print standard chain priorities with textual names | expand

Commit Message

Máté Eckl June 19, 2018, 9:50 a.m. UTC
v3:
 - no tokens are used for priority names, lookup is used instead
 - names and values are moved out to a structure
 - the helper function became unnecessary, thus I removed it

-- 8< --
This patch adds the possibility to use textual names to set the chain priority
to basic values so that numeric values do not need to be learnt any more for
basic usage.

Basic arithmetic can also be done with them to ease the addition of
relatively higher/lower priority chains.
Addition and substraction is possible.

Values are also printed with their friendly name within the range of
<basicprio> +- 10.

Example:
	nft> add table x
	nft> add chain x y { type filter hook prerouting priority mangleprio; }
	nft> add chain x z { type filter hook prerouting priority mangleprio + 1; }
	nft> add chain x w { type filter hook prerouting priority mangleprio - 5; }
	nft> add chain x r { type filter hook prerouting priority filterprio + 10; }
	nft> add chain x t { type filter hook prerouting priority rawprio; }
	nft> add chain x q { type filter hook prerouting priority rawprio + 11; }
	nft> add chain x h { type filter hook prerouting priority 15; }
	nft> list ruleset
	table ip x {
		chain y {
			type filter hook prerouting priority mangle; policy accept;
		}

		chain z {
			type filter hook prerouting priority mangle + 1; policy accept;
		}

		chain w {
			type filter hook prerouting priority mangle - 5; policy accept;
		}

		chain r {
			type filter hook prerouting priority filter + 10; policy accept;
		}

		chain t {
			type filter hook prerouting priority raw; policy accept;
		}

		chain q {
			type filter hook prerouting priority -289; policy accept;
		}

		chain h {
			type filter hook prerouting priority 15; policy accept;
		}
	}

	nft> add chain x h { type filter hook rerouting priority first; }
	Error: priority name 'first' is invalid
	add chain x h { type filter hook prerouting priority first; }
	                                                     ^^^^^

Signed-off-by: Máté Eckl <ecklm94@gmail.com>
---
 include/rule.h     |  1 +
 src/parser_bison.y | 24 ++++++++++++++++---
 src/rule.c         | 59 ++++++++++++++++++++++++++++++++++++++++++----
 src/scanner.l      |  2 ++
 4 files changed, 79 insertions(+), 7 deletions(-)

Comments

Pablo Neira Ayuso June 20, 2018, 12:09 p.m. UTC | #1
Hi!

This looks good, but some comments.

On Tue, Jun 19, 2018 at 11:50:24AM +0200, Máté Eckl wrote:
> v3:
>  - no tokens are used for priority names, lookup is used instead
>  - names and values are moved out to a structure
>  - the helper function became unnecessary, thus I removed it
> 
> -- 8< --
> This patch adds the possibility to use textual names to set the chain priority
> to basic values so that numeric values do not need to be learnt any more for
> basic usage.
> 
> Basic arithmetic can also be done with them to ease the addition of
> relatively higher/lower priority chains.
> Addition and substraction is possible.
> 
> Values are also printed with their friendly name within the range of
> <basicprio> +- 10.
> 
> Example:
> 	nft> add table x
> 	nft> add chain x y { type filter hook prerouting priority mangleprio; }

The command uses "mangleprio"

[...]
> 	nft> list ruleset
> 	table ip x {
> 		chain y {
> 			type filter hook prerouting priority mangle; policy accept;

listing uses "mangle"

I guess you only need to amend the commit message, it's a leftover,
right?

We already agreed to use "mangle" for this, as the listing shows.

> 		}
> 
> 		chain z {
> 			type filter hook prerouting priority mangle + 1; policy accept;
> 		}
> 
> 		chain w {
> 			type filter hook prerouting priority mangle - 5; policy accept;
> 		}
> 
> 		chain r {
> 			type filter hook prerouting priority filter + 10; policy accept;
> 		}
> 
> 		chain t {
> 			type filter hook prerouting priority raw; policy accept;
> 		}
> 
> 		chain q {
> 			type filter hook prerouting priority -289; policy accept;
> 		}
> 
> 		chain h {
> 			type filter hook prerouting priority 15; policy accept;
> 		}
> 	}
> 
> 	nft> add chain x h { type filter hook rerouting priority first; }
> 	Error: priority name 'first' is invalid
> 	add chain x h { type filter hook prerouting priority first; }
> 	                                                     ^^^^^
> 
> Signed-off-by: Máté Eckl <ecklm94@gmail.com>
> ---
>  include/rule.h     |  1 +
>  src/parser_bison.y | 24 ++++++++++++++++---
>  src/rule.c         | 59 ++++++++++++++++++++++++++++++++++++++++++----
>  src/scanner.l      |  2 ++
>  4 files changed, 79 insertions(+), 7 deletions(-)
> 
> diff --git a/include/rule.h b/include/rule.h
> index 909ff36..bed9c2a 100644
> --- a/include/rule.h
> +++ b/include/rule.h
> @@ -193,6 +193,7 @@ struct chain {
>  	struct list_head	rules;
>  };
>  
> +extern int chain_std_prio_lookup(const char *std_prio_name);
>  extern const char *chain_type_name_lookup(const char *name);
>  extern const char *chain_hookname_lookup(const char *name);
>  extern struct chain *chain_alloc(const char *name);
> 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;
> +			}
>  			;
>  
>  dev_spec		:	DEVICE	STRING		{ $$ = $2; }
> diff --git a/src/rule.c b/src/rule.c
> index 56b956a..11d11b1 100644
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -28,6 +28,7 @@
>  #include <netinet/ip.h>
>  #include <linux/netfilter.h>
>  #include <linux/netfilter_arp.h>
> +#include <linux/netfilter_ipv4.h>
>  
>  void handle_free(struct handle *h)
>  {
> @@ -769,6 +770,56 @@ const char *chain_policy2str(uint32_t policy)
>  	return "unknown";
>  }
>  
> +struct chain_prio_tag{
> +	int val;
> +	const char *str;
> +};
> +
> +const static struct chain_prio_tag chain_std_prios[] = {
> +	{ NF_IP_PRI_RAW,      "raw" },
> +	{ NF_IP_PRI_MANGLE,   "mangle" },
> +	{ NF_IP_PRI_NAT_DST,  "dnat" },
> +	{ NF_IP_PRI_FILTER,   "filter" },
> +	{ NF_IP_PRI_SECURITY, "security" },
> +	{ NF_IP_PRI_NAT_SRC,  "snat" },
> +};
> +
> +int chain_std_prio_lookup(const char *std_prio_name) {
> +	long unsigned int i;
> +
> +	for (i = 0; i < sizeof(chain_std_prios) / sizeof(struct chain_prio_tag); ++i) {

Add ARRAY_SIZE() and use it, eg.

#define ARRAY_SIZE(a) (sizeof(a)/sizeof((a)[0]))

So this looks like:

        for (i = 0; i < ARRAY_SIZE(chain_std_prios; ++i) {

> +		if (!strcmp(chain_std_prios[i].str, std_prio_name))
> +			return chain_std_prios[i].val;
> +	}
> +	return NF_IP_PRI_LAST;

Probably return -1 instead?

> +}
> +
> +static const char *chain_prio2str(int prio)
> +{
> +	static char ret[256];

Can we avoid this 'static char', better pass a buffer to this function
as parameter. Return 0 in case of matching, otherwise, return -1.

> +	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) {
> +		const int sdt_prio = chain_std_prios[i].val;
> +		const char *sdt_prio_str = chain_std_prios[i].str;

Better define sdt_prio at the very beginning of this function (BTW, I
guess you meant std instead of sdt).

> +		if (prio >= sdt_prio - reach && prio <= sdt_prio + reach) {
> +			int offset = prio - sdt_prio;

Same thing with the offset, define it at the beginning.

> +			if (offset != 0) {
> +				snprintf(offstr, sizeof(offstr), " %c %d",
> +					 offset > 0 ? '+' : '-', abs(offset));
> +			}
> +			sprintf(ret, "%s%s", sdt_prio_str, offstr);
> +			return ret;
> +		}
> +	}
> +	sprintf(ret, "%d", prio);
> +	return ret;
> +}
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phil Sutter June 21, 2018, 8:42 a.m. UTC | #2
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; }

[...]
> +	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.

[...]
> +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?

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Máté Eckl June 21, 2018, 9:03 a.m. UTC | #3
On Wed, Jun 20, 2018 at 02:09:32PM +0200, Pablo Neira Ayuso wrote:
> Hi!
> 
> This looks good, but some comments.
> 
> On Tue, Jun 19, 2018 at 11:50:24AM +0200, Máté Eckl wrote:
> > v3:
> >  - no tokens are used for priority names, lookup is used instead
> >  - names and values are moved out to a structure
> >  - the helper function became unnecessary, thus I removed it
> > 
> > -- 8< --
> > This patch adds the possibility to use textual names to set the chain priority
> > to basic values so that numeric values do not need to be learnt any more for
> > basic usage.
> > 
> > Basic arithmetic can also be done with them to ease the addition of
> > relatively higher/lower priority chains.
> > Addition and substraction is possible.
> > 
> > Values are also printed with their friendly name within the range of
> > <basicprio> +- 10.
> > 
> > Example:
> > 	nft> add table x
> > 	nft> add chain x y { type filter hook prerouting priority mangleprio; }
> 
> The command uses "mangleprio"
> 
> [...]
> > 	nft> list ruleset
> > 	table ip x {
> > 		chain y {
> > 			type filter hook prerouting priority mangle; policy accept;
> 
> listing uses "mangle"
> 
> I guess you only need to amend the commit message, it's a leftover,
> right?
> 
> We already agreed to use "mangle" for this, as the listing shows.

Yes sorry, it is a leftover indeed, I'll replace it.

> 
> > 		}
> > 
> > 		chain z {
> > 			type filter hook prerouting priority mangle + 1; policy accept;
> > 		}
> > 
> > 		chain w {
> > 			type filter hook prerouting priority mangle - 5; policy accept;
> > 		}
> > 
> > 		chain r {
> > 			type filter hook prerouting priority filter + 10; policy accept;
> > 		}
> > 
> > 		chain t {
> > 			type filter hook prerouting priority raw; policy accept;
> > 		}
> > 
> > 		chain q {
> > 			type filter hook prerouting priority -289; policy accept;
> > 		}
> > 
> > 		chain h {
> > 			type filter hook prerouting priority 15; policy accept;
> > 		}
> > 	}
> > 
> > 	nft> add chain x h { type filter hook rerouting priority first; }
> > 	Error: priority name 'first' is invalid
> > 	add chain x h { type filter hook prerouting priority first; }
> > 	                                                     ^^^^^
> > 
> > Signed-off-by: Máté Eckl <ecklm94@gmail.com>
> > ---
> >  include/rule.h     |  1 +
> >  src/parser_bison.y | 24 ++++++++++++++++---
> >  src/rule.c         | 59 ++++++++++++++++++++++++++++++++++++++++++----
> >  src/scanner.l      |  2 ++
> >  4 files changed, 79 insertions(+), 7 deletions(-)
> > 
[...]
> > diff --git a/src/rule.c b/src/rule.c
> > index 56b956a..11d11b1 100644
> > --- a/src/rule.c
> > +++ b/src/rule.c
> > @@ -28,6 +28,7 @@
> >  #include <netinet/ip.h>
> >  #include <linux/netfilter.h>
> >  #include <linux/netfilter_arp.h>
> > +#include <linux/netfilter_ipv4.h>
> >  
> >  void handle_free(struct handle *h)
> >  {
> > @@ -769,6 +770,56 @@ const char *chain_policy2str(uint32_t policy)
> >  	return "unknown";
> >  }
> >  
> > +struct chain_prio_tag{
> > +	int val;
> > +	const char *str;
> > +};
> > +
> > +const static struct chain_prio_tag chain_std_prios[] = {
> > +	{ NF_IP_PRI_RAW,      "raw" },
> > +	{ NF_IP_PRI_MANGLE,   "mangle" },
> > +	{ NF_IP_PRI_NAT_DST,  "dnat" },
> > +	{ NF_IP_PRI_FILTER,   "filter" },
> > +	{ NF_IP_PRI_SECURITY, "security" },
> > +	{ NF_IP_PRI_NAT_SRC,  "snat" },
> > +};

I realised that I'll have to use dstnat and srcnat, as dnat and snat would
interfere with existing tokens.

> > +
> > +int chain_std_prio_lookup(const char *std_prio_name) {
> > +	long unsigned int i;
> > +
> > +	for (i = 0; i < sizeof(chain_std_prios) / sizeof(struct chain_prio_tag); ++i) {
> 
> Add ARRAY_SIZE() and use it, eg.
> 
> #define ARRAY_SIZE(a) (sizeof(a)/sizeof((a)[0]))
> 
> So this looks like:
> 
>         for (i = 0; i < ARRAY_SIZE(chain_std_prios; ++i) {

I found a definition in lowercase, so I'll use that, but thanks for the idea.

> 
> > +		if (!strcmp(chain_std_prios[i].str, std_prio_name))
> > +			return chain_std_prios[i].val;
> > +	}
> > +	return NF_IP_PRI_LAST;
> 
> Probably return -1 instead?

Ok.

> 
> > +}
> > +
> > +static const char *chain_prio2str(int prio)
> > +{
> > +	static char ret[256];
> 
> Can we avoid this 'static char', better pass a buffer to this function
> as parameter. Return 0 in case of matching, otherwise, return -1.

How about using a buffer and still returning the pointer in prio2str? Then it
could still be used as a parameter of a print function, and the static variable
is also elimineted. There is no time, when conversion cannot be made, if there
is no corresponding name, the numerical value can still be printed into the
buffer.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Máté Eckl June 21, 2018, 9:26 a.m. UTC | #4
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phil Sutter June 21, 2018, 11:01 a.m. UTC | #5
Hi,

On Thu, Jun 21, 2018 at 11:26:37AM +0200, Máté Eckl wrote:
> 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.

Ah, you're right. I somehow missed the case of positive numerical
priority. Considering this, I see no easier way than yours.

> 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?

No idea, sorry.

Cheers, Phil
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso June 21, 2018, 11:42 a.m. UTC | #6
On Thu, Jun 21, 2018 at 01:01:31PM +0200, Phil Sutter wrote:
[...]
> On Thu, Jun 21, 2018 at 11:26:37AM +0200, Máté Eckl wrote:
> > 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?

Only the filter priority you can apply to the flowtable_block.

Note that standard priorities may depend on family, so you may need to
do the chain_std_prio_lookup() from the evaluation phase, instead of
doing it from the parser.

Telling this only filter applies to arp, bridge and netdev families
IIRC.

Have a look and let us know.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Máté Eckl July 3, 2018, 7:42 a.m. UTC | #7
On Thu, Jun 21, 2018 at 01:42:14PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Jun 21, 2018 at 01:01:31PM +0200, Phil Sutter wrote:
> [...]
> > On Thu, Jun 21, 2018 at 11:26:37AM +0200, Máté Eckl wrote:
> > > 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?
> 
> Only the filter priority you can apply to the flowtable_block.
> 
> Note that standard priorities may depend on family, so you may need to
> do the chain_std_prio_lookup() from the evaluation phase, instead of
> doing it from the parser.
> 
> Telling this only filter applies to arp, bridge and netdev families
> IIRC.
> 
> Have a look and let us know

I found no evaluation for any families so I could do
	nft add chain arp x y { type filter hook input priority -300;}
without any problem, which would be translated to 'raw'.

So I shouldn't have the possibility to add this priority? In this case, I think
the evaluation of the numerical value should be refined, but putting the
text->number translation into the evaluation phase does not seem approppriate to
me.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso July 3, 2018, 9:37 a.m. UTC | #8
On Tue, Jul 03, 2018 at 09:42:47AM +0200, Máté Eckl wrote:
> On Thu, Jun 21, 2018 at 01:42:14PM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Jun 21, 2018 at 01:01:31PM +0200, Phil Sutter wrote:
> > [...]
> > > On Thu, Jun 21, 2018 at 11:26:37AM +0200, Máté Eckl wrote:
> > > > 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?
> > 
> > Only the filter priority you can apply to the flowtable_block.
> > 
> > Note that standard priorities may depend on family, so you may need to
> > do the chain_std_prio_lookup() from the evaluation phase, instead of
> > doing it from the parser.
> > 
> > Telling this only filter applies to arp, bridge and netdev families
> > IIRC.
> > 
> > Have a look and let us know
> 
> I found no evaluation for any families so I could do
> 	nft add chain arp x y { type filter hook input priority -300;}
> without any problem, which would be translated to 'raw'.

Better use 'filter' for this in arp, we never had a 'raw' table.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Máté Eckl July 5, 2018, 3:14 p.m. UTC | #9
On Thu, Jun 21, 2018 at 01:42:14PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Jun 21, 2018 at 01:01:31PM +0200, Phil Sutter wrote:
> [...]
> > On Thu, Jun 21, 2018 at 11:26:37AM +0200, Máté Eckl wrote:
> > > 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?
> 
> Only the filter priority you can apply to the flowtable_block.

Is there a man page you could recommend to read more about flowtables? Maybe one
of an older tool? I haven't find much about this.

> Note that standard priorities may depend on family, so you may need to
> do the chain_std_prio_lookup() from the evaluation phase, instead of
> doing it from the parser.
> 
> Telling this only filter applies to arp, bridge and netdev families
> IIRC.
> 
> Have a look and let us know.

This is what I found:
	iptables
		filter
		nat (dstnat, srcnat)
		mangle
		raw
		security
		arptables
		filter
	ebtables
		filter
		nat (dstnat, srcnat)
		broute (no corresponding priority value)

I have an implementation to handle this, but I'd still like to do the
name->number translation outside the eval funcitons.
Is there any way to get the family of the context in the parser? I'd like to do
something like this:
	standard_prio	:	STRING
		{
			int tmp = chain_std_prio_lookup(something->family, $1);
			[...]
		}
		;

I tried chain family but it is not initialised at this point.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso July 5, 2018, 3:22 p.m. UTC | #10
On Thu, Jul 05, 2018 at 05:14:20PM +0200, Máté Eckl wrote:
> On Thu, Jun 21, 2018 at 01:42:14PM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Jun 21, 2018 at 01:01:31PM +0200, Phil Sutter wrote:
> > [...]
> > > On Thu, Jun 21, 2018 at 11:26:37AM +0200, Máté Eckl wrote:
> > > > 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?
> > 
> > Only the filter priority you can apply to the flowtable_block.
> 
> Is there a man page you could recommend to read more about flowtables? Maybe one
> of an older tool? I haven't find much about this.

man nft.

There is also: Documentation/networking/nf_flowtable.txt

> > Note that standard priorities may depend on family, so you may need to
> > do the chain_std_prio_lookup() from the evaluation phase, instead of
> > doing it from the parser.
> > 
> > Telling this only filter applies to arp, bridge and netdev families
> > IIRC.
> > 
> > Have a look and let us know.
> 
> This is what I found:
> 	iptables
> 		filter
> 		nat (dstnat, srcnat)
> 		mangle
> 		raw
> 		security
> 		arptables
> 		filter
> 	ebtables
> 		filter
> 		nat (dstnat, srcnat)
> 		broute (no corresponding priority value)
> 
> I have an implementation to handle this, but I'd still like to do the
> name->number translation outside the eval funcitons.

Why you willing to make your life so complicated? :-)

> Is there any way to get the family of the context in the parser? I'd like to do
> something like this:
> 	standard_prio	:	STRING
> 		{
> 			int tmp = chain_std_prio_lookup(something->family, $1);
> 			[...]
> 		}
> 		;
> 
> I tried chain family but it is not initialised at this point.

Problem with bison is that context may not even be there by when this
standard_prio rule runs.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Máté Eckl July 5, 2018, 3:56 p.m. UTC | #11
On Thu, Jul 05, 2018 at 05:22:23PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Jul 05, 2018 at 05:14:20PM +0200, Máté Eckl wrote:
> > On Thu, Jun 21, 2018 at 01:42:14PM +0200, Pablo Neira Ayuso wrote:
> > > On Thu, Jun 21, 2018 at 01:01:31PM +0200, Phil Sutter wrote:
> > > [...]
> > > > On Thu, Jun 21, 2018 at 11:26:37AM +0200, Máté Eckl wrote:
> > > > > 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?
> > > 
> > > Only the filter priority you can apply to the flowtable_block.
> > 
> > Is there a man page you could recommend to read more about flowtables? Maybe one
> > of an older tool? I haven't find much about this.
> 
> man nft.
> 
> There is also: Documentation/networking/nf_flowtable.txt

But these don't say anything about filter or anything.. I'd like to see if it
makes any sense here. It seems not to make any for now. How about leaving
flowtables alone with this change and only apply this for chains?

> > > Note that standard priorities may depend on family, so you may need to
> > > do the chain_std_prio_lookup() from the evaluation phase, instead of
> > > doing it from the parser.
> > > 
> > > Telling this only filter applies to arp, bridge and netdev families
> > > IIRC.
> > > 
> > > Have a look and let us know.
> > 
> > This is what I found:
> > 	iptables
> > 		filter
> > 		nat (dstnat, srcnat)
> > 		mangle
> > 		raw
> > 		security
> > 		arptables
> > 		filter
> > 	ebtables
> > 		filter
> > 		nat (dstnat, srcnat)
> > 		broute (no corresponding priority value)
> > 
> > I have an implementation to handle this, but I'd still like to do the
> > name->number translation outside the eval funcitons.
> 
> Why you willing to make your life so complicated? :-)

I will need to refactor the chain structure and initialisation, so it actually
seemed to be less complicatad so far.  But I guess I have no other options.

> > Is there any way to get the family of the context in the parser? I'd like to do
> > something like this:
> > 	standard_prio	:	STRING
> > 		{
> > 			int tmp = chain_std_prio_lookup(something->family, $1);
> > 			[...]
> > 		}
> > 		;
> > 
> > I tried chain family but it is not initialised at this point.
> 
> Problem with bison is that context may not even be there by when this
> standard_prio rule runs.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso July 5, 2018, 4:01 p.m. UTC | #12
On Thu, Jul 05, 2018 at 05:56:00PM +0200, Máté Eckl wrote:
> On Thu, Jul 05, 2018 at 05:22:23PM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Jul 05, 2018 at 05:14:20PM +0200, Máté Eckl wrote:
> > > On Thu, Jun 21, 2018 at 01:42:14PM +0200, Pablo Neira Ayuso wrote:
> > > > On Thu, Jun 21, 2018 at 01:01:31PM +0200, Phil Sutter wrote:
> > > > [...]
> > > > > On Thu, Jun 21, 2018 at 11:26:37AM +0200, Máté Eckl wrote:
> > > > > > 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?
> > > > 
> > > > Only the filter priority you can apply to the flowtable_block.
> > > 
> > > Is there a man page you could recommend to read more about flowtables? Maybe one
> > > of an older tool? I haven't find much about this.
> > 
> > man nft.
> > 
> > There is also: Documentation/networking/nf_flowtable.txt
> 
> But these don't say anything about filter or anything.. I'd like to see if it
> makes any sense here. It seems not to make any for now. How about leaving
> flowtables alone with this change and only apply this for chains?

flowtables can be only beplaced at ingress. And there is only 'filter'
chains there at this stage. So taking filter as 0 there is just fine.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso July 5, 2018, 4:02 p.m. UTC | #13
On Thu, Jul 05, 2018 at 06:01:59PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Jul 05, 2018 at 05:56:00PM +0200, Máté Eckl wrote:
> > On Thu, Jul 05, 2018 at 05:22:23PM +0200, Pablo Neira Ayuso wrote:
> > > On Thu, Jul 05, 2018 at 05:14:20PM +0200, Máté Eckl wrote:
> > > > On Thu, Jun 21, 2018 at 01:42:14PM +0200, Pablo Neira Ayuso wrote:
> > > > > On Thu, Jun 21, 2018 at 01:01:31PM +0200, Phil Sutter wrote:
> > > > > [...]
> > > > > > On Thu, Jun 21, 2018 at 11:26:37AM +0200, Máté Eckl wrote:
> > > > > > > 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?
> > > > > 
> > > > > Only the filter priority you can apply to the flowtable_block.
> > > > 
> > > > Is there a man page you could recommend to read more about flowtables? Maybe one
> > > > of an older tool? I haven't find much about this.
> > > 
> > > man nft.
> > > 
> > > There is also: Documentation/networking/nf_flowtable.txt
> > 
> > But these don't say anything about filter or anything.. I'd like to see if it
> > makes any sense here. It seems not to make any for now. How about leaving
> > flowtables alone with this change and only apply this for chains?
> 
> flowtables can be only beplaced at ingress. And there is only 'filter'
> chains there at this stage. So taking filter as 0 there is just fine.

To clarify flowtables are just like netdev/ingress chains. The
priority allows you to place a netdev/ingress filter chain before
your flowtable.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/include/rule.h b/include/rule.h
index 909ff36..bed9c2a 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -193,6 +193,7 @@  struct chain {
 	struct list_head	rules;
 };
 
+extern int chain_std_prio_lookup(const char *std_prio_name);
 extern const char *chain_type_name_lookup(const char *name);
 extern const char *chain_hookname_lookup(const char *name);
 extern struct chain *chain_alloc(const char *name);
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;
+			}
 			;
 
 dev_spec		:	DEVICE	STRING		{ $$ = $2; }
diff --git a/src/rule.c b/src/rule.c
index 56b956a..11d11b1 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -28,6 +28,7 @@ 
 #include <netinet/ip.h>
 #include <linux/netfilter.h>
 #include <linux/netfilter_arp.h>
+#include <linux/netfilter_ipv4.h>
 
 void handle_free(struct handle *h)
 {
@@ -769,6 +770,56 @@  const char *chain_policy2str(uint32_t policy)
 	return "unknown";
 }
 
+struct chain_prio_tag{
+	int val;
+	const char *str;
+};
+
+const static struct chain_prio_tag chain_std_prios[] = {
+	{ NF_IP_PRI_RAW,      "raw" },
+	{ NF_IP_PRI_MANGLE,   "mangle" },
+	{ NF_IP_PRI_NAT_DST,  "dnat" },
+	{ NF_IP_PRI_FILTER,   "filter" },
+	{ NF_IP_PRI_SECURITY, "security" },
+	{ NF_IP_PRI_NAT_SRC,  "snat" },
+};
+
+int chain_std_prio_lookup(const char *std_prio_name) {
+	long unsigned int i;
+
+	for (i = 0; i < sizeof(chain_std_prios) / sizeof(struct chain_prio_tag); ++i) {
+		if (!strcmp(chain_std_prios[i].str, std_prio_name))
+			return chain_std_prios[i].val;
+	}
+	return NF_IP_PRI_LAST;
+}
+
+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) {
+		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) {
+			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);
+			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));
 	}
 }
 
@@ -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));
 	}
 	if (octx->handle > 0)
 		nft_print(octx, " # handle %" PRIu64, chain->handle.handle.id);
diff --git a/src/scanner.l b/src/scanner.l
index ed01b5e..4fb3a39 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -232,6 +232,8 @@  addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "="			{ return '='; }
 "vmap"			{ return VMAP; }
 
+"+" 		{ return PLUS; }
+
 "include"		{ return INCLUDE; }
 "define"		{ return DEFINE; }
 "redefine"		{ return REDEFINE; }