Re: [PATCH nft v3 0/9] bitwise shift support

[Jeremy Sowden <jeremy@azazel.net>]

[-- Attachment #1: Type: text/plain, Size: 2256 bytes --]

On 2020-02-01, at 12:32:23 +0000, Jeremy Sowden wrote:
> On 2020-01-28, at 20:09:45 +0100, Pablo Neira Ayuso wrote:
> > On Sun, Jan 19, 2020 at 10:57:01PM +0000, Jeremy Sowden wrote:
> > > The kernel supports bitwise shift operations.  This patch-set adds
> > > the support to nft.  There are a few preliminary housekeeping
> > > patches.
> >
> > Actually, this batch goes in the direction of adding the basic
> > lshift/right support.
> >
> > # nft --debug=netlink add rule x y tcp dport set tcp dport lshift 1
> > ip x y
> >   [ meta load l4proto => reg 1 ]
> >   [ cmp eq reg 1 0x00000006 ]
> >   [ payload load 2b @ transport header + 2 => reg 1 ]
> >   [ byteorder reg 1 = ntoh(reg 1, 2, 2) ]
> >   [ bitwise reg 1 = ( reg 1 << 0x00000001 ) ]
> >   [ payload write reg 1 => 2b @ transport header + 2 csum_type 1
> > csum_off 16 csum_flags 0x0 ]
> >
> > I'm applying patches 1, 2, 3, 4, 7 and 8.
> >
> > Regarding patch 5, it would be good to restore the parens when
> > listing.
>
> Will do.

This is already handled by the same code that does it for the other
parenthesized expressions (src/expression.c, ll. 600ff.):

  static void binop_arg_print(const struct expr *op, const struct expr *arg,
                              struct output_ctx *octx)
  {
          bool prec = false;

          if (arg->etype == EXPR_BINOP &&
              expr_binop_precedence[op->op] != 0 &&
              expr_binop_precedence[op->op] < expr_binop_precedence[arg->op])
                  prec = 1;

          if (prec)
                  nft_print(octx, "(");
          expr_print(arg, octx);
          if (prec)
                  nft_print(octx, ")");
  }

> > Patch 6, I guess it will break something else. Did you run tests/py
> > to check this?
>
> I did and I got the same results before and after applying it.  I'll
> take another look.

Evaluation of the shift expression inserts a byte-order conversion if
necessary to enforce host endianness, so by changing it we just avoid
the addition of the extra operation.  I've rewritten the commit message.

> > Patch 9, I'm skipping until 5 and 6 are sorted out.

I've tweaked the shell test-cases to include a parenthesized expression,
and added some matching Python ones.

I'll send a new version out soon.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]