[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[lwip-devel] [patch #6965] PPP improvements
From: |
Simon Goldschmidt |
Subject: |
[lwip-devel] [patch #6965] PPP improvements |
Date: |
Thu, 29 Oct 2009 18:04:34 +0000 |
User-agent: |
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; de; rv:1.9.1.4) Gecko/20091016 Firefox/3.5.4 |
URL:
<http://savannah.nongnu.org/patch/?6965>
Summary: PPP improvements
Project: lwIP - A Lightweight TCP/IP stack
Submitted by: goldsimon
Submitted on: Do 29 Okt 2009 18:04:33 GMT
Category: None
Priority: 5 - Normal
Status: None
Privacy: Public
Assigned to: None
Originator Email:
Open/Closed: Open
Discussion Lock: Any
Planned Release: None
_______________________________________________________
Details:
>From Iordan Neshev (lwip-devel, 29-10-2009):
\src\netif\ppp\ppp.c:
* line 169: Jabobsen should be JaCobsen
* line 312 and line 331: LWIP_UNUSED_ARG(pc); is missing
in pppLinkTerminated() and pppLinkDown() respectively.
This leads to compiler warning when PPPOE_SUPPORT is NOT enabled.
The proper place is under "#if PPPOS_SUPPORT" in both functions if
we insist to be accurate but on the top of the function (beween
PPPControl *pc = .... and the debug message looks better.
Place it wherever you prefer.
* line 394: I guess it's a good idea to add a note above line 394
/* Note: replace both NULLs with valid (pointers to) strings if you
need to authorize with user name and password respectively */
pppSetAuth(PPPAUTHTYPE_NONE, NULL, NULL);
Somebody may find it useful. It took me some time to make sure that
this is
the right place to supply them and not somewhere else.
*line 525: add a note after lcp_init(pd); I propose something like
/* redundant, this is already done by ppp.c:pppInit() { ...
(*protp->init)(i);...} Leaved here for clarity*/
To be honest, I prefer not to remove the explicit initialization of
lcp here
because it looks logical to me and may save us if later someone
decides to change anything anywhere.
On the other hand, the note is too long and can be easily ignored
(sorry for the noise if you decide so)
*line 564: replace
if (pc->errCode) {
pd = pc->errCode;
} else {
pd = PPPERR_CONNECT;
}
with
pd = (pc->errCode ? pc->errCode : PPPERR_CONNECT); /* use pd as
return value */
* line 660: replace
if (pc->errCode) {
pd = pc->errCode;
} else {
pd = PPPERR_CONNECT;
}
with
pd = (pc->errCode ? pc->errCode : PPPERR_CONNECT);
The last 2 changes just make the source shorter, nothing more; ppp.c
too long and hard to read
*line 1325:
I think this is missing:
#if LWIP_NETIF_STATUS_CALLBACK
netif_set_status_callback(netif, pppnetif_status_callback);
#endif
#if LWIP_NETIF_LINK_CALLBACK
netif_set_link_callback(netif, pppnetif_link_callback);
#endif
IMHO it's better to leave it for 1.4. This is related to "patch
#6901: PPP callback with netif",
so I'll make a note there. Btw I'm not sure which callback should be
called first.
*line 1388: add a function call
pc->if_up = 0;
netif_set_down(&pc->netif); // <-- this line is missing
netif_remove(&pc->netif);
This needs discussion. Everybody who rely on the status and link
callbacks
should think about this. It works for me.
*line 1553:
....
out:
PPPDEBUG((LOG_DEBUG, "pppMain: unit %d: linkStatusCB=%lx
errCode=%d\n", pd, pc->linkStatusCB, pc->errCode));
if(pc->linkStatusCB) {
pc->linkStatusCB(pc->linkStatusCtx, pc->errCode ? pc->errCode :
PPPERR_PROTOCOL, NULL);
}
/// ----> this is missing
if (outpacket_buf[pd]) {
mem_free(outpacket_buf[pd]);
}
/// <----
pc->openFlag = 0;
} // (end of pppMain())
This needs serious discussion. I can *not* confirm that this works.
So far my investigation shows that we need to check one more
condition before calling
mem_free: the .used field of the mem struct must be != 0.
Unfortunately it is not
directly accessible through outpacket_buf[pd]. A dedicated function
(based on mem_free),
that returns the value of .used is needed.
Should I file a bug about this _after_ 1.3.2 is released?
\src\netif\ppp\ipcp.c:
*line 192: add "static" in front of
"char *_inet_ntoa(u32_t n)"
I can confirm that this works.
Otherwise we should consider using \core\ipv4\init.c:*inet_ntoa().
The second needs discussion (-> lwip 1.4)
\src\netif\ppp\fsm.c:
* line 556: change the debug message by adding "UNHANDLED"
FSMDEBUG((LOG_INFO, "%s: UNHANDLED Timeout event in state %d (%s)!\n",
I'm currently solving a problem and this one seems related to it.
The default case of the
corresponding switch is a potential source of bugs.
_______________________________________________________
Reply to this item at:
<http://savannah.nongnu.org/patch/?6965>
_______________________________________________
Nachricht geschickt von/durch Savannah
http://savannah.nongnu.org/
- [lwip-devel] [patch #6965] PPP improvements,
Simon Goldschmidt <=