[Patch net-next] net: clean up skb headers code

[Patch net-next] net: clean up skb headers code

[Cong Wang]

commit 1a37e412a0225fcba5587 (net: Use 16bits for *_headers
fields of struct skbuff) converts skb->*_header to u16,
therefore 1) we could just use 0xFFFFF instead of (__u16) ~0U
2) some #if NET_SKBUFF_DATA_USES_OFFSET is useless now.


Cc: David S. Miller <davem@davemloft.net>
Cc: Simon Horman <horms@verge.net.au>
Signed-off-by: Cong Wang <amwang@redhat.com>

---
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 8f2b830..3c896b7 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1584,7 +1584,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 != 0xFFFF;
 }
 
 static inline unsigned char *skb_transport_header(const struct sk_buff *skb)
@@ -1627,7 +1627,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 != 0xFFFF;
 }
 
 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..0c3742f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -199,9 +199,7 @@ struct sk_buff *__alloc_skb_head(gfp_t gfp_mask, int node)
 	skb->truesize = sizeof(struct sk_buff);
 	atomic_set(&skb->users, 1);
 
-#ifdef NET_SKBUFF_DATA_USES_OFFSET
-	skb->mac_header = (__u16) ~0U;
-#endif
+	skb->mac_header = 0xFFFF;
 out:
 	return skb;
 }
@@ -275,10 +273,8 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
 	skb->data = data;
 	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;
-#endif
+	skb->mac_header = 0xFFFF;
+	skb->transport_header = 0xFFFF;
 
 	/* make sure we initialize shinfo sequentially */
 	shinfo = skb_shinfo(skb);
@@ -344,10 +340,8 @@ struct sk_buff *build_skb(void *data, unsigned int frag_size)
 	skb->data = data;
 	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;
-#endif
+	skb->mac_header = 0xFFFF;
+	skb->transport_header = 0xFFFF;
 
 	/* make sure we initialize shinfo sequentially */
 	shinfo = skb_shinfo(skb);

Re: [Patch net-next] net: clean up skb headers code

[David Miller]

From: Cong Wang <amwang@redhat.com>
Date: Wed, 29 May 2013 14:09:00 +0800

> commit 1a37e412a0225fcba5587 (net: Use 16bits for *_headers
> fields of struct skbuff) converts skb->*_header to u16,
> therefore 1) we could just use 0xFFFFF instead of (__u16) ~0U
> 2) some #if NET_SKBUFF_DATA_USES_OFFSET is useless now.
> 
> 
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Simon Horman <horms@verge.net.au>
> Signed-off-by: Cong Wang <amwang@redhat.com>

I want to use something that will either break the build or
automatically work if the type changes again.

So something like "X = (typeof(X)) ~0U;".

Re: [Patch net-next] net: clean up skb headers code

[Cong Wang]

On Tue, 2013-05-28 at 23:36 -0700, David Miller wrote:
> From: Cong Wang <amwang@redhat.com>
> Date: Wed, 29 May 2013 14:09:00 +0800
> 
> > commit 1a37e412a0225fcba5587 (net: Use 16bits for *_headers
> > fields of struct skbuff) converts skb->*_header to u16,
> > therefore 1) we could just use 0xFFFFF instead of (__u16) ~0U
> > 2) some #if NET_SKBUFF_DATA_USES_OFFSET is useless now.
> > 
> > 
> > Cc: David S. Miller <davem@davemloft.net>
> > Cc: Simon Horman <horms@verge.net.au>
> > Signed-off-by: Cong Wang <amwang@redhat.com>
> 
> I want to use something that will either break the build or
> automatically work if the type changes again.
> 
> So something like "X = (typeof(X)) ~0U;".

Yeah, this is even better. I will update this patch.

Thanks.

Re: [Patch net-next] net: clean up skb headers code

[David Miller]

From: Cong Wang <amwang@redhat.com>
Date: Wed, 29 May 2013 14:48:06 +0800

