[PATCH v3 net-next] l2tp: Refactor the codes with existing macros instead of literal number

[PATCH v3 net-next] l2tp: Refactor the codes with existing macros instead of literal number

[fgao]

From: Gao Feng <fgao@ikuai8.com>

Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
0x03, and 2 separately.

Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
 v3: Modify the subject;
 v2: Only replace the literal number with macros according to Guillaume's advice
 v1: Inital patch

 net/l2tp/l2tp_ppp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index d9560aa..65e2fd6 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff *skb)
 	if (!pskb_may_pull(skb, 2))
 		return 1;
 
-	if ((skb->data[0] == 0xff) && (skb->data[1] == 0x03))
+	if ((skb->data[0] == PPP_ALLSTATIONS) && (skb->data[1] == PPP_UI))
 		skb_pull(skb, 2);
 
 	return 0;
@@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct l2tp_session *session)
 static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m,
 			    size_t total_len)
 {
-	static const unsigned char ppph[2] = { 0xff, 0x03 };
+	static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI};
 	struct sock *sk = sock->sk;
 	struct sk_buff *skb;
 	int error;
@@ -369,7 +369,7 @@ error:
  */
 static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
 {
-	static const u8 ppph[2] = { 0xff, 0x03 };
+	static const u8 ppph[2] = {PPP_ALLSTATIONS, PPP_UI};
 	struct sock *sk = (struct sock *) chan->private;
 	struct sock *sk_tun;
 	struct l2tp_session *session;
@@ -440,7 +440,7 @@ static void pppol2tp_session_close(struct l2tp_session *session)
 	BUG_ON(session->magic != L2TP_SESSION_MAGIC);
 
 	if (sock) {
-		inet_shutdown(sock, 2);
+		inet_shutdown(sock, SEND_SHUTDOWN);
 		/* Don't let the session go away before our socket does */
 		l2tp_session_inc_refcount(session);
 	}
-- 
1.9.1

Re: [PATCH v3 net-next] l2tp: Refactor the codes with existing macros instead of literal number

[David Miller]

From: fgao@ikuai8.com
Date: Sat, 20 Aug 2016 23:52:27 +0800

> From: Gao Feng <fgao@ikuai8.com>
> 
> Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
> 0x03, and 2 separately.
> 
> Signed-off-by: Gao Feng <fgao@ikuai8.com>

Applied.

Re: [PATCH v3 net-next] l2tp: Refactor the codes with existing macros instead of literal number

[Philp Prindeville]

Inline


On 08/20/2016 09:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote:
> From: Gao Feng <fgao@ikuai8.com>
>
> Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
> 0x03, and 2 separately.
>
> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> ---
>   v3: Modify the subject;
>   v2: Only replace the literal number with macros according to Guillaume's advice
>   v1: Inital patch
>
>   net/l2tp/l2tp_ppp.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
> index d9560aa..65e2fd6 100644
> --- a/net/l2tp/l2tp_ppp.c
> +++ b/net/l2tp/l2tp_ppp.c
> @@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff *skb)
>   	if (!pskb_may_pull(skb, 2))
>   		return 1;
>   
> -	if ((skb->data[0] == 0xff) && (skb->data[1] == 0x03))
> +	if ((skb->data[0] == PPP_ALLSTATIONS) && (skb->data[1] == PPP_UI))

This should have used PPP_ADDRESS() and PPP_CONTROL() here.

>   		skb_pull(skb, 2);

This magic number should go away.

>   
>   	return 0;
> @@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct l2tp_session *session)
>   static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m,
>   			    size_t total_len)
>   {
> -	static const unsigned char ppph[2] = { 0xff, 0x03 };
> +	static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI};

PPP has a 4-byte header.  Where's the protocol value?

