Re: driver for pptp

[Harald Welte <laforge@gnumonks.org>]

[-- Attachment #1: Type: text/plain, Size: 2120 bytes --]

On Sat, Jun 03, 2006 at 11:06:19AM +0400, xeb@mail.ru wrote:
> I have developed the driver for Point-to-Point Tunneling Protocol (PPTP).

great news.  something that I always thought of a nice-to-have. 

> I have published the project on http://accel-pptp.sourceforge.net/

Please don't expect Linux Kernel networking developers to actually go to
sourceforge download and extract code that you want to have
reviewed/submitted.

Please read Documentation/SubmittingPatches (and CodingStyle) and submit
your kernel patch to netdev.

> Hope this driver will go to a kernel tree and will make linux more productive.

not without you pushing it actively and getting through review cycles
(which I hope you will!).

Some initial comments:

1) why wasn't it possible to use the PPPoX infrastructure of the kernel
   which is already being used by PPPoE ?  Or at least model it somehow
   similar to the existing PPPoE/PPPoX infrastructure?

2) why are you using a timer for asynchronous processing of GRE frames?
   First of all, why does it have to happen asynchronously at all?
   Secondly, why using a timer when there's nothing time related (or do
   I miss something)?  If deferred, out-of-context execution is
   required, there are other primitives such as tasklets.

3) you conflict with the ip_gre.c genric GRE encapsulation driver.  this
   is because both want to reigster a proto handler for GRE.  Ideally,
   there needs to be another demultiplex between the GRE protocl and its
   users.  The code registered for GRE would look at the packet and
   determine whether e.g. it is a PPTP GRE packet and then pass it on to
   the pptp module.

4) your code doesn't look nonlinear skb clean

5) why did you chose to implement  /dev/pptp rather than a socket family
   like the existing pppox/pppoe code?

6) lots of codingstyle issues

-- 
- Harald Welte <laforge@gnumonks.org>          	        http://gnumonks.org/
============================================================================
We all know Linux is great...it does infinite loops in 5 seconds. -- Linus

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]