| Message ID | 51AC7CD4.2060801@asianux.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Jun 3, 2013 at 2:24 PM, Chen Gang <gang.chen@asianux.com> wrote: > > Both 'transport_header' and 'mac_header' are u16, which are never equal > to '~0U'. > > So need use '(u16) ~0U' instead of '~0U'. > > The related warning (with EXTRA_CFLAGS=-W ARCH=m68k for allmodconfig) > include/linux/skbuff.h:1587:2: warning: comparison is always true due to limited range of data type [-Wtype-limits] > ... > > Use meaningful macro instead of hard code number, and better to > initialize 'skb->transport_header' in __alloc_skb_head(), too. Looks okay. Couple of questions below. > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -40,6 +40,8 @@ > #define CHECKSUM_COMPLETE 2 > #define CHECKSUM_PARTIAL 3 > > +#define SKB_HEADER_UNSET_16 ((unsigned short) ~0U) Isn't better to use the same type as used in the structure description? > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -200,7 +200,8 @@ struct sk_buff *__alloc_skb_head(gfp_t gfp_mask, int node) > atomic_set(&skb->users, 1); > > #ifdef NET_SKBUFF_DATA_USES_OFFSET > - skb->mac_header = (__u16) ~0U; > + skb->mac_header = SKB_HEADER_UNSET_16; > + skb->transport_header = SKB_HEADER_UNSET_16; Is it correct to assign transport_header here as well? -- With Best Regards, Andy Shevchenko -- 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
On 06/03/2013 07:34 PM, Andy Shevchenko wrote: >> --- a/include/linux/skbuff.h >> > +++ b/include/linux/skbuff.h >> > @@ -40,6 +40,8 @@ >> > #define CHECKSUM_COMPLETE 2 >> > #define CHECKSUM_PARTIAL 3 >> > >> > +#define SKB_HEADER_UNSET_16 ((unsigned short) ~0U) > Isn't better to use the same type as used in the structure description? > It sounds reasonable, I will wait 1 days, if no additional suggestions or completions, I will send patch v3. >> > --- a/net/core/skbuff.c >> > +++ b/net/core/skbuff.c >> > @@ -200,7 +200,8 @@ struct sk_buff *__alloc_skb_head(gfp_t gfp_mask, int node) >> > atomic_set(&skb->users, 1); >> > >> > #ifdef NET_SKBUFF_DATA_USES_OFFSET >> > - skb->mac_header = (__u16) ~0U; >> > + skb->mac_header = SKB_HEADER_UNSET_16; >> > + skb->transport_header = SKB_HEADER_UNSET_16; > Is it correct to assign transport_header here as well? At least, it is correct: they are in the same structure, and almost a neighbor with each other. Hmm... I guess that may be useless currently which will waste one instruction. But I still suggest to add it, it can avoid the new mistakes in the future. Welcome another members to provide their suggestions for it. Thanks.
> > +#define SKB_HEADER_UNSET_16 ((unsigned short) ~0U) > > + > > The _16 part isn't really correct, the type could be changed > and then it would be wrong. > > I think I might have used SKB_HEADER_OFFSET. I meant SKB_HEADER_UNSET_OFFSET or SKB_HEADER_OFFSET_UNSET > NrybXǧv^){.n+z^)w*jg ݢj/zޖ2ޙ&)ߡa Ghj:+vw٥ I've NFI what adds this! David
On 06/03/2013 08:47 PM, David Laight wrote: >>> +#define SKB_HEADER_UNSET_16 ((unsigned short) ~0U) >>> > > + >> > >> > The _16 part isn't really correct, the type could be changed >> > and then it would be wrong. >> > >> > I think I might have used SKB_HEADER_OFFSET. > I meant SKB_HEADER_UNSET_OFFSET or SKB_HEADER_OFFSET_UNSET > It sound good, I choose the 2nd: 'SKB_HEADER_OFFSET_UNSET', for it may be a 'noun', not a 'sentence'. I will send patch v3 (also use '__u16' instead of 'unsigned short') Thanks.
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 5f93119..59493f8 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -40,6 +40,8 @@ #define CHECKSUM_COMPLETE 2 #define CHECKSUM_PARTIAL 3 +#define SKB_HEADER_UNSET_16 ((unsigned short) ~0U) + #define SKB_DATA_ALIGN(X) (((X) + (SMP_CACHE_BYTES - 1)) & \ ~(SMP_CACHE_BYTES - 1)) #define SKB_WITH_OVERHEAD(X) \ @@ -1593,7 +1595,7 @@ static inline void skb_set_inner_mac_header(struct sk_buff *skb, } static inline bool skb_transport_header_was_set(const struct sk_buff *skb) { - return skb->transport_header != ~0U; + return skb->transport_header != SKB_HEADER_UNSET_16; } static inline unsigned char *skb_transport_header(const struct sk_buff *skb) @@ -1636,7 +1638,7 @@ static inline unsigned char *skb_mac_header(const struct sk_buff *skb) static inline int skb_mac_header_was_set(const struct sk_buff *skb) { - return skb->mac_header != ~0U; + return skb->mac_header != SKB_HEADER_UNSET_16; } static inline void skb_reset_mac_header(struct sk_buff *skb) diff --git a/net/core/skbuff.c b/net/core/skbuff.c index f45de07..18c6974 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -200,7 +200,8 @@ struct sk_buff *__alloc_skb_head(gfp_t gfp_mask, int node) atomic_set(&skb->users, 1); #ifdef NET_SKBUFF_DATA_USES_OFFSET - skb->mac_header = (__u16) ~0U; + skb->mac_header = SKB_HEADER_UNSET_16; + skb->transport_header = SKB_HEADER_UNSET_16; #endif out: return skb; @@ -276,8 +277,8 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask, skb_reset_tail_pointer(skb); skb->end = skb->tail + size; #ifdef NET_SKBUFF_DATA_USES_OFFSET - skb->mac_header = (__u16) ~0U; - skb->transport_header = (__u16) ~0U; + skb->mac_header = SKB_HEADER_UNSET_16; + skb->transport_header = SKB_HEADER_UNSET_16; #endif /* make sure we initialize shinfo sequentially */ @@ -345,8 +346,8 @@ struct sk_buff *build_skb(void *data, unsigned int frag_size) skb_reset_tail_pointer(skb); skb->end = skb->tail + size; #ifdef NET_SKBUFF_DATA_USES_OFFSET - skb->mac_header = (__u16) ~0U; - skb->transport_header = (__u16) ~0U; + skb->mac_header = SKB_HEADER_UNSET_16; + skb->transport_header = SKB_HEADER_UNSET_16; #endif /* make sure we initialize shinfo sequentially */
Both 'transport_header' and 'mac_header' are u16, which are never equal to '~0U'. So need use '(u16) ~0U' instead of '~0U'. The related warning (with EXTRA_CFLAGS=-W ARCH=m68k for allmodconfig) include/linux/skbuff.h:1587:2: warning: comparison is always true due to limited range of data type [-Wtype-limits] ... Use meaningful macro instead of hard code number, and better to initialize 'skb->transport_header' in __alloc_skb_head(), too. Signed-off-by: Chen Gang <gang.chen@asianux.com> --- include/linux/skbuff.h | 6 ++++-- net/core/skbuff.c | 11 ++++++----- 2 files changed, 10 insertions(+), 7 deletions(-)