thanks for your detailed comments and suggestions. pls see inline
On Mon, Aug 8, 2011 at 2:39 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Mon, Aug 08, 2011 at 01:20:52PM -0700, ss...@nicira.com wrote:
>> From: Sanjay Sane <ss...@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.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
