On Wed, Sep 25, 2013 at 08:54:31AM -0700, Ben Pfaff wrote:
> On Tue, Sep 24, 2013 at 04:45:14PM -0700, Ethan Jackson wrote:
> > Both the IPFIX and SFLOW modules are thread safe, so there's no
> > particular reason to pass them up to the main thread. Eliminating
> > this step significantly simplifies the code.
> >
> > Signed-off-by: Ethan Jackson <et...@nicira.com>
>
> The comments on enum upcall_type are wrong, since none of the upcalls
> still require main thread involvement.
>
> The comments on udpif_dispatcher() and udpif_upcall_handler() are
> inaccurate now.
>
> Some comments on pre-existing code:
>
> * I spotted another misspelling of "receiving" (as "receving").
>
> * I think that the VLOG_WARN in recv_upcalls should be rate-limited.
> If it happens once it'll happen a lot.
>
> * Part of this code:
>
> if (type == OVS_KEY_ATTR_IN_PORT
> || type == OVS_KEY_ATTR_TCP
> || type == OVS_KEY_ATTR_UDP) {
> if (nl_attr_get_size(nla) == 4) {
> ovs_be32 attr = nl_attr_get_be32(nla);
> hash = mhash_add(hash, (OVS_FORCE uint32_t) attr);
>
> could be simplified:
>
> hash = mhash_add(hash, nl_attr_get_u32(nla));
>
> It's "incorrect" for TCP and UDP but the pre-existing code is
> "incorrect" in the same way for in_port.
>
> In ofproto/ofproto-dpif-xlate.h, I would put a blank line before the
> #endif ending the file. Also the space after " *" below is not our
> usual style:
> struct dpif_sflow * xlate_get_sflow(const struct ofproto_dpif *)
>
> Acked-by: Ben Pfaff <b...@nicira.com>
Also the compiler still complains, it looks like you initialized the
wrong variable to NULL:
../ofproto/ofproto-dpif-xlate.c:2565:9: error: variable 'ipfix' is used
uninitialized whenever 'if' condition is false
[-Werror,-Wsometimes-uninitialized]
if (xbridge) {
^~~~~~~
../ofproto/ofproto-dpif-xlate.c:2570:12: note: uninitialized use occurs here
return ipfix;
^~~~~
../ofproto/ofproto-dpif-xlate.c:2565:5: note: remove the 'if' if its condition
is always true
if (xbridge) {
^~~~~~~~~~~~~
../ofproto/ofproto-dpif-xlate.c:2561:29: note: initialize the variable 'ipfix'
to silence this warning
struct dpif_ipfix *ipfix;
^
= NULL
1 error generated.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
