[lwip-devel] [patch #6901] PPP callback with netif

[Iordan Neshev]

Follow-up Comment #2, patch #6901 (project lwip):

As far as I have understood, there two methods
to use status and link callbacks with PPP.

If someone knows this for sure, PLEASE tell me.
It's very important, since I'd like to improve the
code when I find some time.

1. The first is to declare separate status and link
callbacks, as Simon said, using:
 - netif_set_status_callback() and
 - netif_set_link_callback()

2. The second is to use one common callback, which 
is called through pc->linkStatusCB. 
This callback is set at the very beginning with pppOpen():

res = pppOpen(ppp_sio, pppLinkStatusCallback, NULL);
3rd argument is the context for this callback, when you need such. I don't
need it, so it is NULL for me.

Christian, obviously you have defined pppLinkStatusCallback:
void pppLinkStatusCallback(void *ctx, int errCode, void *arg);

The original code (in sifup() ) calls this:
pc->linkStatusCB(pc->linkStatusCtx, pc->errCode, &pc->addrs);
Note that this is the only place where this callback is called with 3rd
argument != NULL! This condition is enough for you to 
be aware of the address when the callback is called. You won't need
PPPERR_IFUP to be defined.

According to the patch, your proposal is:
pc->linkStatusCB(pc->linkStatusCtx, PPPERR_IFUP, &pc->netif);

I think that ((errCode == PPPERR_NONE) && (arg != NULL)) is
enough to understand that you have a valid address. Even just (arg != NULL)
is enough, but it's better to check also errCode==NONE, as the PPP code will
evolve (I hope).

One think that I like, is that you pass the netif, not only pc->addrs. It is
not bad idea. I guess such a change requires very little work to change also
pppLinkStatusCallback, so it can handle it properly.

Anyway, this is how my callback looks like (simplified):

void pppLinkStatusCallback(void *ctx, int errCode, void *arg)
{
        switch(errCode)
        {
                case PPPERR_NONE:                                               
/* No error. */
                {
                        struct ppp_addrs *ppp_addrs = arg;      // In all other 
cases arg is NULL. They
should check errCode.

                        PPPDEBUG((LOG_DEBUG, "nnpppLinkStatusCallback: 
PPPERR_NONEn"));
                        PPPDEBUG((LOG_DEBUG, "nn Terminal IP    : %sn", 
inet_ntoa(*(struct
in_addr*)&(ppp_addrs->our_ipaddr.addr))));
                    PPPDEBUG((LOG_DEBUG, " Access point IP: %sn", 
inet_ntoa(*(struct
in_addr*)&(ppp_addrs->his_ipaddr.addr))));
                        PPPDEBUG((LOG_DEBUG, " Subnet mask    : %sn", 
inet_ntoa(*(struct
in_addr*)&(ppp_addrs->netmask.addr))));
                        PPPDEBUG((LOG_DEBUG, " DNS Server1    : %sn", 
inet_ntoa(*(struct
in_addr*)&(ppp_addrs->dns1.addr))));
                        PPPDEBUG((LOG_DEBUG, " DNS Server2    : %sn", 
inet_ntoa(*(struct
in_addr*)&(ppp_addrs->dns2.addr))));
                }       break;

                case PPPERR_PARAM:              /* Invalid parameter. */
                case PPPERR_OPEN:               /* Unable to open PPP session. 
*/
        #if PPPOE_SUPPORT
                case PPPERR_DEVICE:             /* Invalid I/O device for PPP. 
*/
                #endif
                case PPPERR_ALLOC:              /* Unable to allocate 
resources. */
                case PPPERR_USER:               /* User interrupt. */
                case PPPERR_CONNECT:    /* Connection lost. */
                case PPPERR_AUTHFAIL:   /* Failed authentication challenge. */
                case PPPERR_PROTOCOL:   /* Failed to meet protocol. */
                default:
                        LWIP_ASSERT("All cases handled", 0);
                        break;
        }
}

=============================================

The problem with the first method when using PPP is
that  netif_set_status_callback() and netif_set_link_callback()
are not called! I guess someone has forgotten them, most 
probably because he does not use them, and uses the second method instead.

Anyway, I think the right place to call them is here:

/*
 * pppifNetifInit - netif init callback
 */
static err_t
pppifNetifInit(struct netif *netif)
{
  netif->name[0] = 'p';
  netif->name[1] = 'p';
  netif->output = pppifOutput;
  netif->mtu = pppMTU((int)netif->state);

  //  --> 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
  // <--
  return ERR_OK;
}

Can someone confirm this is OK?


Note: in my application pppnetif_status_callback() and
pppnetif_link_callback()are dummy, I defined them only to prove that it
works.

Can someone reopen this bug and schedule it for lwip 1.4?



    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?6901>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/