> On Tue, 2013-05-28 at 23:36 -0700, David Miller wrote:
>> From: Cong Wang <amwang@redhat.com>
>> Date: Wed, 29 May 2013 14:09:00 +0800
>> 
>> > commit 1a37e412a0225fcba5587 (net: Use 16bits for *_headers
>> > fields of struct skbuff) converts skb->*_header to u16,
>> > therefore 1) we could just use 0xFFFFF instead of (__u16) ~0U
>> > 2) some #if NET_SKBUFF_DATA_USES_OFFSET is useless now.
>> > 
>> > 
>> > Cc: David S. Miller <davem@davemloft.net>
>> > Cc: Simon Horman <horms@verge.net.au>
>> > Signed-off-by: Cong Wang <amwang@redhat.com>
>> 
>> I want to use something that will either break the build or
>> automatically work if the type changes again.
>> 
>> So something like "X = (typeof(X)) ~0U;".
> 
> Yeah, this is even better. I will update this patch.

Please sumbit it against Simon's series, which I'm about to push to
net-next.

Thanks.

Re: [Patch net-next] net: clean up skb headers code

[Ben Hutchings]

On Tue, 2013-05-28 at 23:36 -0700, David Miller wrote:
> From: Cong Wang <amwang@redhat.com>
> Date: Wed, 29 May 2013 14:09:00 +0800
> 
> > commit 1a37e412a0225fcba5587 (net: Use 16bits for *_headers
> > fields of struct skbuff) converts skb->*_header to u16,
> > therefore 1) we could just use 0xFFFFF instead of (__u16) ~0U
> > 2) some #if NET_SKBUFF_DATA_USES_OFFSET is useless now.
> > 
> > 
> > Cc: David S. Miller <davem@davemloft.net>
> > Cc: Simon Horman <horms@verge.net.au>
> > Signed-off-by: Cong Wang <amwang@redhat.com>
> 
> I want to use something that will either break the build or
> automatically work if the type changes again.
> 
> So something like "X = (typeof(X)) ~0U;".

I think you mean ~(typeof(X))0.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [Patch net-next] net: clean up skb headers code

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1144 bytes --]

On Wed, May 29, 2013 at 08:34:33PM +0100, Ben Hutchings wrote:
> On Tue, 2013-05-28 at 23:36 -0700, David Miller wrote:
> > From: Cong Wang <amwang@redhat.com>
> > Date: Wed, 29 May 2013 14:09:00 +0800
> > 
> > > commit 1a37e412a0225fcba5587 (net: Use 16bits for *_headers
> > > fields of struct skbuff) converts skb->*_header to u16,
> > > therefore 1) we could just use 0xFFFFF instead of (__u16) ~0U
> > > 2) some #if NET_SKBUFF_DATA_USES_OFFSET is useless now.
> > > 
> > > 
> > > Cc: David S. Miller <davem@davemloft.net>
> > > Cc: Simon Horman <horms@verge.net.au>
> > > Signed-off-by: Cong Wang <amwang@redhat.com>
> > 
> > I want to use something that will either break the build or
> > automatically work if the type changes again.
> > 
> > So something like "X = (typeof(X)) ~0U;".
> 
> I think you mean ~(typeof(X))0.

Am I wrong or you should cast the value once again, like this:

((typeof(X))~(typeof(X))0)

because the ~ operator will implicitly cast the argument to int (if I remember
correctly).


Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Patch net-next] net: clean up skb headers code

[Ben Hutchings]

