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 <[email protected]>
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.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
