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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20160822114649.6zzeq4ef4svh4hrt@alphalink.fr>
Date:   Mon, 22 Aug 2016 13:46:49 +0200
From:   Guillaume Nault <g.nault@...halink.fr>
To:     Feng Gao <gfree.wind@...il.com>
Cc:     Gao Feng <fgao@...ai8.com>, paulus@...ba.org,
        "David S. Miller" <davem@...emloft.net>,
        Philp Prindeville <philipp@...fish-solutions.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH v3 net-next] l2tp: Refactor the codes with existing
 macros instead of literal number

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