Re: [PATCH 7/7] nfsd: nfs4xdr decode_stateid helper function

[Benny Halevy <bhalevy@panasas.com>]

On Aug. 13, 2008, 23:17 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> On Wed, Aug 13, 2008 at 03:35:53PM -0400, Trond Myklebust wrote:
>> On Wed, 2008-08-13 at 15:11 -0400, J. Bruce Fields wrote:
>>> On Wed, Aug 13, 2008 at 02:59:52PM -0400, Trond Myklebust wrote:
>>>> On Wed, 2008-08-13 at 14:30 -0400, J. Bruce Fields wrote:
>>>>> On Wed, Aug 13, 2008 at 01:59:09PM -0400, Trond Myklebust wrote:
>>>>>> Which is a good reason for ditching the entire confusing typedef, and
>>>>>> replacing it with a packed structure instead:
>>>>>>
>>>>>> struct stateid {
>>>>>> 	__be32 generation;
>>>>>> 	char opaque[12];
>>>>>> } __attribute__((packed));
>>>>> So without the ((packed)), all arrays get aligned to 8-byte boundaries
>>>>> on 64-bit machines?  (What do I need to read to catch up here?)
>>>> A quick google showed up:
>>>>
>>>> 	http://sig9.com/articles/gcc-packed-structures
>>>>
>>>> In any case, yes, the idea behind the packed attribute is to turn off
>>>> the field alignment.
>>> Yeah, I was more curious about how to decide when it's necessary.  (Why
>>> didn't we need it before?  Is an embedded struct always aligned as if
>>> the fields of the embedded struct were declared directly in the
>>> containing struct?  Or should we really just be using the packed
>>> attribute *any* time we depend on that alignment, even if it seems
>>> obvious the compiler wouldn't need to add padding?)
>> The advantage of having it packed like the above is that you can still
>> use WRITEMEM() to write out the whole structure in one fell swoop.

True, just you need to keep generation in network order in memory
(hence Trond defined it as __be32...)

> 
> Right, I understand.  But the code has been doing exactly that (a
> WRITEMEM of the whole thing) since the beginning, so I wondered if there
> was some reason you thought the switch to the extra char opaque[12] in
> particular was something that was likely to trigger the addition of
> padding.
> 
> Sounds instead like your policy would be just to declare any struct
> "packed" if we might depend on the absence of padding, without making
> any assumptions about what compilers might do.  Which is fine.

Agreed.  If you care about how the structure is laid out in memory then
pack it.

> 
> --b.
> 
>> If you don't specify 'packed', then the C standard allows the compiler
>> to add padding between the fields in order align them. I'm not sure
>> that compilers will usually do that for a 'char[]' field, but they
>> will definitely for the integer types.

gcc (/c90)  seems to align the field based on its size and arrays based
on their element size.

Benny