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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
