[v6,10/12] xfs: move xfs_parseargs() validation to a helper

Commit Message

Ian Kent Oct. 16, 2019, 12:41 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 <[e-mail]>
---
 fs/xfs/xfs_super.c |  136 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 75 insertions(+), 61 deletions(-)

Comments

Christoph Hellwig Oct. 16, 2019, 8:40 a.m. UTC | #1

On Wed, Oct 16, 2019 at 08:41:38AM +0800, Ian Kent wrote:
> +static int
> +xfs_validate_params(
> +	struct xfs_mount        *mp,
> +	int			dsunit,
> +	int			dswidth,
> +	uint8_t			iosizelog,
> +	bool			nooptions)

Please add a refactor patch before this one to always set the stripe
unit / width / log I/O size values in the mount structure at parsing
time as suggested before.  The will also remove the need for the
xfs_fs_context structure in the last patch.

> +	if (nooptions)
> +		goto noopts;

This option is always false in this patch.  I'd suggest you only add
it once we actually add a user.

Ian Kent Oct. 17, 2019, 12:58 a.m. UTC | #2

On Wed, 2019-10-16 at 01:40 -0700, Christoph Hellwig wrote:
> On Wed, Oct 16, 2019 at 08:41:38AM +0800, Ian Kent wrote:
> > +static int
> > +xfs_validate_params(
> > +	struct xfs_mount        *mp,
> > +	int			dsunit,
> > +	int			dswidth,
> > +	uint8_t			iosizelog,
> > +	bool			nooptions)
> 
> Please add a refactor patch before this one to always set the stripe
> unit / width / log I/O size values in the mount structure at parsing
> time as suggested before.  The will also remove the need for the
> xfs_fs_context structure in the last patch.
> 
> > +	if (nooptions)
> > +		goto noopts;
> 
> This option is always false in this patch.  I'd suggest you only add
> it once we actually add a user.

Thanks for looking at the series and for the comments.
I'll get on with the recommended changes and post an update.

Ian

Christoph Hellwig Oct. 17, 2019, 6:48 a.m. UTC | #3

On Thu, Oct 17, 2019 at 08:58:26AM +0800, Ian Kent wrote:
> > > +	if (nooptions)
> > > +		goto noopts;
> > 
> > This option is always false in this patch.  I'd suggest you only add
> > it once we actually add a user.
> 
> Thanks for looking at the series and for the comments.
> I'll get on with the recommended changes and post an update.

And for this one I'm not sure the goto option actually ever makes sense,
even with the final patch applied.  To me it looks like splitting the
function into two might be much cleaner.

Ian Kent Oct. 23, 2019, 2:59 a.m. UTC | #4

On Wed, 2019-10-16 at 01:40 -0700, Christoph Hellwig wrote:
> On Wed, Oct 16, 2019 at 08:41:38AM +0800, Ian Kent wrote:
> > +static int
> > +xfs_validate_params(
> > +	struct xfs_mount        *mp,
> > +	int			dsunit,
> > +	int			dswidth,
> > +	uint8_t			iosizelog,
> > +	bool			nooptions)
> 
> Please add a refactor patch before this one to always set the stripe
> unit / width / log I/O size values in the mount structure at parsing
> time as suggested before.  The will also remove the need for the
> xfs_fs_context structure in the last patch.

With the mount api it looks like this means adding the validation
checks to xfs_parse_param().

That could be done for iosizelog without problem other than the check
being redone for each instance of the "allocsize" option which is
probably ok.

But the dsunit and dswidth checks depend on one another so that check
needs to be done after all the options have been handled.

After the change to use the mount api the options string separation
is done in the VFS and xfs_parse_param() called separately for each
option.

This is done so that the new system call fsconfig() can be implemented
which will call the .parse_param() method separately for each option
too.

So it isn't known if both "sunit" and "swidth" have been given until
aft
er all the options have been looked at.

Ian

Patch

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index cd17e4e6b922..f5ea96073d11 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -305,67 +305,16 @@  xfs_parse_param(
 	return 0;
 }
 
