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 [off-list ref] wrote:

quoted

On Sat, Aug 20, 2016 at 11:52:27PM +0800, fgao@ikuai8.com wrote:

quoted

From: Gao Feng <redacted>

@@ -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.

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.