Opened 13 years ago

Closed 3 years ago

#812 closed enhancement (fixed)

Port PPP to the new stack

Reported by: axeld Owned by: nobody
Priority: normal Milestone: Unscheduled
Component: Network & Internet/PPP Version:
Keywords: Cc: wkornewald, linlongzhou@…
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

Port over Waldemar's PPP code to the new networking stack.

Attachments (6)

0001-merge-pppoe_branch.patch (259.8 KB ) - added by mshlyn 7 years ago.
porting ppp to new netstack
ppp1 (300 bytes ) - added by mshlyn 7 years ago.
pppoe configuration file
ppp_patch.tar.gz (187.3 KB ) - added by mshlyn 7 years ago.
git format-patch origin/master Output
ppp_patches.tar.gz (277.4 KB ) - added by mshlyn 6 years ago.
Update patch as axeld suggested
patches_2014_01_04.tar.bz2 (128.4 KB ) - added by mshlyn 6 years ago.
'git pull --rebase' to the latest master
0001-Add-pppoe-to-haiku-image-and-minor-compilation-fixes.patch (4.3 KB ) - added by jessicah 6 years ago.
Replaces patch 0042 of the patchset

Download all attachments as: .zip

Change History (29)

comment:1 by wkornewald, 13 years ago

Cc: wkornewald added

I should at least watch this task...

comment:2 by axeld, 12 years ago

Milestone: R1 Network StackR1
Platform: All

by mshlyn, 7 years ago

porting ppp to new netstack

comment:3 by mshlyn, 7 years ago

Has a Patch: set

by mshlyn, 7 years ago

Attachment: ppp1 added

pppoe configuration file

comment:4 by mshlyn, 7 years ago

Usage: 1, merge the patch and compile 2, put ppp1 template configuration file to /boot/home/config/settings/kernel/drivers/ptpnet/ppp1 and change the settings. 3, In terminal type: ifconfig ppp1 up

What is done: 1, port mbuf to net_buffer 2, basic pppoe support

Todo: 1, KPPPManager optimization, now only support basic function 2, autoconnect 3, DNS server update to resolv.conf

done in latest patch

4, spelling, code style, debug information cleanup.

Problem: 1, All lock should be recheckd! 2, Files 'haiku/src/add-ons/kernel/network/ppp/ipcp/{port_before.h, port_after.h, bittypes.h, config.h} are copied from haiku/src/kits/network/libbind/ dir. There should be better solution.

The 2nd one is solved by using simplified inet_addr.c located src/system/kernel/util/. It has no header dependency as the old one located in libbind.

Last edited 7 years ago by mshlyn (previous) (diff)

comment:5 by mshlyn, 7 years ago

Cc: linlongzhou@… added

in reply to:  4 ; comment:6 by siarzhuk, 7 years ago

Replying to mshlyn:

Usage: 1, merge the patch and compile 2, put ppp1 template configuration file to /boot/home/config/settings/kernel/drivers/ptpnet/ppp1 and change the settings. 3, In terminal type: ifconfig ppp1 up

I think it is a good idea to ask for testing help in the haiku-development mailing list too. BTW, are you using any Source Code Management systems for your work? Lot of Haiku development branches, hosted on github, for example, send commit notifications into haiku-development mail list. IMO, it is a good way to stay in the light and receive immediate advices from our developers. ;-)

And thank you very much for this work! It was surprising delight seeing it.

in reply to:  6 ; comment:7 by mshlyn, 7 years ago

Replying to siarzhuk:

Replying to mshlyn:

Usage: 1, merge the patch and compile 2, put ppp1 template configuration file to /boot/home/config/settings/kernel/drivers/ptpnet/ppp1 and change the settings. 3, In terminal type: ifconfig ppp1 up

I think it is a good idea to ask for testing help in the haiku-development mailing list too. BTW, are you using any Source Code Management systems for your work? Lot of Haiku development branches, hosted on github, for example, send commit notifications into haiku-development mail list. IMO, it is a good way to stay in the light and receive immediate advices from our developers. ;-)

This is the first time I submit patch. There is much for me to learn. I use git on my local PC. Due to bad plan,there were lots experimental code in my git repo. That is why I only submitted pure diff here. I can upload the format-patch if it helps. As for haiku branch, I only know the official one. Do you have any suggest? I would like to receive immediate advice from the developers.

And thank you very much for this work! It was surprising delight seeing it.

My pleasure if it helps a bit. And thanks very much for you good advice!

by mshlyn, 7 years ago

Attachment: ppp_patch.tar.gz added

git format-patch origin/master Output

in reply to:  7 comment:8 by siarzhuk, 7 years ago

Replying to mshlyn:

As for haiku branch, I only know the official one. Do you have any suggest? I would like to receive immediate advice from the developers.

If you asking about forked branches of Haiku - there are no such ones at the moment, AFAIK. Most of known repositories are just development branches - like your one. And all mentioned "experiments" are normal development workflow, in my opinion. ;-)

BTW, Trac is targeted mainly for problem discussions so you are welcome to http://www.freelists.org/list/haiku-development :-)

