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