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
> <phil...@redfish-solutions.com> wrote:
> > Inline
> >
> >
> > On 08/20/2016 09:52 AM, f...@48lvckh6395k16k5.yundunddos.com wrote:
> >>
> >> From: Gao Feng <f...@ikuai8.com>
> >>
> >> Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
> >> 0x03, and 2 separately.
> >>
> >> Signed-off-by: Gao Feng <f...@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.
