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

[Neil McKee]

On Mon, Dec 30, 2013 at 4:08 PM, Ben Pfaff <blp at nicira.com> wrote:

> 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.
>
>
OK to all that.


> 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 wasn't expecting you to say that.  The purpose of the callback is
really just to access the state held within ofproto-dpif,  but in an
encapsulated way so that the exposure is limited and controlled.
The alternatives were (1) to make those private structure fields
visible to the ofproto-dpif-sflow module so it could just read them
directly, or (2) to copy all the relevant state into a separate
hash-table just for ofproto-dpif-sflow to use when processing packet
samples. It sounds like you prefer (2)?   To understand why I think
I just need to know what you mean by "surprising inversion of control".
Are you referring to the possibility of mutex contention/deadlock
if the data-structures within ofproto-dpif are interrogated from
within the packet-processing path (i.e. when processing an
sFlow packet sample)?

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?
>
>
Agreed.  I just try to avoid making changes to files that don't match
*sflow*.  I confess I was half-expecting you to refactor all that
yourself,  but I'm happy to have a go at it once I understand the
callback issue above.

Neil



> Thanks,
>
> Ben.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140103/0d16525f/attachment-0003.html>