buglwIP - A Lightweight TCP/IP stack - Bugs: bug #21664, PPP code is broken

 
 

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

bug #21664: PPP code is broken

Submitter:  Frédéric Bernon <fbernon>
Submitted:  Wed 28 Nov 2007 07:23:23 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  Other

Jump to the original submission

Sat 02 May 2009 04:58:27 PM UTC, comment #19: 

Agree with you

Frédéric Bernon <fbernon>
Group Member
Sat 02 May 2009 04:14:17 PM UTC, comment #18: 

Frédéric, this is mainly to you: I think we can close this bug - for me, PPP at least compiles (unfortunately I cannot test it).

There may well be other bugs and I'd like PPP to be usable with NO_SYS=1 too (mainly to prevent the need for a separate thread, for NO_SYS=0, too); but I think these are different bugs.

Simon Goldschmidt <goldsimon>
Group administrator
Sat 02 May 2009 04:12:06 PM UTC, comment #17: 

May be a little late, but just my 2cents on comment #14 and the attached files:
- sio.h is already done
- ppp.h: why do we need ip.h here?
- ppp.c: SYS_THREAD_NULL is not defined, sys_thread_new is expected to never fail, so these changes are not needed right now.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 27 Feb 2008 03:25:26 PM UTC, comment #16: 

I made a mistake on the ppp.c changes.  I don't know what I was thinking in that there was sys_thread_remove() for lwIP along with sys_thread_new().  I am sorry about wasting your time with that.  As a side note for anyone using the PPP client, if you end a PPP session, the PPP thread must be removed.

Dustin Stahlback <dstahlback>
Tue 26 Feb 2008 09:30:56 PM UTC, comment #15: 

For ppp.c changes, I see you call sys_arch_thread_remove and sys_arch_thread_current, but these functions are not defined for lwIP. So, your patch can't be integrate like this...


Frédéric Bernon <fbernon>
Group Member
Tue 26 Feb 2008 08:44:16 PM UTC, comment #14: 

I have an interest in a proper working serial PPP client.  I have made changes to a couple of the PPP files to allow the client to work for me.  I would like to submit the changes for review.  Could you tell me how you would like patches submitted? I have attached the three files modified from the current working CVS as of 02/26/2008.

sio.h : wrap header file in a define to disallow multiple includes
ppp.h : include lwip/ip.h
ppp.c : check if sys_thread_new() failed and cleanup accordingly
      : fix calls to IN_CLASSA, IN_CLASSB to use htonl()
      : call sys_arch_thread_remove() when ppp session is done


Thanks,

Dustin

Dustin Stahlback <dstahlback>
Wed 19 Dec 2007 08:48:40 PM UTC, comment #13: 

Some changes for coding style.

Frédéric Bernon <fbernon>
Group Member
Sun 02 Dec 2007 08:30:44 PM UTC, comment #12: 


> So, without adding a temporary file, we could copy/paste the
> content of queue.h directly inside ppp_oe.c


That's the best solution for a temporary workaround, I think. Better than temporarily adding queue.h, which is alway visible in the CVS repository log.

Simon Goldschmidt <goldsimon>
Group administrator
Sun 02 Dec 2007 06:52:10 PM UTC, comment #11: 


>Maybe we can ask Marc Boucher (he implemented the ppp_oe file) to remove the need for queue.h?


I catched him on MSN some minutes, but his battery was low and I didn't have the time to ask him.

So, without adding a temporary file, we could copy/paste the content of queue.h directly inside ppp_oe.c, and when Marc or anyone else could find the time, we could remove this dependancy.

Once again, using current queue.h is only a temporary workaround.


Frédéric Bernon <fbernon>
Group Member
Sun 02 Dec 2007 06:21:18 PM UTC, comment #10: 

I agree to almost all points. But the queue.h file is only needed in the relatively new file ppp_oe.c, so it doesn't really seem necessary to test ppp only... Maybe we can ask Marc Boucher (he implemented the ppp_oe file) to remove the need for queue.h?

Simon Goldschmidt <goldsimon>
Group administrator
Sun 02 Dec 2007 05:22:26 PM UTC, comment #9: 

In a first step, I want to fix major build problems before really do more code changes. Adding this file is a temporary solution to let user use it.

Once the code can be build and executed, and tested, I'm agree to remove any dependancy to linux code. I also would like to use lwIP debug tools and not PPP specific ones. Perhaps also change PPP_ options to LWIP_PPP_ (or something like that). And last, find a solution to use PPP with NO_SYS=1.

Frédéric Bernon <fbernon>
Group Member
Sat 01 Dec 2007 01:13:29 PM UTC, comment #8: 

