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