On Wed, 2013-05-29 at 21:44 +0200, Antonio Quartulli wrote:
> On Wed, May 29, 2013 at 08:34:33PM +0100, Ben Hutchings wrote:
> > On Tue, 2013-05-28 at 23:36 -0700, David Miller wrote:
> > > From: Cong Wang <amwang@redhat.com>
> > > Date: Wed, 29 May 2013 14:09:00 +0800
> > > 
> > > > commit 1a37e412a0225fcba5587 (net: Use 16bits for *_headers
> > > > fields of struct skbuff) converts skb->*_header to u16,
> > > > therefore 1) we could just use 0xFFFFF instead of (__u16) ~0U
> > > > 2) some #if NET_SKBUFF_DATA_USES_OFFSET is useless now.
> > > > 
> > > > 
> > > > Cc: David S. Miller <davem@davemloft.net>
> > > > Cc: Simon Horman <horms@verge.net.au>
> > > > Signed-off-by: Cong Wang <amwang@redhat.com>
> > > 
> > > I want to use something that will either break the build or
> > > automatically work if the type changes again.
> > > 
> > > So something like "X = (typeof(X)) ~0U;".
> > 
> > I think you mean ~(typeof(X))0.
> 
> Am I wrong or you should cast the value once again, like this:
> 
> ((typeof(X))~(typeof(X))0)
> 
> because the ~ operator will implicitly cast the argument to int (if I remember
> correctly).

It will promote to at least int, but that still results in the right
value and doesn't provoke a warning.  Try this test (with -Wall
-Wextra):

#include <stdio.h>

#define ALL_ONES(v) (~(typeof(v))0)
/* #define ALL_ONES(v) (typeof(v))~0U) */

int main(void)
{
    char a = ALL_ONES(a);
    unsigned char b = ALL_ONES(b);
    short c = ALL_ONES(c);
    unsigned short d = ALL_ONES(d);
    int e = ALL_ONES(e);
    unsigned int f = ALL_ONES(f);
    long g = ALL_ONES(g);
    unsigned long h = ALL_ONES(h);
    long long i = ALL_ONES(i);
    unsigned long long j = ALL_ONES(j);

    printf("%hhx %hhx %hx %hx %x %x %lx %lx %llx %llx\n",
	   a, b, c, d, e, f, g, h, i, j);
    return 0;
}

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [Patch net-next] net: clean up skb headers code

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1803 bytes --]

On Wed, May 29, 2013 at 10:02:06PM +0100, Ben Hutchings wrote:
> On Wed, 2013-05-29 at 21:44 +0200, Antonio Quartulli wrote:
> > On Wed, May 29, 2013 at 08:34:33PM +0100, Ben Hutchings wrote:
> > > On Tue, 2013-05-28 at 23:36 -0700, David Miller wrote:
> > > > From: Cong Wang <amwang@redhat.com>
> > > > Date: Wed, 29 May 2013 14:09:00 +0800
> > > > 
> > > > > commit 1a37e412a0225fcba5587 (net: Use 16bits for *_headers
> > > > > fields of struct skbuff) converts skb->*_header to u16,
> > > > > therefore 1) we could just use 0xFFFFF instead of (__u16) ~0U
> > > > > 2) some #if NET_SKBUFF_DATA_USES_OFFSET is useless now.
> > > > > 
> > > > > 
> > > > > Cc: David S. Miller <davem@davemloft.net>
> > > > > Cc: Simon Horman <horms@verge.net.au>
> > > > > Signed-off-by: Cong Wang <amwang@redhat.com>
> > > > 
> > > > I want to use something that will either break the build or
> > > > automatically work if the type changes again.
> > > > 
> > > > So something like "X = (typeof(X)) ~0U;".
> > > 
> > > I think you mean ~(typeof(X))0.
> > 
> > Am I wrong or you should cast the value once again, like this:
> > 
> > ((typeof(X))~(typeof(X))0)
> > 
> > because the ~ operator will implicitly cast the argument to int (if I remember
> > correctly).
> 
> It will promote to at least int, but that still results in the right
> value and doesn't provoke a warning.  Try this test (with -Wall
> -Wextra):

thanks for the test code Ben.

Yeah it gives the results you expected. Then I don't know why somebody suggested
us to do this double cast in batman-adv when we wanted to obtain the same.

At this point I'll remove the double cast in our code too.

Thanks.

Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [Patch net-next] net: clean up skb headers code

[David Laight]

