[ovs-dev] [PATCH 04/18] lib: Add igmp generic snooping library bits

[Ben Pfaff]

On Fri, Apr 11, 2014 at 06:34:09PM -0300, Flavio Leitner wrote:
> This patch adds generic IPv4 library code to deal with
> igmp snooping that is implemented in follow-up patches.
> 
> Signed-off-by: Cong Wang <amwang at redhat.com>
> Signed-off-by: Daniel Borkmann <dborkman at redhat.com>
> Acked-by: Thomas Graf <tgraf at redhat.com>
> Signed-off-by: Flavio Leitner <fbl at redhat.com>

Clang says:

    In file included from ../lib/mcast-snooping.c:20:
    ../lib/mcast-snooping.h:142:20: error: use of undeclared identifier 'ml'
        OVS_REQ_WRLOCK(ml->rwlock);
                       ^
    ../lib/compiler.h:117:45: note: expanded from macro 'OVS_REQ_WRLOCK'
        __attribute__((exclusive_locks_required(__VA_ARGS__)))
                                                ^
    In file included from ../lib/mcast-snooping.c:20:
    ../lib/mcast-snooping.h:145:20: error: use of undeclared identifier 'ml'
        OVS_REQ_WRLOCK(ml->rwlock);
                       ^
    ../lib/compiler.h:117:45: note: expanded from macro 'OVS_REQ_WRLOCK'
        __attribute__((exclusive_locks_required(__VA_ARGS__)))

sparse says:

    ../lib/mcast-snooping.c:89:28: warning: restricted __be16 degrades to integer
    ../lib/mcast-snooping.c:90:34: warning: incorrect type in argument 1 (different base types)
    ../lib/mcast-snooping.c:90:34:    expected unsigned int [unsigned] [usertype] <noident>
    ../lib/mcast-snooping.c:90:34:    got restricted __be32 const [usertype] ip4
    ../lib/mcast-snooping.c:112:28: warning: restricted __be16 degrades to integer
    ../lib/mcast-snooping.c:137:15: warning: incorrect type in assignment (different base types)
    ../lib/mcast-snooping.c:137:15:    expected unsigned char [unsigned] [assigned] [usertype] proto
    ../lib/mcast-snooping.c:137:15:    got restricted __be16

It looks like a comment in mcast-snooping.h copied a spelling error in
mac-learning.h (s/hodling/holding/):
     * over or accessing a mcast_entry without hodling the parent

Indentation in mcast_snooping_is_membership() doesn't quite match the
OVS standard, the 'break' is spurious after the 'return', and the ';'
on the switch is odd.  So this:
    switch (ntohs(igmp_type)) {
        case IGMP_HOST_MEMBERSHIP_REPORT:
        case IGMPV2_HOST_MEMBERSHIP_REPORT:
        case IGMP_HOST_LEAVE_MESSAGE:
            return true;
            break;
    };
would ordinarily be written as:
    switch (ntohs(igmp_type)) {
    case IGMP_HOST_MEMBERSHIP_REPORT:
    case IGMPV2_HOST_MEMBERSHIP_REPORT:
    case IGMP_HOST_LEAVE_MESSAGE:
        return true;
    }

mcast_table_hash() compares the 8-bit 'proto' against a 16-bit htons()
return value.  (I guess sparse complained about that above, but I
noticed too.)  Ditto in mcast_entry_match_group().

In mcast_snooping_create() would you mind reordering the assignments
slightly to put them in the same order as the member list within the
struct definition?  It is easier to visually verify that every member
gets initialized that way.

OVS style has moved along slightly since you wrote this code, to use a
"struct ovs_refcount" instead of direct atomic ops, would you mind
switching this code over to that?  You can follow the model in
mac-learning.c.

The definition of mcast_snooping_set_idle_time(), not just the
prototype, should include the OVS_REQ_WRLOCK annotation, because Clang
isn't too good about remembering such things from the prototypes.

I don't think that the code in mcast_entry_insert_bundle() is really
safe because LIST_FOR_EACH leaves 'b' pointing to somewhere near the
list head (essentially garbage) if it terminates in the usual way (not
via break).  Same problem in mcast_snooping_add_mrouter().

We don't usually do assertions like the one at the beginning of
mcast_entry_has_bundles():
    ovs_assert(e != NULL);
    return !list_is_empty(&e->lru_bundles);
because the following list_is_empty() call will pretty effectively
also give a good indication of the failure ;-)

This:
bool mcast_snooping_add_group(struct mcast_snooping *ms,
                              const struct mcast_ip *grp, uint16_t vlan,
                              void *port)
should be:
bool
mcast_snooping_add_group(struct mcast_snooping *ms,
                         const struct mcast_ip *grp, uint16_t vlan,
                         void *port)

Similarly for mcast_snooping_leave_group().

We usually write a space after LIST_FOR_EACH, e.g.
    LIST_FOR_EACH (mrouter, lru_node, &ms->mrouter_lru) {

mcast_snooping_port_disable_flood() ends with a spurious 'return'.

mcast_snooping_wait__() begins with a spurious blank line.

How much of a common case do you expect multicast lookups to be?  Do
you think it will be worth trying to RCUify this, to avoid taking a
lock on the read path?