diff mbox series

[v2,06/15] xfs: mount-api - move xfs_parseargs() validation to a helper

Message ID 156652198915.2607.7532914515862448103.stgit@fedora-28 (mailing list archive)
State Superseded
Headers show
Series xfs: mount API patch series | expand

Commit Message

Ian Kent Aug. 23, 2019, 12:59 a.m. UTC 
Move the validation code of xfs_parseargs() into a helper for later
use within the mount context methods.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/xfs/xfs_super.c |  180 ++++++++++++++++++++++++++++------------------------
 1 file changed, 98 insertions(+), 82 deletions(-)

Comments

Brian Foster Aug. 27, 2019, 12:41 p.m. UTC | #1 
On Fri, Aug 23, 2019 at 08:59:49AM +0800, Ian Kent wrote:
> Move the validation code of xfs_parseargs() into a helper for later
> use within the mount context methods.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---
>  fs/xfs/xfs_super.c |  180 ++++++++++++++++++++++++++++------------------------
>  1 file changed, 98 insertions(+), 82 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 754d2ccfd960..7cdda17ee0ff 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
...
> @@ -442,89 +535,12 @@ xfs_parseargs(
>  		ret = xfs_parse_param(&fc, &param);
>  		kfree(param.string);
>  		if (ret < 0)
> -			return ret;
> -	}
> -
> -	/*
> -	 * no recovery flag requires a read-only mount
> -	 */
> -	if ((mp->m_flags & XFS_MOUNT_NORECOVERY) &&
> -	    !(mp->m_flags & XFS_MOUNT_RDONLY)) {
> -		xfs_warn(mp, "no-recovery mounts must be read-only.");
> -		return -EINVAL;
> -	}
> -
> -	if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (ctx->dsunit || ctx->dswidth)) {
> -		xfs_warn(mp,
> -	"sunit and swidth options incompatible with the noalign option");
> -		return -EINVAL;
> -	}
> -
> -#ifndef CONFIG_XFS_QUOTA
> -	if (XFS_IS_QUOTA_RUNNING(mp)) {
> -		xfs_warn(mp, "quota support not available in this kernel.");
> -		return -EINVAL;
> -	}
> -#endif
> -
> -	if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit && ctx->dswidth)) {
> -		xfs_warn(mp, "sunit and swidth must be specified together");
> -		return -EINVAL;
> -	}
> -
> -	if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) {
> -		xfs_warn(mp,
> -	"stripe width (%d) must be a multiple of the stripe unit (%d)",
> -			ctx->dswidth, ctx->dsunit);
> -		return -EINVAL;
> +			goto done;
>  	}
>  
> +	ret = xfs_validate_params(mp, ctx, false);
>  done:

This label now directly returns, which means it's not that useful in its
current form. How about we move the validate call below the label
(and perhaps rename the label to validate or some such) and just return
directly from the other user of done?

Brian

> -	if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
> -		/*
> -		 * At this point the superblock has not been read
> -		 * in, therefore we do not know the block size.
> -		 * Before the mount call ends we will convert
> -		 * these to FSBs.
> -		 */
> -		mp->m_dalign = ctx->dsunit;
> -		mp->m_swidth = ctx->dswidth;
> -	}
> -
> -	if (mp->m_logbufs != -1 &&
> -	    mp->m_logbufs != 0 &&
> -	    (mp->m_logbufs < XLOG_MIN_ICLOGS ||
> -	     mp->m_logbufs > XLOG_MAX_ICLOGS)) {
> -		xfs_warn(mp, "invalid logbufs value: %d [not %d-%d]",
> -			mp->m_logbufs, XLOG_MIN_ICLOGS, XLOG_MAX_ICLOGS);
> -		return -EINVAL;
> -	}
> -	if (mp->m_logbsize != -1 &&
> -	    mp->m_logbsize !=  0 &&
> -	    (mp->m_logbsize < XLOG_MIN_RECORD_BSIZE ||
> -	     mp->m_logbsize > XLOG_MAX_RECORD_BSIZE ||
> -	     !is_power_of_2(mp->m_logbsize))) {
> -		xfs_warn(mp,
> -			"invalid logbufsize: %d [not 16k,32k,64k,128k or 256k]",
> -			mp->m_logbsize);
> -		return -EINVAL;
> -	}
> -
> -	if (ctx->iosizelog) {
> -		if (ctx->iosizelog > XFS_MAX_IO_LOG ||
> -		    ctx->iosizelog < XFS_MIN_IO_LOG) {
> -			xfs_warn(mp, "invalid log iosize: %d [not %d-%d]",
> -				ctx->iosizelog, XFS_MIN_IO_LOG,
> -				XFS_MAX_IO_LOG);
> -			return -EINVAL;
> -		}
> -
> -		mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
> -		mp->m_readio_log = ctx->iosizelog;
> -		mp->m_writeio_log = ctx->iosizelog;
> -	}
> -
> -	return 0;
> +	return ret;
>  }
>  
>  struct proc_xfs_info {
>
Ian Kent Aug. 30, 2019, 10:55 a.m. UTC | #2 
On Tue, 2019-08-27 at 08:41 -0400, Brian Foster wrote:
> On Fri, Aug 23, 2019 at 08:59:49AM +0800, Ian Kent wrote:
> > Move the validation code of xfs_parseargs() into a helper for later
> > use within the mount context methods.
> > 
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > ---
> >  fs/xfs/xfs_super.c |  180 ++++++++++++++++++++++++++++----------
> > --------------
> >  1 file changed, 98 insertions(+), 82 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 754d2ccfd960..7cdda17ee0ff 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> ...
> > @@ -442,89 +535,12 @@ xfs_parseargs(
> >  		ret = xfs_parse_param(&fc, &param);
> >  		kfree(param.string);
> >  		if (ret < 0)
> > -			return ret;
> > -	}
> > -
> > -	/*
> > -	 * no recovery flag requires a read-only mount
> > -	 */
> > -	if ((mp->m_flags & XFS_MOUNT_NORECOVERY) &&
> > -	    !(mp->m_flags & XFS_MOUNT_RDONLY)) {
> > -		xfs_warn(mp, "no-recovery mounts must be read-only.");
> > -		return -EINVAL;
> > -	}
> > -
> > -	if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (ctx->dsunit || ctx-
> > >dswidth)) {
> > -		xfs_warn(mp,
> > -	"sunit and swidth options incompatible with the noalign
> > option");
> > -		return -EINVAL;
> > -	}
> > -
> > -#ifndef CONFIG_XFS_QUOTA
> > -	if (XFS_IS_QUOTA_RUNNING(mp)) {
> > -		xfs_warn(mp, "quota support not available in this
> > kernel.");
> > -		return -EINVAL;
> > -	}
> > -#endif
> > -
> > -	if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit && ctx-
> > >dswidth)) {
> > -		xfs_warn(mp, "sunit and swidth must be specified
> > together");
> > -		return -EINVAL;
> > -	}
> > -
> > -	if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) {
> > -		xfs_warn(mp,
> > -	"stripe width (%d) must be a multiple of the stripe unit (%d)",
> > -			ctx->dswidth, ctx->dsunit);
> > -		return -EINVAL;
> > +			goto done;
> >  	}
> >  
> > +	ret = xfs_validate_params(mp, ctx, false);
> >  done:
> 
> This label now directly returns, which means it's not that useful in
> its
> current form. How about we move the validate call below the label
> (and perhaps rename the label to validate or some such) and just
> return
> directly from the other user of done?

Yes, I saw that too.

But I was trying to duplicate the existing logic, I thought
maybe I got that wrong but couldn't see it and I probably
have since you don't see the correspondence to the original
code.

I'll need re-look at that logic.

