diff mbox series

[17/29] l2tp: avoid precidence issues in L2TP_SKB_CB macro

Message ID 20200721173221.4681-18-tparkin@katalix.com
State Changes Requested
Delegated to: David Miller
Headers show
Series l2tp: cleanup checkpatch.pl warnings | expand

Commit Message

Tom Parkin July 21, 2020, 5:32 p.m. UTC
checkpatch warned about the L2TP_SKB_CB macro's use of its argument: add
braces to avoid the problem.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Joe Perches July 28, 2020, 4:21 p.m. UTC | #1
On Tue, 2020-07-21 at 18:32 +0100, Tom Parkin wrote:
> checkpatch warned about the L2TP_SKB_CB macro's use of its argument: add
> braces to avoid the problem.
[]
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
[]
> @@ -93,7 +93,7 @@ struct l2tp_skb_cb {
>  	unsigned long		expires;
>  };
>  
> -#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&skb->cb[sizeof(struct inet_skb_parm)])
> +#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&(skb)->cb[sizeof(struct inet_skb_parm)])

Likely better to use a static inline.

Something like:

static inline struct l2tp_skb_cb *L2TP_SKB_SB(struct sk_buff *skb)
{
	return &skb->cb[sizeof(struct inet+skb_parm)];
}
Joe Perches July 28, 2020, 6:08 p.m. UTC | #2
On Tue, 2020-07-28 at 09:21 -0700, Joe Perches wrote:
> On Tue, 2020-07-21 at 18:32 +0100, Tom Parkin wrote:
> > checkpatch warned about the L2TP_SKB_CB macro's use of its argument: add
> > braces to avoid the problem.
> []
> > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> []
> > @@ -93,7 +93,7 @@ struct l2tp_skb_cb {
> >  	unsigned long		expires;
> >  };
> >  
> > -#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&skb->cb[sizeof(struct inet_skb_parm)])
> > +#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&(skb)->cb[sizeof(struct inet_skb_parm)])
> 
> Likely better to use a static inline.
> 
> Something like:
> 
> static inline struct l2tp_skb_cb *L2TP_SKB_SB(struct sk_buff *skb)
> {
> 	return &skb->cb[sizeof(struct inet+skb_parm)];
> }

More precisely:
---
 net/l2tp/l2tp_core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index e723828e458b..78ad6d8405c4 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -93,7 +93,10 @@ struct l2tp_skb_cb {
 	unsigned long		expires;
 };
 
-#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&(skb)->cb[sizeof(struct inet_skb_parm)])
+static inline struct l2tp_skb_cb *L2TP_SKB_CB(struct sk_buff *skb)
+{
+	return (struct l2tp_skb_cb *)&skb->cb[sizeof(struct inet_skb_parm)];
+}
 
 static struct workqueue_struct *l2tp_wq;
Tom Parkin July 28, 2020, 7:31 p.m. UTC | #3
On  Tue, Jul 28, 2020 at 11:08:45 -0700, Joe Perches wrote:
> On Tue, 2020-07-28 at 09:21 -0700, Joe Perches wrote:
> > On Tue, 2020-07-21 at 18:32 +0100, Tom Parkin wrote:
> > > checkpatch warned about the L2TP_SKB_CB macro's use of its argument: add
> > > braces to avoid the problem.
> > []
> > > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> > []
> > > @@ -93,7 +93,7 @@ struct l2tp_skb_cb {
> > >  	unsigned long		expires;
> > >  };
> > >  
> > > -#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&skb->cb[sizeof(struct inet_skb_parm)])
> > > +#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&(skb)->cb[sizeof(struct inet_skb_parm)])
> > 
> > Likely better to use a static inline.
> > 
> > Something like:
> > 
> > static inline struct l2tp_skb_cb *L2TP_SKB_SB(struct sk_buff *skb)
> > {
> > 	return &skb->cb[sizeof(struct inet+skb_parm)];
> > }
> 
> More precisely:
> ---
>  net/l2tp/l2tp_core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index e723828e458b..78ad6d8405c4 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -93,7 +93,10 @@ struct l2tp_skb_cb {
>  	unsigned long		expires;
>  };
>  
> -#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&(skb)->cb[sizeof(struct inet_skb_parm)])
> +static inline struct l2tp_skb_cb *L2TP_SKB_CB(struct sk_buff *skb)
> +{
> +	return (struct l2tp_skb_cb *)&skb->cb[sizeof(struct inet_skb_parm)];
> +}
>  
>  static struct workqueue_struct *l2tp_wq;
>  
> 

Thanks Joe.  I can see this is better since we get some type checking
from the compiler for the function argument.

The patchset has been applied already, but I can try to integrate this
change in the future.
diff mbox series

Patch

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 177accb01993..4973a0f035e3 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -93,7 +93,7 @@  struct l2tp_skb_cb {
 	unsigned long		expires;
 };
 
-#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&skb->cb[sizeof(struct inet_skb_parm)])
+#define L2TP_SKB_CB(skb)	((struct l2tp_skb_cb *)&(skb)->cb[sizeof(struct inet_skb_parm)])
 
 static struct workqueue_struct *l2tp_wq;