* Re: [Bridge] [PATCH net-next] bridge: Fix incorrect judgment of promisc
@ 2014-06-05 12:55 ` David Laight
0 siblings, 0 replies; 14+ messages in thread
From: David Laight @ 2014-06-05 12:55 UTC (permalink / raw)
To: 'Toshiaki Makita', David S . Miller, Stephen Hemminger
Cc: Vlad Yasevich, netdev, bridge
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next] bridge: Fix incorrect judgment of promisc
2014-06-05 12:55 ` [Bridge] " David Laight
@ 2014-06-05 13:05 ` Toshiaki Makita
-1 siblings, 0 replies; 14+ messages in thread
From: Toshiaki Makita @ 2014-06-05 13:05 UTC (permalink / raw)
To: David Laight, David S . Miller, Stephen Hemminger
Cc: Vlad Yasevich, netdev, bridge
(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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Bridge] [PATCH net-next] bridge: Fix incorrect judgment of promisc
@ 2014-06-05 13:05 ` Toshiaki Makita
0 siblings, 0 replies; 14+ messages in thread
From: Toshiaki Makita @ 2014-06-05 13:05 UTC (permalink / raw)
To: David Laight, David S . Miller, Stephen Hemminger
Cc: Vlad Yasevich, netdev, bridge
(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
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH net-next] bridge: Fix incorrect judgment of promisc
2014-06-05 13:05 ` [Bridge] " Toshiaki Makita
@ 2014-06-05 13:47 ` David Laight
-1 siblings, 0 replies; 14+ messages in thread
From: David Laight @ 2014-06-05 13:47 UTC (permalink / raw)
To: 'Toshiaki Makita', David S . Miller, Stephen Hemminger
Cc: Vlad Yasevich, netdev, bridge
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Bridge] [PATCH net-next] bridge: Fix incorrect judgment of promisc
@ 2014-06-05 13:47 ` David Laight
0 siblings, 0 replies; 14+ messages in thread
From: David Laight @ 2014-06-05 13:47 UTC (permalink / raw)
To: 'Toshiaki Makita', David S . Miller, Stephen Hemminger
Cc: Vlad Yasevich, netdev, bridge
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next] bridge: Fix incorrect judgment of promisc
2014-06-05 12:55 ` [Bridge] " David Laight
@ 2014-06-05 15:06 ` Vlad Yasevich
-1 siblings, 0 replies; 14+ messages in thread
From: Vlad Yasevich @ 2014-06-05 15:06 UTC (permalink / raw)
To: David Laight, 'Toshiaki Makita',
David S . Miller, Stephen Hemminger
Cc: Vlad Yasevich, netdev, bridge
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Bridge] [PATCH net-next] bridge: Fix incorrect judgment of promisc
@ 2014-06-05 15:06 ` Vlad Yasevich
0 siblings, 0 replies; 14+ messages in thread
From: Vlad Yasevich @ 2014-06-05 15:06 UTC (permalink / raw)
To: David Laight, 'Toshiaki Makita',
David S . Miller, Stephen Hemminger
Cc: Vlad Yasevich, netdev, bridge
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread