[PATCH] net: define __packed for the userspace code

[PATCH] net: define __packed for the userspace code

[Changli Gao]

This commit

 commit bc10502dba37d3b210efd9f3867212298f13b78e
 Author: Eric Dumazet <eric.dumazet@gmail.com>
 Date:   Thu Jun 3 03:21:52 2010 -0700

    net: use __packed annotation

makes use of __packed in the userspace code. So we'd better define __packed
for the userspace code too.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 include/linux/compiler.h |    4 ++++
 include/linux/if_pppox.h |    4 ++--
 2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index c1a62c5..515b174 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -304,4 +304,8 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
  */
 #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
 
+#ifndef __packed
+# define __packed __attribute__((packed))
+#endif
+
 #endif /* __LINUX_COMPILER_H */

Re: [PATCH] net: define __packed for the userspace code

[Changli Gao]

On Sun, Aug 22, 2010 at 7:12 PM, Changli Gao <xiaosuo@gmail.com> wrote:
> This commit
>
>  commit bc10502dba37d3b210efd9f3867212298f13b78e
>  Author: Eric Dumazet <eric.dumazet@gmail.com>
>  Date:   Thu Jun 3 03:21:52 2010 -0700
>
>    net: use __packed annotation
>
> makes use of __packed in the userspace code. So we'd better define __packed
> for the userspace code too.
>

Oh, sorry. This patch can't work as include/linux/compiler.h isn't
exported to the userspace. But where should we define __packed for the
userspace code? include/linux/types.h?


-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH] net: define __packed for the userspace code

[Arnd Bergmann]

On Sunday 22 August 2010 13:23:42 Changli Gao wrote:
> On Sun, Aug 22, 2010 at 7:12 PM, Changli Gao <xiaosuo@gmail.com> wrote:
> > This commit
> >
> >  commit bc10502dba37d3b210efd9f3867212298f13b78e
> >  Author: Eric Dumazet <eric.dumazet@gmail.com>
> >  Date:   Thu Jun 3 03:21:52 2010 -0700
> >
> >    net: use __packed annotation
> >
> > makes use of __packed in the userspace code. So we'd better define __packed
> > for the userspace code too.
> >
> 
> Oh, sorry. This patch can't work as include/linux/compiler.h isn't
> exported to the userspace. But where should we define __packed for the
> userspace code? include/linux/types.h?

I would try to avoid making those structures packed to start with.
>From what I can see, they structures annotated in the above commit
mostly don't even require explicit packing because they are already
packed. Not marking them packed makes the code portable to non-gcc
compilers.

	Arnd

Re: [PATCH] net: define __packed for the userspace code

[Changli Gao]

On Sun, Aug 22, 2010 at 9:07 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Sunday 22 August 2010 13:23:42 Changli Gao wrote:
>> On Sun, Aug 22, 2010 at 7:12 PM, Changli Gao <xiaosuo@gmail.com> wrote:
>> > This commit
>> >
>> >  commit bc10502dba37d3b210efd9f3867212298f13b78e
>> >  Author: Eric Dumazet <eric.dumazet@gmail.com>
>> >  Date:   Thu Jun 3 03:21:52 2010 -0700
>> >
>> >    net: use __packed annotation
>> >
>> > makes use of __packed in the userspace code. So we'd better define __packed
>> > for the userspace code too.
>> >
>>
>> Oh, sorry. This patch can't work as include/linux/compiler.h isn't
>> exported to the userspace. But where should we define __packed for the
>> userspace code? include/linux/types.h?
>
> I would try to avoid making those structures packed to start with.
> From what I can see, they structures annotated in the above commit
> mostly don't even require explicit packing because they are already
> packed. Not marking them packed makes the code portable to non-gcc
> compilers.
>

Maybe __packed is used somewhere to hint that some members of a
structure maybe unaligned.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH] net: define __packed for the userspace code

[David Miller]

From: Changli Gao <xiaosuo@gmail.com>
Date: Sun, 22 Aug 2010 21:47:06 +0800

> On Sun, Aug 22, 2010 at 9:07 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Sunday 22 August 2010 13:23:42 Changli Gao wrote:
>>> On Sun, Aug 22, 2010 at 7:12 PM, Changli Gao <xiaosuo@gmail.com> wrote:
>>> > This commit
>>> >
>>> >  commit bc10502dba37d3b210efd9f3867212298f13b78e
>>> >  Author: Eric Dumazet <eric.dumazet@gmail.com>
>>> >  Date:   Thu Jun 3 03:21:52 2010 -0700
>>> >
>>> >    net: use __packed annotation
>>> >
>>> > makes use of __packed in the userspace code. So we'd better define __packed
>>> > for the userspace code too.
>>> >
>>>
>>> Oh, sorry. This patch can't work as include/linux/compiler.h isn't
>>> exported to the userspace. But where should we define __packed for the
>>> userspace code? include/linux/types.h?
>>
>> I would try to avoid making those structures packed to start with.
>> From what I can see, they structures annotated in the above commit
>> mostly don't even require explicit packing because they are already
>> packed. Not marking them packed makes the code portable to non-gcc
>> compilers.
>>
> 
> Maybe __packed is used somewhere to hint that some members of a
> structure maybe unaligned.

