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.na...@alphalink.fr> wrote: > > On Sat, Aug 20, 2016 at 11:52:27PM +0800, f...@ikuai8.com wrote: > >> From: Gao Feng <f...@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.