Re: [PATCH v2 nf-next 5/5] netfilter: nft: rt nexthop for inet family

["Anders K. Pedersen | Cohaesio" <akp@cohaesio.com>]

Hi Liping,

On tor, 2016-10-20 at 21:27 +0800, Liping Zhang wrote:
> Hi Anders,
> 
> 2016-10-20 20:36 GMT+08:00 Anders K. Pedersen | Cohaesio <akp@cohaesi
> o.com>:
> [...]
> > 
> > Well, the suggested userspace implementation doesn't allow a rule
> > that
> > would hit this warning. It is required to use 'ether type ip ... rt
> > ip
> > nexthop' or 'ether type ip6 ... rt ip6 nexthop', when nexthop is
> > used
> > from the inet table.
> > 
> > So I found it reasonable to generate a warning if such a rule
> > somehow
> > got configured anyway, but if you still feel that it should be
> > removed,
> > I can do that.
> 
> I think WARN_ON was used to hint that maybe there's a bug in kernel.
> It is not appropriate to warn the user's wrong configuration.

OK, so would it be okay to replace it with

printk_once(KERN_WARNING KBUILD_MODNAME " Address families do not match\n");

?

> > 
> > > 
> > > switch (priv->key) {
> > >         case NFT_RT_NEXTHOP:
> > >                 switch (pkt->pf)
> > >                 ...
> > >         default:
> > >                 return nft_rt_get_eval(expr, regs, pkt);
> > > }
> > 
> > My thinking was that for future rt sub-expressions, it will
> > probably
> > make sense to move
> > 
> >                 const struct rtable *rt = skb_rtable(skb);
> > 
> >                 if (!rt)
> >                         goto err;
> > 
> > outside the inner switch statement, so it doesn't need to be
> > duplicated
> > for each sub-expression. And ditto for the IPV6 part.
> > 
> 
> If so, for future rt sub-expressions extension, I think the following
> codes
> maybe better?
> 
> switch (pkt->pf)
>      case IPV4:
>            nft_rt_ipv4_get_eval();
>            break;
>      case IPV6:
>            nft_rt_ipv6_get_eval();
>            break;
> 
> This can reduce the duplicate codes. Just a suggestion:)

Then NFT_RT_INET would have to select NFT_RT_IPV4 and NFT_RT_IPV6, but
I guess that is okay, since NFT_RT_INET depends on NF_TABLES_INET,
which selects NF_TABLES_IPV4 and NF_TABLES_IPV6, so I'll use this
solution.

Regards,
Anders K. Pedersen