On Wed, May 28, 2014 at 10:10:28PM -0300, Flavio Leitner wrote:
> This patch adds generic IGMP snooping library code
> that is used in follow-up patches.
>
> Signed-off-by: Flavio Leitner <f...@redhat.com>
Thanks for v2!
Some structures in mcast-learning.h have union members that look like
this:
union {
void *p;
ofp_port_t ofp_port;
} port OVS_GUARDED;
I guess this is because of the similar member in struct mac_entry.
I'm not really happy with that member there. It is there because
lib/learning-switch.c uses the 'ofp_port' member, whereas
learning-switch is in turn used only by tests/test-controller. I
don't expect test-controller to want to use mcast-learning. Unless
you have a specific reason to have the union in these places, I'd
rather just have a single void pointer without a nested union.
In mcast-learning.h, I would add to the comments on mcast_snooping the
structures referenced, because that makes the data structures easier
to understand, e.g.:
/* Contains struct mcast_mrouter_bundles, least recently used at the front,
* most recently used at the back. */
struct list lrus OVS_GUARDED;
/* Contains struct mcast_mrouter_bundles for ports connected to mcast
* routers, least recently used at the front, most recently used at the
* back. */
struct list mrouter_lru OVS_GUARDED;
/* Contains struct mcast_fport_bundles that list the ports to be flooded
* with multicast packets. */
struct list fport_list OVS_GUARDED;
In mcast_snooping_is_query(), please write this:
+ if (ntohs(igmp_type) == IGMP_HOST_MEMBERSHIP_QUERY) {
as:
+ if (igmp_type == htons(IGMP_HOST_MEMBERSHIP_QUERY)) {
because the compiler can easily optimize the constant expression on
the right side.
After reading through the code, I am confused about the meaning, if
any, of "mcast_ip"s with proto != ETH_TYPE_IP. These are treated as
special cases all over, but it doesn't look like they're useful. For
example, mcast_entry_match_group() will never report that a non-IP
mcast_ip is a match, so it looks like every time
mcast_snooping_add_group() is called with a non-IP mcast_ip, it will
add a new entry, but that entry will never get found. Also, all
non-IP mcast_ips hash to the same value (0). So I suspect that non-IP
just doesn't make sense.
I think that the "if (needs_update)" code in
mcast_snooping_add_group() could be moved inside the block that sets
needs_update, and then there wouldn't be a need for the "if" test.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
