Re: [RFC PATCH 5/5] openvswitch: Interface with NAT.

[Thomas Graf <tgraf@suug.ch>]

On 10/21/15 at 02:04pm, Jarno Rajahalme wrote:
> 
> > On Oct 21, 2015, at 3:59 AM, Thomas Graf <tgraf@suug.ch> wrote:
> > Simplify this with an OVS_NAT_ATTR_FLAGS?
> 
> The use of separate flag attributes was actually intentional, as it makes the interface easier to understand, and code also easier to read.

OK, either is fine with me.

> >> +					  &ctinfo);
> >> +	if (!ct || nf_ct_is_untracked(ct))
> >> +		/* A NAT action may only be performed on tracked packets. */
> >> +		return NF_ACCEPT;
> > 
> > Braces
> > 
> 
> Needed due to the comment?

The compiler would be fine but most other places like this seem to
put braces on for this single comment + single statement case.

> >> +		if (type > OVS_NAT_ATTR_MAX) {
> >> +			OVS_NLERR(log, "Unknown nat attribute (type=%d, max=%d).\n",
> >> +			type, OVS_NAT_ATTR_MAX);
> > 
> > Formatting
> 
> Not readily apparent what you mean here, care to elaborate?

The argument list should be indented up to the (.

> >> +#ifdef CONFIG_NF_NAT_NEEDED
> >> +	[OVS_CT_ATTR_NAT]	= { .minlen = 0,
> >> +				    .maxlen = 96 }
> >> +#endif
> > 
> > Is the 96 a temporary hack here?
> > 
> 
> It is not an exact value. It is much better than my temporary hack of 512 was. As trailing garbage is checked for, I???m not sure if this should be very accurately calculated? Maybe it would be better to disable the length checks for this altogether?

I would just drop the maxlen then. The nested content should be
verified separately anyway later on.

> > We should probably bark if user space provides a OVS_CT_ATTR_NAT but the
> > kernel is compiled without support for it.
> > 
> 
> We do issue -EINVAL and log ???Unknown conntrack attr??? in that case.

Misread then, sorry.