Some notes about lwip 1.3.2 RC1

[Iordan Neshev]

Hello,
I'm very glad to see 1.3.2 RC1 out.

Here is a short list with notes:

 \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.

\src\core\tcp_in.c

        *line 337:
        TCP_EVENT_RECV(pcb, NULL, ERR_OK, err);
        A note may be added:
        /* the NULL argument causes compiler warning (unreachable code)
that can be safely ignored */
        I spent some time to find out why this happens. May be helpful
for somebody.
   
   
        tcp.in:tcp_timewait_input()
       
        *lines 690, 707, 718, 728: Add description in the debug messages
what the current state is:
            ("(FIN_WAIT_1)TCP connection closed %"U16_F" ->
%"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest));
            LWIP_DEBUGF(TCP_DEBUG, ("(FIN_WAIT_2)TCP connection closed
%"U16_F" -> %"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest));
            LWIP_DEBUGF(TCP_DEBUG, ("(FIN_WAIT_2)TCP connection closed
%"U16_F" -> %"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest));
            LWIP_DEBUGF(TCP_DEBUG, ("(CLOSING) TCP connection closed
%"U16_F" -> %"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest));
            LWIP_DEBUGF(TCP_DEBUG, ("(LAST_ACK) TCP connection closed
%"U16_F" -> %"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest));
           
        It's possible to use tcp_debug_state_str(), but this would make
the source less readable.
           
        *lines 814, 820: there are 12 spaces that should be removed.
They are invisible until the file is compared against
                           some older version.
       
\src\core\mem.c

    *line 175:    add LWIP_HEAP_RAM_SECTION to specify where the heap
should be placed
    static LWIP_HEAP_RAM_SECTION u8_t ram_heap[MEM_SIZE_ALIGNED +
(2*SIZEOF_STRUCT_MEM) + MEM_ALIGNMENT];
       
    then this should be added either in opt.h or a couple of lines upper
in mem.c:
   
    #ifndef LWIP_HEAP_RAM_SECTION
      #define LWIP_HEAP_RAM_SECTION
    #endif
   
    Then somewhere (eventually lwipopts.h) the user may add (as I did):
    #define LWIP_HEAP_RAM_SECTION        __attribute__ ((section
(".ethram")))
   
    Of course the linker has to know what .ethram is. That's haw I managed
    to utilize the RAM which is otherwise used by the integrated
Ethernet controller in
    LPC2368 and in my application would be lost. This change will not
affect in any way
    users that do not want to specify the memory location.
   
    Greetings,
    Iordan.
   
   


_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

Thanks for the ideas. I have added bug/patch entries on savannah for
most of these. The PPP-related ones will need someone who knows PPP
though, since I could make the modifications but couldn't test them.

Simon


Iordan Neshev wrote:

> Hello,
> I'm very glad to see 1.3.2 RC1 out.
>
> Here is a short list with notes:
>
>  \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.
>
> \src\core\tcp_in.c
>
>         *line 337:
>         TCP_EVENT_RECV(pcb, NULL, ERR_OK, err);
>         A note may be added:
>         /* the NULL argument causes compiler warning (unreachable code)
> that can be safely ignored */
>         I spent some time to find out why this happens. May be helpful
> for somebody.
>    
>    
>         tcp.in:tcp_timewait_input()
>        
>         *lines 690, 707, 718, 728: Add description in the debug messages
> what the current state is:
>             ("(FIN_WAIT_1)TCP connection closed %"U16_F" ->
> %"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest));
>             LWIP_DEBUGF(TCP_DEBUG, ("(FIN_WAIT_2)TCP connection closed
> %"U16_F" -> %"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest));
>             LWIP_DEBUGF(TCP_DEBUG, ("(FIN_WAIT_2)TCP connection closed
> %"U16_F" -> %"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest));
>             LWIP_DEBUGF(TCP_DEBUG, ("(CLOSING) TCP connection closed
> %"U16_F" -> %"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest));
>             LWIP_DEBUGF(TCP_DEBUG, ("(LAST_ACK) TCP connection closed
> %"U16_F" -> %"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest));
>            
>         It's possible to use tcp_debug_state_str(), but this would make
> the source less readable.
>            
>         *lines 814, 820: there are 12 spaces that should be removed.
> They are invisible until the file is compared against
>                            some older version.
>        
> \src\core\mem.c
>
>     *line 175:    add LWIP_HEAP_RAM_SECTION to specify where the heap
> should be placed
>     static LWIP_HEAP_RAM_SECTION u8_t ram_heap[MEM_SIZE_ALIGNED +
> (2*SIZEOF_STRUCT_MEM) + MEM_ALIGNMENT];
>        
>     then this should be added either in opt.h or a couple of lines upper
> in mem.c:
>    
>     #ifndef LWIP_HEAP_RAM_SECTION
>       #define LWIP_HEAP_RAM_SECTION
>     #endif
>    
>     Then somewhere (eventually lwipopts.h) the user may add (as I did):
>     #define LWIP_HEAP_RAM_SECTION        __attribute__ ((section
> (".ethram")))
>    
>     Of course the linker has to know what .ethram is. That's haw I managed
>     to utilize the RAM which is otherwise used by the integrated
> Ethernet controller in
>     LPC2368 and in my application would be lost. This change will not
> affect in any way
>     users that do not want to specify the memory location.
>    
>     Greetings,
>     Iordan.
>    
>    
>
>
> _______________________________________________
> lwip-devel mailing list
> lwip-devel@...
> http://lists.nongnu.org/mailman/listinfo/lwip-devel
>
>  

_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel