| Message ID | 06b937e3-afaf-41c0-3477-a4b1a88fee48@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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>
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)) || > >
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.
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 --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)) ||
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