comment:9 by mmadia, 7 years ago

Would it be worthwhile for mshlyn to rewrite his git history, so as to remove/consolidate the experimental changes? The archive attachment contains 37 patches. :-D

comment:10 by axeld, 7 years ago

Sorry for the long delay, mshlyn! Your work is much appreciated, and also thanks for providing it in git format-patch format!

Your patch has a few issues so that I would not like to submit it as is:

  • Why do you copy files like asm_defs.h, net_stack_driver.h, and directories.h? If you need them, they are already in the tree. You may need to alter the Jamfile to make visible. For example, to add private kernel headers, you can use something like UsePrivateKernelHeaders.
  • Your patch could use some general cleanup: things like commenting out includes, etc., debug output like the changes to , or the changes made to build/jam/ReleaseBuildProfiles, src/add-ons/kernel/network/datalink_protocols/ethernet_frame/ethernet_frame.cpp, and src/add-ons/kernel/network/devices/ethernet/ethernet.cpp should not be part of it.
  • Why is KPPPManager.cpp in its own directory? Besides that, we always use lower case for directory names (there are a few exceptions, but those should be fixed instead of introducing new ones).
  • A bit more documentation would be great. For example, what is Interface::ChangeAddress() doing exactly, and why is it needed? I mean, why don't you just set the address like ifconfig would do?
  • What does "src for Y470" mean, and why is it just empty?
  • The get_interface_address_by_name you introduce deviates from the usual argument ordering (domain comes first).
  • Why do you need those functions, anyway?

And also, there is a ton of coding style issues - I mention a few of those that I came across:

  • There is a space between 'if', 'while', 'switch', etc. and '('
  • Names like 'nd' should be avoided -- use domain instead.
  • Space before and after operators (ie. name == NULL instead of name==NULL).
  • We use camel case for variables, ie. pppInterfaceAddress instead of ppp_interface_address.
  • Two lines between functions.
  • Includes are grouped by origin, the groups are ordered most generic to most specific, and the includes within the groups are ordered alphabetically.
  • The '{' goes to the end of the line, and is preceded by a space (like src/add-ons/kernel/network/ppp/shared/libkernelppp/settings_tools.cpp).
  • If you always exit an 'if' block with 'return', don't use 'else' when it's not needed.
  • Space after ''
  • Space after ','
  • Don't use superfluous parenthesis as in pppoe_input().

I hope you are willing to continue working on this?

by mshlyn, 6 years ago

Attachment: ppp_patches.tar.gz added

Update patch as axeld suggested

comment:11 by mshlyn, 6 years ago

Fixed lots of issues pointed out by axeld, including style issues. What is left:

1, Integrate PPPManager into libkernelppp 2, pppconfig, ppp_up are still not working

comment:12 by phoudoin, 6 years ago

Thanks, I will give it a look any time soon.

by mshlyn, 6 years ago

Attachment: patches_2014_01_04.tar.bz2 added

'git pull --rebase' to the latest master

comment:13 by luroh, 6 years ago

phoudoin: ping?

comment:14 by umccullough, 6 years ago

Would be nice to see some of this merged soon so it doesn't rot :)

comment:15 by luroh, 6 years ago

Bitrot has set in, patch 0042 no longer applies.

by jessicah, 6 years ago

Replaces patch 0042 of the patchset

comment:16 by jessicah, 6 years ago

It at least compiles again. Am not sure if the use of volatile integers should be replaced with just int32 or not. The use of atomic_add family of functions require casting to int32* to compile. Probably better to switch to int32 and use atomic getters instead?

comment:17 by pdziepak, 6 years ago

Volatile integers are not needed here. Unfortunately, this patch series in current form is very difficult to review as the majority of the patches are fixes to the previous ones and definitely should not be merged. However, I have noticed that IPCP::fID is accessed both using normal and atomic operations and that doesn't look good. Actually, I am not even sure that increment of fID really needs to be atomic (but I may be wrong).

comment:18 by waddlesplash, 5 years ago

Owner: set to waddlesplash
Status: newassigned

Hmm, so if I understand correctly:

  1. all teh patchez should be combined into logical chunks
  2. Volaitle ints should be removed
  3. Atomic-only operations should be used on IPCP::fID

...and it should be tested on an actual dialup line. Right?

comment:19 by luroh, 5 years ago

Milestone: R1Unscheduled

Moving PPP related tickets out of R1 milestone (Prop #2).

comment:20 by waddlesplash, 4 years ago

The patchset now lives at https://github.com/waddlesplash/haiku/tree/pppp; I've rebased it against master and will work on it as time allows.

comment:21 by waddlesplash, 4 years ago

Entire patchset merged in hrev49969.

comment:22 by waddlesplash, 4 years ago

Owner: changed from waddlesplash to nobody

deassigning various things from me

comment:23 by kallisti5, 3 years ago

Resolution: fixed
Status: assignedclosed

Since this was all merged i'm going to close this one. Any additional issues should be tracked in new tickets.

Note: See TracTickets for help on using tickets.