-/*
- * This function fills in xfs_mount_t fields based on mount args.
- * Note: the superblock has _not_ yet been read in.
- *
- * Note that this function leaks the various device name allocations on
- * failure.  The caller takes care of them.
- *
- * *sb is const because this is also used to test options on the remount
- * path, and we don't want this to have any side effects at remount time.
- * Today this function does not change *sb, but just to future-proof...
- */
-STATIC int
-xfs_parseargs(
-	struct xfs_mount	*mp,
-	char			*options)
+static int
+xfs_validate_params(
+	struct xfs_mount        *mp,
+	int			dsunit,
+	int			dswidth,
+	uint8_t			iosizelog,
+	bool			nooptions)
 {
-	const struct super_block *sb = mp->m_super;
-	char			*p;
-	substring_t		args[MAX_OPT_ARGS];
-	int			dsunit = 0;
-	int			dswidth = 0;
-	uint8_t			iosizelog = 0;
-
-	/*
-	 * Copy binary VFS mount flags we are interested in.
-	 */
-	if (sb_rdonly(sb))
-		mp->m_flags |= XFS_MOUNT_RDONLY;
-	if (sb->s_flags & SB_DIRSYNC)
-		mp->m_flags |= XFS_MOUNT_DIRSYNC;
-	if (sb->s_flags & SB_SYNCHRONOUS)
-		mp->m_flags |= XFS_MOUNT_WSYNC;
-
-	/*
-	 * Set some default flags that could be cleared by the mount option
-	 * parsing.
-	 */
-	mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE;
-
-	/*
-	 * These can be overridden by the mount option parsing.
-	 */
-	mp->m_logbufs = -1;
-	mp->m_logbsize = -1;
-
-	if (!options)
-		goto done;
-
-	while ((p = strsep(&options, ",")) != NULL) {
-		int		token;
-		int		ret;
-
-		if (!*p)
-			continue;
-
-		token = match_token(p, tokens, args);
-		ret = xfs_parse_param(token, p, args, mp,
-				      &dsunit, &dswidth, &iosizelog);
-		if (ret)
-			return ret;
-	}
+	if (nooptions)
+		goto noopts;
 
 	/*
 	 * no recovery flag requires a read-only mount
@@ -401,7 +350,7 @@  xfs_parseargs(
 		return -EINVAL;
 	}
 
-done:
+noopts:
 	if (dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
 		/*
 		 * At this point the superblock has not been read
@@ -449,6 +398,71 @@  xfs_parseargs(
 	return 0;
 }
 
+/*
+ * This function fills in xfs_mount_t fields based on mount args.
+ * Note: the superblock has _not_ yet been read in.
+ *
+ * Note that this function leaks the various device name allocations on
+ * failure.  The caller takes care of them.
+ *
+ * *sb is const because this is also used to test options on the remount
+ * path, and we don't want this to have any side effects at remount time.
+ * Today this function does not change *sb, but just to future-proof...
+ */
+static int
+xfs_parseargs(
+	struct xfs_mount	*mp,
+	char			*options)
+{
+	const struct super_block *sb = mp->m_super;
+	char			*p;
+	substring_t		args[MAX_OPT_ARGS];
+	int			dsunit = 0;
+	int			dswidth = 0;
+	uint8_t			iosizelog = 0;
+
+	/*
+	 * Copy binary VFS mount flags we are interested in.
+	 */
+	if (sb_rdonly(sb))
+		mp->m_flags |= XFS_MOUNT_RDONLY;
+	if (sb->s_flags & SB_DIRSYNC)
+		mp->m_flags |= XFS_MOUNT_DIRSYNC;
+	if (sb->s_flags & SB_SYNCHRONOUS)
+		mp->m_flags |= XFS_MOUNT_WSYNC;
+
+	/*
+	 * Set some default flags that could be cleared by the mount option
+	 * parsing.
+	 */
+	mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE;
+
+	/*
+	 * These can be overridden by the mount option parsing.
+	 */
+	mp->m_logbufs = -1;
+	mp->m_logbsize = -1;
+
+	if (!options)
+		goto done;
+
+	while ((p = strsep(&options, ",")) != NULL) {
+		int		token;
+		int		ret;
+
+		if (!*p)
+			continue;
+
+		token = match_token(p, tokens, args);
+		ret = xfs_parse_param(token, p, args, mp,
+				      &dsunit, &dswidth, &iosizelog);
+		if (ret)
+			return ret;
+	}
+done:
+	return xfs_validate_params(mp, dsunit, dswidth, iosizelog, false);
+}
+
 struct proc_xfs_info {
 	uint64_t	flag;
 	char		*str;