[patch #6901] PPP callback with netif

[Evgenii Philippov]

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

                 Summary: PPP callback with netif
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: cwalter_at
            Submitted on: Tue 25 Aug 2009 09:56:35 PM 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:

After the PPP link has been negotiated it might be useful for some users to
get a callback with a pointer to the netif allocated by PPP.

In our case we use this for configuring the NAT and PORT forwarding tools
because the PPP ipaddress is not known a-priori.





    _______________________________________________________

File Attachments:


-------------------------------------------------------
Date: Tue 25 Aug 2009 09:56:35 PM GMT  Name: ppp-netifup.diff  Size: 1kB  
By: cwalter_at

<http://savannah.nongnu.org/patch/download.php?file_id=18641>

    _______________________________________________________

Reply to this item at:

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

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



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

[Evgenii Philippov]

Update of patch #6901 (project lwip):

                  Status:                    None => Wont Do                
             Assigned to:                    None => goldsimon              
             Open/Closed:                    Open => Closed                

    _______________________________________________________

Follow-up Comment #1:

I just checked and you can use LWIP_NETIF_STATUS_CALLBACK for that:

- define LWIP_NETIF_STATUS_CALLBACK to 1 in your lwipopts.h
- set the status callback function using netif_set_status_callback()
- once the ppp code calls netif_add()/netif_set_up(), your status callback
will be called and you can get the netif's IP address

    _______________________________________________________

Reply to this item at:

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

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



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

[Evgenii Philippov]

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/



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

[Evgenii Philippov]

Update of patch #6901 (project lwip):

                  Status:                 Wont Do => Need Info              
             Open/Closed:                  Closed => Open                  

    _______________________________________________________

Follow-up Comment #3:

It's great to see someone's actually using lwIP PPP :-)

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

For implementing the originally requested change or to fix setting the netif
status/link callbacks?

> [..]
> // --> 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?

I would think so, although (unfortunately) I can't confirm it as I'm not
totally familiar with the PPP code myself.

Re: "Some notes about lwip 1.3.2 RC1"
> *line 1388: add a function call
>
> pc->if_up = 0;
> netif_set_down(&pc->netif); // <-- this line is missing
> netif_remove(&pc->netif);

Looks OK, too (and also relevant to this bug/patch).

    _______________________________________________________

Reply to this item at:

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

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



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

[Evgenii Philippov]

Update of patch #6901 (project lwip):

         Planned Release:                    None => 1.4.0                  


    _______________________________________________________

Reply to this item at:

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

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



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

[Evgenii Philippov]

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


>> Can someone reopen this bug and schedule it for lwip 1.4?
>For implementing the originally requested change or to fix ?>setting the
netif status/link callbacks?

Both. I guess they should be filed as different patches.


    _______________________________________________________

Reply to this item at:

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

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



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

[Evgenii Philippov]

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

As the patch has been applied, we should open a bug to discuss any problems
or improvements, rather than re-open this patch tracker.

    _______________________________________________________

Reply to this item at:

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

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



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

[Evgenii Philippov]

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

> As the patch has been applied,

Has it? I marked it as "Won't Do" with comment #1. However, from comment #2,
it seems we don't need the original patch (file #18641:  ppp-netifup.diff; it
calls the status callback twice in a row!) but we only need to set the netif
link- and status-callbacks correctly. That is worth a new bug indeed.

-> bug #27856: PPP: Set netif link- and status-callback

    _______________________________________________________

Reply to this item at:

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

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



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

[Evgenii Philippov]

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

This patch should be closed again.
Sorry for requesting to be reopen.

> *line 1388: add a function call
>
> pc->if_up = 0;
> netif_set_down(&pc->netif); // <-- this line is missing
> netif_remove(&pc->netif);

This should be open as new bug (I'll do it),
while the discussion for the callbacks continues
in bug 27856.

    _______________________________________________________

Reply to this item at:

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

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



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

[Evgenii Philippov]

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

Sorry - I was clearly reading too fast and misunderstood.  

    _______________________________________________________

Reply to this item at:

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

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



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

[Evgenii Philippov]

Update of patch #6901 (project lwip):

                  Status:               Need Info => Works For Me          
             Open/Closed:                    Open => Closed                

    _______________________________________________________

Follow-up Comment #9:

> *line 1388: add a function call
>
> pc->if_up = 0;
> netif_set_down(&pc->netif); // <-- this line is missing
> netif_remove(&pc->netif);

This is covered in patch #6965: PPP improvements

    _______________________________________________________

Reply to this item at:

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

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



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

[Evgenii Philippov]

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

I now totally confused about where
is the right place to discuss
everything that I posted yesterday.

> Can someone reopen this bug and schedule it for lwip 1.4?
And this is the source of the trouble. Sorry.

First:
This particular thread started as a note from
Christian how the callback can be used to get the
IP when connection is established. Then I
wrote (and asked for confirmation), that
there are two approaches to use callbacks.

Second:
Meanwhile I noticed that the second method is
not fully implemented and can not be used.
This is something that I already wrote about
in patch #6965: PPP improvements.
Now this discussion is moved to
https://savannah.nongnu.org/bugs/?27856
I think this is the proper place.(Thanks, Simon).

Third:
> *line 1388: add a function call
> pc->if_up = 0;
> netif_set_down(&pc->netif); // <-- this line is missing
> netif_remove(&pc->netif);
>This is covered in patch #6965: PPP improvements

I think this is a harmless bug and needs to be
filed as a bug (just like the missing netif_set_xxx_callback).

Fourth:
If we continue this thread, we should discuss only
how the callbacks can be used and how they should
look like. Note that they are defined by the user
if he wants to.

    _______________________________________________________

Reply to this item at:

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

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



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