Do we really need that? Other lists are made up of a simple 'next' pointer, too. I currently don't know what the queue is used for, but if a next pointer is enough, I think we should use that instead of adding an extra queue.h file...

Simon Goldschmidt <goldsimon>
Group administrator
Fri 30 Nov 2007 07:00:17 PM UTC, comment #7: 

I would like to add this file (modified from FreeBSD). I'm not a license expert, but I think there is not real problems to use it. Of course, it's a temporary workaround until we provide our own list.

(file #14518)

Frédéric Bernon <fbernon>
Group Member
Fri 30 Nov 2007 05:38:48 PM UTC, comment #6: 


>I think it would be better to implement our own list there (as in every other modules). Both because of implementation size (I'm not sure the queue.h implementation is so small) and because many C compilers for embedded systems won't have a full C library including queue.h...


Agree. But in the same spirit of sys_xxx others functions, or a separate file ?

Actually, this is a last reason why ppp_oe.c doesn't build.

Frédéric Bernon <fbernon>
Group Member
Fri 30 Nov 2007 05:35:00 PM UTC, comment #5: 

About comment #3, the problem is always the CVS history. There is lot of files to move, so, it's more a separate issue.

Frédéric Bernon <fbernon>
Group Member
Fri 30 Nov 2007 11:59:55 AM UTC, comment #4: 

embed9527 wrote on lwip-users:

> 4、In the file vjbsdhdr.h, the struct in_addr in the file
> ip_addr.h(line 70-72) must to changed as:

> PACK_STRUCT_BEGIN
> struct in_addr {
>   PACK_STRUCT_FIELD(u32_t s_addr);
> }PACK_STRUCT_STRUCT;
> PACK_STRUCT_END

> after change, the struct ip in file vjbsdhdr.h, the variable
> struct in_addr ip_src,ip_dst(line 39) can compiler successfully.


I think the error is in vjbsdhdr.h: The struct ip.ip_src and .ip_dst should be of type struct ip_addr, not in_addr: struct in_addr is used for API only and shouldn't need to be packed!

On the other hand this type (and struct tcphdr) is really not needed, since lwIP defines them in ip.h and tcp.h already!

Seems like there are many duplicates in ppp...

Simon Goldschmidt <goldsimon>
Group administrator
Fri 30 Nov 2007 11:55:57 AM UTC, comment #3: 

Should we move the ppp headers from src/netif/ppp/ to src/include/netif/ppp/ (create ppp include directory)?

Also, src/include/netif/ppp_oe.h should then be moved to src/include/netif/ppp/.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 30 Nov 2007 07:42:50 AM UTC, comment #2: 

on lwip-users, embed9527 wrote:

> ppp_oe.c module can't find file queue.h. I'm not use it, but I
> want to known how to do if I can make ppp_oe.c works well.


I think it would be better to implement our own list there (as in every other modules). Both because of implementation size (I'm not sure the queue.h implementation is so small) and because many C compilers for embedded systems won't have a full C library including queue.h...

Simon Goldschmidt <goldsimon>
Group administrator
Thu 29 Nov 2007 01:45:42 AM UTC, comment #1: 

I ever used RASPPPoE to make windows work as a PPPoE server
Hope this will help you.
Here is the guidance
http://www.nat32.com/nat32e/htm/testbed.htm

Bockham Zhao <bockham>
Wed 28 Nov 2007 07:23:23 PM UTC, original submission:  

This is not a news, but PPP code is actually broken. I have start to do some fix to be able to build it, and to test it.

Actually, PPPoE changes give some problems :

- undefined "queue.h" code
- some #if PPPOE_SUPPORT missing
- pppOpen undefined now

There is also a very specific coding style (brackets, tabs, names...), and some conflicts with some tcp.h enum (CLOSED by example).

I will post a patch for that. But now I can build it, I would like to do some tests. Is anyone knows a good freeware running a PPPoS server under Windows (yes, sorry, I use it at home)?

Frédéric Bernon <fbernon>
Group Member

 

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

Attached Files
file #15118:  ppp.h added by dstahlback (16KiB - text/x-chdr - files concerning comment #14)
file #15119:  ppp.c added by dstahlback (56KiB - application/octet-stream - files concerning comment #14)
file #15120:  sio.h added by dstahlback (2KiB - text/x-chdr - files concerning comment #14)
file #14518:  queue.h added by fbernon (4KiB - text/x-chdr)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dstahlback (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by bockham (Posted a comment)
  • -email is unavailable- added by fbernon (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-05-02 fbernon StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2008-02-26 dstahlback Attached File- Added ppp.h, #15118
        Attached File- Added ppp.c, #15119
        Attached File- Added sio.h, #15120
    2007-11-30 fbernon Attached File- Added queue.h, #14518

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code