> > > > I want to use something that will either break the build or
> > > > automatically work if the type changes again.
> > > >
> > > > So something like "X = (typeof(X)) ~0U;".
> > >
> > > I think you mean ~(typeof(X))0.
> >
> > Am I wrong or you should cast the value once again, like this:
> >
> > ((typeof(X))~(typeof(X))0)
> >
> > because the ~ operator will implicitly cast the argument to int (if I remember
> > correctly).
> 
> It will promote to at least int, but that still results in the right
> value and doesn't provoke a warning.  Try this test (with -Wall
> -Wextra):
> 
> #include <stdio.h>
> 
> #define ALL_ONES(v) (~(typeof(v))0)
> /* #define ALL_ONES(v) (typeof(v))~0U) */
> 
> int main(void)
> {
>     char a = ALL_ONES(a);
>     unsigned char b = ALL_ONES(b);
>     short c = ALL_ONES(c);
>     unsigned short d = ALL_ONES(d);
>     int e = ALL_ONES(e);
>     unsigned int f = ALL_ONES(f);
>     long g = ALL_ONES(g);
>     unsigned long h = ALL_ONES(h);
>     long long i = ALL_ONES(i);
>     unsigned long long j = ALL_ONES(j);
> 
>     printf("%hhx %hhx %hx %hx %x %x %lx %lx %llx %llx\n",
> 	   a, b, c, d, e, f, g, h, i, j);
>     return 0;
> }

The printf format is masking the high bits for you.
If you change to:
	printf("%x %x %x %x %x %x %lx %lx %llx %llx\n", ...)
you get different values.
To get the 'expected' values you need casts both sides of the ~

	David

Re: [Patch net-next] net: clean up skb headers code

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1843 bytes --]

On Thu, May 30, 2013 at 12:41:03PM +0100, David Laight wrote:
> > > > > I want to use something that will either break the build or
> > > > > automatically work if the type changes again.
> > > > >
> > > > > So something like "X = (typeof(X)) ~0U;".
> > > >
> > > > I think you mean ~(typeof(X))0.
> > >
> > > Am I wrong or you should cast the value once again, like this:
> > >
> > > ((typeof(X))~(typeof(X))0)
> > >
> > > because the ~ operator will implicitly cast the argument to int (if I remember
> > > correctly).
> > 
> > It will promote to at least int, but that still results in the right
> > value and doesn't provoke a warning.  Try this test (with -Wall
> > -Wextra):
> > 
> > #include <stdio.h>
> > 
> > #define ALL_ONES(v) (~(typeof(v))0)
> > /* #define ALL_ONES(v) (typeof(v))~0U) */
> > 
> > int main(void)
> > {
> >     char a = ALL_ONES(a);
> >     unsigned char b = ALL_ONES(b);
> >     short c = ALL_ONES(c);
> >     unsigned short d = ALL_ONES(d);
> >     int e = ALL_ONES(e);
> >     unsigned int f = ALL_ONES(f);
> >     long g = ALL_ONES(g);
> >     unsigned long h = ALL_ONES(h);
> >     long long i = ALL_ONES(i);
> >     unsigned long long j = ALL_ONES(j);
> > 
> >     printf("%hhx %hhx %hx %hx %x %x %lx %lx %llx %llx\n",
> > 	   a, b, c, d, e, f, g, h, i, j);
> >     return 0;
> > }
> 
> The printf format is masking the high bits for you.
> If you change to:
> 	printf("%x %x %x %x %x %x %lx %lx %llx %llx\n", ...)
> you get different values.
> To get the 'expected' values you need casts both sides of the ~

I tried with

