Skip to contentSkip to conversation

skb: add a comment to skb_csum_unnecessary to avoid miuse

PATCH In review Koki Sanagi 2 participants 6 messages Jan 23, 2013

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Koki Sanagi

author
#0 lore raw

PATCH 1/1 skb: add a comment to skb_csum_unnecessary to avoid miuse

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 <[email protected]>
---
 include/linux/skbuff.h |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
include/linux/skbuff.h +10 -0
index 8b2256e..bc41f64 100644
@@ -2522,6 +2522,16 @@ void skb_complete_wifi_ack(struct sk_buff *skb, bool acked);
 extern __sum16 __skb_checksum_complete_head(struct sk_buff *skb, int len);
 extern __sum16 __skb_checksum_complete(struct sk_buff *skb);
 
+/**
+ * skb_csum_unnecessary - check if the checksum needs to be verified
+ * @skb: skb to check
+ *
+ * check if the checksum of this skb needs to be verified.  This function is
+ * effective only against skbs on inbound path.
+ *
+ * NB: This returns true if ip_summed is CHECKSUM_UNNECESSARY or
+ *     CHECKSUM_PARTIAL.
+ **/
 static inline int skb_csum_unnecessary(const struct sk_buff *skb)
 {
 	return skb->ip_summed & CHECKSUM_UNNECESSARY;
maintainer
#1 lore raw

From: Koki Sanagi <[email protected]>
Date: Wed, 23 Jan 2013 09:30:16 +0900

Quoted 6 lines

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 <[email protected]>

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.

author
#2 lore raw
Quoted 8 lines

From: Koki Sanagi <[email protected]>
Date: Wed, 23 Jan 2013 09:30:16 +0900

Quoted 5 lines

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.

Quoted 4 lines
Quoted 1 line
Signed-off-by Koki Sanagi <[email protected]>

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.

maintainer
#3 lore raw

From: "Sanagi, Koki" <[email protected]>
Date: Wed, 23 Jan 2013 02:32:52 +0000

Quoted 8 lines
Quoted 8 lines

From: Koki Sanagi <[email protected]>
Date: Wed, 23 Jan 2013 09:30:16 +0900

Quoted 5 lines

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.

Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Koki Sanagi <[email protected]>

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.

author
#4 lore raw
Quoted 13 lines

From: "Sanagi, Koki" <[email protected]>
Date: Wed, 23 Jan 2013 02:32:52 +0000

Quoted 10 lines
Quoted 10 lines

From: Koki Sanagi <[email protected]>
Date: Wed, 23 Jan 2013 09:30:16 +0900

Quoted 6 lines

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 <[email protected]>
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

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 ?

include/linux/skbuff.h +3 -1
index 8b2256e..b1dbd36 100644
@@ -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;
 }
 
 /**
maintainer
#5 lore raw

From: "Sanagi, Koki" <[email protected]>
Date: Wed, 23 Jan 2013 05:11:14 +0000

Quoted 4 lines

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.