[ovs-dev] [PATCH] sFlow LAG and Tunnel export

Ben Pfaff blp at nicira.com
Tue Dec 31 00:08:22 UTC 2013


On Fri, Dec 20, 2013 at 03:45:37PM -0800, Neil McKee wrote:
> Sorry.  Grappling with a new email client.  Here it as an attachment, if
> that's OK.  Let me know if I need to rebase again.

This patch provokes one warning from GCC (and Clang):

    ../lib/odp-util.c: In function 'parse_odp_action':
    ../lib/odp-util.c:515:27: error: format '%lli' expects argument of type 'long long int *', but argument 3 has type 'uint32_t *' [-Werror=format]

(It looks like you want %"SCNi32" instead of %lli.)

This patch provokes several warnings from "sparse":

    ../ofproto/ofproto-dpif-sflow.c:156:31: warning: cast to restricted ofp_port_t
    ../ofproto/ofproto-dpif-sflow.c:405:44: warning: cast from restricted ofp_port_t
    ../ofproto/ofproto-dpif-sflow.c:431:68: warning: cast from restricted ofp_port_t
    ../tests/test-sflow.c:255:19: warning: incorrect type in assignment (different base types)
    ../tests/test-sflow.c:255:19:    expected unsigned int [unsigned] [usertype] all
    ../tests/test-sflow.c:255:19:    got restricted __be32
    ../tests/test-sflow.c:308:9: warning: incorrect type in assignment (different base types)
    ../tests/test-sflow.c:308:9:    expected unsigned int [unsigned] [usertype] src
    ../tests/test-sflow.c:308:9:    got restricted __be32
    ../tests/test-sflow.c:309:9: warning: incorrect type in assignment (different base types)
    ../tests/test-sflow.c:309:9:    expected unsigned int [unsigned] [usertype] dst
    ../tests/test-sflow.c:309:9:    got restricted __be32
    ../tests/test-sflow.c:310:45: warning: incorrect type in argument 1 (different base types)
    ../tests/test-sflow.c:310:45:    expected restricted __be32 [usertype] x
    ../tests/test-sflow.c:310:45:    got unsigned int [unsigned] [usertype] src
    ../tests/test-sflow.c:310:45: warning: incorrect type in argument 1 (different base types)
    ../tests/test-sflow.c:310:45:    expected restricted __be32 [usertype] x
    ../tests/test-sflow.c:310:45:    got unsigned int [unsigned] [usertype] src
    ../tests/test-sflow.c:310:45: warning: incorrect type in argument 1 (different base types)
    ../tests/test-sflow.c:310:45:    expected restricted __be32 [usertype] x
    ../tests/test-sflow.c:310:45:    got unsigned int [unsigned] [usertype] src
    ../tests/test-sflow.c:310:45: warning: incorrect type in argument 1 (different base types)
    ../tests/test-sflow.c:310:45:    expected restricted __be32 [usertype] x
    ../tests/test-sflow.c:310:45:    got unsigned int [unsigned] [usertype] src
    ../tests/test-sflow.c:311:45: warning: incorrect type in argument 1 (different base types)
    ../tests/test-sflow.c:311:45:    expected restricted __be32 [usertype] x
    ../tests/test-sflow.c:311:45:    got unsigned int [unsigned] [usertype] dst
    ../tests/test-sflow.c:311:45: warning: incorrect type in argument 1 (different base types)
    ../tests/test-sflow.c:311:45:    expected restricted __be32 [usertype] x
    ../tests/test-sflow.c:311:45:    got unsigned int [unsigned] [usertype] dst
    ../tests/test-sflow.c:311:45: warning: incorrect type in argument 1 (different base types)
    ../tests/test-sflow.c:311:45:    expected restricted __be32 [usertype] x
    ../tests/test-sflow.c:311:45:    got unsigned int [unsigned] [usertype] dst
    ../tests/test-sflow.c:311:45: warning: incorrect type in argument 1 (different base types)
    ../tests/test-sflow.c:311:45:    expected restricted __be32 [usertype] x
    ../tests/test-sflow.c:311:45:    got unsigned int [unsigned] [usertype] dst
    ../ofproto/ofproto-dpif.c:1495:49: warning: incorrect type in argument 2 (different base types)
    ../ofproto/ofproto-dpif.c:1495:49:    expected restricted ofp_port_t [usertype] ofp_port
    ../ofproto/ofproto-dpif.c:1495:49:    got restricted odp_port_t [usertype] odp_port
    ../ofproto/ofproto-dpif-sflow.c:623:33: error: undefined identifier 'IPPROTO_GRE'
    ../ofproto/ofproto-dpif-sflow.c:631:33: error: undefined identifier 'IPPROTO_ESP'

(The two just above mean that we need to add IPPROTO_* declarations to
include/sparse/netinet/in.h.)

Please limit lines to 79 characters.

Please put a space after the 'if' keyword.

I'm not sure why this adds an unnamed sub-struct to struct slave in
lacp.c.  I'd just put those members right in struct slave.

In parse_odp_action(), please use ovs_scanf() instead of sscanf(), as
the existing code does.

In a couple of places,
            } else {
instead of
	    }
	    else {

In union user_action_cookie, padding should not be necessary.  (I see
that there is an obsolete comment there; I'll fix it.)

Also in union user_action_cookie, the doubly nested union looks odd to
me.  Let's just use one level.  You can name the outer member
sflow_single and sflow_multi, or some other appropriate names.

In ofproto/ofproto-dpif-sflow.c, please write
        #include "byte-order.h"
instead of
        #include "lib/byte-order.h".
(I see that there's a bad example in that file; I'll fix it.)
Also, please keep the list of #includes in alphabetical order.

I'm not a fan of callbacks because of the surprising inversion of
control that they cause, and especially not of layered callbacks like
the one that this introduces.  Instead of calling back into
dpif_sflow_port_lookup_callback() from sflow_agent_get_counters(), I
think that it would be better to provide the necessary information in
the call to dpif_sflow_add_port().

I'm pretty nervous about all the code added at the end of
ofproto-dpif-sflow.c that checks netdev types based on strings, etc.
It seems brittle: no one is going to remember to update it.  It would
be better to integrate it into the netdev code by adding appropriate
netdev_*() functions for returning the information you need (such as
the IP protocol used for tunneling, the ports, the keys, and so on).
Did you consider that?

Thanks,

Ben.



More information about the dev mailing list