On Mon, Dec 30, 2013 at 4:08 PM, Ben Pfaff <b...@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.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
