| Message ID | 1401965851-6449-1-git-send-email-makita.toshiaki@lab.ntt.co.jp |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Toshiaki Makita > br_manage_promisc() incorrectly expects br_auto_port() to return only 0 > or 1, while it actually returns flags, i.e., a subset of BR_AUTO_MASK. > > Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> > --- > net/bridge/br_if.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c > index a08d2b8..6a07a40 100644 > --- a/net/bridge/br_if.c > +++ b/net/bridge/br_if.c > @@ -153,7 +153,7 @@ void br_manage_promisc(struct net_bridge *br) > * This lets us disable promiscuous mode and write > * this config to hw. > */ > - if (br->auto_cnt <= br_auto_port(p)) > + if (br->auto_cnt <= !!br_auto_port(p)) > br_port_clear_promisc(p); > else > br_port_set_promisc(p); Why not the less confusing: if (br->auto_cnt || br_auto_port(p)) and reverse the then/else lines? David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
(2014/06/05 20:03), David Laight wrote: > From: Toshiaki Makita >> br_manage_promisc() incorrectly expects br_auto_port() to return only 0 >> or 1, while it actually returns flags, i.e., a subset of BR_AUTO_MASK. >> >> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> >> --- >> net/bridge/br_if.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c >> index a08d2b8..6a07a40 100644 >> --- a/net/bridge/br_if.c >> +++ b/net/bridge/br_if.c >> @@ -153,7 +153,7 @@ void br_manage_promisc(struct net_bridge *br) >> * This lets us disable promiscuous mode and write >> * this config to hw. >> */ >> - if (br->auto_cnt <= br_auto_port(p)) >> + if (br->auto_cnt <= !!br_auto_port(p)) >> br_port_clear_promisc(p); >> else >> br_port_set_promisc(p); > > Why not the less confusing: > if (br->auto_cnt || br_auto_port(p)) > and reverse the then/else lines? I'm respecting the original style, but I'm not particular about this style. I'll make less confusing one, thanks :) (Your suggested condition is not exactly the same as current one, even if reversing if/else. v2 will be different than it. Anyway, thanks.) Toshiaki Makita -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Toshiaki Makita > (2014/06/05 20:03), David Laight wrote: > > From: Toshiaki Makita > >> br_manage_promisc() incorrectly expects br_auto_port() to return only 0 > >> or 1, while it actually returns flags, i.e., a subset of BR_AUTO_MASK. > >> > >> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> > >> --- > >> net/bridge/br_if.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c > >> index a08d2b8..6a07a40 100644 > >> --- a/net/bridge/br_if.c > >> +++ b/net/bridge/br_if.c > >> @@ -153,7 +153,7 @@ void br_manage_promisc(struct net_bridge *br) > >> * This lets us disable promiscuous mode and write > >> * this config to hw. > >> */ > >> - if (br->auto_cnt <= br_auto_port(p)) > >> + if (br->auto_cnt <= !!br_auto_port(p)) > >> br_port_clear_promisc(p); > >> else > >> br_port_set_promisc(p); > > > > Why not the less confusing: > > if (br->auto_cnt || br_auto_port(p)) > > and reverse the then/else lines? > > I'm respecting the original style, but I'm not particular about this style. > I'll make less confusing one, thanks :) > > (Your suggested condition is not exactly the same as current one, even > if reversing if/else. v2 will be different than it. Anyway, thanks.) A quick truth table: auto_cnt auto_port set/clear 0 0 clear 0 1 clear 1 0 set 1 1 clear 2+ 0/1 clear So you want: if (br->auto_cnt && !br_auto_port(p)) br_port_set_promisc(p); else br_port_clear_promisc(p); Does seem like a strange condition. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
(2014/06/05 21:55), David Laight wrote: > From: Toshiaki Makita >> (2014/06/05 20:03), David Laight wrote: >>> From: Toshiaki Makita >>>> br_manage_promisc() incorrectly expects br_auto_port() to return only 0 >>>> or 1, while it actually returns flags, i.e., a subset of BR_AUTO_MASK. >>>> >>>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> >>>> --- >>>> net/bridge/br_if.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c >>>> index a08d2b8..6a07a40 100644 >>>> --- a/net/bridge/br_if.c >>>> +++ b/net/bridge/br_if.c >>>> @@ -153,7 +153,7 @@ void br_manage_promisc(struct net_bridge *br) >>>> * This lets us disable promiscuous mode and write >>>> * this config to hw. >>>> */ >>>> - if (br->auto_cnt <= br_auto_port(p)) >>>> + if (br->auto_cnt <= !!br_auto_port(p)) >>>> br_port_clear_promisc(p); >>>> else >>>> br_port_set_promisc(p); >>> >>> Why not the less confusing: >>> if (br->auto_cnt || br_auto_port(p)) >>> and reverse the then/else lines? >> >> I'm respecting the original style, but I'm not particular about this style. >> I'll make less confusing one, thanks :) >> >> (Your suggested condition is not exactly the same as current one, even >> if reversing if/else. v2 will be different than it. Anyway, thanks.) > > A quick truth table: > auto_cnt auto_port set/clear > 0 0 clear > 0 1 clear > 1 0 set > 1 1 clear > 2+ 0/1 clear The last line should be set. Thanks, Toshiaki Makita > > So you want: > if (br->auto_cnt && !br_auto_port(p)) > br_port_set_promisc(p); > else > br_port_clear_promisc(p); > > Does seem like a strange condition. > > David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Toshiaki Makita > > A quick truth table: > > auto_cnt auto_port set/clear > > 0 0 clear > > 0 1 clear > > 1 0 set > > 1 1 clear > > 2+ 0/1 clear > > The last line should be set. I've clearly not drunk enough coffee today... I suspect the 0-1 line is impossible. Since the check is probably for 'any other ports in 'auto' mode'. So: if (auto_cnt - auto_port > 0) (which is much the same as the original) is a good descriptive test. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/05/2014 08:55 AM, David Laight wrote: > From: Toshiaki Makita >> (2014/06/05 20:03), David Laight wrote: >>> From: Toshiaki Makita >>>> br_manage_promisc() incorrectly expects br_auto_port() to return only 0 >>>> or 1, while it actually returns flags, i.e., a subset of BR_AUTO_MASK. >>>> >>>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> >>>> --- >>>> net/bridge/br_if.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c >>>> index a08d2b8..6a07a40 100644 >>>> --- a/net/bridge/br_if.c >>>> +++ b/net/bridge/br_if.c >>>> @@ -153,7 +153,7 @@ void br_manage_promisc(struct net_bridge *br) >>>> * This lets us disable promiscuous mode and write >>>> * this config to hw. >>>> */ >>>> - if (br->auto_cnt <= br_auto_port(p)) >>>> + if (br->auto_cnt <= !!br_auto_port(p)) >>>> br_port_clear_promisc(p); >>>> else >>>> br_port_set_promisc(p); >>> >>> Why not the less confusing: >>> if (br->auto_cnt || br_auto_port(p)) >>> and reverse the then/else lines? >> >> I'm respecting the original style, but I'm not particular about this style. >> I'll make less confusing one, thanks :) >> >> (Your suggested condition is not exactly the same as current one, even >> if reversing if/else. v2 will be different than it. Anyway, thanks.) > > A quick truth table: > auto_cnt auto_port set/clear > 0 0 clear > 0 1 clear Can't happen > 1 0 set Can't happen > 1 1 clear > 2+ 0/1 clear > > So you want: > if (br->auto_cnt && !br_auto_port(p)) > br_port_set_promisc(p); > else > br_port_clear_promisc(p); Some versions of the series that added this had an explicit check for count. Essentially, the expanded condition is this: if (count == 0) clear else if (count == 1 && auto_port(p)) clear else set The suggestion was that we could use a boolean (0|1) to check reduce the above to if (count <= auto_port(p)) clear else set Personally, I prefer the extended version since it is much clearer and is easy to understand. -vlad > > Does seem like a strange condition. > > David > > > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c index a08d2b8..6a07a40 100644 --- a/net/bridge/br_if.c +++ b/net/bridge/br_if.c @@ -153,7 +153,7 @@ void br_manage_promisc(struct net_bridge *br) * This lets us disable promiscuous mode and write * this config to hw. */ - if (br->auto_cnt <= br_auto_port(p)) + if (br->auto_cnt <= !!br_auto_port(p)) br_port_clear_promisc(p); else br_port_set_promisc(p);
br_manage_promisc() incorrectly expects br_auto_port() to return only 0 or 1, while it actually returns flags, i.e., a subset of BR_AUTO_MASK. Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> --- net/bridge/br_if.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)