[ovs-dev] [PATCH 2/2] mirroring: Allow learning to be disabled on a VLAN.

Jesse Gross jesse at nicira.com
Tue Nov 10 21:52:07 UTC 2009



Ben Pfaff wrote:
> Jesse Gross <jesse at nicira.com> writes:
>
>   
>> RSPAN does not work properly unless MAC learning for the VLAN is
>> disabled on all switches between the origin and monitoring point.
>> This allows learning to be disabled on a given VLAN so vSwitch can
>> acts as an intermediate switch.
>>     
>
> I find double negatives more difficult to understand than single
> ones, so could non_learning_vlan() be changed to
> is_learning_vlan()?  It makes sense to keep the array name the
> same, since most of the time it will be a null pointer.
>   

That's fine, it makes it a little more readable I guess.

> This could be implemented inside the mac_learning table itself,
> by adding mac_learning functions to enable or disable MAC
> learning on given VLANs.  Do you think that is a better way to do
> it?  It would push a small amount of complexity out of bridge.c,
> which is already too complex.
>
>   

Good idea, there is too much stuff in bridge.c.

>> @@ -1839,6 +1841,13 @@ is_bcast_arp_reply(const flow_t *flow)
>>              && eth_addr_is_broadcast(flow->dl_dst));
>>  }
>>  
>> +static bool
>> +non_learning_vlan(const struct bridge *br, uint16_t vlan)
>> +{
>> +    return br->non_learning_vlans &&
>> +           bitmap_is_set(br->non_learning_vlans, vlan);
>> +}
>> +
>>     
>
> CodingStyle calls for:
>     return (br->non_learning_vlans
>             && bitmap_is_set(br->non_learning_vlans, vlan));
>
>   

OK.

>> +    if (rspan_vlans == NULL
>> +        ? br->non_learning_vlans != NULL
>> +        : br->non_learning_vlans == NULL ||
>> +          !bitmap_equal(rspan_vlans, br->non_learning_vlans, 4096)) {
>>     
>
> CodingStyle calls for:
>
>     if (rspan_vlans == NULL
>         ? br->non_learning_vlans != NULL
>         : (br->non_learning_vlans == NULL
>            || !bitmap_equal(rspan_vlans, br->non_learning_vlans, 4096))) {
>
>   

OK.

I pushed it with the above changes.




More information about the dev mailing list