[haiku-bugs] Re: [Haiku] #812: Port PPP to the new stack
- From: "axeld" <trac@xxxxxxxxxxxx>
- Date: Thu, 23 May 2013 14:50:41 -0000
#812: Port PPP to the new stack
--------------------------------------+-----------------
Reporter: axeld | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: R1
Component: Network & Internet/PPP | Version:
Resolution: | Keywords:
Blocked By: | Blocking:
Has a Patch: 1 | Platform: All
--------------------------------------+-----------------
Comment (by axeld):
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?
--
Ticket URL: <http://dev.haiku-os.org/ticket/812#comment:10>
Haiku <http://dev.haiku-os.org>
Haiku - the operating system.
Other related posts:
