Re: [nft PATCH 6/6] netlink: Call tunnel getters unconditionally

[Phil Sutter <phil@nwl.cc>]

Hi Pablo,

On Fri, Jun 26, 2026 at 01:53:27PM +0200, Pablo Neira Ayuso wrote:
> Series LGTM, only one nitpick, see below.

Thanks for your review!

[...]
> >  static struct expr *
> > -netlink_obj_tunnel_parse_addr(struct nftnl_obj *nlo, int attr)
> > +netlink_obj_tunnel_parse_addr(struct nftnl_obj *nlo, int attr, int alt_attr)
> 
> Maybe I would suggest:
> 
> netlink_obj_tunnel_parse_addr(struct nftnl_obj *nlo, int ipv4_attr, int ipv6_attr)
> 
> for easier reading.

Fine with me! I'll introduce 'int attr' auto-variable then, because the
code would otherwise conditionally do 'ipv4_attr = ipv6_attr' which is
more confusing than the original version.

> Just a nitpick, no need to send v2, just amend before pushing.
> 
> BTW, if you take my proposal, maybe this needs to be reversed:
> 
> > +			netlink_obj_tunnel_parse_addr(nlo,
> > +						      NFTNL_OBJ_TUNNEL_IPV6_SRC,
> > +						      NFTNL_OBJ_TUNNEL_IPV4_SRC);
> 
> so it is:
> 
> > +			netlink_obj_tunnel_parse_addr(nlo, NFTNL_OBJ_TUNNEL_IPV4_SRC,
> > +                                                   NFTNL_OBJ_TUNNEL_IPV6_SRC);
> 
> Although you migh consider that ipv6 is more important, which also
> makes sense to me in such case, alternative is:

While we are currently transitioning to IPv6 and so in only about 10-20
years (my guess) everyone will use IPv6 by default, I merely wanted to
avoid a functional change:

[...]
> > -		if (nftnl_obj_is_set(nlo, NFTNL_OBJ_TUNNEL_IPV4_SRC)) {
> > -			obj->tunnel.src =
> > -				netlink_obj_tunnel_parse_addr(nlo, NFTNL_OBJ_TUNNEL_IPV4_SRC);
> > -		}
> > -		if (nftnl_obj_is_set(nlo, NFTNL_OBJ_TUNNEL_IPV4_DST)) {
> > -			obj->tunnel.dst =
> > -				netlink_obj_tunnel_parse_addr(nlo, NFTNL_OBJ_TUNNEL_IPV4_DST);
> > -		}
> > -		if (nftnl_obj_is_set(nlo, NFTNL_OBJ_TUNNEL_IPV6_SRC)) {
> > -			obj->tunnel.src =
> > -				netlink_obj_tunnel_parse_addr(nlo, NFTNL_OBJ_TUNNEL_IPV6_SRC);
> > -		}
> > -		if (nftnl_obj_is_set(nlo, NFTNL_OBJ_TUNNEL_IPV6_DST)) {
> > -			obj->tunnel.dst =
> > -				netlink_obj_tunnel_parse_addr(nlo, NFTNL_OBJ_TUNNEL_IPV6_DST);
> > -		}

See, the old code preferred the IPV6 variants in the odd case of both
attributes present. So that is why my patch checks the IPV6 attribute
first and only falls back to the IPV4 one if missing.

Cheers, Phil

> > +		obj->tunnel.id = nftnl_obj_get_u32(nlo, NFTNL_OBJ_TUNNEL_ID);
> > +		obj->tunnel.sport =
> > +			nftnl_obj_get_u16(nlo, NFTNL_OBJ_TUNNEL_SPORT);
> > +		obj->tunnel.dport =
> > +			nftnl_obj_get_u16(nlo, NFTNL_OBJ_TUNNEL_DPORT);
> > +		obj->tunnel.tos = nftnl_obj_get_u8(nlo, NFTNL_OBJ_TUNNEL_TOS);
> > +		obj->tunnel.ttl = nftnl_obj_get_u8(nlo, NFTNL_OBJ_TUNNEL_TTL);
> > +		obj->tunnel.src =
> > +			netlink_obj_tunnel_parse_addr(nlo,
> > +						      NFTNL_OBJ_TUNNEL_IPV6_SRC,
> > +						      NFTNL_OBJ_TUNNEL_IPV4_SRC);
> > +		obj->tunnel.dst =
> > +			netlink_obj_tunnel_parse_addr(nlo,
> > +						      NFTNL_OBJ_TUNNEL_IPV6_DST,
> > +						      NFTNL_OBJ_TUNNEL_IPV4_DST);
> >  		if (nftnl_obj_is_set(nlo, NFTNL_OBJ_TUNNEL_OPTS)) {
> >  			nftnl_obj_tunnel_opts_foreach(nlo, tunnel_parse_opt_cb, obj);
> >  		}
> > -- 
> > 2.54.0
> > 
> 
>