>   	struct sock *sk = sock->sk;
>   	struct sk_buff *skb;
>   	int error;
> @@ -369,7 +369,7 @@ error:
>    */
>   static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
>   {
> -	static const u8 ppph[2] = { 0xff, 0x03 };
> +	static const u8 ppph[2] = {PPP_ALLSTATIONS, PPP_UI};

Same as above.


>   	struct sock *sk = (struct sock *) chan->private;
>   	struct sock *sk_tun;
>   	struct l2tp_session *session;
> @@ -440,7 +440,7 @@ static void pppol2tp_session_close(struct l2tp_session *session)
>   	BUG_ON(session->magic != L2TP_SESSION_MAGIC);
>   
>   	if (sock) {
> -		inet_shutdown(sock, 2);
> +		inet_shutdown(sock, SEND_SHUTDOWN);
>   		/* Don't let the session go away before our socket does */
>   		l2tp_session_inc_refcount(session);
>   	}

Re: [PATCH v3 net-next] l2tp: Refactor the codes with existing macros instead of literal number

[David Miller]

From: Philp Prindeville <philipp@redfish-solutions.com>
Date: Sun, 21 Aug 2016 16:36:52 -0600

> Inline

Sorry, I applied this already, I'll revert it.

Re: [PATCH v3 net-next] l2tp: Refactor the codes with existing macros instead of literal number

[Feng Gao]

inline

On Mon, Aug 22, 2016 at 6:36 AM, Philp Prindeville
<philipp@redfish-solutions.com> wrote:
> Inline
>
>
> On 08/20/2016 09:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote:
>>
>> From: Gao Feng <fgao@ikuai8.com>
>>
>> Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
>> 0x03, and 2 separately.
>>
>> Signed-off-by: Gao Feng <fgao@ikuai8.com>
>> ---
>>   v3: Modify the subject;
>>   v2: Only replace the literal number with macros according to Guillaume's
>> advice
>>   v1: Inital patch
>>
>>   net/l2tp/l2tp_ppp.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
>> index d9560aa..65e2fd6 100644
>> --- a/net/l2tp/l2tp_ppp.c
>> +++ b/net/l2tp/l2tp_ppp.c
>> @@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff
>> *skb)
>>         if (!pskb_may_pull(skb, 2))
>>                 return 1;
>>   -     if ((skb->data[0] == 0xff) && (skb->data[1] == 0x03))
>> +       if ((skb->data[0] == PPP_ALLSTATIONS) && (skb->data[1] == PPP_UI))
>
>
> This should have used PPP_ADDRESS() and PPP_CONTROL() here.

In my initial patch, I replace them with PPP_ADDRESS() and PPP_CONTROL.
But Guillaume thought it was not clear as before.
So I revert it.

>
>>                 skb_pull(skb, 2);
>
>
> This magic number should go away.

Same as above.

>
>>         return 0;
>> @@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct
>> l2tp_session *session)
>>   static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m,
>>                             size_t total_len)
>>   {
>> -       static const unsigned char ppph[2] = { 0xff, 0x03 };
>> +       static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI};
>
>
> PPP has a 4-byte header.  Where's the protocol value?

In the original code, I fail to find the code which is used to fill
the protocol value.
So I keep the two bytes header. And I thought the protocol value may be filled
by the upper layer.

>
>>         struct sock *sk = sock->sk;
>>         struct sk_buff *skb;
>>         int error;
>> @@ -369,7 +369,7 @@ error:
>>    */
>>   static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
>>   {
>> -       static const u8 ppph[2] = { 0xff, 0x03 };
>> +       static const u8 ppph[2] = {PPP_ALLSTATIONS, PPP_UI};
>
>
> Same as above.
>
>
>
>>         struct sock *sk = (struct sock *) chan->private;
>>         struct sock *sk_tun;
>>         struct l2tp_session *session;
>> @@ -440,7 +440,7 @@ static void pppol2tp_session_close(struct l2tp_session
>> *session)
>>         BUG_ON(session->magic != L2TP_SESSION_MAGIC);
>>         if (sock) {
>> -               inet_shutdown(sock, 2);
>> +               inet_shutdown(sock, SEND_SHUTDOWN);
>>                 /* Don't let the session go away before our socket does */
>>                 l2tp_session_inc_refcount(session);
>>         }
>
>

Re: [PATCH v3 net-next] l2tp: Refactor the codes with existing macros instead of literal number

[Guillaume Nault]

On Sun, Aug 21, 2016 at 04:36:52PM -0600, Philp Prindeville wrote:
> Inline
> 
> 
> On 08/20/2016 09:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote:
> > From: Gao Feng <fgao@ikuai8.com>
> > 
> > Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
> > 0x03, and 2 separately.
> > 
> > Signed-off-by: Gao Feng <fgao@ikuai8.com>
> > ---
> >   v3: Modify the subject;
> >   v2: Only replace the literal number with macros according to Guillaume's advice
> >   v1: Inital patch
> > 
> >   net/l2tp/l2tp_ppp.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
> > index d9560aa..65e2fd6 100644
> > --- a/net/l2tp/l2tp_ppp.c
> > +++ b/net/l2tp/l2tp_ppp.c
> > @@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff *skb)
> >   	if (!pskb_may_pull(skb, 2))
> >   		return 1;
> > -	if ((skb->data[0] == 0xff) && (skb->data[1] == 0x03))
> > +	if ((skb->data[0] == PPP_ALLSTATIONS) && (skb->data[1] == PPP_UI))
> 
> This should have used PPP_ADDRESS() and PPP_CONTROL() here.
>
Then please justify how would that make the code more readable.
We're not trying to interpret a known valid PPP header here.

