Hi Neil. I'm on vacation until next Monday, so I'll read your message in
detail and get back to you sometime next week. Sorry for the delay.
On Jan 3, 2014 7:39 PM, "Neil McKee" <neil.mc...@inmon.com> wrote:
>
> 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
