From: Gao Feng <redacted>
Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
0x03, and 2 separately.
Signed-off-by: Gao Feng <redacted>
---
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(-)
@@ -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);}
Inline
On 08/20/2016 09:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote:
quoted hunk
From: Gao Feng <redacted>
Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
0x03, and 2 separately.
Signed-off-by: Gao Feng <redacted>
---
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(-)
inline
On Mon, Aug 22, 2016 at 6:36 AM, Philp Prindeville
[off-list ref] wrote:
Inline
On 08/20/2016 09:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote:
quoted
From: Gao Feng <redacted>
Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
0x03, and 2 separately.
Signed-off-by: Gao Feng <redacted>
---
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(-)
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.
quoted
struct sock *sk = sock->sk;
struct sk_buff *skb;
int error;
*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);
}
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:
quoted
From: Gao Feng <redacted>
Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
0x03, and 2 separately.
Signed-off-by: Gao Feng <redacted>
---
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(-)
@@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff *skb)if(!pskb_may_pull(skb,2))return1;-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.
quoted
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.
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
[off-list ref] wrote:
quoted
Inline
On 08/20/2016 09:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote:
quoted
From: Gao Feng <redacted>
Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
0x03, and 2 separately.
Signed-off-by: Gao Feng <redacted>
---
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(-)
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.
On Sat, Aug 20, 2016 at 11:52:27PM +0800, fgao@ikuai8.com wrote:
quoted hunk
From: Gao Feng <redacted>
Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
0x03, and 2 separately.
Signed-off-by: Gao Feng <redacted>
---
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(-)
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.
inline
On Mon, Aug 22, 2016 at 6:07 PM, Guillaume Nault [off-list ref] wrote:
On Sat, Aug 20, 2016 at 11:52:27PM +0800, fgao@ikuai8.com wrote:
quoted
From: Gao Feng <redacted>
Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
0x03, and 2 separately.
Signed-off-by: Gao Feng <redacted>
---
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(-)
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
inline
On Mon, Aug 22, 2016 at 5:48 PM, Guillaume Nault [off-list ref] wrote:
On Sun, Aug 21, 2016 at 04:36:52PM -0600, Philp Prindeville wrote:
quoted
Inline
On 08/20/2016 09:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote:
quoted
From: Gao Feng <redacted>
Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
0x03, and 2 separately.
Signed-off-by: Gao Feng <redacted>
---
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(-)
@@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff *skb)if(!pskb_may_pull(skb,2))return1;-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.
quoted
quoted
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
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.
quoted
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.