19     printf("%x %x %x %x %x %x %lx %lx %llx %llx\n",

but I still see all FF.

Somewhere else we have some hidden conversion?

Cheers,


-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [Patch net-next] net: clean up skb headers code

[David Laight]

> > > > > > I want to use something that will either break the build or
> > > > > > automatically work if the type changes again.
> > > > > >
> > > > > > So something like "X = (typeof(X)) ~0U;".
> > > > >
> > > > > I think you mean ~(typeof(X))0.
> > > >
> > > > Am I wrong or you should cast the value once again, like this:
> > > >
> > > > ((typeof(X))~(typeof(X))0)
> > > >
> > > > because the ~ operator will implicitly cast the argument to int (if I remember
> > > > correctly).
> > >
> > > It will promote to at least int, but that still results in the right
> > > value and doesn't provoke a warning.  Try this test (with -Wall
> > > -Wextra):
> > >
> > > #include <stdio.h>
> > >
> > > #define ALL_ONES(v) (~(typeof(v))0)
> > > /* #define ALL_ONES(v) (typeof(v))~0U) */
> > >
> > > int main(void)
> > > {
> > >     char a = ALL_ONES(a);
> > >     unsigned char b = ALL_ONES(b);
> > >     short c = ALL_ONES(c);
> > >     unsigned short d = ALL_ONES(d);
> > >     int e = ALL_ONES(e);
> > >     unsigned int f = ALL_ONES(f);
> > >     long g = ALL_ONES(g);
> > >     unsigned long h = ALL_ONES(h);
> > >     long long i = ALL_ONES(i);
> > >     unsigned long long j = ALL_ONES(j);
> > >
> > >     printf("%hhx %hhx %hx %hx %x %x %lx %lx %llx %llx\n",
> > > 	   a, b, c, d, e, f, g, h, i, j);
> > >     return 0;
> > > }
> >
> > The printf format is masking the high bits for you.
> > If you change to:
> > 	printf("%x %x %x %x %x %x %lx %lx %llx %llx\n", ...)
> > you get different values.
> > To get the 'expected' values you need casts both sides of the ~
> 
> I tried with
> 
> 19     printf("%x %x %x %x %x %x %lx %lx %llx %llx\n",
> 
> but I still see all FF.
> 
> Somewhere else we have some hidden conversion?

Of course - we need to look at the value that ALL_ONES()
generates in an expression. So the test print needs to be:
	printf("%hhx %hhx %hx %hx %x %x %lx %lx %llx %llx\n",
		ALL_ONES(a), ALL_ONES(b), ALL_ONES(c), ALL_ONES(d),
		ALL_ONES(e), ALL_ONES(f), ALL_ONES(g), ALL_ONES(h),
		ALL_ONES(i), ALL_ONES(j));
which then gives 0xffffffff for the first 4 entries.

	David

RE: [Patch net-next] net: clean up skb headers code

[David Laight]

> Of course - we need to look at the value that ALL_ONES()
> generates in an expression. So the test print needs to be:
> 	printf("%hhx %hhx %hx %hx %x %x %lx %lx %llx %llx\n",
> 		ALL_ONES(a), ALL_ONES(b), ALL_ONES(c), ALL_ONES(d),
> 		ALL_ONES(e), ALL_ONES(f), ALL_ONES(g), ALL_ONES(h),
> 		ALL_ONES(i), ALL_ONES(j));
> which then gives 0xffffffff for the first 4 entries.

Except I copied the wrong format line :-( It should be:
 	printf("%x %x %x %x %x %x %lx %lx %llx %llx\n",
 		ALL_ONES(a), ALL_ONES(b), ALL_ONES(c), ALL_ONES(d),
 		ALL_ONES(e), ALL_ONES(f), ALL_ONES(g), ALL_ONES(h),
 		ALL_ONES(i), ALL_ONES(j));

	David

Re: [Patch net-next] net: clean up skb headers code

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

On Thu, May 30, 2013 at 01:22:14PM +0100, David Laight wrote:
> > Of course - we need to look at the value that ALL_ONES()
> > generates in an expression. So the test print needs to be:
> > 	printf("%hhx %hhx %hx %hx %x %x %lx %lx %llx %llx\n",
> > 		ALL_ONES(a), ALL_ONES(b), ALL_ONES(c), ALL_ONES(d),
> > 		ALL_ONES(e), ALL_ONES(f), ALL_ONES(g), ALL_ONES(h),
> > 		ALL_ONES(i), ALL_ONES(j));
> > which then gives 0xffffffff for the first 4 entries.
> 
> Except I copied the wrong format line :-( It should be:
>  	printf("%x %x %x %x %x %x %lx %lx %llx %llx\n",
>  		ALL_ONES(a), ALL_ONES(b), ALL_ONES(c), ALL_ONES(d),
>  		ALL_ONES(e), ALL_ONES(f), ALL_ONES(g), ALL_ONES(h),
>  		ALL_ONES(i), ALL_ONES(j));

Oh ok, I see the issue now.

So the problem is that this value gets bigger than what the maximal
value for the type passed as argument is supposed to be.

I think the issue is clear now and why a double cast is good.

Cheers,


-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [Patch net-next] net: clean up skb headers code

[David Laight]

> > Except I copied the wrong format line :-( It should be:
> >  	printf("%x %x %x %x %x %x %lx %lx %llx %llx\n",
> >  		ALL_ONES(a), ALL_ONES(b), ALL_ONES(c), ALL_ONES(d),
> >  		ALL_ONES(e), ALL_ONES(f), ALL_ONES(g), ALL_ONES(h),
> >  		ALL_ONES(i), ALL_ONES(j));
> 
> Oh ok, I see the issue now.
> 
> So the problem is that this value gets bigger than what the maximal
> value for the type passed as argument is supposed to be.
> 
> I think the issue is clear now and why a double cast is good.

It is much clearer to just use 0xffff.
Although a named #define constant is more appropriate
for the code itself.

	David

Re: [Patch net-next] net: clean up skb headers code

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 2556 bytes --]

On Thu, May 30, 2013 at 01:12:15PM +0100, David Laight wrote:
> > > > > > > I want to use something that will either break the build or
> > > > > > > automatically work if the type changes again.
> > > > > > >
> > > > > > > So something like "X = (typeof(X)) ~0U;".
> > > > > >
> > > > > > I think you mean ~(typeof(X))0.
> > > > >
> > > > > Am I wrong or you should cast the value once again, like this:
> > > > >
> > > > > ((typeof(X))~(typeof(X))0)
> > > > >
> > > > > because the ~ operator will implicitly cast the argument to int (if I remember
> > > > > correctly).
> > > >
> > > > It will promote to at least int, but that still results in the right
> > > > value and doesn't provoke a warning.  Try this test (with -Wall
> > > > -Wextra):
> > > >
> > > > #include <stdio.h>
> > > >
> > > > #define ALL_ONES(v) (~(typeof(v))0)
> > > > /* #define ALL_ONES(v) (typeof(v))~0U) */
> > > >
> > > > int main(void)
> > > > {
> > > >     char a = ALL_ONES(a);
> > > >     unsigned char b = ALL_ONES(b);
> > > >     short c = ALL_ONES(c);
> > > >     unsigned short d = ALL_ONES(d);
> > > >     int e = ALL_ONES(e);
> > > >     unsigned int f = ALL_ONES(f);
> > > >     long g = ALL_ONES(g);
> > > >     unsigned long h = ALL_ONES(h);
> > > >     long long i = ALL_ONES(i);
> > > >     unsigned long long j = ALL_ONES(j);
> > > >
> > > >     printf("%hhx %hhx %hx %hx %x %x %lx %lx %llx %llx\n",
> > > > 	   a, b, c, d, e, f, g, h, i, j);
> > > >     return 0;
> > > > }
> > >
> > > The printf format is masking the high bits for you.
> > > If you change to:
> > > 	printf("%x %x %x %x %x %x %lx %lx %llx %llx\n", ...)
> > > you get different values.
> > > To get the 'expected' values you need casts both sides of the ~
> > 
> > I tried with
> > 
> > 19     printf("%x %x %x %x %x %x %lx %lx %llx %llx\n",
> > 
> > but I still see all FF.
> > 
> > Somewhere else we have some hidden conversion?
> 
> Of course - we need to look at the value that ALL_ONES()
> generates in an expression. So the test print needs to be:
> 	printf("%hhx %hhx %hx %hx %x %x %lx %lx %llx %llx\n",
> 		ALL_ONES(a), ALL_ONES(b), ALL_ONES(c), ALL_ONES(d),
> 		ALL_ONES(e), ALL_ONES(f), ALL_ONES(g), ALL_ONES(h),
> 		ALL_ONES(i), ALL_ONES(j));
> which then gives 0xffffffff for the first 4 entries.

It gives all FF for every argument. So, I'm not understanding the problem..this
is what Ben wanted to achieve, no?

Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Patch net-next] net: clean up skb headers code

[Cong Wang]

On Wed, 2013-05-29 at 20:34 +0100, Ben Hutchings wrote:
> On Tue, 2013-05-28 at 23:36 -0700, David Miller wrote:
> > 
> > So something like "X = (typeof(X)) ~0U;".
> 
> I think you mean ~(typeof(X))0.
> 

Good catch, I will change it to ~(typeof(X))0.

Re: [Patch net-next] net: clean up skb headers code

[Sergei Shtylyov]

Hello.

On 30-05-2013 5:59, Cong Wang wrote:

>>> So something like "X = (typeof(X)) ~0U;".

>> I think you mean ~(typeof(X))0.


> Good catch,

    I doubt it.

> I will change it to ~(typeof(X))0.

    Then (typeof(X))0 will be promoted to *int* before doing ~.

WBR, Sergei

Re: [Patch net-next] net: clean up skb headers code

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Wed, 29 May 2013 20:34:33 +0100

> On Tue, 2013-05-28 at 23:36 -0700, David Miller wrote:
>> From: Cong Wang <amwang@redhat.com>
>> Date: Wed, 29 May 2013 14:09:00 +0800
>> 
>> > commit 1a37e412a0225fcba5587 (net: Use 16bits for *_headers
>> > fields of struct skbuff) converts skb->*_header to u16,
>> > therefore 1) we could just use 0xFFFFF instead of (__u16) ~0U
>> > 2) some #if NET_SKBUFF_DATA_USES_OFFSET is useless now.
>> > 
>> > 
>> > Cc: David S. Miller <davem@davemloft.net>
>> > Cc: Simon Horman <horms@verge.net.au>
>> > Signed-off-by: Cong Wang <amwang@redhat.com>
>> 
>> I want to use something that will either break the build or
>> automatically work if the type changes again.
>> 
>> So something like "X = (typeof(X)) ~0U;".
> 
> I think you mean ~(typeof(X))0.