> >   		skb_pull(skb, 2);
> 
> This magic number should go away.
>
Again, this is *not* a magic number. We've explicitely accessed the
first _two_ header bytes and want to skip them.
pskb_may_pull(2), ->data[0], ->data[1] and skb_pull(2) all go together.

There's even a nice comment telling you what is done and why:
	/* Skip PPP header, if present.	 In testing, Microsoft L2TP clients
	 * don't send the PPP header (PPP header compression enabled), but
	 * other clients can include the header. So we cope with both cases
	 * here. The PPP header is always FF03 when using L2TP.
	 *
	 * Note that skb->data[] isn't dereferenced from a u16 ptr here since
	 * the field may be unaligned.
	 */
Apart from the unprecise "PPP header" term, which should be read as
"address and control fields", things should be quite clear.

> > @@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct l2tp_session *session)
> >   static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m,
> >   			    size_t total_len)
> >   {
> > -	static const unsigned char ppph[2] = { 0xff, 0x03 };
> > +	static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI};
> 
> PPP has a 4-byte header.  Where's the protocol value?
>
No, PPP header (whatever you include in it) is of variable length. And
the protocol has already been set by the PPP layer anyway.
We're in L2TP here.

Re: [PATCH v3 net-next] l2tp: Refactor the codes with existing macros instead of literal number

[Guillaume Nault]

On Mon, Aug 22, 2016 at 08:13:48AM +0800, Feng Gao wrote:
> inline
> 
> On Mon, Aug 22, 2016 at 6:36 AM, Philp Prindeville
> <philipp@redfish-solutions.com> wrote:
> > Inline
> >
> >
> > On 08/20/2016 09:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote:
> >>
> >> From: Gao Feng <fgao@ikuai8.com>
> >>
> >> Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
> >> 0x03, and 2 separately.
> >>
> >> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> >> ---
> >>   v3: Modify the subject;
> >>   v2: Only replace the literal number with macros according to Guillaume's
> >> advice
> >>   v1: Inital patch
> >>
> >>   net/l2tp/l2tp_ppp.c | 8 ++++----
> >>   1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
> >> index d9560aa..65e2fd6 100644
> >> --- a/net/l2tp/l2tp_ppp.c
> >> +++ b/net/l2tp/l2tp_ppp.c
> >> @@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff
> >> *skb)
> >>         if (!pskb_may_pull(skb, 2))
> >>                 return 1;
> >>   -     if ((skb->data[0] == 0xff) && (skb->data[1] == 0x03))
> >> +       if ((skb->data[0] == PPP_ALLSTATIONS) && (skb->data[1] == PPP_UI))
> >
> >
> > This should have used PPP_ADDRESS() and PPP_CONTROL() here.
> 
> In my initial patch, I replace them with PPP_ADDRESS() and PPP_CONTROL.
> But Guillaume thought it was not clear as before.
> So I revert it.
> 
> >
> >>                 skb_pull(skb, 2);
> >
> >
> > This magic number should go away.
> 
> Same as above.
> 
> >
> >>         return 0;
> >> @@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct
> >> l2tp_session *session)
> >>   static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m,
> >>                             size_t total_len)
> >>   {
> >> -       static const unsigned char ppph[2] = { 0xff, 0x03 };
> >> +       static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI};
> >
> >
> > PPP has a 4-byte header.  Where's the protocol value?
> 
> In the original code, I fail to find the code which is used to fill
> the protocol value.
> So I keep the two bytes header. And I thought the protocol value may be filled
> by the upper layer.
> 
And you were right. This was a macro replacement patch anyway, so you
didn't have to bring functional changes with it.
And if the protocol field was really missing, the L2TP module would
have never worked.

Re: [PATCH v3 net-next] l2tp: Refactor the codes with existing macros instead of literal number

[Guillaume Nault]

