On Mon, Aug 08, 2011 at 01:20:52PM -0700, [email protected] wrote:
> From: Sanjay Sane <[email protected]>
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?
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.
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. 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.
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?
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.
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.
I noticed one instance of "if(". Please add a space: "if (".
It looks like your editor inserted a hard tab below. Would you mind
using only spaces for indentation in OVS userspace code?
> + /* 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:
> + /* 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).
> @@ -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?
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
