[ovs-dev] [PATCH] Allow passthrough of BPDU (control class) frames

[Sanjay Sane]

thanks for your detailed comments and suggestions. pls see inline

On Mon, Aug 8, 2011 at 2:39 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Mon, Aug 08, 2011 at 01:20:52PM -0700, ssane at nicira.com wrote:
>> From: Sanjay Sane <ssane at nicira.com>
>
> A From: line should only appear in the body of the email when you send
> out a patch whose author is someone else.  I guess that git-send-email
> or git-format-patch doesn't realize that you are the author.  Do you
> have sendemail.from set?

I had provided a --from option to git-send-email, due to which the
additional From line was being inserted. I'll delete this option
during the next patch revision.

>
> Overall this looks good.  I have many comments but most of them are
> minor points of style.
>
> Some of the lines in the commit message are too long.  Please wrap the
> commit message to about 70 to 75 columns.

will do.

>
> The commit message is very good.
>
> The ovs-vsctl-only tests that it lists are probably not too
> informative, because the database server is very well tested.

ok. will remove them from the revised patch


 The
> other tests that combine ovs-dpctl and ovs-vsctl are nice, though.
>
> I would consider adding a line to NEWS to mention this new feature.
>> when LACP is enabled, even if bpdu-passthrough is enabled
>> {it seems we always consume LACP frames, even if LACP is not enabled..}
>
> Is this a bug that we should plan to fix separately?  I think I know
> why it happens, if so.


Based on Ethan and your subsequent comments, I'll remove this note
from my patch, as this will be fixed soon.

>
> I'm not sure of the naming of "bpdu_passthru".  I don't think that we
> use the term "passthru" anywhere else in OVS.  Usually in OVS we speak
> of "admitting" or "accepting" or "forwarding" or (the opposite)
> "dropping" packets.  Nicer names might be "forward_bpdus" or (with the
> opposite meaning) "drop_bpdus".  Also I wonder whether "bpdus" is the
> correct term.  I believe that this setting covers all packets to
> reserved MAC addresses.  Are all of those properly called BPDUs?

in STP/switching world, there's something called bpdufilter. Even in
L2TP config, BPDU usually refers to these frames. So, the term "bpdu"
is well-known for these control class frames

I've changed the keyword to forward_bpdu (instead of "bpdus), does
that sound ok ?

>
> I believe that bpdu_passthru only takes the values "true" and "false",
> so if that is correct please declare change "uint8_t"s to "bool"
> throughout the patch, and use "true" and "false" in place of 1 and 0.

yep, I'll make this change.


>
> Also throughout the patch, we like comments to resemble sentences as
> much as possible within space limitations.  Please capitalize the
> first letter in comments and end comments with periods.

ok.

>
> I noticed one instance of "if(".  Please add a space: "if (".

ack.

>
> It looks like your editor inserted a hard tab below.  Would you mind
> using only spaces for indentation in OVS userspace code?

will do.

>
>> +    /* Drop frames for reserved multicast addresses
>> +     * only if enable-bpdu-passthrough option is absent */
>> +    if (eth_addr_is_reserved(flow->dl_dst) &&
>> +     !ofproto->up.bpdu_passthru) {
>>          return false;
>>      }
>
> I'd prefer to see ->bpdu_passthru_changed take only a single "bool"
> argument that is the new value and have the comment assure the
> implementor that it will only be called when the value actually
> changes.  Then the interface is simpler and the implementations can
> skip the test:

yeah. as of now, I dont have any reason to pass the old and new
values, so will do as you suggest.

>
>> +    /* Notifies change in configuration option of bpdu passthrough */
>> +    void (*bpdu_passthru_changed)(struct ofproto *ofproto,
>> +                                  uint8_t old_val, uint8_t new_val);
>>  };
>>
>>  extern const struct ofproto_class ofproto_dpif_class;
>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index f40f995..0b98438 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -322,6 +322,8 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
>>      ofproto->datapath_id = 0;
>>      ofproto_set_flow_eviction_threshold(ofproto,
>>                                          OFPROTO_FLOW_EVICTON_THRESHOLD_DEFAULT);
>> +    /* by default, disable bpdu passthrough */
>> +    ofproto_set_bpdu_passthru(ofproto,0);
>
> At this point in ofproto_create() the implementation has not been
> initialized (by calling ->construct()), so ofproto_set_bpdu_passthru()
> shouldn't be used, because it calls into the implementation.
>
> Instead, just set ofproto->bdpu_passthru to "false" directly.
> ->construct() can examine its initial value, if necessary
> (ofproto-dpif doesn't care about its initial value).
>

ack.

>> @@ -421,6 +423,21 @@ ofproto_set_flow_eviction_threshold(struct ofproto *ofproto, unsigned threshold)
>>      }
>>  }
>>
>> +/* sets the option to enable passthrough of BPDUs when NORMAL action
>> + * is invoked. */
>
> It's nice to be very explicit in at least one place about the intended
> meaning of a setting.  This is one good place.  I would add an
> explanation like: "If bdpu_passthru is true, the NORMAL action will
> forward frames with reserved (e.g. STP) destination Ethernet
> addresses.  If bpdu_passthru is false, the NORMAL action will drop
> these frames."
>
>> +void
>> +ofproto_set_bpdu_passthru(struct ofproto *ofproto, uint8_t bpdu_passthru)
>> +{
>> +    uint8_t old_passthru = ofproto->bpdu_passthru ;
>> +    ofproto->bpdu_passthru = bpdu_passthru;
>> +    if (old_passthru != ofproto->bpdu_passthru){
>> +        if (ofproto->ofproto_class->bpdu_passthru_changed){
>> +            ofproto->ofproto_class->bpdu_passthru_changed(
>> +                ofproto, old_passthru, ofproto->bpdu_passthru);
>> +        }
>> +    }
>> +}
>> +
>
> I think that the user documentation in vswitch.xml can be improved.
> Now it refers to the "NORMAL" action, but I think that most users will
> not know what this is.  A lot of what the documentation describes
> already applies only to either the "NORMAL" action does or to OVS when
> no controller is configured (which uses the normal action internally),
> so it would at least be consistent to not mention that at all.
>
> This is an obscure enough setting that it might be a good idea to give
> enough background to allow a user some idea of why one would choose
> one setting or another.  Do you think you could add a sentence or two
> to help with that?

Added more context to make this generally useful.

I'll be sending a new/revised patch soon.

thanks !
Sanjay

>
> Thanks,
>
> Ben.
>