| Message ID | CABKoBm0z4s5ZP9sgiaC__Nn52RHzg6RiemANKe3Q7h4AWAqR5w@mail.gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
> On Feb 15, 2017, at 11:51 AM, Andy Zhou <azhou@ovn.org> wrote: > > On Mon, Feb 13, 2017 at 10:52 PM, nickcooper-zhangtonghao > <nic@opencloud.tech <mailto:nic@opencloud.tech>> wrote: >> The balance-tcp mode requires the upstream switch to support 802.3ad >> with successful LACP negotiation. When bond ports are configured to >> balance-tcp mode without lacp or lacp is disabled, drop the traffic. >> >> Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech <mailto:nic@opencloud.tech>> > > I think the following change is equivalent and may be easier to > follow, what do you think? > > diff --git a/ofproto/bond.c b/ofproto/bond.c > index 2703bc9..4c2781d 100644 > --- a/ofproto/bond.c > +++ b/ofproto/bond.c > @@ -787,6 +787,9 @@ bond_check_admissibility(struct bond *bond, const > void *slave_, > goto out; > } break; We should add ‘break' here. If not, traffic(e.g lacp) will be dropped. > case LACP_DISABLED: > + if (bond->balance == BM_TCP) { > + goto out; > + } > break; > } Thanks for your review.
On Tue, Feb 14, 2017 at 11:45 PM, nickcooper-zhangtonghao <nic@opencloud.tech> wrote: > > On Feb 15, 2017, at 11:51 AM, Andy Zhou <azhou@ovn.org> wrote: > > On Mon, Feb 13, 2017 at 10:52 PM, nickcooper-zhangtonghao > <nic@opencloud.tech> wrote: > > The balance-tcp mode requires the upstream switch to support 802.3ad > with successful LACP negotiation. When bond ports are configured to > balance-tcp mode without lacp or lacp is disabled, drop the traffic. > > Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech> > > > I think the following change is equivalent and may be easier to > follow, what do you think? > > diff --git a/ofproto/bond.c b/ofproto/bond.c > index 2703bc9..4c2781d 100644 > --- a/ofproto/bond.c > +++ b/ofproto/bond.c > @@ -787,6 +787,9 @@ bond_check_admissibility(struct bond *bond, const > void *slave_, > goto out; > } > > break; > We should add ‘break' here. If not, traffic(e.g lacp) will be dropped. Good catch. Did not expect a fall through case there. > > case LACP_DISABLED: > + if (bond->balance == BM_TCP) { > + goto out; > + } > break; > } > > > Thanks for your review. > > Do you mind posting a V2? Note, OVS commit messages usually starts with the "area" of change. For example, "ofproto/bond", can be used for this patch. Thanks.
diff --git a/ofproto/bond.c b/ofproto/bond.c index 2703bc9..4c2781d 100644 --- a/ofproto/bond.c +++ b/ofproto/bond.c @@ -787,6 +787,9 @@ bond_check_admissibility(struct bond *bond, const void *slave_, goto out; } case LACP_DISABLED: + if (bond->balance == BM_TCP) { + goto out; + } break; }