I don't think this it the reason it was being used here.

Any, for one thing, we definitely cannot remove the existing packed
markers or else we will break every single userland tool out there
using these socket address structures.

Even the first two members (sa_family_t and unsigned int) will be
positioned differently if we remove the marker.

I suspect the packed attribute is there to make sure the pppo* socket
address structures fit within the generic socket address object size.
(see struct __kernel_sockaddr_storage and struct sockaddr).

As to the problem at-hand, I think we need to use __attribute__((packed))
here.  And that's what I'll commit into net-2.6 and net-next-2.6

Re: [PATCH] net: define __packed for the userspace code

[Changli Gao]

On Mon, Aug 23, 2010 at 9:36 AM, David Miller <davem@davemloft.net> wrote:
>
> I don't think this it the reason it was being used here.
>
> Any, for one thing, we definitely cannot remove the existing packed
> markers or else we will break every single userland tool out there
> using these socket address structures.
>
> Even the first two members (sa_family_t and unsigned int) will be
> positioned differently if we remove the marker.
>
> I suspect the packed attribute is there to make sure the pppo* socket
> address structures fit within the generic socket address object size.
> (see struct __kernel_sockaddr_storage and struct sockaddr).
>
> As to the problem at-hand, I think we need to use __attribute__((packed))
> here.  And that's what I'll commit into net-2.6 and net-next-2.6
>

Do you mean that use the __attribute__((packed)) annotation in all of
these files:

localhost linux # grep "\<__packed\>" usr/include/ -r | uniq
usr/include/linux/if_hippi.h:} __packed;
usr/include/linux/if_fddi.h:} __packed;
usr/include/linux/nbd.h:} __packed;
usr/include/linux/ncp.h:} __packed;
usr/include/linux/rfkill.h:} __packed;
usr/include/linux/if_pppox.h:} __packed;
usr/include/linux/phonet.h:} __packed;
usr/include/linux/ipv6.h:} __packed;    /* required for some archs */
usr/include/linux/ipv6.h:} __packed;
usr/include/linux/if_ether.h:} __packed;

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH] net: define __packed for the userspace code

[David Miller]

From: Changli Gao <xiaosuo@gmail.com>
Date: Mon, 23 Aug 2010 10:29:49 +0800

> Do you mean that use the __attribute__((packed)) annotation in all of
> these files:
> 
> localhost linux # grep "\<__packed\>" usr/include/ -r | uniq
> usr/include/linux/if_hippi.h:} __packed;
> usr/include/linux/if_fddi.h:} __packed;
> usr/include/linux/nbd.h:} __packed;
> usr/include/linux/ncp.h:} __packed;
> usr/include/linux/rfkill.h:} __packed;
> usr/include/linux/if_pppox.h:} __packed;
> usr/include/linux/phonet.h:} __packed;
> usr/include/linux/ipv6.h:} __packed;    /* required for some archs */
> usr/include/linux/ipv6.h:} __packed;
> usr/include/linux/if_ether.h:} __packed;

It seems so, yes.

There is no way that anybody has tried to compile anything in
userspace using these headers with the __packed usage there.

If they would, they would surely see a compile failure.

Re: [PATCH] net: define __packed for the userspace code

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Sun, 22 Aug 2010 19:36:09 -0700 (PDT)

> From: Changli Gao <xiaosuo@gmail.com>
> Date: Mon, 23 Aug 2010 10:29:49 +0800
> 
>> Do you mean that use the __attribute__((packed)) annotation in all of
>> these files:
>> 
>> localhost linux # grep "\<__packed\>" usr/include/ -r | uniq
>> usr/include/linux/if_hippi.h:} __packed;
>> usr/include/linux/if_fddi.h:} __packed;
>> usr/include/linux/nbd.h:} __packed;
>> usr/include/linux/ncp.h:} __packed;
>> usr/include/linux/rfkill.h:} __packed;
>> usr/include/linux/if_pppox.h:} __packed;
>> usr/include/linux/phonet.h:} __packed;
>> usr/include/linux/ipv6.h:} __packed;    /* required for some archs */
>> usr/include/linux/ipv6.h:} __packed;
>> usr/include/linux/if_ether.h:} __packed;
> 
> It seems so, yes.

Changli I applied your patch to convert these __packed tags back to
the original expansion.

Thanks!