Patchwork skb: add a comment to skb_csum_unnecessary to avoid miuse

login
register
mail settings
Submitter Koki Sanagi
Date Jan. 23, 2013, 5:11 a.m.
Message ID <F6CB40852BC6EF489E0AA83EFF537F470876BBCF@G01JPEXMBYT04>
Download mbox | patch
Permalink /patch/214791/
State Rejected
Delegated to: David Miller
Headers show

Comments

Koki Sanagi - Jan. 23, 2013, 5:11 a.m.
> From: "Sanagi, Koki" <sanagi.koki@jp.fujitsu.com>
> Date: Wed, 23 Jan 2013 02:32:52 +0000
> 
> >> From: Koki Sanagi <sanagi.koki@jp.fujitsu.com>
> >> Date: Wed, 23 Jan 2013 09:30:16 +0900
> >>
> >> > Due to its name and appearance, someone thinks this only checks if
> >> > ip_summed is CHECKSUM_UNNECESARRY.  But actually, this returns true
> >> > even if ip_summed is CHECKSUM_PARTIAL.  To avoid misuse, this patch
> >> > a comment which specifies that CHECKSUM_PARTIAL is OK.
> >> >
> >> > Signed-off-by: Koki Sanagi <sanagi.koki@jp.fujitsu.com>
> >>
> >> I'm not applying this, sorry.  It's a one line function and it's not
> >> so non-obvious that it deserves an 8 line comment.
> >
> > OK.  I just felt weird that CHECKSUM_* is not bit flag but this
> > function handles it as if it was bit flag.
> 
> The function name says what it does, it determines whether a checksum is
> necessary or not.  How that is implemented is another issue.

As for name of the function which confused me, I'm ok now.  But as for how to implement,
I still think it is inappropriate because of the above reason.  So, how about introducing
CHECKSUM_UNNECESSARY_BIT like below ?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Jan. 23, 2013, 5:21 a.m.
From: "Sanagi, Koki" <sanagi.koki@jp.fujitsu.com>
Date: Wed, 23 Jan 2013 05:11:14 +0000

> As for name of the function which confused me, I'm ok now.  But as
> for how to implement, I still think it is inappropriate because of
> the above reason.  So, how about introducing
> CHECKSUM_UNNECESSARY_BIT like below ?

If the user has to look at the implementation, they will see
the "&" and there is no confusion.

Please stop splitting hairs, this is a waste of time.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 8b2256e..b1dbd36 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -39,6 +39,8 @@ 
 #define CHECKSUM_COMPLETE 2
 #define CHECKSUM_PARTIAL 3

+#define CHECKSUM_UNNECESSARY_BIT (1 << 0)
+
 #define SKB_DATA_ALIGN(X)      (((X) + (SMP_CACHE_BYTES - 1)) & \
                                 ~(SMP_CACHE_BYTES - 1))
 #define SKB_WITH_OVERHEAD(X)   \
@@ -2524,7 +2526,7 @@  extern __sum16 __skb_checksum_complete(struct sk_buff *skb);

 static inline int skb_csum_unnecessary(const struct sk_buff *skb)
 {
-       return skb->ip_summed & CHECKSUM_UNNECESSARY;
+       return skb->ip_summed & CHECKSUM_UNNECESSARY_BIT;
 }

 /**