On Fri, Sep 28, 2012 at 12:54 PM, Ben Pfaff <b...@nicira.com> wrote: > 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 <je...@nicira.com> > > Is this gracefully extensible if later we need to support IPv6 tunnel > endpoints?
We'll need to add extra space for the longer addresses and update the tunnel code that cares, of course, but I don't think there is any other real work. > 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'. OK. > 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. On 64-bit machines that would still result in the compiler adding 4 bytes of padding since it's no long 8-byte aligned. However, this patch already makes sure that we memset the whole struct so we can take the benefit on 32-bit machines. > 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. There are no current plans for any of this to be visible to 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. Hmm, yes, I'm not sure why I wrote it that way. > 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)". It's fine to get rid of it. > match_format() doesn't print any of the new fields. I assume that's > intentional? They're not currently part of the match. > 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(). Yes, I guess that's cleaner. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
