On 10/21/15 at 02:04pm, Jarno Rajahalme wrote:
>
> > On Oct 21, 2015, at 3:59 AM, Thomas Graf <[email protected]> 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.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