On Sat, Aug 20, 2016 at 11:52:27PM +0800, fgao@ikuai8.com wrote:
> From: Gao Feng <fgao@ikuai8.com>
> 
> Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
> 0x03, and 2 separately.
> 
> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> ---
>  v3: Modify the subject;
>  v2: Only replace the literal number with macros according to Guillaume's advice
>  v1: Inital patch
> 
>  net/l2tp/l2tp_ppp.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
> index d9560aa..65e2fd6 100644
> --- a/net/l2tp/l2tp_ppp.c
> +++ b/net/l2tp/l2tp_ppp.c
> @@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff *skb)
>  	if (!pskb_may_pull(skb, 2))
>  		return 1;
>  
> -	if ((skb->data[0] == 0xff) && (skb->data[1] == 0x03))
> +	if ((skb->data[0] == PPP_ALLSTATIONS) && (skb->data[1] == PPP_UI))
>  		skb_pull(skb, 2);
>  
>  	return 0;
> @@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct l2tp_session *session)
>  static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m,
>  			    size_t total_len)
>  {
> -	static const unsigned char ppph[2] = { 0xff, 0x03 };
> +	static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI};
> 
Minor nit: I'd prefer to keep the space after '{' and before '}'.
I didn't want to bother you with this, but since it seems you'll have
to repost...

>  	struct sock *sk = sock->sk;
>  	struct sk_buff *skb;
>  	int error;
> @@ -369,7 +369,7 @@ error:
>   */
>  static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
>  {
> -	static const u8 ppph[2] = { 0xff, 0x03 };
> +	static const u8 ppph[2] = {PPP_ALLSTATIONS, PPP_UI};
> 
Same here.

BTW, I thought you also wanted to remove the static ppph variable
from pppol2tp_xmit() / pppol2tp_sendmsg(), to directly assign
skb->data[0/1] with PPP_ALLSTATIONS/PPP_UI.

Re: [PATCH v3 net-next] l2tp: Refactor the codes with existing macros instead of literal number

[Feng Gao]

inline

On Mon, Aug 22, 2016 at 6:07 PM, Guillaume Nault <g.nault@alphalink.fr> wrote:
> On Sat, Aug 20, 2016 at 11:52:27PM +0800, fgao@ikuai8.com wrote:
>> From: Gao Feng <fgao@ikuai8.com>
>>
>> Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
>> 0x03, and 2 separately.
>>
>> Signed-off-by: Gao Feng <fgao@ikuai8.com>
>> ---
>>  v3: Modify the subject;
>>  v2: Only replace the literal number with macros according to Guillaume's advice
>>  v1: Inital patch
>>
>>  net/l2tp/l2tp_ppp.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
>> index d9560aa..65e2fd6 100644
>> --- a/net/l2tp/l2tp_ppp.c
>> +++ b/net/l2tp/l2tp_ppp.c
>> @@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff *skb)
>>       if (!pskb_may_pull(skb, 2))
>>               return 1;
>>
>> -     if ((skb->data[0] == 0xff) && (skb->data[1] == 0x03))
>> +     if ((skb->data[0] == PPP_ALLSTATIONS) && (skb->data[1] == PPP_UI))
>>               skb_pull(skb, 2);
>>
>>       return 0;
>> @@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct l2tp_session *session)
>>  static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m,
>>                           size_t total_len)
>>  {
>> -     static const unsigned char ppph[2] = { 0xff, 0x03 };
>> +     static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI};
>>
> Minor nit: I'd prefer to keep the space after '{' and before '}'.
> I didn't want to bother you with this, but since it seems you'll have
> to repost...

I don't know if it is the coding style of Linux kernel.