> 
> Brian
> 
> > -	if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
> > -		/*
> > -		 * At this point the superblock has not been read
> > -		 * in, therefore we do not know the block size.
> > -		 * Before the mount call ends we will convert
> > -		 * these to FSBs.
> > -		 */
> > -		mp->m_dalign = ctx->dsunit;
> > -		mp->m_swidth = ctx->dswidth;
> > -	}
> > -
> > -	if (mp->m_logbufs != -1 &&
> > -	    mp->m_logbufs != 0 &&
> > -	    (mp->m_logbufs < XLOG_MIN_ICLOGS ||
> > -	     mp->m_logbufs > XLOG_MAX_ICLOGS)) {
> > -		xfs_warn(mp, "invalid logbufs value: %d [not %d-%d]",
> > -			mp->m_logbufs, XLOG_MIN_ICLOGS,
> > XLOG_MAX_ICLOGS);
> > -		return -EINVAL;
> > -	}
> > -	if (mp->m_logbsize != -1 &&
> > -	    mp->m_logbsize !=  0 &&
> > -	    (mp->m_logbsize < XLOG_MIN_RECORD_BSIZE ||
> > -	     mp->m_logbsize > XLOG_MAX_RECORD_BSIZE ||
> > -	     !is_power_of_2(mp->m_logbsize))) {
> > -		xfs_warn(mp,
> > -			"invalid logbufsize: %d [not 16k,32k,64k,128k
> > or 256k]",
> > -			mp->m_logbsize);
> > -		return -EINVAL;
> > -	}
> > -
> > -	if (ctx->iosizelog) {
> > -		if (ctx->iosizelog > XFS_MAX_IO_LOG ||
> > -		    ctx->iosizelog < XFS_MIN_IO_LOG) {
> > -			xfs_warn(mp, "invalid log iosize: %d [not %d-
> > %d]",
> > -				ctx->iosizelog, XFS_MIN_IO_LOG,
> > -				XFS_MAX_IO_LOG);
> > -			return -EINVAL;
> > -		}
> > -
> > -		mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
> > -		mp->m_readio_log = ctx->iosizelog;
> > -		mp->m_writeio_log = ctx->iosizelog;
> > -	}
> > -
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  struct proc_xfs_info {
> >
diff mbox series

Patch

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 754d2ccfd960..7cdda17ee0ff 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -350,6 +350,97 @@  xfs_parse_param(
 	return 0;
 }
 
+STATIC int
+xfs_validate_params(
+	struct xfs_mount        *mp,
+	struct xfs_fs_context   *ctx,
+	bool			nooptions)
+{
+	if (nooptions)
+		goto noopts;
+
+	/*
+	 * no recovery flag requires a read-only mount
+	 */
+	if ((mp->m_flags & XFS_MOUNT_NORECOVERY) &&
+	    !(mp->m_flags & XFS_MOUNT_RDONLY)) {
+		xfs_warn(mp, "no-recovery mounts must be read-only.");
+		return -EINVAL;
+	}
+
+	if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (ctx->dsunit || ctx->dswidth)) {
+		xfs_warn(mp,
+	"sunit and swidth options incompatible with the noalign option");
+		return -EINVAL;
+	}
+
+#ifndef CONFIG_XFS_QUOTA
+	if (XFS_IS_QUOTA_RUNNING(mp)) {
+		xfs_warn(mp, "quota support not available in this kernel.");
+		return -EINVAL;
+	}
+#endif
+
+	if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit && ctx->dswidth)) {
+		xfs_warn(mp, "sunit and swidth must be specified together");
+		return -EINVAL;
+	}
+
+	if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) {
+		xfs_warn(mp,
+	"stripe width (%d) must be a multiple of the stripe unit (%d)",
+			ctx->dswidth, ctx->dsunit);
+		return -EINVAL;
+	}
+
+noopts:
+	if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
+		/*
+		 * At this point the superblock has not been read
+		 * in, therefore we do not know the block size.
+		 * Before the mount call ends we will convert
+		 * these to FSBs.
+		 */
+		mp->m_dalign = ctx->dsunit;
+		mp->m_swidth = ctx->dswidth;
+	}
+
+	if (mp->m_logbufs != -1 &&
+	    mp->m_logbufs != 0 &&
+	    (mp->m_logbufs < XLOG_MIN_ICLOGS ||
+	     mp->m_logbufs > XLOG_MAX_ICLOGS)) {
+		xfs_warn(mp, "invalid logbufs value: %d [not %d-%d]",
+			mp->m_logbufs, XLOG_MIN_ICLOGS, XLOG_MAX_ICLOGS);
+		return -EINVAL;
+	}
+	if (mp->m_logbsize != -1 &&
+	    mp->m_logbsize !=  0 &&
+	    (mp->m_logbsize < XLOG_MIN_RECORD_BSIZE ||
+	     mp->m_logbsize > XLOG_MAX_RECORD_BSIZE ||
+	     !is_power_of_2(mp->m_logbsize))) {
+		xfs_warn(mp,
+			"invalid logbufsize: %d [not 16k,32k,64k,128k or 256k]",
+			mp->m_logbsize);
+		return -EINVAL;
+	}
+
+	if (ctx->iosizelog) {
+		if (ctx->iosizelog > XFS_MAX_IO_LOG ||
+		    ctx->iosizelog < XFS_MIN_IO_LOG) {
+			xfs_warn(mp, "invalid log iosize: %d [not %d-%d]",
+				ctx->iosizelog, XFS_MIN_IO_LOG,
+				XFS_MAX_IO_LOG);
+			return -EINVAL;
+		}
+
+		mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
+		mp->m_readio_log = ctx->iosizelog;
+		mp->m_writeio_log = ctx->iosizelog;
+	}
+
+	return 0;
+}
+
 /*
  * This function fills in xfs_mount_t fields based on mount args.
  * Note: the superblock has _not_ yet been read in.
@@ -372,6 +463,7 @@  xfs_parseargs(
 	struct fs_context	fc;
 	struct xfs_fs_context	context;
 	struct xfs_fs_context	*ctx = &context;
+	int			ret;
 
 	/*
 	 * set up the mount name first so all the errors will refer to the
@@ -404,8 +496,10 @@  xfs_parseargs(
 	mp->m_logbufs = -1;
 	mp->m_logbsize = -1;
 
-	if (!options)
+	if (!options) {
+		ret = xfs_validate_params(mp, ctx, true);
 		goto done;
+	}
 
 	memset(&fc, 0, sizeof(fc));
 	memset(&ctx, 0, sizeof(ctx));
@@ -415,7 +509,6 @@  xfs_parseargs(
 	while ((p = strsep(&options, ",")) != NULL) {
 		struct fs_parameter	param;
 		char			*value;
-		int			ret;
 
 		if (!*p)
 			continue;
@@ -442,89 +535,12 @@  xfs_parseargs(
 		ret = xfs_parse_param(&fc, &param);
 		kfree(param.string);
 		if (ret < 0)
-			return ret;
-	}
-
-	/*
-	 * no recovery flag requires a read-only mount
-	 */
-	if ((mp->m_flags & XFS_MOUNT_NORECOVERY) &&
-	    !(mp->m_flags & XFS_MOUNT_RDONLY)) {
-		xfs_warn(mp, "no-recovery mounts must be read-only.");
-		return -EINVAL;
-	}
-
-	if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (ctx->dsunit || ctx->dswidth)) {
-		xfs_warn(mp,
-	"sunit and swidth options incompatible with the noalign option");
-		return -EINVAL;
-	}
-
-#ifndef CONFIG_XFS_QUOTA
-	if (XFS_IS_QUOTA_RUNNING(mp)) {
-		xfs_warn(mp, "quota support not available in this kernel.");
-		return -EINVAL;
-	}
-#endif
-
-	if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit && ctx->dswidth)) {
-		xfs_warn(mp, "sunit and swidth must be specified together");
-		return -EINVAL;
-	}
-
-	if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) {
-		xfs_warn(mp,
-	"stripe width (%d) must be a multiple of the stripe unit (%d)",
-			ctx->dswidth, ctx->dsunit);
-		return -EINVAL;
+			goto done;
 	}
 
