Hi Máté,
Thanks for working on this.
See comments below.
On Wed, Jun 06, 2018 at 09:33:56PM +0200, Máté Eckl wrote:
> v2:
> - more comprehensive names
> - expose basic priorities used by iptables
> - use arithmetics with new names (+-)
> - print friendly names with arithmetics with an epsilon of 10
>
> -- 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> list ruleset
> table ip x {
> chain y {
> type filter hook prerouting priority mangleprio; policy
> accept;
> }
>
> chain z {
> type filter hook prerouting priority mangleprio + 1;
> policy accept;
> }
>
> chain w {
> type filter hook prerouting priority mangleprio - 5;
> policy accept;
> }
>
> chain r {
> type filter hook prerouting priority filterprio + 10;
> policy accept;
> }
>
> chain t {
> type filter hook prerouting priority rawprio; policy
> accept;
> }
>
> chain q {
> type filter hook prerouting priority -289; policy
> accept;
> }
> }
>
> Signed-off-by: Máté Eckl <[email protected]>
> ---
> src/parser_bison.y | 27 ++++++++++++++++++++++++---
> src/rule.c | 46 ++++++++++++++++++++++++++++++++++++++++++----
> src/scanner.l | 9 +++++++++
> 3 files changed, 75 insertions(+), 7 deletions(-)
>
> diff --git a/src/parser_bison.y b/src/parser_bison.y
> index 33915ed..10b4f96 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"
> @@ -313,6 +316,13 @@ int nft_lex(void *, void *, void *);
> %token NEXTHDR "nexthdr"
> %token HOPLIMIT "hoplimit"
>
> +%token PRIO_RAW "rawprio"
> +%token PRIO_MANGLE "mangleprio"
> +%token PRIO_NAT_DST "dstnatprio"
> +%token PRIO_FILTER "filterprio"
> +%token PRIO_SECURITY "securityprio"
> +%token PRIO_NAT_SRC "srcnatprio"
I would use:
raw
mangle
dstnat
filter
security
srcnat
for the priority tags.
> %token ICMP6 "icmpv6"
> %token PPTR "param-problem"
> %token MAXDELAY "max-delay"
> @@ -520,7 +530,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 prio_base
>
> %type <string> dev_spec quota_unit
> %destructor { xfree($$); } dev_spec quota_unit
> @@ -1792,8 +1802,19 @@ 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; }
> + | prio_base
> + | prio_base PLUS NUM { $$ = $1 + $3; }
> + | prio_base DASH NUM { $$ = $1 - $3; }
> + ;
> +
> +prio_base : PRIO_RAW { $$ = NF_IP_PRI_RAW; }
> + | PRIO_MANGLE { $$ =
> NF_IP_PRI_MANGLE; }
> + | PRIO_NAT_DST { $$ =
> NF_IP_PRI_NAT_DST; }
> + | PRIO_FILTER { $$ =
> NF_IP_PRI_FILTER; }
> + | PRIO_SECURITY { $$ =
> NF_IP_PRI_SECURITY; }
> + | PRIO_NAT_SRC { $$ =
> NF_IP_PRI_NAT_SRC; }
> ;
>
> dev_spec : DEVICE STRING { $$ = $2; }
> diff --git a/src/rule.c b/src/rule.c
> index 3e8dea4..afb9510 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)
> {
> @@ -752,6 +753,43 @@ const char *chain_policy2str(uint32_t policy)
> return "unknown";
> }
>
> +static bool chain_prio2str_helper(char *dst, const int origo, const char*
> origostr,
> + const int prio, const int e)
Interesting code, this chain_prio2str_helper() function.
> +{
> + char t[15];
> +
> + t[0] = 0;
> + if (prio >= origo - e && prio <= origo + e) {
> + int tp = prio - origo;
'tp' is the offset, right?
> + if (tp != 0) {
> + sprintf(t, " %c %d",tp > 0 ? '+' : '-', abs(tp));
> + }
> + sprintf(dst, "%s%s", origostr, t);
> + return true;
> + }
> + return false;
'origo' is the standard priority, right? I would rename this to
standard_prio.
'e' looks like the reach for approximate matching, so I would rename
it to 'reach'. This is a fixed value, 10 in this patch, so better
define a constant with a meaningful name. Any reason for choosing 10,
BTW? Don't worry, I understand this may be just an arbitrary value you
just picked.
> +}
> +
> +static const char *chain_prio2str(int prio)
> +{
> + static char pe[256];
> + const int e = 10;
> +
> + pe[0] = 0;
> + if(chain_prio2str_helper(pe, NF_IP_PRI_RAW, "rawprio", prio, e) ||
> + chain_prio2str_helper(pe, NF_IP_PRI_MANGLE, "mangleprio", prio, e) ||
> + chain_prio2str_helper(pe, NF_IP_PRI_NAT_DST, "dstnatprio", prio, e)
> ||
> + chain_prio2str_helper(pe, NF_IP_PRI_FILTER, "filterprio", prio, e) ||
> + chain_prio2str_helper(pe, NF_IP_PRI_SECURITY, "securityprio", prio,
> e) ||
> + chain_prio2str_helper(pe, NF_IP_PRI_NAT_SRC, "srcnatprio", prio, e)
Probably place this in an array and loop over it.
> + )
> + return pe;
> + else {
> + sprintf(pe, "%d", prio);
> + return pe;
> + }
> +}
> +
> static void chain_print_declaration(const struct chain *chain,
> struct output_ctx *octx)
> {
> @@ -764,8 +802,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));
> }
> }
>
> @@ -789,9 +827,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 416bd27..887e80c 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; }
> @@ -427,6 +429,13 @@ addrstring ({macaddr}|{ip4addr}|{ip6addr})
> "nexthdr" { return NEXTHDR; }
> "hoplimit" { return HOPLIMIT; }
>
> +"rawprio" { return PRIO_RAW; }
> +"mangleprio" { return PRIO_MANGLE; }
> +"dstnatprio" { return PRIO_NAT_DST; }
> +"filterprio" { return PRIO_FILTER; }
> +"securityprio" { return PRIO_SECURITY; }
> +"srcnatprio" { return PRIO_NAT_SRC; }
Any way we can avoid defining tokens?
> "icmpv6" { return ICMP6; }
> "param-problem" { return PPTR; }
> "max-delay" { return MAXDELAY; }
> --
> ecklm
>
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
