Re: [PATCH 19/19] lockd: clean up 64-bit alignment fix in nsm_init_private()

["J. Bruce Fields" <bfields@fieldses.org>]

On Tue, Apr 28, 2009 at 01:24:24PM -0400, Chuck Lever wrote:
> On Apr 28, 2009, at 12:40 PM, J. Bruce Fields wrote:
>> On Tue, Apr 28, 2009 at 12:35:50PM -0400, Chuck Lever wrote:
>>> On Apr 28, 2009, at 12:31 PM, J. Bruce Fields wrote:
>>>> On Thu, Apr 23, 2009 at 07:33:40PM -0400, Chuck Lever wrote:
>>>>> Recently, commit ad5b365c fixed a data type alignment issue in
>>>>> nsm_init_private() by introducing put_unaligned().  We don't  
>>>>> actually
>>>>> _require_ an unaligned access to nsm_private here.
>>>>>
>>>>> Instead, we should always use memcpy/memcmp to access the  
>>>>> contents of
>>>>> RPC opaque data types.  This permits us to continue to define these
>>>>> as
>>>>> simple character arrays, as most legacy RPC code does, and to  
>>>>> rely on
>>>>> gcc to pack the fields in in-core structures optimally without
>>>>> breaking
>>>>> on platforms that require strict alignment.
>>>>
>>>> OK, I'm confused.  What structures will get packed differently?
>>>
>>> Any struct that has an nsm_private embedded in it, such as struct
>>> nlm_reboot.
>>
>> I don't see how that or any structure is changed by this patch.
>
> It's not.  Note the phrase above in the description: "permits us to  
> _continue_ to define these" -- meaning, I'm not changing the structures.

Err, but that's not right either, is it?:  We don't need to apply this
patch in order to continue to define the structures as they're currently
defined.

Help! I'm confused!

> It has been suggested that we use a union or __aligned attribute for RPC 
> opaques.  The problem with that is that it would cause structs like 
> nlm_reboot to balloon in size; char x[] is aligned on bytes, but a union 
> of u8, u32, and u64 would be aligned on a double-word boundary.  That 
> would make such structures larger.

OK, I agree, so let's not do that.

--b.

> Legacy RPC code, and any code generated by rpcgen, generally defines an 
> opaque as a character array.  So again, using a union would be  
> inconsistent with other uses of RPC opaques.
>
> The point is we want to define and access RPC opaque's consistently,  
> using memset() and memcpy(), since these are nothing more than byte  
> arrays.  The code in nsm_init_private() was an exception to this, for no 
> reason.  We don't really need special alignment macros in there, as long 
> as we access the RPC opaque field with the byte sets and copies.
>
> Would it help if I described this patch as a "clean up" ?