+	ret = xfs_validate_params(mp, ctx, false);
 done:
-	if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
-		/*
-		 * At this point the superblock has not been read
-		 * in, therefore we do not know the block size.
-		 * Before the mount call ends we will convert
-		 * these to FSBs.
-		 */
-		mp->m_dalign = ctx->dsunit;
-		mp->m_swidth = ctx->dswidth;
-	}
-
-	if (mp->m_logbufs != -1 &&
-	    mp->m_logbufs != 0 &&
-	    (mp->m_logbufs < XLOG_MIN_ICLOGS ||
-	     mp->m_logbufs > XLOG_MAX_ICLOGS)) {
-		xfs_warn(mp, "invalid logbufs value: %d [not %d-%d]",
-			mp->m_logbufs, XLOG_MIN_ICLOGS, XLOG_MAX_ICLOGS);
-		return -EINVAL;
-	}
-	if (mp->m_logbsize != -1 &&
-	    mp->m_logbsize !=  0 &&
-	    (mp->m_logbsize < XLOG_MIN_RECORD_BSIZE ||
-	     mp->m_logbsize > XLOG_MAX_RECORD_BSIZE ||
-	     !is_power_of_2(mp->m_logbsize))) {
-		xfs_warn(mp,
-			"invalid logbufsize: %d [not 16k,32k,64k,128k or 256k]",
-			mp->m_logbsize);
-		return -EINVAL;
-	}
-
-	if (ctx->iosizelog) {
-		if (ctx->iosizelog > XFS_MAX_IO_LOG ||
-		    ctx->iosizelog < XFS_MIN_IO_LOG) {
-			xfs_warn(mp, "invalid log iosize: %d [not %d-%d]",
-				ctx->iosizelog, XFS_MIN_IO_LOG,
-				XFS_MAX_IO_LOG);
-			return -EINVAL;
-		}
-
-		mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
-		mp->m_readio_log = ctx->iosizelog;
-		mp->m_writeio_log = ctx->iosizelog;
-	}
-
-	return 0;
+	return ret;
 }
 
 struct proc_xfs_info {