Take a look at what that does for ~(u8)0.

We had a bug fix in this are recently in ieee802154 if I remember
correctly.

Re: [Patch net-next] net: clean up skb headers code

[Ben Hutchings]

On Thu, 2013-05-30 at 03:40 -0700, David Miller wrote:
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Wed, 29 May 2013 20:34:33 +0100
> 
> > On Tue, 2013-05-28 at 23:36 -0700, David Miller wrote:
> >> From: Cong Wang <amwang@redhat.com>
> >> Date: Wed, 29 May 2013 14:09:00 +0800
> >> 
> >> > commit 1a37e412a0225fcba5587 (net: Use 16bits for *_headers
> >> > fields of struct skbuff) converts skb->*_header to u16,
> >> > therefore 1) we could just use 0xFFFFF instead of (__u16) ~0U
> >> > 2) some #if NET_SKBUFF_DATA_USES_OFFSET is useless now.
> >> > 
> >> > 
> >> > Cc: David S. Miller <davem@davemloft.net>
> >> > Cc: Simon Horman <horms@verge.net.au>
> >> > Signed-off-by: Cong Wang <amwang@redhat.com>
> >> 
> >> I want to use something that will either break the build or
> >> automatically work if the type changes again.
> >> 
> >> So something like "X = (typeof(X)) ~0U;".
> > 
> > I think you mean ~(typeof(X))0.
> 
> Take a look at what that does for ~(u8)0.
> 
> We had a bug fix in this are recently in ieee802154 if I remember
> correctly.

It's fine if you assign the result to X.  The second cast is needed if
you need the temporary value to have the type of X, but that's not
needed here.  (Of course it may be promoted as soon as you do anything
other than assign to X...)

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.