[ovs-dev] [RFC PATCH 1/5] flow: Extend struct flow to contain tunnel outer header.

[Ben Pfaff]

On Fri, Sep 28, 2012 at 11:12:21AM -0700, Jesse Gross wrote:
> Now that the kernel is supplying information about the outer IP
> header for tunneled packets userspace needs to be able to track it
> as part of the flow.  For the time being this is only used internally
> by OVS and not exposed outwards to OpenFlow.  As a result, this
> threads the information throughout userspace but simply stores the
> existing tun_id in it.
> 
> Signed-off-by: Jesse Gross <jesse at nicira.com>

Is this gracefully extensible if later we need to support IPv6 tunnel
endpoints?

It took me a while to figure out the purpose of the 'tnl' parameter to
flow_extract().  I think it would have been more obvious if the
parameter were marked 'const'.

If you can be satisfied with 16 bits of flags (only 3 flags exist so
far) then struct flow_tnl can be reduced by 4 bytes, including 2 bytes
of padding.

Should struct flow_metadata get a struct flow_tnl member?  I guess it
depends on whether we plan to expose anything other than tunnel ID via
OpenFlow.

format_tunnel_flags() is kind of oddly written.  It doesn't need to be
a loop if you dropped the "else"s and put "if (flags)" before the
final "else" clause.

The flow_format() output might be easier to read if it omitted all
tunnel information if there isn't any, instead of writing "tunnel(0)".

match_format() doesn't print any of the new fields.  I assume that's
intentional?

In process_packet_in(),
    memset(&tnl, 0, sizeof tnl);
    tnl.tun_id = pi.fmd.tun_id;

    flow_extract(&pkt, 0, &tnl, pi.fmd.in_port, &flow);
might more simply be written as:
    flow_extract(&pkt, 0, NULL, pi.fmd.in_port, &flow);
    flow.tunnel.tun_id = pi.fmd.tun_id;
and I do see it written that way in other places.
Similarly in ofproto_unixctl_trace().

Thanks,

Ben.