[ovs-dev] [PATCH v1] ipfix: support tunnel information for Flow IPFIX

[Ben Pfaff]

On Sun, Jun 05, 2016 at 09:56:01PM -0700, Ben Pfaff wrote:
> On Thu, Jun 02, 2016 at 06:34:53PM +0800, Benli Ye wrote:
> > Add support to export tunnel information for flow-based IPFIX.
> 
> Thanks for the patch!
> 
> I believe that this changes the NXAST_SAMPLE action in a
> backward-incompatible way.  We never do that, because it breaks any
> software that already uses the action.  Usually, if the need to change
> an action comes up, we add a new version of the action.  For example, in
> this case, you might add an NXAST_SAMPLE2 action that has the new
> behavior (if there is a better name than NXAST_SAMPLE2, please use it).
> 
> In dpif_ipfix_flow_sample(), I am not sure that it makes sense to
> exclude BFD packets.  For bridge-level sampling, the controller cannot
> otherwise exclude packets from sampling, but with flow-level sampling,
> the controller can exclude sampling any packets it likes, with
> appropriate use of flows.  Also, the switch processes BFD without
> packets going through the flow table anyway, so I'm not sure that
> this code will do anything.
> 
> Please write comments as full sentences that begin with a capital letter
> and end in a period, e.g. here in adjust_sample_action():
> +    /* set_tunnel action should be in front
> +     * of sample action which contains OVS_USERSPACE_ATTR_EGRESS_TUN_PORT
> +     * attribute */

A few more things based on compiler warnings.

dpif_ipfix_flow_sample() is annotated with OVS_EXCLUDED(mutex), meaning
that it must not be called holding 'mutex', but it contains two calls to
ovs_mutex_unlock() on 'mutex', so Clang says:

        ../ofproto/ofproto-dpif-ipfix.c:1798:9: error: releasing mutex 'mutex' that was not held [-Werror,-Wthread-safety-analysis]

"sparse" complains about mixing up types.  To avoid warnings, integer
types, ofp_port_t, and odp_port_t require conversions via u16_to_ofp(),
u32_to_odp(), and so on:

    ../ofproto/ofproto-dpif-xlate.c:2695:17: warning: restricted odp_port_t degrades to integer
    ../ofproto/ofproto-dpif-xlate.c:4242:30: warning: restricted ofp_port_t degrades to integer
    ../ofproto/ofproto-dpif-xlate.c:4245:56: warning: restricted ofp_port_t degrades to integer
    ../ofproto/ofproto-dpif-xlate.c:4249:36: warning: cast to restricted ofp_port_t

Thanks,

Ben.