| Message ID | 20170509090018.30094-1-hch@lst.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, May 09, 2017 at 11:00:18AM +0200, Christoph Hellwig wrote: > XFS_HSIZE is an extremly confusing way to calculate the size of handle_t. > Given that handle_t always only had two sizes, and one of them isn't > even covered by XFS_HSIZE to start with just remove the macro and use > a constant sizeof expression. > > Note that XFS_HSIZE isn't used in xfsprogs, xfsdump or xfstests either. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/libxfs/xfs_fs.h | 4 ---- > fs/xfs/xfs_ioctl.c | 3 +-- > 2 files changed, 1 insertion(+), 6 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h > index 095bdf049a3f..a9aa13e66046 100644 > --- a/fs/xfs/libxfs/xfs_fs.h > +++ b/fs/xfs/libxfs/xfs_fs.h > @@ -446,10 +446,6 @@ typedef struct xfs_handle { > } xfs_handle_t; > #define ha_fsid ha_u._ha_fsid > > -#define XFS_HSIZE(handle) (((char *) &(handle).ha_fid.fid_pad \ > - - (char *) &(handle)) \ > - + (handle).ha_fid.fid_len) > - > /* > * Structure passed to XFS_IOC_SWAPEXT > */ > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index 6190697603c9..f6af76975bc8 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -120,8 +120,7 @@ xfs_find_handle( > handle.ha_fid.fid_pad = 0; > handle.ha_fid.fid_gen = inode->i_generation; > handle.ha_fid.fid_ino = ip->i_ino; > - > - hsize = XFS_HSIZE(handle); > + hsize = sizeof(xfs_handle_t); > } > > error = -EFAULT; > -- > 2.11.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/9/17 4:00 AM, Christoph Hellwig wrote: > XFS_HSIZE is an extremly confusing way to calculate the size of handle_t. > Given that handle_t always only had two sizes, and one of them isn't > even covered by XFS_HSIZE to start with just remove the macro and use > a constant sizeof expression. > > Note that XFS_HSIZE isn't used in xfsprogs, xfsdump or xfstests either. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Ok, so with this assignment a few lines before: handle.ha_fid.fid_len = sizeof(xfs_fid_t) - sizeof(handle.ha_fid.fid_len); (so: fid_len == size of (fid_pad + fid_gen + fid_ino)) and: (((char *) &(handle).ha_fid.fid_pad - (char *) &(handle)) + (handle).ha_fid.fid_len) |< -- size of ha_u + sizeof fid_len --> | |<-sizeof(pad+gen+ino)->| this does indeed seem to come out to sizeof(xfs_handle_t). Looks winning, minor comment below > --- > fs/xfs/libxfs/xfs_fs.h | 4 ---- > fs/xfs/xfs_ioctl.c | 3 +-- > 2 files changed, 1 insertion(+), 6 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h > index 095bdf049a3f..a9aa13e66046 100644 > --- a/fs/xfs/libxfs/xfs_fs.h > +++ b/fs/xfs/libxfs/xfs_fs.h > @@ -446,10 +446,6 @@ typedef struct xfs_handle { > } xfs_handle_t; > #define ha_fsid ha_u._ha_fsid > > -#define XFS_HSIZE(handle) (((char *) &(handle).ha_fid.fid_pad \ > - - (char *) &(handle)) \ > - + (handle).ha_fid.fid_len) > - > /* > * Structure passed to XFS_IOC_SWAPEXT > */ > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index 6190697603c9..f6af76975bc8 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -120,8 +120,7 @@ xfs_find_handle( > handle.ha_fid.fid_pad = 0; > handle.ha_fid.fid_gen = inode->i_generation; > handle.ha_fid.fid_ino = ip->i_ino; > - > - hsize = XFS_HSIZE(handle); > + hsize = sizeof(xfs_handle_t); at this point, no need to have the hsize assignment in if { } as well as else { }, right? -Eric > } > > error = -EFAULT; > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 09, 2017 at 10:40:31AM -0500, Eric Sandeen wrote: > > - > > - hsize = XFS_HSIZE(handle); > > + hsize = sizeof(xfs_handle_t); > > at this point, no need to have the hsize assignment in if { } as well as else { }, right? > In the other branch hsize is smaller, it only covers the fsid. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On May 9, 2017, at 4:18 PM, Christoph Hellwig <hch@lst.de> wrote: > > On Tue, May 09, 2017 at 10:40:31AM -0500, Eric Sandeen wrote: >>> - >>> - hsize = XFS_HSIZE(handle); >>> + hsize = sizeof(xfs_handle_t); >> >> at this point, no need to have the hsize assignment in if { } as well as else { }, right? >> > > In the other branch hsize is smaller, it only covers the fsid. Ugh I misread, sorry. Reviewed-by: Eric Sandeen <sandeen@sandeen.net> > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 09, 2017 at 08:40:00AM -0700, Darrick J. Wong wrote: > On Tue, May 09, 2017 at 11:00:18AM +0200, Christoph Hellwig wrote: > > XFS_HSIZE is an extremly confusing way to calculate the size of handle_t. > > Given that handle_t always only had two sizes, and one of them isn't > > even covered by XFS_HSIZE to start with just remove the macro and use > > a constant sizeof expression. > > > > Note that XFS_HSIZE isn't used in xfsprogs, xfsdump or xfstests either. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> By the way, do you want me to try to slip this cleanup into 4.12, or should it wait for 4.13? --D > > > --- > > fs/xfs/libxfs/xfs_fs.h | 4 ---- > > fs/xfs/xfs_ioctl.c | 3 +-- > > 2 files changed, 1 insertion(+), 6 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h > > index 095bdf049a3f..a9aa13e66046 100644 > > --- a/fs/xfs/libxfs/xfs_fs.h > > +++ b/fs/xfs/libxfs/xfs_fs.h > > @@ -446,10 +446,6 @@ typedef struct xfs_handle { > > } xfs_handle_t; > > #define ha_fsid ha_u._ha_fsid > > > > -#define XFS_HSIZE(handle) (((char *) &(handle).ha_fid.fid_pad \ > > - - (char *) &(handle)) \ > > - + (handle).ha_fid.fid_len) > > - > > /* > > * Structure passed to XFS_IOC_SWAPEXT > > */ > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > > index 6190697603c9..f6af76975bc8 100644 > > --- a/fs/xfs/xfs_ioctl.c > > +++ b/fs/xfs/xfs_ioctl.c > > @@ -120,8 +120,7 @@ xfs_find_handle( > > handle.ha_fid.fid_pad = 0; > > handle.ha_fid.fid_gen = inode->i_generation; > > handle.ha_fid.fid_ino = ip->i_ino; > > - > > - hsize = XFS_HSIZE(handle); > > + hsize = sizeof(xfs_handle_t); > > } > > > > error = -EFAULT; > > -- > > 2.11.0 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
No need to expedite a simple cleanup. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h index 095bdf049a3f..a9aa13e66046 100644 --- a/fs/xfs/libxfs/xfs_fs.h +++ b/fs/xfs/libxfs/xfs_fs.h @@ -446,10 +446,6 @@ typedef struct xfs_handle { } xfs_handle_t; #define ha_fsid ha_u._ha_fsid -#define XFS_HSIZE(handle) (((char *) &(handle).ha_fid.fid_pad \ - - (char *) &(handle)) \ - + (handle).ha_fid.fid_len) - /* * Structure passed to XFS_IOC_SWAPEXT */ diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index 6190697603c9..f6af76975bc8 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -120,8 +120,7 @@ xfs_find_handle( handle.ha_fid.fid_pad = 0; handle.ha_fid.fid_gen = inode->i_generation; handle.ha_fid.fid_ino = ip->i_ino; - - hsize = XFS_HSIZE(handle); + hsize = sizeof(xfs_handle_t); } error = -EFAULT;
XFS_HSIZE is an extremly confusing way to calculate the size of handle_t. Given that handle_t always only had two sizes, and one of them isn't even covered by XFS_HSIZE to start with just remove the macro and use a constant sizeof expression. Note that XFS_HSIZE isn't used in xfsprogs, xfsdump or xfstests either. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_fs.h | 4 ---- fs/xfs/xfs_ioctl.c | 3 +-- 2 files changed, 1 insertion(+), 6 deletions(-)