lwIP - A Lightweight TCP/IP stack - Bugs: bug #20409, Add netif callback function for...
You are not allowed to post comments on this tracker with your current authentication level.
bug #20409: Add netif callback function for link change event
Submitter: | Jared Grubb <jgrubb> | ||
Submitted: | Mon 09 Jul 2007 04:20:04 PM UTC | ||
Category: | None | Severity: | 3 - Normal |
Item Group: | Feature Request | Status: | Fixed |
Privacy: | Public | Assigned to: | jgrubb |
Open/Closed: | Closed | Planned Release: | None |
lwIP version: | None |
Jump to the original submission
Mon 30 Jul 2007 10:12:47 PM UTC, comment #62: |
Jonathan Larmour <jifl> |
Fri 13 Jul 2007 10:34:46 PM UTC, comment #61: Ok, so, I close it |
Frédéric Bernon <fbernon> |
Fri 13 Jul 2007 10:08:09 PM UTC, comment #60:
|
Jared Grubb <jgrubb> |
Fri 13 Jul 2007 09:57:00 PM UTC, comment #59: As long as the way to call netif_set_link_up depend of driver design, the choice to use tcpip_callback or a lock is outside the problem. The best thing to do is perhaps to add a comment and/or documentation to remember that this function have to be called in a safe-thread way and to propose some solutions.
|
Frédéric Bernon <fbernon> |
Fri 13 Jul 2007 04:46:31 PM UTC, comment #58:
|
Simon Goldschmidt <goldsimon> |
Fri 13 Jul 2007 03:24:00 PM UTC, comment #57: Also... what about thread safety? Is calling the etharp_query in the netif_set_link_up a problem? |
Jared Grubb <jgrubb> |
Fri 13 Jul 2007 03:20:06 PM UTC, comment #56: I have looked at the commits. I would maybe suggest that we add in a NULL-check for netif_set_link_up/down functions, i.e.
|
Jared Grubb <jgrubb> |
Fri 13 Jul 2007 03:03:08 PM UTC, comment #55: Ok, I have check in some changes, hoping it will be good for everyone... Jared, if - except up/down semantics - it's good for you, tell me, I will close it...
|
Frédéric Bernon <fbernon> |
Fri 13 Jul 2007 01:22:41 PM UTC, comment #54: Yes. |
Kieran Mansley <kieranm> |
Fri 13 Jul 2007 01:18:10 PM UTC, comment #53: So, if I understand, we can check in the callback part, but we will decide of up/down semantics in a other task, right? |
Frédéric Bernon <fbernon> |
Fri 13 Jul 2007 11:22:30 AM UTC, comment #52: Apologies for not giving more input to this - the amount of time I've had spare for lwIP recently is laughable. It seems like you folks have managed to thrash something out between you all, but if there's something specific you'd like me to decide upon, please let me know.
|
Kieran Mansley <kieranm> |
Fri 13 Jul 2007 07:25:14 AM UTC, comment #51:
|
Frédéric Bernon <fbernon> |
Fri 13 Jul 2007 07:12:30 AM UTC, comment #50: OK, I've took the time to look over the 'patched' netif.c. Like Frédéric said: why call dhcp_stop in netif_set_down? It may be called by dhcp, so calling dhcp_stop might be wrong. Same thing for autoip?
|
Simon Goldschmidt <goldsimon> |
Fri 13 Jul 2007 06:53:21 AM UTC, comment #49: So Simon, are you agree with last Jared patch+changes from comment #44 (like this, the change is really minor, and people get the information only if they want)?
|
Frédéric Bernon <fbernon> |
Fri 13 Jul 2007 06:43:33 AM UTC, comment #48: Coming back to the original problem:
|
Simon Goldschmidt <goldsimon> |
Thu 12 Jul 2007 05:46:46 PM UTC, comment #47: ( side note: comment #46 by Frederic relates to Task #7068 dealing with documentation not to this bug... but, yeah, my mistake on that code. I'll fix it. )
|
Jared Grubb <jgrubb> |
Thu 12 Jul 2007 05:39:13 PM UTC, comment #46: netconn_delete(conn); should be call with newconn ? |
Frédéric Bernon <fbernon> |
Thu 12 Jul 2007 05:37:32 PM UTC, comment #45:
|
Frédéric Bernon <fbernon> |
Thu 12 Jul 2007 05:29:15 PM UTC, comment #44: Ok, so, about patch from comment #41:
|
Frédéric Bernon <fbernon> |
Thu 12 Jul 2007 05:27:02 PM UTC, comment #43:
Sorry for the confusion... My "fine" means "that works for me" So yes I agree :)
If we stick to the current UP/DOWN definition, then you are absolutely right. |
Jared Grubb <jgrubb> |
Thu 12 Jul 2007 05:08:45 PM UTC, comment #42: Lot of "fine", can we hope to be agree ?? ;)
|
Frédéric Bernon <fbernon> |
Thu 12 Jul 2007 05:03:22 PM UTC, comment #41: Here is the updated file for what def#1 of UP in comment #40 would look like. This has the changes proposed by Frederic in #38. (I added a DIFF too) (It still calls dhcp_stop() in netif_set_down(), but that can be hashed out after we agree on what UP/DOWN mean)
|
Jared Grubb <jgrubb> |
Thu 12 Jul 2007 04:36:12 PM UTC, comment #40:
|
Jared Grubb <jgrubb> |
Thu 12 Jul 2007 04:26:44 PM UTC, comment #39:
|
Frédéric Bernon <fbernon> |
Thu 12 Jul 2007 07:42:15 AM UTC, comment #38:
I think it's wrong, since down the netif doesn't have to stop the dhcp client by example. It's another reason to let the module do the job...
|
Frédéric Bernon <fbernon> |
Thu 12 Jul 2007 06:42:30 AM UTC, comment #37:
|
Simon Goldschmidt <goldsimon> |
Thu 12 Jul 2007 02:59:45 AM UTC, comment #36: (attachment for comment #35 proposal ... messed up first time) |
Jared Grubb <jgrubb> |
Thu 12 Jul 2007 02:58:00 AM UTC, comment #35: Based on my last comment, I changed netif.c to show what it would look like. It will have minimal impact on how things already work, while offering lots of optional new features. The changes to netif.c:
|
Jared Grubb <jgrubb> |
Thu 12 Jul 2007 01:55:24 AM UTC, comment #34:
|
Jared Grubb <jgrubb> |
Wed 11 Jul 2007 11:52:48 PM UTC, comment #33: Som points from comment #25 to change:
|
Frédéric Bernon <fbernon> |
Wed 11 Jul 2007 11:43:51 PM UTC, comment #32:
|
Frédéric Bernon <fbernon> |
Wed 11 Jul 2007 10:59:52 PM UTC, comment #31:
|
Jared Grubb <jgrubb> |
Wed 11 Jul 2007 10:53:29 PM UTC, comment #30:
|
Jared Grubb <jgrubb> |
Wed 11 Jul 2007 10:33:34 PM UTC, comment #29:
|
Frédéric Bernon <fbernon> |
Wed 11 Jul 2007 10:29:55 PM UTC, comment #28:
|
Frédéric Bernon <fbernon> |
Wed 11 Jul 2007 10:22:27 PM UTC, comment #27: Are DHCP, AUTOIP, and static IP the only three choices we support for IP acquisition? |
Jared Grubb <jgrubb> |
Wed 11 Jul 2007 08:57:44 PM UTC, comment #26: The reason for a flag LINK_WAS_DOWN is that if the link changes fast, a timer function might only see UP and UP again, not the DOWN in between. Thus seeing no change, it wouldn't do anything.
|
Simon Goldschmidt <goldsimon> |
Wed 11 Jul 2007 07:05:53 PM UTC, comment #25: So, to resume my last comments, I prefer (it's my point of view):
|
Frédéric Bernon <fbernon> |
Wed 11 Jul 2007 06:51:29 PM UTC, comment #24: Comment #21
|
Frédéric Bernon <fbernon> |
Wed 11 Jul 2007 06:38:45 PM UTC, comment #23: Comment #18:
|
Frédéric Bernon <fbernon> |
Wed 11 Jul 2007 06:27:43 PM UTC, comment #22:
|
Frédéric Bernon <fbernon> |
Wed 11 Jul 2007 06:25:44 PM UTC, comment #21:
|
Jared Grubb <jgrubb> |
Wed 11 Jul 2007 06:25:04 PM UTC, comment #20: Comment #15 seems good, but I don't like the idea to have to extend netif_add, since the field can be set inside the "low_level_init"... |
Frédéric Bernon <fbernon> |
Wed 11 Jul 2007 06:22:57 PM UTC, comment #19: About comment #16, direct access is not very "clean", I prefer netif_set_link_up()/_down(). But I prefer a message than a direct access to netif fields in the driver .
Yes
Not necessary, as long the driver error is forwarded...
|
Frédéric Bernon <fbernon> |
Wed 11 Jul 2007 05:41:22 PM UTC, comment #18: I still think that DHCP and AUTOIP could check the LINK_UP flag (compared to the current state) in their timer routines. For gratouitous ARP, this would also work:
|
Simon Goldschmidt <goldsimon> |
Wed 11 Jul 2007 04:42:24 PM UTC, comment #17:
This can all be marked off by #if..#endif if this is not mandatory. (I use "state" instead of "link" since we are handling UP<->DOWN and LINK<->NOLINK. Therefore netif_set_up/down calls it for UP/DOWN and driver calls it for LINK/NOLINK)
|
Jared Grubb <jgrubb> |
Wed 11 Jul 2007 07:23:36 AM UTC, comment #16:
|
Simon Goldschmidt <goldsimon> |
Tue 10 Jul 2007 11:19:55 PM UTC, comment #15: Alright, how does this sound:
|
Jared Grubb <jgrubb> |
Tue 10 Jul 2007 09:07:13 PM UTC, comment #14:
|
Frédéric Bernon <fbernon> |
Tue 10 Jul 2007 08:55:46 PM UTC, comment #13: Sounds good.
|
Jared Grubb <jgrubb> |
Tue 10 Jul 2007 08:40:44 PM UTC, comment #12: For the last four states you give, I'm agree with you. In cases where LINK DOWN, this is because the "link_output" will return an error that the "core" know there is a problem.
|
Frédéric Bernon <fbernon> |
Tue 10 Jul 2007 07:41:20 PM UTC, comment #11: As I think about it further, the stack would never need to know about STARTED and STOPPED (as I think you are arguing, Frederic). I want my driver to be startable and stoppable, but I can do that myself:
|
Jared Grubb <jgrubb> |
Tue 10 Jul 2007 07:20:05 PM UTC, comment #10:
|
Jared Grubb <jgrubb> |
Tue 10 Jul 2007 06:35:59 PM UTC, comment #9:
|
Frédéric Bernon <fbernon> |
Tue 10 Jul 2007 06:10:06 PM UTC, comment #8:
|
Jared Grubb <jgrubb> |
Tue 10 Jul 2007 05:08:07 PM UTC, comment #7:
To my point of view, yes for the first, but using tcpip_callback or locking the core (this callback is the one you propose), yes for the second one (this could be the current status_callback, but with perhaps some parameters to notify the kind of event), but I'm not sure for the third (due to the DHCP problem, which is the same for AUTOIP).
Not sure to understand...
|
Frédéric Bernon <fbernon> |
Tue 10 Jul 2007 04:48:52 PM UTC, comment #6:
|
Jared Grubb <jgrubb> |
Tue 10 Jul 2007 12:20:06 PM UTC, comment #5:
|
Frédéric Bernon <fbernon> |
Tue 10 Jul 2007 10:40:01 AM UTC, comment #4:
|
Simon Goldschmidt <goldsimon> |
Tue 10 Jul 2007 10:33:47 AM UTC, comment #3:
|
Frédéric Bernon <fbernon> |
Mon 09 Jul 2007 04:40:52 PM UTC, comment #2: status_callback is a call from netif TO the driver for the netif_set_up and *_down events.
|
Jared Grubb <jgrubb> |
Mon 09 Jul 2007 04:30:57 PM UTC, comment #1: I think we should "renew" any DHCP or AUTOIP IP Address to respect some IP mobility requirements...
|
Frédéric Bernon <fbernon> |
Mon 09 Jul 2007 04:20:04 PM UTC, original submission:
We should delay the gratutitous ARP until the link is actually UP, and regenerate it each time the link comes up new. Therefore, we can use a link change callback function to hold all the things that should be done when a new link is established or when a link is broken.
|
Jared Grubb <jgrubb> |
Depends on the following items: None found
Items that depend on this one: None found
There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.
Follow 9 latest changes.
Date | Changed by | Updated Field | Previous Value | => | Replaced by |
---|---|---|---|---|---|
2007-07-13 | fbernon | Status | None | Fixed | |
Assigned to | None | jgrubb | |||
Open/Closed | Open | Closed | |||
2007-07-13 | kieranm | Planned Release | 1.3.0 | ||
2007-07-12 | jgrubb | Attached File | - | Added netif_comment_40.c, #13307 | |
Attached File | - | Added netif_comment_40.diff.txt, #13308 | |||
2007-07-12 | jgrubb | Attached File | - | Added netif.c, #13292 | |
2007-07-12 | jgrubb | Attached File | - | Added netif.2.c, #13291 | |
2007-07-09 | jgrubb | Attached File | - | Added netif.c.diff, #13272 |
Powered by Savane 3.13-02a9.
Corresponding source code
Re comment #58: Why would netif->linkoutput be thread-safe. I'm sure it would only be expected to be called by one thread at once!
Also I know this was going to be punted to a different issue, but since I haven't seen that issue created, I'll say now: when Jared talked about the way "Linux" did it, in fact he's talking about all Unix, including BSD. And since BSD pretty much dictates what TCP/IP is, I think that's quite an important precedent in fact, arguably the most important.