| Message ID | 1612341558-22171-1-git-send-email-zhangshaokun@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-next] xfs: Fix unused variable 'mp' warning | expand |
On Wed, Feb 03, 2021 at 04:39:18PM +0800, Shaokun Zhang wrote: > There is a warning on arm64 platform: > CC [M] fs/xfs/xfs_ioctl32.o > fs/xfs/xfs_ioctl32.c: In function ‘xfs_file_compat_ioctl’: > fs/xfs/xfs_ioctl32.c:441:20: warning: unused variable ‘mp’ [-Wunused-variable] > 441 | struct xfs_mount *mp = ip->i_mount; > | ^~ > LD [M] fs/xfs/xfs.o > > Fix this warning. > > Fixes: f736d93d76d3 ("xfs: support idmapped mounts") > Cc: "Darrick J. Wong" <djwong@kernel.org> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Christian Brauner <christian.brauner@ubuntu.com> > Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> > --- Want me to take that on top of the series, Christoph? Christian
I don't think declaring a variable inside a switch statement is a good idea. This is what I had lying around but never got around finishing up and submitting: --- From 5e79886f08ca4dd96c9a508a380dfeb73cd4b529 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig <hch@lst.de> Date: Wed, 3 Feb 2021 13:38:27 +0100 Subject: xfs: remove the possibly unused mp variable in xfs_file_compat_ioctl The mp variable in xfs_file_compat_ioctl is only used when BROKEN_X86_ALIGNMENT is define. Remove it and just open code the dereference in a few places. Fixes: f736d93d76d3 ("xfs: support idmapped mounts") Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_ioctl32.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c index e11139e18021c1..daf73cb53a05bb 100644 --- a/fs/xfs/xfs_ioctl32.c +++ b/fs/xfs/xfs_ioctl32.c @@ -420,7 +420,6 @@ xfs_file_compat_ioctl( { struct inode *inode = file_inode(filp); struct xfs_inode *ip = XFS_I(inode); - struct xfs_mount *mp = ip->i_mount; void __user *arg = compat_ptr(p); int error; @@ -429,7 +428,7 @@ xfs_file_compat_ioctl( switch (cmd) { #if defined(BROKEN_X86_ALIGNMENT) case XFS_IOC_FSGEOMETRY_V1_32: - return xfs_compat_ioc_fsgeometry_v1(mp, arg); + return xfs_compat_ioc_fsgeometry_v1(ip->i_mount, arg); case XFS_IOC_FSGROWFSDATA_32: { struct xfs_growfs_data in; @@ -438,7 +437,7 @@ xfs_file_compat_ioctl( error = mnt_want_write_file(filp); if (error) return error; - error = xfs_growfs_data(mp, &in); + error = xfs_growfs_data(ip->i_mount, &in); mnt_drop_write_file(filp); return error; } @@ -450,7 +449,7 @@ xfs_file_compat_ioctl( error = mnt_want_write_file(filp); if (error) return error; - error = xfs_growfs_rt(mp, &in); + error = xfs_growfs_rt(ip->i_mount, &in); mnt_drop_write_file(filp); return error; } @@ -480,7 +479,7 @@ xfs_file_compat_ioctl( case XFS_IOC_FSBULKSTAT_32: case XFS_IOC_FSBULKSTAT_SINGLE_32: case XFS_IOC_FSINUMBERS_32: - return xfs_compat_ioc_fsbulkstat(mp, cmd, arg); + return xfs_compat_ioc_fsbulkstat(ip->i_mount, cmd, arg); case XFS_IOC_FD_TO_HANDLE_32: case XFS_IOC_PATH_TO_HANDLE_32: case XFS_IOC_PATH_TO_FSHANDLE_32: {
On Wed, Feb 03, 2021 at 01:41:17PM +0100, Christoph Hellwig wrote: > I don't think declaring a variable inside a switch statement is a good Yeah. (I would think that the compiler would complain about this.) > idea. This is what I had lying around but never got around finishing > up and submitting: > > --- > From 5e79886f08ca4dd96c9a508a380dfeb73cd4b529 Mon Sep 17 00:00:00 2001 > From: Christoph Hellwig <hch@lst.de> > Date: Wed, 3 Feb 2021 13:38:27 +0100 > Subject: xfs: remove the possibly unused mp variable in xfs_file_compat_ioctl > > The mp variable in xfs_file_compat_ioctl is only used when > BROKEN_X86_ALIGNMENT is define. Remove it and just open code the > dereference in a few places. > > Fixes: f736d93d76d3 ("xfs: support idmapped mounts") > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_ioctl32.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c > index e11139e18021c1..daf73cb53a05bb 100644 > --- a/fs/xfs/xfs_ioctl32.c > +++ b/fs/xfs/xfs_ioctl32.c > @@ -420,7 +420,6 @@ xfs_file_compat_ioctl( > { > struct inode *inode = file_inode(filp); > struct xfs_inode *ip = XFS_I(inode); > - struct xfs_mount *mp = ip->i_mount; > void __user *arg = compat_ptr(p); > int error; > > @@ -429,7 +428,7 @@ xfs_file_compat_ioctl( > switch (cmd) { > #if defined(BROKEN_X86_ALIGNMENT) > case XFS_IOC_FSGEOMETRY_V1_32: > - return xfs_compat_ioc_fsgeometry_v1(mp, arg); > + return xfs_compat_ioc_fsgeometry_v1(ip->i_mount, arg); > case XFS_IOC_FSGROWFSDATA_32: { > struct xfs_growfs_data in; > > @@ -438,7 +437,7 @@ xfs_file_compat_ioctl( > error = mnt_want_write_file(filp); > if (error) > return error; > - error = xfs_growfs_data(mp, &in); > + error = xfs_growfs_data(ip->i_mount, &in); > mnt_drop_write_file(filp); > return error; > } > @@ -450,7 +449,7 @@ xfs_file_compat_ioctl( > error = mnt_want_write_file(filp); > if (error) > return error; > - error = xfs_growfs_rt(mp, &in); > + error = xfs_growfs_rt(ip->i_mount, &in); > mnt_drop_write_file(filp); > return error; > } > @@ -480,7 +479,7 @@ xfs_file_compat_ioctl( > case XFS_IOC_FSBULKSTAT_32: > case XFS_IOC_FSBULKSTAT_SINGLE_32: > case XFS_IOC_FSINUMBERS_32: > - return xfs_compat_ioc_fsbulkstat(mp, cmd, arg); > + return xfs_compat_ioc_fsbulkstat(ip->i_mount, cmd, arg); In the final version of you conversion (after the file_user_ns() introduction) we simply pass down the fp so the patch needs to be? If you're happy with it I can apply it on top. I don't want to rebase this late. I can also send it separate as a reply in case this too much in the body of this mail. Patch passes cross-compilation for arm64 and native x864-64 and xfstests pass too: --- From a364f6e9de91cea671765cbd0e33fb823ebbba3c Mon Sep 17 00:00:00 2001 From: Christoph Hellwig <hch@lst.de> Date: Wed, 3 Feb 2021 14:34:16 +0100 Subject: [PATCH] xfs: remove the possibly unused mp variable in xfs_file_compat_ioctl The mp variable in xfs_file_compat_ioctl is only used when BROKEN_X86_ALIGNMENT is define. Remove it and just open code the dereference in a few places. Fixes: f736d93d76d3 ("xfs: support idmapped mounts") Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_ioctl32.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c index 926427b19573..33c09ec8e6c0 100644 --- a/fs/xfs/xfs_ioctl32.c +++ b/fs/xfs/xfs_ioctl32.c @@ -438,7 +438,6 @@ xfs_file_compat_ioctl( { struct inode *inode = file_inode(filp); struct xfs_inode *ip = XFS_I(inode); - struct xfs_mount *mp = ip->i_mount; void __user *arg = compat_ptr(p); int error; @@ -458,7 +457,7 @@ xfs_file_compat_ioctl( return xfs_ioc_space(filp, &bf); } case XFS_IOC_FSGEOMETRY_V1_32: - return xfs_compat_ioc_fsgeometry_v1(mp, arg); + return xfs_compat_ioc_fsgeometry_v1(ip->i_mount, arg); case XFS_IOC_FSGROWFSDATA_32: { struct xfs_growfs_data in; @@ -467,7 +466,7 @@ xfs_file_compat_ioctl( error = mnt_want_write_file(filp); if (error) return error; - error = xfs_growfs_data(mp, &in); + error = xfs_growfs_data(ip->i_mount, &in); mnt_drop_write_file(filp); return error; } @@ -479,7 +478,7 @@ xfs_file_compat_ioctl( error = mnt_want_write_file(filp); if (error) return error; - error = xfs_growfs_rt(mp, &in); + error = xfs_growfs_rt(ip->i_mount, &in); mnt_drop_write_file(filp); return error; } base-commit: f736d93d76d3e97d6986c6d26c8eaa32536ccc5c
On Wed, Feb 03, 2021 at 02:47:34PM +0100, Christian Brauner wrote: > In the final version of you conversion (after the file_user_ns() > introduction) we simply pass down the fp so the patch needs to be? > > If you're happy with it I can apply it on top. I don't want to rebase > this late. I can also send it separate as a reply in case this too much > in the body of this mail. > > Patch passes cross-compilation for arm64 and native x864-64 and xfstests > pass too: Let's wait for an ACK from Darrick, but I'd be fine with this.
On Wed, Feb 03, 2021 at 03:30:17PM +0100, Christoph Hellwig wrote: > On Wed, Feb 03, 2021 at 02:47:34PM +0100, Christian Brauner wrote: > > In the final version of you conversion (after the file_user_ns() > > introduction) we simply pass down the fp so the patch needs to be? > > > > If you're happy with it I can apply it on top. I don't want to rebase > > this late. I can also send it separate as a reply in case this too much > > in the body of this mail. > > > > Patch passes cross-compilation for arm64 and native x864-64 and xfstests > > pass too: > > Let's wait for an ACK from Darrick, but I'd be fine with this. Sounds good! Christian
On Wed, Feb 03, 2021 at 03:41:25PM +0100, Christian Brauner wrote: > On Wed, Feb 03, 2021 at 03:30:17PM +0100, Christoph Hellwig wrote: > > On Wed, Feb 03, 2021 at 02:47:34PM +0100, Christian Brauner wrote: > > > In the final version of you conversion (after the file_user_ns() > > > introduction) we simply pass down the fp so the patch needs to be? > > > > > > If you're happy with it I can apply it on top. I don't want to rebase > > > this late. I can also send it separate as a reply in case this too much > > > in the body of this mail. > > > > > > Patch passes cross-compilation for arm64 and native x864-64 and xfstests > > > pass too: > > > > Let's wait for an ACK from Darrick, but I'd be fine with this. > > Sounds good! > Christian Seems fine to me, but can one of you please send this as a proper patch? :) FWIW I really would prefer we make the tooling smart enough to shut up about "unused" variables and "dead" code simply because we #define'd their usage out of existence. But since refactoring gcc/clang is probably even more of an ocean-boiling approach I'll just invite you all to join the XFS quota refactoring party. :P --D
On Wed, Feb 03, 2021 at 09:16:33AM -0800, Darrick J. Wong wrote: > On Wed, Feb 03, 2021 at 03:41:25PM +0100, Christian Brauner wrote: > > On Wed, Feb 03, 2021 at 03:30:17PM +0100, Christoph Hellwig wrote: > > > On Wed, Feb 03, 2021 at 02:47:34PM +0100, Christian Brauner wrote: > > > > In the final version of you conversion (after the file_user_ns() > > > > introduction) we simply pass down the fp so the patch needs to be? > > > > > > > > If you're happy with it I can apply it on top. I don't want to rebase > > > > this late. I can also send it separate as a reply in case this too much > > > > in the body of this mail. > > > > > > > > Patch passes cross-compilation for arm64 and native x864-64 and xfstests > > > > pass too: > > > > > > Let's wait for an ACK from Darrick, but I'd be fine with this. > > > > Sounds good! > > Christian > > Seems fine to me, but can one of you please send this as a proper patch? > :) Yeah, for sure! > > FWIW I really would prefer we make the tooling smart enough to shut up > about "unused" variables and "dead" code simply because we #define'd > their usage out of existence. But since refactoring gcc/clang is > probably even more of an ocean-boiling approach I'll just invite you all > to join the XFS quota refactoring party. :P In 2021 that might indeed qualify as a party... :) Christian
diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c index 926427b19573..fd590c0b5d3b 100644 --- a/fs/xfs/xfs_ioctl32.c +++ b/fs/xfs/xfs_ioctl32.c @@ -438,7 +438,6 @@ xfs_file_compat_ioctl( { struct inode *inode = file_inode(filp); struct xfs_inode *ip = XFS_I(inode); - struct xfs_mount *mp = ip->i_mount; void __user *arg = compat_ptr(p); int error; @@ -446,6 +445,8 @@ xfs_file_compat_ioctl( switch (cmd) { #if defined(BROKEN_X86_ALIGNMENT) + struct xfs_mount *mp = ip->i_mount; + case XFS_IOC_ALLOCSP_32: case XFS_IOC_FREESP_32: case XFS_IOC_ALLOCSP64_32:
There is a warning on arm64 platform: CC [M] fs/xfs/xfs_ioctl32.o fs/xfs/xfs_ioctl32.c: In function ‘xfs_file_compat_ioctl’: fs/xfs/xfs_ioctl32.c:441:20: warning: unused variable ‘mp’ [-Wunused-variable] 441 | struct xfs_mount *mp = ip->i_mount; | ^~ LD [M] fs/xfs/xfs.o Fix this warning. Fixes: f736d93d76d3 ("xfs: support idmapped mounts") Cc: "Darrick J. Wong" <djwong@kernel.org> Cc: Christoph Hellwig <hch@lst.de> Cc: Christian Brauner <christian.brauner@ubuntu.com> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> --- fs/xfs/xfs_ioctl32.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)