patchlwIP - A Lightweight TCP/IP stack - Patches: patch #6965, PPP improvements

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #6965: PPP improvements

Submitter:  Simon Goldschmidt <goldsimon>
Submitted:  Thu 29 Oct 2009 06:04:33 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  goldsimon Open/Closed:  Closed
Planned Release:  1.4.0

Sun 06 Dec 2009 12:04:36 PM UTC, comment #4: 

Oh, I forgot to add: please reopen if I missed something (the list is very long, after all).

Simon Goldschmidt <goldsimon>
Group administrator
Sun 06 Dec 2009 12:03:29 PM UTC, comment #3: 

ppp.c:
lines 169, 312, 331: done

line 394: I'd rather add a configuration option for the pppSetAuth() parameters.

line 564, 660: For most compilers that results in exactly the same assembler code (in fact, that code isn't very thread-safe anyway and I'd like to remove it)

line 1388: done

ipcp.c line 192: removed that function, a correct define will do

fsm.c line 556: done

tcp_in.c: add state description: done

Since all other suggestions now have their own tracker entry, I'll close this as done now. Thanks for the input.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 30 Oct 2009 10:58:01 AM UTC, comment #2: 

Since the first post here (originally
sent to lwip-developers) is too long
and has many points to be discussed,
I decided to split it. I suggest to continue
to discuss what does not need much discussion.


\src\netif\ppp\ppp.c:
    * line 169: Jabobsen should be JaCobsen
    * line 312 and line 331: LWIP_UNUSED_ARG(pc) is missing
    * line 394: 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 and use the proper PPPAUTHTYPE_*/
   pppSetAuth(PPPAUTHTYPE_NONE, NULL, NULL);

    *line 525: add a note after lcp_init(pd);
   
    *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);


     *line 1325: this is now bug #27856

     *line 1388: add a call to netif_set_down() - not discussed
      here

     *line 1553: something tricky about ppp memory - this is
      bug #27079

\src\netif\ppp\ipcp.c:

  *line 192: add "static" in front of "char *_inet_ntoa(u32_t n)"

   * line 556: change the debug message by adding "UNHANDLED"
     FSMDEBUG((LOG_INFO, "%s: UNHANDLED Timeout event in state
     %d (%s)!\n",


    *line 337: TCP_EVENT_RECV(pcb, NULL, ERR_OK, err);
       This is now bug #27851

\src\core\tcp_in.c
    *lines 690, 707, 718, 728: Add description in the debug
    messages what the current state is.

    *lines 814, 820: some more spaces. Seems like it's solved
      now.

\src\core\mem.c

     *line 175: add LWIP_HEAP_RAM_SECTION
       discussed in patch #6822


And, as Simon suggested:
     * make the PPP code use the common lwIP header structs.


Iordan Neshev <iordan_neshev>
Thu 29 Oct 2009 06:06:20 PM UTC, comment #1: 

And one from me:

Since ported from ucos-Net (later ucIP), the PPP code brings its own protocol header definitions. We should delete the duplicate structs and make the PPP code use the common lwIP header structs.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 29 Oct 2009 06:04:33 PM UTC, original submission:  

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.

Simon Goldschmidt <goldsimon>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by iordan_neshev (Posted a comment)
  • -email is unavailable- added by kieranm (Updated the item)
  • -email is unavailable- added by goldsimon (Submitted the item)
  •  

    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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-12-06 goldsimon StatusNone Done
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2009-10-30 kieranm Planned ReleaseNone 1.4.0

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code