diff mbox series

[2/2,V3] xfs: don't take addresses of packed xfs_rmap_key member

Message ID 06b937e3-afaf-41c0-3477-a4b1a88fee48@redhat.com
State New
Headers show
Series None | expand

Commit Message

Eric Sandeen Jan. 29, 2020, 6:35 p.m. UTC
gcc now warns about taking an address of a packed structure member.

This happens here because of how be32_add_cpu() works; just open-code
the modification instead.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
V2: fix key-> vs rec-> derp derp thinko
V3: drop local temp variable, add comment

Comments

Christoph Hellwig Jan. 29, 2020, 6:36 p.m. UTC | #1
On Wed, Jan 29, 2020 at 12:35:21PM -0600, Eric Sandeen wrote:
> gcc now warns about taking an address of a packed structure member.
> 
> This happens here because of how be32_add_cpu() works; just open-code
> the modification instead.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
Darrick J. Wong Feb. 26, 2020, 6:06 p.m. UTC | #2
On Wed, Jan 29, 2020 at 12:35:21PM -0600, Eric Sandeen wrote:
> gcc now warns about taking an address of a packed structure member.
> 
> This happens here because of how be32_add_cpu() works; just open-code
> the modification instead.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> V2: fix key-> vs rec-> derp derp thinko
> V3: drop local temp variable, add comment
> 
> diff --git a/fs/xfs/libxfs/xfs_rmap_btree.c b/fs/xfs/libxfs/xfs_rmap_btree.c
> index fc78efa52c94..c6f6a7ec6121 100644
> --- a/fs/xfs/libxfs/xfs_rmap_btree.c
> +++ b/fs/xfs/libxfs/xfs_rmap_btree.c
> @@ -187,7 +187,9 @@ xfs_rmapbt_init_high_key_from_rec(
>  	adj = be32_to_cpu(rec->rmap.rm_blockcount) - 1;
>  
>  	key->rmap.rm_startblock = rec->rmap.rm_startblock;
> -	be32_add_cpu(&key->rmap.rm_startblock, adj);
> +	/* do this manually to avoid gcc warning about alignment */
> +	key->rmap.rm_startblock =
> +		cpu_to_be32(be32_to_cpu(key->rmap.rm_startblock) - adj);

<blink>

This should be getting the value from rec->rmap, not key->rmap.

This should be adding adj, not subtracting it, since that's what the
original code did.

And finally, there's no need to set the value twice.

--D

>  	key->rmap.rm_owner = rec->rmap.rm_owner;
>  	key->rmap.rm_offset = rec->rmap.rm_offset;
>  	if (XFS_RMAP_NON_INODE_OWNER(be64_to_cpu(rec->rmap.rm_owner)) ||
> 
>
Eric Sandeen Feb. 26, 2020, 6:21 p.m. UTC | #3
On 2/26/20 10:06 AM, Darrick J. Wong wrote:
> On Wed, Jan 29, 2020 at 12:35:21PM -0600, Eric Sandeen wrote:

...

>> @@ -187,7 +187,9 @@ xfs_rmapbt_init_high_key_from_rec(
>>  	adj = be32_to_cpu(rec->rmap.rm_blockcount) - 1;
>>  
>>  	key->rmap.rm_startblock = rec->rmap.rm_startblock;
>> -	be32_add_cpu(&key->rmap.rm_startblock, adj);
>> +	/* do this manually to avoid gcc warning about alignment */
>> +	key->rmap.rm_startblock =
>> +		cpu_to_be32(be32_to_cpu(key->rmap.rm_startblock) - adj);
> 
> <blink>
> 
> This should be getting the value from rec->rmap, not key->rmap.
> 
> This should be adding adj, not subtracting it, since that's what the
> original code did.

*sigh* I got nothin' here tbh.
Eric Sandeen Feb. 26, 2020, 6:45 p.m. UTC | #4
On 2/26/20 10:21 AM, Eric Sandeen wrote:
> On 2/26/20 10:06 AM, Darrick J. Wong wrote:
>> On Wed, Jan 29, 2020 at 12:35:21PM -0600, Eric Sandeen wrote:
> 
> ...
> 
>>> @@ -187,7 +187,9 @@ xfs_rmapbt_init_high_key_from_rec(
>>>  	adj = be32_to_cpu(rec->rmap.rm_blockcount) - 1;
>>>  
>>>  	key->rmap.rm_startblock = rec->rmap.rm_startblock;
>>> -	be32_add_cpu(&key->rmap.rm_startblock, adj);
>>> +	/* do this manually to avoid gcc warning about alignment */
>>> +	key->rmap.rm_startblock =
>>> +		cpu_to_be32(be32_to_cpu(key->rmap.rm_startblock) - adj);
>>
>> <blink>
>>
>> This should be getting the value from rec->rmap, not key->rmap.
>>
>> This should be adding adj, not subtracting it, since that's what the
>> original code did.
> 
> *sigh* I got nothin' here tbh.
> 

Let's just drop this patch.  The first patch in this series is obviated by
hch's work and AFAIK the kernel just turned off this warning, we can do
the same in userspace and make this 2nd issue go away.

-Eric
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_rmap_btree.c b/fs/xfs/libxfs/xfs_rmap_btree.c
index fc78efa52c94..c6f6a7ec6121 100644
--- a/fs/xfs/libxfs/xfs_rmap_btree.c
+++ b/fs/xfs/libxfs/xfs_rmap_btree.c
@@ -187,7 +187,9 @@  xfs_rmapbt_init_high_key_from_rec(
 	adj = be32_to_cpu(rec->rmap.rm_blockcount) - 1;
 
 	key->rmap.rm_startblock = rec->rmap.rm_startblock;
-	be32_add_cpu(&key->rmap.rm_startblock, adj);
+	/* do this manually to avoid gcc warning about alignment */
+	key->rmap.rm_startblock =
+		cpu_to_be32(be32_to_cpu(key->rmap.rm_startblock) - adj);
 	key->rmap.rm_owner = rec->rmap.rm_owner;
 	key->rmap.rm_offset = rec->rmap.rm_offset;
 	if (XFS_RMAP_NON_INODE_OWNER(be64_to_cpu(rec->rmap.rm_owner)) ||