Mon 08 Feb 2010 07:52:06 PM UTC, comment #9:
Fixed. Thanks for the input. I've internalized many functions and defines, too, in order to cleanly separate the usage. I'd be happy if you could have a look at the result, only to verify I didn't internalize too much...
|
Mon 08 Feb 2010 07:19:23 PM UTC, comment #8:
>What do you think, is using the supplied value OK or do we have to set the timer lower than that?
The RFC says:
"Each timer is set to a different random value, using the highest clock granularity available on the host, selected from the range (0, Max Response Time]"
(In order to spread responses and avoid duplicate answers when we have multiple devices)
As the next call to igmp_tmr() may actually be faster than 100ms, I think it's ok to use the supplied value.
Anyway, if it didn't receive any answer to a general query, an IGMP-enabled switch will actually send & retry group-specific queries with a fast timeout.
(We could also change the fixed timer and group-list parsing and use sys_timeout for each group)
|
Mon 08 Feb 2010 06:47:09 PM UTC, comment #7:
> For historic reasons, there was not LWIP_RAND() function, so it > was set arbitrarily to maxresp/2.
>
> When we changed to use random delays inside igmp_start_timer(),
> we forgot the change the call to that function.
Oh, I didn't know that this was meant as "random". I thought it was rather a question of safety to make sure the response arrives in time (due to latencies or timer jitter) - by using a smaller value than the one provided. Only I thought "half" was a bit too small.
What do you think, is using the supplied value OK or do we have to set the timer lower than that?
|
Mon 08 Feb 2010 06:31:38 PM UTC, comment #6:
>Could you please separate coding style functions from bugfixes from other improvements next time?
ok
>I changed the check in igmp_delaying_member() to compare igmp->timer to (maxresp/2), not maxresp, since we set that later. In general, why do we use maxresp/2 here, anyway?
For historic reasons, there was not LWIP_RAND() function, so it was set arbitrarily to maxresp/2.
When we changed to use random delays inside igmp_start_timer(),
we forgot the change the call to that function.
|
Mon 08 Feb 2010 06:14:38 PM UTC, comment #5:
I've fixed the actual bug but didn't yet check in the stats changes.
Stéphane, I changed the check in igmp_delaying_member() to compare igmp->timer to (maxresp/2), not maxresp, since we set that later. In general, why do we use maxresp/2 here, anyway?
|
Mon 08 Feb 2010 05:46:20 PM UTC, comment #4:
Could you please separate coding style functions from bugfixes from other improvements next time? It's more time consuming to analyse what you changed and why if you send one big patch with all your changes!
|
Thu 04 Feb 2010 06:24:57 PM UTC, comment #3:
Here are patches against the current CVS head for:
- max response time bug
- random=0 bug
- statistics cleaning & improvement
(file #19620, file #19621, file #19622, file #19623)
|
Thu 04 Feb 2010 10:57:45 AM UTC, comment #2:
Thanks for reporting and suggesting a fix.
Could you make a patch with the necessary changes, or say which version of icmp.c (e.g. lwIP release number, or CVS version) you've modified? It's hard to look at your icmp.c and see what you've changed.
|
Wed 03 Feb 2010 04:46:08 PM UTC, comment #1:
Actually the comparison alone is wrong, because the timer can be inactive at 0, so we must also compare to 0.
Moreover, there is another bug since using random delays.
This can result in timer=0 which means OFF.
We must ensure timer>=1.
(or change to OFF = -1 and modify other functions accordingly)
I attached my igmp.c with the corrections.
(file #19609)
|
Wed 03 Feb 2010 10:32:08 AM UTC, original submission:
Hi,
I think I found a bug in IGMP input query processing.
My device does a leave, then join again on the same group.
The switch issues a group-specific query with 1s timeout.
Lwip reports ok after 0.5s.
The switch issues another group-specific query with 1s timeout.
Before lwip responds, the switch issues a general query with 10s time-out.
Then the switch issues other group-specific queries because there's no response of lwip, and the multicast stream is interrupted.
5s after the general query, lwip answers and the multicast stream is back.
What happens here is that the general query resets the
group timer to 5s.
And according to RFC 2236, it should not, because the new max response time is greater than the current group timer.
In paragraph 3:
"If a timer for the group is already running, it is reset to the random value only if the requested Max Response Time is less than the remaining value of the running timer"
The bug is the comparaison in function igmp_delaying_member(),
which should be "maxresp < group->timer" instead of "maxresp > group->timer".
|