From: Brian Foster <bfoster@redhat.com>
To: Dave Chinner <david@fromorbit.com>
Cc: iustin@k1024.org, xfs@oss.sgi.com
Subject: Re: [PATCH 5/9] xfs: kill xfs_ioctl_setattr behaviour mask
Date: Thu, 29 Jan 2015 10:35:00 -0500 [thread overview]
Message-ID: <20150129153500.GE17652@laptop.bfoster> (raw)
In-Reply-To: <1422328486-24661-6-git-send-email-david@fromorbit.com>
On Tue, Jan 27, 2015 at 02:14:42PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Now there is only one caller to xfs_ioctl_setattr that uses all the
> functionality of the function we can kill the behviour mask and
> start cleaning up the code.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_ioctl.c | 168 +++++++++++++++++++++--------------------------------
> 1 file changed, 65 insertions(+), 103 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index c71f32d..563d2b4 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1098,15 +1098,10 @@ out_cancel:
> return ERR_PTR(error);
> }
>
> -#define FSX_PROJID 1
> -#define FSX_EXTSIZE 2
> -#define FSX_XFLAGS 4
> -
> STATIC int
> xfs_ioctl_setattr(
> xfs_inode_t *ip,
> - struct fsxattr *fa,
> - int mask)
> + struct fsxattr *fa)
> {
> struct xfs_mount *mp = ip->i_mount;
> struct xfs_trans *tp;
> @@ -1120,8 +1115,8 @@ xfs_ioctl_setattr(
> /*
> * Disallow 32bit project ids when projid32bit feature is not enabled.
> */
> - if ((mask & FSX_PROJID) && (fa->fsx_projid > (__uint16_t)-1) &&
> - !xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb))
> + if (fa->fsx_projid > (__uint16_t)-1 &&
> + !xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb))
> return -EINVAL;
>
> /*
> @@ -1132,7 +1127,7 @@ xfs_ioctl_setattr(
> * If the IDs do change before we take the ilock, we're covered
> * because the i_*dquot fields will get updated anyway.
> */
> - if (XFS_IS_QUOTA_ON(mp) && (mask & FSX_PROJID)) {
> + if (XFS_IS_QUOTA_ON(mp)) {
> code = xfs_qm_vop_dqalloc(ip, ip->i_d.di_uid,
> ip->i_d.di_gid, fa->fsx_projid,
> XFS_QMOPT_PQUOTA, &udqp, NULL, &pdqp);
> @@ -1149,72 +1144,53 @@ xfs_ioctl_setattr(
> * Only allow changing of projid from init_user_ns since it is a
> * non user namespace aware identifier.
> */
> - if (mask & FSX_PROJID) {
> - if (current_user_ns() != &init_user_ns) {
> - code = -EINVAL;
> - goto error_return;
> - }
> -
> - if (XFS_IS_QUOTA_RUNNING(mp) &&
> - XFS_IS_PQUOTA_ON(mp) &&
> - xfs_get_projid(ip) != fa->fsx_projid) {
> - ASSERT(tp);
> - code = xfs_qm_vop_chown_reserve(tp, ip, udqp, NULL,
> - pdqp, capable(CAP_FOWNER) ?
> - XFS_QMOPT_FORCE_RES : 0);
> - if (code) /* out of quota */
> - goto error_return;
> - }
> + if (current_user_ns() != &init_user_ns) {
> + code = -EINVAL;
> + goto error_return;
> }
>
> - if (mask & FSX_EXTSIZE) {
> - /*
> - * Can't change extent size if any extents are allocated.
> - */
> - if (ip->i_d.di_nextents &&
> - ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) !=
> - fa->fsx_extsize)) {
> - code = -EINVAL; /* EFBIG? */
> + if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp) &&
> + xfs_get_projid(ip) != fa->fsx_projid) {
> + code = xfs_qm_vop_chown_reserve(tp, ip, udqp, NULL, pdqp,
> + capable(CAP_FOWNER) ? XFS_QMOPT_FORCE_RES : 0);
> + if (code) /* out of quota */
> goto error_return;
> - }
> + }
>
> - /*
> - * Extent size must be a multiple of the appropriate block
> - * size, if set at all. It must also be smaller than the
> - * maximum extent size supported by the filesystem.
> - *
> - * Also, for non-realtime files, limit the extent size hint to
> - * half the size of the AGs in the filesystem so alignment
> - * doesn't result in extents larger than an AG.
> - */
> - if (fa->fsx_extsize != 0) {
> - xfs_extlen_t size;
> - xfs_fsblock_t extsize_fsb;
> + /* Can't change extent size if any extents are allocated. */
> + code = -EINVAL;
> + if (ip->i_d.di_nextents &&
> + ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) != fa->fsx_extsize))
> + goto error_return;
>
> - extsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_extsize);
> - if (extsize_fsb > MAXEXTLEN) {
> - code = -EINVAL;
> - goto error_return;
> - }
> -
> - if (XFS_IS_REALTIME_INODE(ip) ||
> - ((mask & FSX_XFLAGS) &&
> - (fa->fsx_xflags & XFS_XFLAG_REALTIME))) {
> - size = mp->m_sb.sb_rextsize <<
> - mp->m_sb.sb_blocklog;
> - } else {
> - size = mp->m_sb.sb_blocksize;
> - if (extsize_fsb > mp->m_sb.sb_agblocks / 2) {
> - code = -EINVAL;
> - goto error_return;
> - }
> - }
> -
> - if (fa->fsx_extsize % size) {
> - code = -EINVAL;
> + /*
> + * Extent size must be a multiple of the appropriate block size, if set
> + * at all. It must also be smaller than the maximum extent size
> + * supported by the filesystem.
> + *
> + * Also, for non-realtime files, limit the extent size hint to half the
> + * size of the AGs in the filesystem so alignment doesn't result in
> + * extents larger than an AG.
> + */
> + if (fa->fsx_extsize != 0) {
> + xfs_extlen_t size;
> + xfs_fsblock_t extsize_fsb;
> +
> + extsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_extsize);
> + if (extsize_fsb > MAXEXTLEN)
> + goto error_return;
> +
> + if (XFS_IS_REALTIME_INODE(ip) ||
> + (fa->fsx_xflags & XFS_XFLAG_REALTIME)) {
> + size = mp->m_sb.sb_rextsize << mp->m_sb.sb_blocklog;
> + } else {
> + size = mp->m_sb.sb_blocksize;
> + if (extsize_fsb > mp->m_sb.sb_agblocks / 2)
> goto error_return;
> - }
> }
> +
> + if (fa->fsx_extsize % size)
> + goto error_return;
> }
>
>
> @@ -1223,32 +1199,25 @@ xfs_ioctl_setattr(
> goto error_return;
>
> /*
> - * Change file ownership. Must be the owner or privileged.
> + * Change file ownership. Must be the owner or privileged. CAP_FSETID
> + * overrides the following restrictions:
> + *
> + * The set-user-ID and set-group-ID bits of a file will be cleared upon
> + * successful return from chown()
> */
> - if (mask & FSX_PROJID) {
> - /*
> - * CAP_FSETID overrides the following restrictions:
> - *
> - * The set-user-ID and set-group-ID bits of a file will be
> - * cleared upon successful return from chown()
> - */
> - if ((ip->i_d.di_mode & (S_ISUID|S_ISGID)) &&
> - !capable_wrt_inode_uidgid(VFS_I(ip), CAP_FSETID))
> - ip->i_d.di_mode &= ~(S_ISUID|S_ISGID);
>
> - /*
> - * Change the ownerships and register quota modifications
> - * in the transaction.
> - */
> - if (xfs_get_projid(ip) != fa->fsx_projid) {
> - if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp)) {
> - olddquot = xfs_qm_vop_chown(tp, ip,
> - &ip->i_pdquot, pdqp);
> - }
> - ASSERT(ip->i_d.di_version > 1);
> - xfs_set_projid(ip, fa->fsx_projid);
> - }
> + if ((ip->i_d.di_mode & (S_ISUID|S_ISGID)) &&
> + !capable_wrt_inode_uidgid(VFS_I(ip), CAP_FSETID))
> + ip->i_d.di_mode &= ~(S_ISUID|S_ISGID);
>
> + /* Change the ownerships and register project quota modifications */
> + if (xfs_get_projid(ip) != fa->fsx_projid) {
> + if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp)) {
> + olddquot = xfs_qm_vop_chown(tp, ip,
> + &ip->i_pdquot, pdqp);
> + }
> + ASSERT(ip->i_d.di_version > 1);
> + xfs_set_projid(ip, fa->fsx_projid);
> }
>
> /*
> @@ -1256,14 +1225,10 @@ xfs_ioctl_setattr(
> * extent size hint should be set on the inode. If no extent size flags
> * are set on the inode then unconditionally clear the extent size hint.
> */
> - if (mask & FSX_EXTSIZE) {
> - int extsize = 0;
> -
> - if (ip->i_d.di_flags &
> - (XFS_DIFLAG_EXTSIZE | XFS_DIFLAG_EXTSZINHERIT))
> - extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
> - ip->i_d.di_extsize = extsize;
> - }
> + if (ip->i_d.di_flags & (XFS_DIFLAG_EXTSIZE | XFS_DIFLAG_EXTSZINHERIT))
> + ip->i_d.di_extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
> + else
> + ip->i_d.di_extsize = 0;
>
> code = xfs_trans_commit(tp, 0);
>
> @@ -1290,18 +1255,15 @@ xfs_ioc_fssetxattr(
> void __user *arg)
> {
> struct fsxattr fa;
> - unsigned int mask;
> int error;
>
> if (copy_from_user(&fa, arg, sizeof(fa)))
> return -EFAULT;
>
> - mask = FSX_XFLAGS | FSX_EXTSIZE | FSX_PROJID;
> -
> error = mnt_want_write_file(filp);
> if (error)
> return error;
> - error = xfs_ioctl_setattr(ip, &fa, mask);
> + error = xfs_ioctl_setattr(ip, &fa);
> mnt_drop_write_file(filp);
> return error;
> }
> --
> 2.0.0
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
next prev parent reply other threads:[~2015-01-29 15:35 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-01-27 3:14 [PATCH 0/9] xfs: xfs_ioctl_setxattr rework Dave Chinner
2015-01-27 3:14 ` [PATCH 1/9] xfs: FSX_NONBLOCK is not used Dave Chinner
2015-01-29 15:33 ` Brian Foster
2015-01-27 3:14 ` [PATCH 2/9] xfs: separate xflags from xfs_ioctl_setattr Dave Chinner
2015-01-29 15:34 ` Brian Foster
2015-01-27 3:14 ` [PATCH 3/9] xfs: factor out xfs_ioctl_setattr transaciton preamble Dave Chinner
2015-01-29 15:34 ` Brian Foster
2015-01-27 3:14 ` [PATCH 4/9] xfs: disaggregate xfs_ioctl_setattr Dave Chinner
2015-01-29 15:34 ` Brian Foster
2015-01-27 3:14 ` [PATCH 5/9] xfs: kill xfs_ioctl_setattr behaviour mask Dave Chinner
2015-01-29 15:35 ` Brian Foster [this message]
2015-01-27 3:14 ` [PATCH 6/9] xfs: XFS_IOCTL_SETXATTR can run in user namespaces Dave Chinner
2015-01-29 15:35 ` Brian Foster
2015-01-29 23:53 ` Dave Chinner
2015-01-30 3:04 ` Brian Foster
2015-01-30 7:44 ` Dave Chinner
2015-01-27 3:14 ` [PATCH 7/9] xfs; factor extsize hint checking out of xfs_ioctl_setattr Dave Chinner
2015-01-29 15:35 ` Brian Foster
2015-01-27 3:14 ` [PATCH 8/9] xfs; factor projid " Dave Chinner
2015-01-29 15:35 ` Brian Foster
2015-01-27 3:14 ` [PATCH 9/9] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories Dave Chinner
2015-01-29 15:35 ` Brian Foster
2015-01-29 15:38 ` [PATCH 0/9] xfs: xfs_ioctl_setxattr rework Brian Foster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150129153500.GE17652@laptop.bfoster \
--to=bfoster@redhat.com \
--cc=david@fromorbit.com \
--cc=iustin@k1024.org \
--cc=xfs@oss.sgi.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.