>
>>       struct sock *sk = sock->sk;
>>       struct sk_buff *skb;
>>       int error;
>> @@ -369,7 +369,7 @@ error:
>>   */
>>  static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
>>  {
>> -     static const u8 ppph[2] = { 0xff, 0x03 };
>> +     static const u8 ppph[2] = {PPP_ALLSTATIONS, PPP_UI};
>>
> Same here.
>
> BTW, I thought you also wanted to remove the static ppph variable
> from pppol2tp_xmit() / pppol2tp_sendmsg(), to directly assign
> skb->data[0/1] with PPP_ALLSTATIONS/PPP_UI.

If removed static ppph, there will be some codes which use literal "2"
instead of sizeof ppph.
Is it ok?

Regards
Feng

Re: [PATCH v3 net-next] l2tp: Refactor the codes with existing macros instead of literal number

[Feng Gao]

inline

On Mon, Aug 22, 2016 at 5:48 PM, Guillaume Nault <g.nault@alphalink.fr> wrote:
> On Sun, Aug 21, 2016 at 04:36:52PM -0600, Philp Prindeville wrote:
>> Inline
>>
>>
>> On 08/20/2016 09:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote:
>> > From: Gao Feng <fgao@ikuai8.com>
>> >
>> > Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
>> > 0x03, and 2 separately.
>> >
>> > Signed-off-by: Gao Feng <fgao@ikuai8.com>
>> > ---
>> >   v3: Modify the subject;
>> >   v2: Only replace the literal number with macros according to Guillaume's advice
>> >   v1: Inital patch
>> >
>> >   net/l2tp/l2tp_ppp.c | 8 ++++----
>> >   1 file changed, 4 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
>> > index d9560aa..65e2fd6 100644
>> > --- a/net/l2tp/l2tp_ppp.c
>> > +++ b/net/l2tp/l2tp_ppp.c
>> > @@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff *skb)
>> >     if (!pskb_may_pull(skb, 2))
>> >             return 1;
>> > -   if ((skb->data[0] == 0xff) && (skb->data[1] == 0x03))
>> > +   if ((skb->data[0] == PPP_ALLSTATIONS) && (skb->data[1] == PPP_UI))
>>
>> This should have used PPP_ADDRESS() and PPP_CONTROL() here.
>>
> Then please justify how would that make the code more readable.
> We're not trying to interpret a known valid PPP header here.
>
>> >             skb_pull(skb, 2);
>>
>> This magic number should go away.
>>
> Again, this is *not* a magic number. We've explicitely accessed the
> first _two_ header bytes and want to skip them.
> pskb_may_pull(2), ->data[0], ->data[1] and skb_pull(2) all go together.
>
> There's even a nice comment telling you what is done and why:
>         /* Skip PPP header, if present.  In testing, Microsoft L2TP clients
>          * don't send the PPP header (PPP header compression enabled), but
>          * other clients can include the header. So we cope with both cases
>          * here. The PPP header is always FF03 when using L2TP.
>          *
>          * Note that skb->data[] isn't dereferenced from a u16 ptr here since
>          * the field may be unaligned.
>          */
> Apart from the unprecise "PPP header" term, which should be read as
> "address and control fields", things should be quite clear.

If remove the static ppph, may be more clear. Because it will cause
person think about the ppp header.

Regards
Feng

>
>> > @@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct l2tp_session *session)
>> >   static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m,
>> >                         size_t total_len)
>> >   {
>> > -   static const unsigned char ppph[2] = { 0xff, 0x03 };
>> > +   static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI};
>>
>> PPP has a 4-byte header.  Where's the protocol value?
>>
> No, PPP header (whatever you include in it) is of variable length. And
> the protocol has already been set by the PPP layer anyway.
> We're in L2TP here.

Re: [PATCH v3 net-next] l2tp: Refactor the codes with existing macros instead of literal number

[Guillaume Nault]

On Mon, Aug 22, 2016 at 06:22:42PM +0800, Feng Gao wrote:
> inline
> 
> On Mon, Aug 22, 2016 at 6:07 PM, Guillaume Nault <g.nault@alphalink.fr> wrote:
> > On Sat, Aug 20, 2016 at 11:52:27PM +0800, fgao@ikuai8.com wrote:
> >> From: Gao Feng <fgao@ikuai8.com>
> >> @@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct l2tp_session *session)
> >>  static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m,
> >>                           size_t total_len)
> >>  {
> >> -     static const unsigned char ppph[2] = { 0xff, 0x03 };
> >> +     static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI};
> >>
> > Minor nit: I'd prefer to keep the space after '{' and before '}'.
> > I didn't want to bother you with this, but since it seems you'll have
> > to repost...
> 
> I don't know if it is the coding style of Linux kernel.
>
Both forms are used currently and I can't recall any explicit
preference statement. So unless David has an opinion, you can just use
the form you like the best.

> > BTW, I thought you also wanted to remove the static ppph variable
> > from pppol2tp_xmit() / pppol2tp_sendmsg(), to directly assign
> > skb->data[0/1] with PPP_ALLSTATIONS/PPP_UI.
> 
> If removed static ppph, there will be some codes which use literal "2"
> instead of sizeof ppph.
> Is it ok?
> 
The literal "2" would be used in the sock_wmalloc() call only (or for
assigning the headroom variable in the case of pppol2tp_xmit()). Given
the number of data summed, I agree that having a plain "2" in the
middle could look odd. You can either add a comment for each data summed
(like in pppol2tp_xmit()), something like:
sock_wmalloc(sk, NET_SKB_PAD +
	     sizeof(struct iphdr) + /* IP header */
	     ...
	     2 +                    /* PPP Address and control field */
	     ...);

Or use a simple macro like:
/* Size of the PPP address and control fields */
#define PPP_ACF_LEN 2

Or event use macro and comment. That's up to you.
You can even drop this change entirely if you prefer, I don't mind.
I just raised this point because you said you'd remove ppph.