All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] xfs: mount option handling fixups
@ 2016-02-10 23:36 Eric Sandeen
  2016-02-10 23:38 ` [PATCH 1/3] xfs: convert mount option parsing to tokens Eric Sandeen
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Eric Sandeen @ 2016-02-10 23:36 UTC (permalink / raw)
  To: xfs

1) convert to token parsing
2) basic sanitization of remount options, via normal option parsing
3) attempt (this is RFE-ish) to discern remount problems with otherwise valid options

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/3] xfs: convert mount option parsing to tokens
  2016-02-10 23:36 [PATCH 0/3] xfs: mount option handling fixups Eric Sandeen
@ 2016-02-10 23:38 ` Eric Sandeen
  2016-02-15 18:54   ` Brian Foster
                     ` (2 more replies)
  2016-02-10 23:40 ` [PATCH 2/3] xfs: sanitize remount options Eric Sandeen
  2016-02-10 23:45 ` [PATCH 3/3] xfs: test for valid remount options, error if not Eric Sandeen
  2 siblings, 3 replies; 14+ messages in thread
From: Eric Sandeen @ 2016-02-10 23:38 UTC (permalink / raw)
  To: xfs

This should be a no-op change, just switch to token parsing
like every other respectable filesystem does.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 59c9b7b..934233a 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -65,83 +65,82 @@ static struct kset *xfs_kset;		/* top-level xfs sysfs dir */
 static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs attrs */
 #endif
 
-#define MNTOPT_LOGBUFS	"logbufs"	/* number of XFS log buffers */
-#define MNTOPT_LOGBSIZE	"logbsize"	/* size of XFS log buffers */
-#define MNTOPT_LOGDEV	"logdev"	/* log device */
-#define MNTOPT_RTDEV	"rtdev"		/* realtime I/O device */
-#define MNTOPT_BIOSIZE	"biosize"	/* log2 of preferred buffered io size */
-#define MNTOPT_WSYNC	"wsync"		/* safe-mode nfs compatible mount */
-#define MNTOPT_NOALIGN	"noalign"	/* turn off stripe alignment */
-#define MNTOPT_SWALLOC	"swalloc"	/* turn on stripe width allocation */
-#define MNTOPT_SUNIT	"sunit"		/* data volume stripe unit */
-#define MNTOPT_SWIDTH	"swidth"	/* data volume stripe width */
-#define MNTOPT_NOUUID	"nouuid"	/* ignore filesystem UUID */
-#define MNTOPT_MTPT	"mtpt"		/* filesystem mount point */
-#define MNTOPT_GRPID	"grpid"		/* group-ID from parent directory */
-#define MNTOPT_NOGRPID	"nogrpid"	/* group-ID from current process */
-#define MNTOPT_BSDGROUPS    "bsdgroups"    /* group-ID from parent directory */
-#define MNTOPT_SYSVGROUPS   "sysvgroups"   /* group-ID from current process */
-#define MNTOPT_ALLOCSIZE    "allocsize"    /* preferred allocation size */
-#define MNTOPT_NORECOVERY   "norecovery"   /* don't run XFS recovery */
-#define MNTOPT_BARRIER	"barrier"	/* use writer barriers for log write and
-					 * unwritten extent conversion */
-#define MNTOPT_NOBARRIER "nobarrier"	/* .. disable */
-#define MNTOPT_64BITINODE   "inode64"	/* inodes can be allocated anywhere */
-#define MNTOPT_32BITINODE   "inode32"	/* inode allocation limited to
-					 * XFS_MAXINUMBER_32 */
-#define MNTOPT_IKEEP	"ikeep"		/* do not free empty inode clusters */
-#define MNTOPT_NOIKEEP	"noikeep"	/* free empty inode clusters */
-#define MNTOPT_LARGEIO	   "largeio"	/* report large I/O sizes in stat() */
-#define MNTOPT_NOLARGEIO   "nolargeio"	/* do not report large I/O sizes
-					 * in stat(). */
-#define MNTOPT_ATTR2	"attr2"		/* do use attr2 attribute format */
-#define MNTOPT_NOATTR2	"noattr2"	/* do not use attr2 attribute format */
-#define MNTOPT_FILESTREAM  "filestreams" /* use filestreams allocator */
-#define MNTOPT_QUOTA	"quota"		/* disk quotas (user) */
-#define MNTOPT_NOQUOTA	"noquota"	/* no quotas */
-#define MNTOPT_USRQUOTA	"usrquota"	/* user quota enabled */
-#define MNTOPT_GRPQUOTA	"grpquota"	/* group quota enabled */
-#define MNTOPT_PRJQUOTA	"prjquota"	/* project quota enabled */
-#define MNTOPT_UQUOTA	"uquota"	/* user quota (IRIX variant) */
-#define MNTOPT_GQUOTA	"gquota"	/* group quota (IRIX variant) */
-#define MNTOPT_PQUOTA	"pquota"	/* project quota (IRIX variant) */
-#define MNTOPT_UQUOTANOENF "uqnoenforce"/* user quota limit enforcement */
-#define MNTOPT_GQUOTANOENF "gqnoenforce"/* group quota limit enforcement */
-#define MNTOPT_PQUOTANOENF "pqnoenforce"/* project quota limit enforcement */
-#define MNTOPT_QUOTANOENF  "qnoenforce"	/* same as uqnoenforce */
-#define MNTOPT_DISCARD	   "discard"	/* Discard unused blocks */
-#define MNTOPT_NODISCARD   "nodiscard"	/* Do not discard unused blocks */
-
-#define MNTOPT_DAX	"dax"		/* Enable direct access to bdev pages */
-
 /*
  * Table driven mount option parser.
- *
- * Currently only used for remount, but it will be used for mount
- * in the future, too.
  */
 enum {
-	Opt_barrier,
-	Opt_nobarrier,
-	Opt_inode64,
-	Opt_inode32,
-	Opt_err
+	Opt_logbufs, Opt_logbsize, Opt_logdev, Opt_rtdev, Opt_biosize,
+	Opt_wsync, Opt_noalign, Opt_swalloc, Opt_sunit, Opt_swidth, Opt_nouuid,
+	Opt_mtpt, Opt_grpid, Opt_nogrpid, Opt_bsdgroups, Opt_sysvgroups,
+	Opt_allocsize, Opt_norecovery, Opt_barrier, Opt_nobarrier,
+	Opt_inode64, Opt_inode32, Opt_ikeep, Opt_noikeep,
+	Opt_largeio, Opt_nolargeio, Opt_attr2, Opt_noattr2, Opt_filestreams,
+	Opt_quota, Opt_noquota, Opt_usrquota, Opt_grpquota, Opt_prjquota,
+	Opt_uquota, Opt_gquota, Opt_pquota,
+	Opt_uqnoenforce, Opt_gqnoenforce, Opt_pqnoenforce, Opt_qnoenforce,
+	Opt_discard, Opt_nodiscard, Opt_dax, Opt_err,
 };
 
 static const match_table_t tokens = {
-	{Opt_barrier, "barrier"},
-	{Opt_nobarrier, "nobarrier"},
-	{Opt_inode64, "inode64"},
-	{Opt_inode32, "inode32"},
-	{Opt_err, NULL}
+	{Opt_logbufs,	"logbufs=%u"},	/* number of XFS log buffers */
+	{Opt_logbsize,	"logbsize=%s"},	/* size of XFS log buffers */
+	{Opt_logdev,	"logdev=%s"},	/* log device */
+	{Opt_rtdev,	"rtdev=%s"},	/* realtime I/O device */
+	{Opt_biosize,	"biosize=%u"},	/* log2 of preferred buffered io size */
+	{Opt_wsync,	"wsync"},	/* safe-mode nfs compatible mount */
+	{Opt_noalign,	"noalign"},	/* turn off stripe alignment */
+	{Opt_swalloc,	"swalloc"},	/* turn on stripe width allocation */
+	{Opt_sunit,	"sunit=%u"},	/* data volume stripe unit */
+	{Opt_swidth,	"swidth=%u"},	/* data volume stripe width */
+	{Opt_nouuid,	"nouuid"},	/* ignore filesystem UUID */
+	{Opt_mtpt,	"mtpt"},	/* filesystem mount point */
+	{Opt_grpid,	"grpid"},	/* group-ID from parent directory */
+	{Opt_nogrpid,	"nogrpid"},	/* group-ID from current process */
+	{Opt_bsdgroups,	"bsdgroups"},	/* group-ID from parent directory */
+	{Opt_sysvgroups,"sysvgroups"},	/* group-ID from current process */
+	{Opt_allocsize,	"allocsize=%s"},/* preferred allocation size */
+	{Opt_norecovery,"norecovery"},	/* don't run XFS recovery */
+	{Opt_barrier,	"barrier"},	/* use writer barriers for log write and
+					 * unwritten extent conversion */
+	{Opt_nobarrier,	"nobarrier"},	/* .. disable */
+	{Opt_inode64,	"inode64"},	/* inodes can be allocated anywhere */
+	{Opt_inode32,   "inode32"},	/* inode allocation limited to
+					 * XFS_MAXINUMBER_32 */
+	{Opt_ikeep,	"ikeep"},	/* do not free empty inode clusters */
+	{Opt_noikeep,	"noikeep"},	/* free empty inode clusters */
+	{Opt_largeio,	"largeio"},	/* report large I/O sizes in stat() */
+	{Opt_nolargeio,	"nolargeio"},	/* do not report large I/O sizes
+					 * in stat(). */
+	{Opt_attr2,	"attr2"},	/* do use attr2 attribute format */
+	{Opt_noattr2,	"noattr2"},	/* do not use attr2 attribute format */
+	{Opt_filestreams,"filestreams"},/* use filestreams allocator */
+	{Opt_quota,	"quota"},	/* disk quotas (user) */
+	{Opt_noquota,	"noquota"},	/* no quotas */
+	{Opt_usrquota,	"usrquota"},	/* user quota enabled */
+	{Opt_grpquota,	"grpquota"},	/* group quota enabled */
+	{Opt_prjquota,	"prjquota"},	/* project quota enabled */
+	{Opt_uquota,	"uquota"},	/* user quota (IRIX variant) */
+	{Opt_gquota,	"gquota"},	/* group quota (IRIX variant) */
+	{Opt_pquota,	"pquota"},	/* project quota (IRIX variant) */
+	{Opt_uqnoenforce,"uqnoenforce"},/* user quota limit enforcement */
+	{Opt_gqnoenforce,"gqnoenforce"},/* group quota limit enforcement */
+	{Opt_pqnoenforce,"pqnoenforce"},/* project quota limit enforcement */
+	{Opt_qnoenforce, "qnoenforce"},	/* same as uqnoenforce */
+	{Opt_discard,	"discard"},	/* Discard unused blocks */
+	{Opt_nodiscard,	"nodiscard"},	/* Do not discard unused blocks */
+
+	{Opt_dax,	"dax"},		/* Enable direct access to bdev pages */
+	{Opt_err,	NULL},
 };
 
 
 STATIC int
-suffix_kstrtoint(char *s, unsigned int base, int *res)
+suffix_kstrtoint(const substring_t *s, unsigned int base, int *res)
 {
 	int	last, shift_left_factor = 0, _res;
-	char	*value = s;
+	char	*value = match_strdup(s);
+	int	ret = 0;
+	
 
 	last = strlen(value) - 1;
 	if (value[last] == 'K' || value[last] == 'k') {
@@ -157,10 +156,11 @@ suffix_kstrtoint(char *s, unsigned int base, int *res)
 		value[last] = '\0';
 	}
 
-	if (kstrtoint(s, base, &_res))
-		return -EINVAL;
+	if (kstrtoint(value, base, &_res))
+		ret = -EINVAL;
+	kfree(value);
 	*res = _res << shift_left_factor;
-	return 0;
+	return ret;
 }
 
 /*
@@ -176,7 +176,8 @@ xfs_parseargs(
 	char			*options)
 {
 	struct super_block	*sb = mp->m_super;
-	char			*this_char, *value;
+	char			*p;
+	substring_t		args[MAX_OPT_ARGS];
 	int			dsunit = 0;
 	int			dswidth = 0;
 	int			iosize = 0;
@@ -217,152 +218,153 @@ xfs_parseargs(
 	if (!options)
 		goto done;
 
-	while ((this_char = strsep(&options, ",")) != NULL) {
-		if (!*this_char)
+	while ((p = strsep(&options, ",")) != NULL) {
+		int		token;
+
+		if (!*p)
 			continue;
-		if ((value = strchr(this_char, '=')) != NULL)
-			*value++ = 0;
 
-		if (!strcmp(this_char, MNTOPT_LOGBUFS)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
-				return -EINVAL;
-			}
-			if (kstrtoint(value, 10, &mp->m_logbufs))
-				return -EINVAL;
-		} else if (!strcmp(this_char, MNTOPT_LOGBSIZE)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
-				return -EINVAL;
-			}
-			if (suffix_kstrtoint(value, 10, &mp->m_logbsize))
+                token = match_token(p, tokens, args);
+		switch (token) {
+		case Opt_logbufs:
+			if (match_int(args, &mp->m_logbufs))
 				return -EINVAL;
-		} else if (!strcmp(this_char, MNTOPT_LOGDEV)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
+			break;
+		case Opt_logbsize:
+			if (suffix_kstrtoint(args, 10, &mp->m_logbsize))
 				return -EINVAL;
-			}
-			mp->m_logname = kstrndup(value, MAXNAMELEN, GFP_KERNEL);
+			break;
+		case Opt_logdev:
+			mp->m_logname = match_strdup(args);
 			if (!mp->m_logname)
 				return -ENOMEM;
-		} else if (!strcmp(this_char, MNTOPT_MTPT)) {
-			xfs_warn(mp, "%s option not allowed on this system",
-				this_char);
+			break;
+		case Opt_mtpt:
+			xfs_warn(mp, "%s option not allowed on this system", p);
 			return -EINVAL;
-		} else if (!strcmp(this_char, MNTOPT_RTDEV)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
-				return -EINVAL;
-			}
-			mp->m_rtname = kstrndup(value, MAXNAMELEN, GFP_KERNEL);
+			break;
+		case Opt_rtdev:
+			mp->m_rtname = match_strdup(args);
 			if (!mp->m_rtname)
 				return -ENOMEM;
-		} else if (!strcmp(this_char, MNTOPT_ALLOCSIZE) ||
-			   !strcmp(this_char, MNTOPT_BIOSIZE)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
-				return -EINVAL;
-			}
-			if (suffix_kstrtoint(value, 10, &iosize))
+			break;
+		case Opt_allocsize:
+		case Opt_biosize:
+			if (suffix_kstrtoint(args, 10, &iosize))
 				return -EINVAL;
 			iosizelog = ffs(iosize) - 1;
-		} else if (!strcmp(this_char, MNTOPT_GRPID) ||
-			   !strcmp(this_char, MNTOPT_BSDGROUPS)) {
+			break;
+		case Opt_grpid:
+		case Opt_bsdgroups:
 			mp->m_flags |= XFS_MOUNT_GRPID;
-		} else if (!strcmp(this_char, MNTOPT_NOGRPID) ||
-			   !strcmp(this_char, MNTOPT_SYSVGROUPS)) {
+			break;
+		case Opt_nogrpid:
+		case Opt_sysvgroups:
 			mp->m_flags &= ~XFS_MOUNT_GRPID;
-		} else if (!strcmp(this_char, MNTOPT_WSYNC)) {
+			break;
+		case Opt_wsync:
 			mp->m_flags |= XFS_MOUNT_WSYNC;
-		} else if (!strcmp(this_char, MNTOPT_NORECOVERY)) {
+			break;
+		case Opt_norecovery:
 			mp->m_flags |= XFS_MOUNT_NORECOVERY;
-		} else if (!strcmp(this_char, MNTOPT_NOALIGN)) {
+			break;
+		case Opt_noalign:
 			mp->m_flags |= XFS_MOUNT_NOALIGN;
-		} else if (!strcmp(this_char, MNTOPT_SWALLOC)) {
+			break;
+		case Opt_swalloc:
 			mp->m_flags |= XFS_MOUNT_SWALLOC;
-		} else if (!strcmp(this_char, MNTOPT_SUNIT)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
-				return -EINVAL;
-			}
-			if (kstrtoint(value, 10, &dsunit))
-				return -EINVAL;
-		} else if (!strcmp(this_char, MNTOPT_SWIDTH)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
+			break;
+		case Opt_sunit:
+			if (match_int(args, &dsunit))
 				return -EINVAL;
-			}
-			if (kstrtoint(value, 10, &dswidth))
+			break;
+		case Opt_swidth:
+			if (match_int(args, &dswidth))
 				return -EINVAL;
-		} else if (!strcmp(this_char, MNTOPT_32BITINODE)) {
+			break;
+		case Opt_inode32:
 			mp->m_flags |= XFS_MOUNT_SMALL_INUMS;
-		} else if (!strcmp(this_char, MNTOPT_64BITINODE)) {
+			break;
+		case Opt_inode64:
 			mp->m_flags &= ~XFS_MOUNT_SMALL_INUMS;
-		} else if (!strcmp(this_char, MNTOPT_NOUUID)) {
+			break;
+		case Opt_nouuid:
 			mp->m_flags |= XFS_MOUNT_NOUUID;
-		} else if (!strcmp(this_char, MNTOPT_BARRIER)) {
+			break;
+		case Opt_barrier:
 			mp->m_flags |= XFS_MOUNT_BARRIER;
-		} else if (!strcmp(this_char, MNTOPT_NOBARRIER)) {
+			break;
+		case Opt_nobarrier:
 			mp->m_flags &= ~XFS_MOUNT_BARRIER;
-		} else if (!strcmp(this_char, MNTOPT_IKEEP)) {
+			break;
+		case Opt_ikeep:
 			mp->m_flags |= XFS_MOUNT_IKEEP;
-		} else if (!strcmp(this_char, MNTOPT_NOIKEEP)) {
+			break;
+		case Opt_noikeep:
 			mp->m_flags &= ~XFS_MOUNT_IKEEP;
-		} else if (!strcmp(this_char, MNTOPT_LARGEIO)) {
+			break;
+		case Opt_largeio:
 			mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE;
-		} else if (!strcmp(this_char, MNTOPT_NOLARGEIO)) {
+			break;
+		case Opt_nolargeio:
 			mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE;
-		} else if (!strcmp(this_char, MNTOPT_ATTR2)) {
+			break;
+		case Opt_attr2:
 			mp->m_flags |= XFS_MOUNT_ATTR2;
-		} else if (!strcmp(this_char, MNTOPT_NOATTR2)) {
+			break;
+		case Opt_noattr2:
 			mp->m_flags &= ~XFS_MOUNT_ATTR2;
 			mp->m_flags |= XFS_MOUNT_NOATTR2;
-		} else if (!strcmp(this_char, MNTOPT_FILESTREAM)) {
+			break;
+		case Opt_filestreams:
 			mp->m_flags |= XFS_MOUNT_FILESTREAMS;
-		} else if (!strcmp(this_char, MNTOPT_NOQUOTA)) {
+			break;
+		case Opt_noquota:
 			mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT;
 			mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD;
 			mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE;
-		} else if (!strcmp(this_char, MNTOPT_QUOTA) ||
-			   !strcmp(this_char, MNTOPT_UQUOTA) ||
-			   !strcmp(this_char, MNTOPT_USRQUOTA)) {
+			break;
+		case Opt_quota:
+		case Opt_uquota:
+		case Opt_usrquota:
 			mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE |
 					 XFS_UQUOTA_ENFD);
-		} else if (!strcmp(this_char, MNTOPT_QUOTANOENF) ||
-			   !strcmp(this_char, MNTOPT_UQUOTANOENF)) {
+			break;
+		case Opt_qnoenforce:
+		case Opt_uqnoenforce:
 			mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE);
 			mp->m_qflags &= ~XFS_UQUOTA_ENFD;
-		} else if (!strcmp(this_char, MNTOPT_PQUOTA) ||
-			   !strcmp(this_char, MNTOPT_PRJQUOTA)) {
+			break;
+		case Opt_pquota:
+		case Opt_prjquota:
 			mp->m_qflags |= (XFS_PQUOTA_ACCT | XFS_PQUOTA_ACTIVE |
 					 XFS_PQUOTA_ENFD);
-		} else if (!strcmp(this_char, MNTOPT_PQUOTANOENF)) {
+			break;
+		case Opt_pqnoenforce:
 			mp->m_qflags |= (XFS_PQUOTA_ACCT | XFS_PQUOTA_ACTIVE);
 			mp->m_qflags &= ~XFS_PQUOTA_ENFD;
-		} else if (!strcmp(this_char, MNTOPT_GQUOTA) ||
-			   !strcmp(this_char, MNTOPT_GRPQUOTA)) {
+		case Opt_gquota:
+		case Opt_grpquota:
 			mp->m_qflags |= (XFS_GQUOTA_ACCT | XFS_GQUOTA_ACTIVE |
 					 XFS_GQUOTA_ENFD);
-		} else if (!strcmp(this_char, MNTOPT_GQUOTANOENF)) {
+			break;
+		case Opt_gqnoenforce:
 			mp->m_qflags |= (XFS_GQUOTA_ACCT | XFS_GQUOTA_ACTIVE);
 			mp->m_qflags &= ~XFS_GQUOTA_ENFD;
-		} else if (!strcmp(this_char, MNTOPT_DISCARD)) {
+			break;
+		case Opt_discard:
 			mp->m_flags |= XFS_MOUNT_DISCARD;
-		} else if (!strcmp(this_char, MNTOPT_NODISCARD)) {
+			break;
+		case Opt_nodiscard:
 			mp->m_flags &= ~XFS_MOUNT_DISCARD;
+			break;
 #ifdef CONFIG_FS_DAX
-		} else if (!strcmp(this_char, MNTOPT_DAX)) {
+		case Opt_dax:
 			mp->m_flags |= XFS_MOUNT_DAX;
+			break;
 #endif
-		} else {
-			xfs_warn(mp, "unknown mount option [%s].", this_char);
+		default:
+			xfs_warn(mp, "unknown mount option [%s].", p);
 			return -EINVAL;
 		}
 	}
@@ -461,25 +463,25 @@ xfs_showargs(
 {
 	static struct proc_xfs_info xfs_info_set[] = {
 		/* the few simple ones we can get from the mount struct */
-		{ XFS_MOUNT_IKEEP,		"," MNTOPT_IKEEP },
-		{ XFS_MOUNT_WSYNC,		"," MNTOPT_WSYNC },
-		{ XFS_MOUNT_NOALIGN,		"," MNTOPT_NOALIGN },
-		{ XFS_MOUNT_SWALLOC,		"," MNTOPT_SWALLOC },
-		{ XFS_MOUNT_NOUUID,		"," MNTOPT_NOUUID },
-		{ XFS_MOUNT_NORECOVERY,		"," MNTOPT_NORECOVERY },
-		{ XFS_MOUNT_ATTR2,		"," MNTOPT_ATTR2 },
-		{ XFS_MOUNT_FILESTREAMS,	"," MNTOPT_FILESTREAM },
-		{ XFS_MOUNT_GRPID,		"," MNTOPT_GRPID },
-		{ XFS_MOUNT_DISCARD,		"," MNTOPT_DISCARD },
-		{ XFS_MOUNT_SMALL_INUMS,	"," MNTOPT_32BITINODE },
-		{ XFS_MOUNT_DAX,		"," MNTOPT_DAX },
+		{ XFS_MOUNT_IKEEP,		",ikeep" },
+		{ XFS_MOUNT_WSYNC,		",wsync" },
+		{ XFS_MOUNT_NOALIGN,		",noalign" },
+		{ XFS_MOUNT_SWALLOC,		",swalloc" },
+		{ XFS_MOUNT_NOUUID,		",nouuid" },
+		{ XFS_MOUNT_NORECOVERY,		",norecovery" },
+		{ XFS_MOUNT_ATTR2,		",attr2" },
+		{ XFS_MOUNT_FILESTREAMS,	",filestreams" },
+		{ XFS_MOUNT_GRPID,		",grpid" },
+		{ XFS_MOUNT_DISCARD,		",discard" },
+		{ XFS_MOUNT_SMALL_INUMS,	",inode32" },
+		{ XFS_MOUNT_DAX,		",dax" },
 		{ 0, NULL }
 	};
 	static struct proc_xfs_info xfs_info_unset[] = {
 		/* the few simple ones we can get from the mount struct */
-		{ XFS_MOUNT_COMPAT_IOSIZE,	"," MNTOPT_LARGEIO },
-		{ XFS_MOUNT_BARRIER,		"," MNTOPT_NOBARRIER },
-		{ XFS_MOUNT_SMALL_INUMS,	"," MNTOPT_64BITINODE },
+		{ XFS_MOUNT_COMPAT_IOSIZE,	",largeio" },
+		{ XFS_MOUNT_BARRIER,		",nobarrier" },
+		{ XFS_MOUNT_SMALL_INUMS,	",inode64" },
 		{ 0, NULL }
 	};
 	struct proc_xfs_info	*xfs_infop;
@@ -494,46 +496,46 @@ xfs_showargs(
 	}
 
 	if (mp->m_flags & XFS_MOUNT_DFLT_IOSIZE)
-		seq_printf(m, "," MNTOPT_ALLOCSIZE "=%dk",
+		seq_printf(m, ",allocsize=%dk",
 				(int)(1 << mp->m_writeio_log) >> 10);
 
 	if (mp->m_logbufs > 0)
-		seq_printf(m, "," MNTOPT_LOGBUFS "=%d", mp->m_logbufs);
+		seq_printf(m, ",logbufs=%d", mp->m_logbufs);
 	if (mp->m_logbsize > 0)
-		seq_printf(m, "," MNTOPT_LOGBSIZE "=%dk", mp->m_logbsize >> 10);
+		seq_printf(m, ",logbsize=%dk", mp->m_logbsize >> 10);
 
 	if (mp->m_logname)
-		seq_show_option(m, MNTOPT_LOGDEV, mp->m_logname);
+		seq_show_option(m, "logdev", mp->m_logname);
 	if (mp->m_rtname)
-		seq_show_option(m, MNTOPT_RTDEV, mp->m_rtname);
+		seq_show_option(m, "rtdev", mp->m_rtname);
 
 	if (mp->m_dalign > 0)
-		seq_printf(m, "," MNTOPT_SUNIT "=%d",
+		seq_printf(m, ",sunit=%d",
 				(int)XFS_FSB_TO_BB(mp, mp->m_dalign));
 	if (mp->m_swidth > 0)
-		seq_printf(m, "," MNTOPT_SWIDTH "=%d",
+		seq_printf(m, ",swidth=%d",
 				(int)XFS_FSB_TO_BB(mp, mp->m_swidth));
 
 	if (mp->m_qflags & (XFS_UQUOTA_ACCT|XFS_UQUOTA_ENFD))
-		seq_puts(m, "," MNTOPT_USRQUOTA);
+		seq_puts(m, ",usrquota");
 	else if (mp->m_qflags & XFS_UQUOTA_ACCT)
-		seq_puts(m, "," MNTOPT_UQUOTANOENF);
+		seq_puts(m, ",uqnoenforce");
 
 	if (mp->m_qflags & XFS_PQUOTA_ACCT) {
 		if (mp->m_qflags & XFS_PQUOTA_ENFD)
-			seq_puts(m, "," MNTOPT_PRJQUOTA);
+			seq_puts(m, ",prjquota");
 		else
-			seq_puts(m, "," MNTOPT_PQUOTANOENF);
+			seq_puts(m, ",pqnoenforce");
 	}
 	if (mp->m_qflags & XFS_GQUOTA_ACCT) {
 		if (mp->m_qflags & XFS_GQUOTA_ENFD)
-			seq_puts(m, "," MNTOPT_GRPQUOTA);
+			seq_puts(m, ",grpquota");
 		else
-			seq_puts(m, "," MNTOPT_GQUOTANOENF);
+			seq_puts(m, ",gqnoenforce");
 	}
 
 	if (!(mp->m_qflags & XFS_ALL_QUOTA_ACCT))
-		seq_puts(m, "," MNTOPT_NOQUOTA);
+		seq_puts(m, ",noquota");
 
 	return 0;
 }
@@ -1344,9 +1346,8 @@ xfs_finish_flags(
 	 */
 	if (xfs_sb_version_hascrc(&mp->m_sb) &&
 	    (mp->m_flags & XFS_MOUNT_NOATTR2)) {
-		xfs_warn(mp,
-"Cannot mount a V5 filesystem as %s. %s is always enabled for V5 filesystems.",
-			MNTOPT_NOATTR2, MNTOPT_ATTR2);
+		xfs_warn(mp, "Cannot mount a V5 filesystem as noattr2. "
+			     "attr2 is always enabled for V5 filesystems.");
 		return -EINVAL;
 	}
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/3] xfs: sanitize remount options
  2016-02-10 23:36 [PATCH 0/3] xfs: mount option handling fixups Eric Sandeen
  2016-02-10 23:38 ` [PATCH 1/3] xfs: convert mount option parsing to tokens Eric Sandeen
@ 2016-02-10 23:40 ` Eric Sandeen
  2016-02-15 18:54   ` Brian Foster
  2016-02-17  4:29   ` [PATCH 2/3 V2] " Eric Sandeen
  2016-02-10 23:45 ` [PATCH 3/3] xfs: test for valid remount options, error if not Eric Sandeen
  2 siblings, 2 replies; 14+ messages in thread
From: Eric Sandeen @ 2016-02-10 23:40 UTC (permalink / raw)
  To: xfs

Perform basic sanitization of remount options by
passing the option string and a dummy mount structure
through xfs_parseargs and returning the result.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 934233a..d1cd4fa 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1168,6 +1168,27 @@ xfs_quiesce_attr(
 }
 
 STATIC int
+xfs_test_remount_options(
+	struct super_block	*sb,
+	struct xfs_mount	*mp,
+	char			*options)
+{
+	int			error = 0;
+	struct xfs_mount	*tmp;
+
+	tmp = kmem_zalloc(sizeof(*tmp), KM_MAYFAIL);
+	if (!tmp)
+		return -ENOMEM;
+
+	tmp->m_super = sb;
+	error = xfs_parseargs(tmp, options);
+	xfs_free_fsname(tmp);
+	kfree(tmp);
+
+	return error;
+}
+
+STATIC int
 xfs_fs_remount(
 	struct super_block	*sb,
 	int			*flags,
@@ -1179,6 +1200,11 @@ xfs_fs_remount(
 	char			*p;
 	int			error;
 
+	/* First, check for complete junk; i.e. invalid options */
+	error = xfs_test_remount_options(sb, mp, options);
+	if (error)
+		return error;
+
 	sync_filesystem(sb);
 	while ((p = strsep(&options, ",")) != NULL) {
 		int token;


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/3] xfs: test for valid remount options, error if not
  2016-02-10 23:36 [PATCH 0/3] xfs: mount option handling fixups Eric Sandeen
  2016-02-10 23:38 ` [PATCH 1/3] xfs: convert mount option parsing to tokens Eric Sandeen
  2016-02-10 23:40 ` [PATCH 2/3] xfs: sanitize remount options Eric Sandeen
@ 2016-02-10 23:45 ` Eric Sandeen
  2016-02-15 18:54   ` Brian Foster
  2016-02-15 20:25   ` Dave Chinner
  2 siblings, 2 replies; 14+ messages in thread
From: Eric Sandeen @ 2016-02-10 23:45 UTC (permalink / raw)
  To: xfs

This patch attempts to check for a valid set of remount
options.  As far as I can tell, it's tricky; as the old
comment says, on remount we may get a long string of
options from /proc/mounts and/or /etc/mtab, as well
as options specified on the commandline.  Later options
may negate previous options, etc.

At the most basic level, we may be handed a mount option
which we do not handle on remount, but which may not actually
be a change from the current mount option set.

Unfortunately our mount option state is somewhat far flung;
a combinations of m_flags, and values in various other
mount structure members; see the showargs function for
a taste of that.

So this extends xfs_test_remount_options() to do a full set
of mount processing of the options remount sees, to arrive
at a final state, then compares that state to the current
state, and determines if we can proceed.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

This is lightly tested; mostly just a sanity check to see
if this approach is a "wtf?" or a "yeah, seems ok."

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 986290c..3d4187c 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -455,7 +455,7 @@ xfs_set_maxicount(xfs_mount_t *mp)
  * We use smaller I/O sizes when the file system
  * is being used for NFS service (wsync mount option).
  */
-STATIC void
+void
 xfs_set_rw_sizes(xfs_mount_t *mp)
 {
 	xfs_sb_t	*sbp = &(mp->m_sb);
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index a4e03ab..bee9284 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -335,6 +335,7 @@ extern int	xfs_sb_validate_fsb_count(struct xfs_sb *, __uint64_t);
 
 extern int	xfs_dev_is_read_only(struct xfs_mount *, char *);
 
+extern void	xfs_set_rw_sizes(xfs_mount_t *);
 extern void	xfs_set_low_space_thresholds(struct xfs_mount *);
 
 int	xfs_zero_extent(struct xfs_inode *ip, xfs_fsblock_t start_fsb,
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index d1cd4fa..50e15d8 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -65,6 +65,8 @@ static struct kset *xfs_kset;		/* top-level xfs sysfs dir */
 static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs attrs */
 #endif
 
+STATIC int xfs_finish_flags(struct xfs_mount *mp);
+
 /*
  * Table driven mount option parser.
  */
@@ -1167,24 +1169,87 @@ xfs_quiesce_attr(
 	xfs_log_quiesce(mp);
 }
 
+#define XFS_BAD_REMOUNT_GOTO(mp, option, l)	\
+	{ \
+		xfs_warn(mp, \
+		  option " options may not be changed via remount");	\
+		goto l;	\
+	}
+/*
+ * Remount can only handle a few options today.
+ * First, check for completely bogus options by looking for errors
+ * from xfs_parseargs.
+ * But if that passes, look at the result to make sure we're not changin
+ * anything that is not, in fact, remountable.  Do this by parsing all
+ * options into a dummy mount structure, and compare the final result
+ * to the current one so we can see what actually changed.
+ */
 STATIC int
 xfs_test_remount_options(
 	struct super_block	*sb,
 	struct xfs_mount	*mp,
 	char			*options)
 {
-	int			error = 0;
-	struct xfs_mount	*tmp;
+	int			error;
+	struct xfs_mount	*tmp_mp;
+
+	__uint64_t		ok_flags, changed_flags;
 
-	tmp = kmem_zalloc(sizeof(*tmp), KM_MAYFAIL);
-	if (!tmp)
+	tmp_mp = kmem_zalloc(sizeof(*tmp_mp), KM_MAYFAIL);
+	if (!tmp_mp)
 		return -ENOMEM;
 
-	tmp->m_super = sb;
-	error = xfs_parseargs(tmp, options);
-	xfs_free_fsname(tmp);
-	kfree(tmp);
+	tmp_mp->m_super = sb;
+	/* structure copy */
+	tmp_mp->m_sb = mp->m_sb;
+
+	/* Emulate mount parsing & flag setting on tmp_mp */
+	error = xfs_parseargs(tmp_mp, options);
+	if (error)
+		goto out;
+
+	error = xfs_finish_flags(tmp_mp);
+	if (error)
+		goto out;
+
+	xfs_set_rw_sizes(tmp_mp);
 
+	/* The only flags we can change on remount */
+	ok_flags = XFS_MOUNT_BARRIER | XFS_MOUNT_RDONLY |
+		   XFS_MOUNT_SMALL_INUMS | XFS_MOUNT_32BITINODES; 
+	/* This is only used internally, so OK as well */
+	ok_flags |= XFS_MOUNT_WAS_CLEAN;
+
+	/* The flags that *did* change */
+	changed_flags = (tmp_mp->m_flags ^ mp->m_flags);
+
+	error = -EINVAL;
+
+	if (tmp_mp->m_qflags != mp->m_qflags)
+		XFS_BAD_REMOUNT_GOTO(mp, "quota", out);
+
+	if (tmp_mp->m_logbufs != mp->m_logbufs ||
+	    tmp_mp->m_logbsize != mp->m_logbsize)
+		XFS_BAD_REMOUNT_GOTO(mp, "logbufs/logbsize", out);
+
+	if (tmp_mp->m_readio_log != mp->m_readio_log ||
+	    tmp_mp->m_writeio_log != mp->m_writeio_log)
+		XFS_BAD_REMOUNT_GOTO(mp, "allocsize/biosize", out);
+
+	if ((tmp_mp->m_logname && mp->m_logname &&
+	     strcmp(tmp_mp->m_logname, mp->m_logname)) ||
+	    (tmp_mp->m_rtname &&  mp->m_rtname &&
+	     strcmp(tmp_mp->m_rtname, mp->m_rtname)))
+		XFS_BAD_REMOUNT_GOTO(mp, "logdev/rtdev", out);
+
+	/* Catch-all for anything else */
+	if (changed_flags & ~ok_flags)
+		XFS_BAD_REMOUNT_GOTO(mp, "specified", out);
+
+	error = 0;
+out:
+	xfs_free_fsname(tmp_mp);
+	kfree(tmp_mp);
 	return error;
 }
 
@@ -1200,7 +1265,12 @@ xfs_fs_remount(
 	char			*p;
 	int			error;
 
-	/* First, check for complete junk; i.e. invalid options */
+	/*
+	 * Remounting is tricky; we get various combinations
+	 * of options, both pre-existing and changed, here.
+	 * This function tries to ensure that what we got
+	 * is a sane set for remounting, and errors if not.
+	 */
 	error = xfs_test_remount_options(sb, mp, options);
 	if (error)
 		return error;
@@ -1228,28 +1298,13 @@ xfs_fs_remount(
 			break;
 		default:
 			/*
-			 * Logically we would return an error here to prevent
-			 * users from believing they might have changed
-			 * mount options using remount which can't be changed.
-			 *
-			 * But unfortunately mount(8) adds all options from
-			 * mtab and fstab to the mount arguments in some cases
-			 * so we can't blindly reject options, but have to
-			 * check for each specified option if it actually
-			 * differs from the currently set option and only
-			 * reject it if that's the case.
-			 *
-			 * Until that is implemented we return success for
-			 * every remount request, and silently ignore all
-			 * options that we can't actually change.
+			 * xfs_test_remount_options really should have errored
+			 * out on any non-remountable options; anything that got 
+			 * here should be a no-op; a re-statement of existing
+			 * options. If something slipped through: too bad!
+			 * We'll just ignore it.
 			 */
-#if 0
-			xfs_info(mp,
-		"mount option \"%s\" not supported for remount", p);
-			return -EINVAL;
-#else
 			break;
-#endif
 		}
 	}
 


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] xfs: convert mount option parsing to tokens
  2016-02-10 23:38 ` [PATCH 1/3] xfs: convert mount option parsing to tokens Eric Sandeen
@ 2016-02-15 18:54   ` Brian Foster
  2016-02-15 21:20   ` [PATCH 1/3 V2] " Eric Sandeen
  2016-02-17 17:19   ` [PATCH 1/3 V3] " Eric Sandeen
  2 siblings, 0 replies; 14+ messages in thread
From: Brian Foster @ 2016-02-15 18:54 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: xfs

On Wed, Feb 10, 2016 at 05:38:14PM -0600, Eric Sandeen wrote:
> This should be a no-op change, just switch to token parsing
> like every other respectable filesystem does.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 59c9b7b..934233a 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -65,83 +65,82 @@ static struct kset *xfs_kset;		/* top-level xfs sysfs dir */
...
>  STATIC int
> -suffix_kstrtoint(char *s, unsigned int base, int *res)
> +suffix_kstrtoint(const substring_t *s, unsigned int base, int *res)
>  {
>  	int	last, shift_left_factor = 0, _res;
> -	char	*value = s;
> +	char	*value = match_strdup(s);
> +	int	ret = 0;
> +	

Trailing whitespace here ^^

>  
>  	last = strlen(value) - 1;
>  	if (value[last] == 'K' || value[last] == 'k') {
...
> @@ -217,152 +218,153 @@ xfs_parseargs(
>  	if (!options)
>  		goto done;
>  
> -	while ((this_char = strsep(&options, ",")) != NULL) {
> -		if (!*this_char)
> +	while ((p = strsep(&options, ",")) != NULL) {
> +		int		token;
> +
> +		if (!*p)
>  			continue;
> -		if ((value = strchr(this_char, '=')) != NULL)
> -			*value++ = 0;
>  
> -		if (!strcmp(this_char, MNTOPT_LOGBUFS)) {
> -			if (!value || !*value) {
> -				xfs_warn(mp, "%s option requires an argument",
> -					this_char);
> -				return -EINVAL;
> -			}
> -			if (kstrtoint(value, 10, &mp->m_logbufs))
> -				return -EINVAL;
> -		} else if (!strcmp(this_char, MNTOPT_LOGBSIZE)) {
> -			if (!value || !*value) {
> -				xfs_warn(mp, "%s option requires an argument",
> -					this_char);
> -				return -EINVAL;
> -			}
> -			if (suffix_kstrtoint(value, 10, &mp->m_logbsize))
> +                token = match_token(p, tokens, args);

^^^ spaces instead of tabs.

> +		switch (token) {
> +		case Opt_logbufs:
> +			if (match_int(args, &mp->m_logbufs))
>  				return -EINVAL;
> -		} else if (!strcmp(this_char, MNTOPT_LOGDEV)) {
> -			if (!value || !*value) {
> -				xfs_warn(mp, "%s option requires an argument",
> -					this_char);
> +			break;
> +		case Opt_logbsize:
> +			if (suffix_kstrtoint(args, 10, &mp->m_logbsize))
>  				return -EINVAL;
> -			}
> -			mp->m_logname = kstrndup(value, MAXNAMELEN, GFP_KERNEL);
> +			break;
> +		case Opt_logdev:
> +			mp->m_logname = match_strdup(args);
>  			if (!mp->m_logname)
>  				return -ENOMEM;
> -		} else if (!strcmp(this_char, MNTOPT_MTPT)) {
> -			xfs_warn(mp, "%s option not allowed on this system",
> -				this_char);
> +			break;
> +		case Opt_mtpt:
> +			xfs_warn(mp, "%s option not allowed on this system", p);
>  			return -EINVAL;
> -		} else if (!strcmp(this_char, MNTOPT_RTDEV)) {
> -			if (!value || !*value) {
> -				xfs_warn(mp, "%s option requires an argument",
> -					this_char);
> -				return -EINVAL;
> -			}
> -			mp->m_rtname = kstrndup(value, MAXNAMELEN, GFP_KERNEL);
> +			break;

Any reason for the break after the return here? Otherwise seems fine:

Reviewed-by: Brian Foster <bfoster@redhat.com>

Brian

> +		case Opt_rtdev:
> +			mp->m_rtname = match_strdup(args);
>  			if (!mp->m_rtname)
>  				return -ENOMEM;
> -		} else if (!strcmp(this_char, MNTOPT_ALLOCSIZE) ||
> -			   !strcmp(this_char, MNTOPT_BIOSIZE)) {
> -			if (!value || !*value) {
> -				xfs_warn(mp, "%s option requires an argument",
> -					this_char);
> -				return -EINVAL;
> -			}
> -			if (suffix_kstrtoint(value, 10, &iosize))
> +			break;
> +		case Opt_allocsize:
> +		case Opt_biosize:
> +			if (suffix_kstrtoint(args, 10, &iosize))
>  				return -EINVAL;
>  			iosizelog = ffs(iosize) - 1;
> -		} else if (!strcmp(this_char, MNTOPT_GRPID) ||
> -			   !strcmp(this_char, MNTOPT_BSDGROUPS)) {
> +			break;
> +		case Opt_grpid:
> +		case Opt_bsdgroups:
>  			mp->m_flags |= XFS_MOUNT_GRPID;
> -		} else if (!strcmp(this_char, MNTOPT_NOGRPID) ||
> -			   !strcmp(this_char, MNTOPT_SYSVGROUPS)) {
> +			break;
> +		case Opt_nogrpid:
> +		case Opt_sysvgroups:
>  			mp->m_flags &= ~XFS_MOUNT_GRPID;
> -		} else if (!strcmp(this_char, MNTOPT_WSYNC)) {
> +			break;
> +		case Opt_wsync:
>  			mp->m_flags |= XFS_MOUNT_WSYNC;
> -		} else if (!strcmp(this_char, MNTOPT_NORECOVERY)) {
> +			break;
> +		case Opt_norecovery:
>  			mp->m_flags |= XFS_MOUNT_NORECOVERY;
> -		} else if (!strcmp(this_char, MNTOPT_NOALIGN)) {
> +			break;
> +		case Opt_noalign:
>  			mp->m_flags |= XFS_MOUNT_NOALIGN;
> -		} else if (!strcmp(this_char, MNTOPT_SWALLOC)) {
> +			break;
> +		case Opt_swalloc:
>  			mp->m_flags |= XFS_MOUNT_SWALLOC;
> -		} else if (!strcmp(this_char, MNTOPT_SUNIT)) {
> -			if (!value || !*value) {
> -				xfs_warn(mp, "%s option requires an argument",
> -					this_char);
> -				return -EINVAL;
> -			}
> -			if (kstrtoint(value, 10, &dsunit))
> -				return -EINVAL;
> -		} else if (!strcmp(this_char, MNTOPT_SWIDTH)) {
> -			if (!value || !*value) {
> -				xfs_warn(mp, "%s option requires an argument",
> -					this_char);
> +			break;
> +		case Opt_sunit:
> +			if (match_int(args, &dsunit))
>  				return -EINVAL;
> -			}
> -			if (kstrtoint(value, 10, &dswidth))
> +			break;
> +		case Opt_swidth:
> +			if (match_int(args, &dswidth))
>  				return -EINVAL;
> -		} else if (!strcmp(this_char, MNTOPT_32BITINODE)) {
> +			break;
> +		case Opt_inode32:
>  			mp->m_flags |= XFS_MOUNT_SMALL_INUMS;
> -		} else if (!strcmp(this_char, MNTOPT_64BITINODE)) {
> +			break;
> +		case Opt_inode64:
>  			mp->m_flags &= ~XFS_MOUNT_SMALL_INUMS;
> -		} else if (!strcmp(this_char, MNTOPT_NOUUID)) {
> +			break;
> +		case Opt_nouuid:
>  			mp->m_flags |= XFS_MOUNT_NOUUID;
> -		} else if (!strcmp(this_char, MNTOPT_BARRIER)) {
> +			break;
> +		case Opt_barrier:
>  			mp->m_flags |= XFS_MOUNT_BARRIER;
> -		} else if (!strcmp(this_char, MNTOPT_NOBARRIER)) {
> +			break;
> +		case Opt_nobarrier:
>  			mp->m_flags &= ~XFS_MOUNT_BARRIER;
> -		} else if (!strcmp(this_char, MNTOPT_IKEEP)) {
> +			break;
> +		case Opt_ikeep:
>  			mp->m_flags |= XFS_MOUNT_IKEEP;
> -		} else if (!strcmp(this_char, MNTOPT_NOIKEEP)) {
> +			break;
> +		case Opt_noikeep:
>  			mp->m_flags &= ~XFS_MOUNT_IKEEP;
> -		} else if (!strcmp(this_char, MNTOPT_LARGEIO)) {
> +			break;
> +		case Opt_largeio:
>  			mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE;
> -		} else if (!strcmp(this_char, MNTOPT_NOLARGEIO)) {
> +			break;
> +		case Opt_nolargeio:
>  			mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE;
> -		} else if (!strcmp(this_char, MNTOPT_ATTR2)) {
> +			break;
> +		case Opt_attr2:
>  			mp->m_flags |= XFS_MOUNT_ATTR2;
> -		} else if (!strcmp(this_char, MNTOPT_NOATTR2)) {
> +			break;
> +		case Opt_noattr2:
>  			mp->m_flags &= ~XFS_MOUNT_ATTR2;
>  			mp->m_flags |= XFS_MOUNT_NOATTR2;
> -		} else if (!strcmp(this_char, MNTOPT_FILESTREAM)) {
> +			break;
> +		case Opt_filestreams:
>  			mp->m_flags |= XFS_MOUNT_FILESTREAMS;
> -		} else if (!strcmp(this_char, MNTOPT_NOQUOTA)) {
> +			break;
> +		case Opt_noquota:
>  			mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT;
>  			mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD;
>  			mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE;
> -		} else if (!strcmp(this_char, MNTOPT_QUOTA) ||
> -			   !strcmp(this_char, MNTOPT_UQUOTA) ||
> -			   !strcmp(this_char, MNTOPT_USRQUOTA)) {
> +			break;
> +		case Opt_quota:
> +		case Opt_uquota:
> +		case Opt_usrquota:
>  			mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE |
>  					 XFS_UQUOTA_ENFD);
> -		} else if (!strcmp(this_char, MNTOPT_QUOTANOENF) ||
> -			   !strcmp(this_char, MNTOPT_UQUOTANOENF)) {
> +			break;
> +		case Opt_qnoenforce:
> +		case Opt_uqnoenforce:
>  			mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE);
>  			mp->m_qflags &= ~XFS_UQUOTA_ENFD;
> -		} else if (!strcmp(this_char, MNTOPT_PQUOTA) ||
> -			   !strcmp(this_char, MNTOPT_PRJQUOTA)) {
> +			break;
> +		case Opt_pquota:
> +		case Opt_prjquota:
>  			mp->m_qflags |= (XFS_PQUOTA_ACCT | XFS_PQUOTA_ACTIVE |
>  					 XFS_PQUOTA_ENFD);
> -		} else if (!strcmp(this_char, MNTOPT_PQUOTANOENF)) {
> +			break;
> +		case Opt_pqnoenforce:
>  			mp->m_qflags |= (XFS_PQUOTA_ACCT | XFS_PQUOTA_ACTIVE);
>  			mp->m_qflags &= ~XFS_PQUOTA_ENFD;
> -		} else if (!strcmp(this_char, MNTOPT_GQUOTA) ||
> -			   !strcmp(this_char, MNTOPT_GRPQUOTA)) {
> +		case Opt_gquota:
> +		case Opt_grpquota:
>  			mp->m_qflags |= (XFS_GQUOTA_ACCT | XFS_GQUOTA_ACTIVE |
>  					 XFS_GQUOTA_ENFD);
> -		} else if (!strcmp(this_char, MNTOPT_GQUOTANOENF)) {
> +			break;
> +		case Opt_gqnoenforce:
>  			mp->m_qflags |= (XFS_GQUOTA_ACCT | XFS_GQUOTA_ACTIVE);
>  			mp->m_qflags &= ~XFS_GQUOTA_ENFD;
> -		} else if (!strcmp(this_char, MNTOPT_DISCARD)) {
> +			break;
> +		case Opt_discard:
>  			mp->m_flags |= XFS_MOUNT_DISCARD;
> -		} else if (!strcmp(this_char, MNTOPT_NODISCARD)) {
> +			break;
> +		case Opt_nodiscard:
>  			mp->m_flags &= ~XFS_MOUNT_DISCARD;
> +			break;
>  #ifdef CONFIG_FS_DAX
> -		} else if (!strcmp(this_char, MNTOPT_DAX)) {
> +		case Opt_dax:
>  			mp->m_flags |= XFS_MOUNT_DAX;
> +			break;
>  #endif
> -		} else {
> -			xfs_warn(mp, "unknown mount option [%s].", this_char);
> +		default:
> +			xfs_warn(mp, "unknown mount option [%s].", p);
>  			return -EINVAL;
>  		}
>  	}
> @@ -461,25 +463,25 @@ xfs_showargs(
>  {
>  	static struct proc_xfs_info xfs_info_set[] = {
>  		/* the few simple ones we can get from the mount struct */
> -		{ XFS_MOUNT_IKEEP,		"," MNTOPT_IKEEP },
> -		{ XFS_MOUNT_WSYNC,		"," MNTOPT_WSYNC },
> -		{ XFS_MOUNT_NOALIGN,		"," MNTOPT_NOALIGN },
> -		{ XFS_MOUNT_SWALLOC,		"," MNTOPT_SWALLOC },
> -		{ XFS_MOUNT_NOUUID,		"," MNTOPT_NOUUID },
> -		{ XFS_MOUNT_NORECOVERY,		"," MNTOPT_NORECOVERY },
> -		{ XFS_MOUNT_ATTR2,		"," MNTOPT_ATTR2 },
> -		{ XFS_MOUNT_FILESTREAMS,	"," MNTOPT_FILESTREAM },
> -		{ XFS_MOUNT_GRPID,		"," MNTOPT_GRPID },
> -		{ XFS_MOUNT_DISCARD,		"," MNTOPT_DISCARD },
> -		{ XFS_MOUNT_SMALL_INUMS,	"," MNTOPT_32BITINODE },
> -		{ XFS_MOUNT_DAX,		"," MNTOPT_DAX },
> +		{ XFS_MOUNT_IKEEP,		",ikeep" },
> +		{ XFS_MOUNT_WSYNC,		",wsync" },
> +		{ XFS_MOUNT_NOALIGN,		",noalign" },
> +		{ XFS_MOUNT_SWALLOC,		",swalloc" },
> +		{ XFS_MOUNT_NOUUID,		",nouuid" },
> +		{ XFS_MOUNT_NORECOVERY,		",norecovery" },
> +		{ XFS_MOUNT_ATTR2,		",attr2" },
> +		{ XFS_MOUNT_FILESTREAMS,	",filestreams" },
> +		{ XFS_MOUNT_GRPID,		",grpid" },
> +		{ XFS_MOUNT_DISCARD,		",discard" },
> +		{ XFS_MOUNT_SMALL_INUMS,	",inode32" },
> +		{ XFS_MOUNT_DAX,		",dax" },
>  		{ 0, NULL }
>  	};
>  	static struct proc_xfs_info xfs_info_unset[] = {
>  		/* the few simple ones we can get from the mount struct */
> -		{ XFS_MOUNT_COMPAT_IOSIZE,	"," MNTOPT_LARGEIO },
> -		{ XFS_MOUNT_BARRIER,		"," MNTOPT_NOBARRIER },
> -		{ XFS_MOUNT_SMALL_INUMS,	"," MNTOPT_64BITINODE },
> +		{ XFS_MOUNT_COMPAT_IOSIZE,	",largeio" },
> +		{ XFS_MOUNT_BARRIER,		",nobarrier" },
> +		{ XFS_MOUNT_SMALL_INUMS,	",inode64" },
>  		{ 0, NULL }
>  	};
>  	struct proc_xfs_info	*xfs_infop;
> @@ -494,46 +496,46 @@ xfs_showargs(
>  	}
>  
>  	if (mp->m_flags & XFS_MOUNT_DFLT_IOSIZE)
> -		seq_printf(m, "," MNTOPT_ALLOCSIZE "=%dk",
> +		seq_printf(m, ",allocsize=%dk",
>  				(int)(1 << mp->m_writeio_log) >> 10);
>  
>  	if (mp->m_logbufs > 0)
> -		seq_printf(m, "," MNTOPT_LOGBUFS "=%d", mp->m_logbufs);
> +		seq_printf(m, ",logbufs=%d", mp->m_logbufs);
>  	if (mp->m_logbsize > 0)
> -		seq_printf(m, "," MNTOPT_LOGBSIZE "=%dk", mp->m_logbsize >> 10);
> +		seq_printf(m, ",logbsize=%dk", mp->m_logbsize >> 10);
>  
>  	if (mp->m_logname)
> -		seq_show_option(m, MNTOPT_LOGDEV, mp->m_logname);
> +		seq_show_option(m, "logdev", mp->m_logname);
>  	if (mp->m_rtname)
> -		seq_show_option(m, MNTOPT_RTDEV, mp->m_rtname);
> +		seq_show_option(m, "rtdev", mp->m_rtname);
>  
>  	if (mp->m_dalign > 0)
> -		seq_printf(m, "," MNTOPT_SUNIT "=%d",
> +		seq_printf(m, ",sunit=%d",
>  				(int)XFS_FSB_TO_BB(mp, mp->m_dalign));
>  	if (mp->m_swidth > 0)
> -		seq_printf(m, "," MNTOPT_SWIDTH "=%d",
> +		seq_printf(m, ",swidth=%d",
>  				(int)XFS_FSB_TO_BB(mp, mp->m_swidth));
>  
>  	if (mp->m_qflags & (XFS_UQUOTA_ACCT|XFS_UQUOTA_ENFD))
> -		seq_puts(m, "," MNTOPT_USRQUOTA);
> +		seq_puts(m, ",usrquota");
>  	else if (mp->m_qflags & XFS_UQUOTA_ACCT)
> -		seq_puts(m, "," MNTOPT_UQUOTANOENF);
> +		seq_puts(m, ",uqnoenforce");
>  
>  	if (mp->m_qflags & XFS_PQUOTA_ACCT) {
>  		if (mp->m_qflags & XFS_PQUOTA_ENFD)
> -			seq_puts(m, "," MNTOPT_PRJQUOTA);
> +			seq_puts(m, ",prjquota");
>  		else
> -			seq_puts(m, "," MNTOPT_PQUOTANOENF);
> +			seq_puts(m, ",pqnoenforce");
>  	}
>  	if (mp->m_qflags & XFS_GQUOTA_ACCT) {
>  		if (mp->m_qflags & XFS_GQUOTA_ENFD)
> -			seq_puts(m, "," MNTOPT_GRPQUOTA);
> +			seq_puts(m, ",grpquota");
>  		else
> -			seq_puts(m, "," MNTOPT_GQUOTANOENF);
> +			seq_puts(m, ",gqnoenforce");
>  	}
>  
>  	if (!(mp->m_qflags & XFS_ALL_QUOTA_ACCT))
> -		seq_puts(m, "," MNTOPT_NOQUOTA);
> +		seq_puts(m, ",noquota");
>  
>  	return 0;
>  }
> @@ -1344,9 +1346,8 @@ xfs_finish_flags(
>  	 */
>  	if (xfs_sb_version_hascrc(&mp->m_sb) &&
>  	    (mp->m_flags & XFS_MOUNT_NOATTR2)) {
> -		xfs_warn(mp,
> -"Cannot mount a V5 filesystem as %s. %s is always enabled for V5 filesystems.",
> -			MNTOPT_NOATTR2, MNTOPT_ATTR2);
> +		xfs_warn(mp, "Cannot mount a V5 filesystem as noattr2. "
> +			     "attr2 is always enabled for V5 filesystems.");
>  		return -EINVAL;
>  	}
>  
> 
> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] xfs: sanitize remount options
  2016-02-10 23:40 ` [PATCH 2/3] xfs: sanitize remount options Eric Sandeen
@ 2016-02-15 18:54   ` Brian Foster
  2016-02-17  4:29   ` [PATCH 2/3 V2] " Eric Sandeen
  1 sibling, 0 replies; 14+ messages in thread
From: Brian Foster @ 2016-02-15 18:54 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: xfs

On Wed, Feb 10, 2016 at 05:40:05PM -0600, Eric Sandeen wrote:
> Perform basic sanitization of remount options by
> passing the option string and a dummy mount structure
> through xfs_parseargs and returning the result.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 934233a..d1cd4fa 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1168,6 +1168,27 @@ xfs_quiesce_attr(
>  }
>  
>  STATIC int
> +xfs_test_remount_options(
> +	struct super_block	*sb,
> +	struct xfs_mount	*mp,
> +	char			*options)
> +{
> +	int			error = 0;
> +	struct xfs_mount	*tmp;
> +
> +	tmp = kmem_zalloc(sizeof(*tmp), KM_MAYFAIL);
> +	if (!tmp)
> +		return -ENOMEM;
> +
> +	tmp->m_super = sb;
> +	error = xfs_parseargs(tmp, options);
> +	xfs_free_fsname(tmp);
> +	kfree(tmp);
> +
> +	return error;
> +}

This seems fine:

Reviewed-by: Brian Foster <bfoster@redhat.com>

... though I get a little bit nervous about xfs_parseargs() changing
some other data structure down the road. I wonder if we should constify
the sb pointer in xfs_parseargs() with a quick comment as to why..?

Brian

> +
> +STATIC int
>  xfs_fs_remount(
>  	struct super_block	*sb,
>  	int			*flags,
> @@ -1179,6 +1200,11 @@ xfs_fs_remount(
>  	char			*p;
>  	int			error;
>  
> +	/* First, check for complete junk; i.e. invalid options */
> +	error = xfs_test_remount_options(sb, mp, options);
> +	if (error)
> +		return error;
> +
>  	sync_filesystem(sb);
>  	while ((p = strsep(&options, ",")) != NULL) {
>  		int token;
> 
> 
> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] xfs: test for valid remount options, error if not
  2016-02-10 23:45 ` [PATCH 3/3] xfs: test for valid remount options, error if not Eric Sandeen
@ 2016-02-15 18:54   ` Brian Foster
  2016-02-15 20:25   ` Dave Chinner
  1 sibling, 0 replies; 14+ messages in thread
From: Brian Foster @ 2016-02-15 18:54 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: xfs

On Wed, Feb 10, 2016 at 05:45:33PM -0600, Eric Sandeen wrote:
> This patch attempts to check for a valid set of remount
> options.  As far as I can tell, it's tricky; as the old
> comment says, on remount we may get a long string of
> options from /proc/mounts and/or /etc/mtab, as well
> as options specified on the commandline.  Later options
> may negate previous options, etc.
> 
> At the most basic level, we may be handed a mount option
> which we do not handle on remount, but which may not actually
> be a change from the current mount option set.
> 
> Unfortunately our mount option state is somewhat far flung;
> a combinations of m_flags, and values in various other
> mount structure members; see the showargs function for
> a taste of that.
> 
> So this extends xfs_test_remount_options() to do a full set
> of mount processing of the options remount sees, to arrive
> at a final state, then compares that state to the current
> state, and determines if we can proceed.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> This is lightly tested; mostly just a sanity check to see
> if this approach is a "wtf?" or a "yeah, seems ok."
> 
...
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index a4e03ab..bee9284 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -335,6 +335,7 @@ extern int	xfs_sb_validate_fsb_count(struct xfs_sb *, __uint64_t);
>  
>  extern int	xfs_dev_is_read_only(struct xfs_mount *, char *);
>  
> +extern void	xfs_set_rw_sizes(xfs_mount_t *);
>  extern void	xfs_set_low_space_thresholds(struct xfs_mount *);
>  
>  int	xfs_zero_extent(struct xfs_inode *ip, xfs_fsblock_t start_fsb,
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index d1cd4fa..50e15d8 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
...
> @@ -1167,24 +1169,87 @@ xfs_quiesce_attr(
...
>  STATIC int
>  xfs_test_remount_options(
>  	struct super_block	*sb,
>  	struct xfs_mount	*mp,
>  	char			*options)
>  {
...
> +	/* The only flags we can change on remount */
> +	ok_flags = XFS_MOUNT_BARRIER | XFS_MOUNT_RDONLY |
> +		   XFS_MOUNT_SMALL_INUMS | XFS_MOUNT_32BITINODES; 

Trailing whitespace at the end of the above line.

> +	/* This is only used internally, so OK as well */
> +	ok_flags |= XFS_MOUNT_WAS_CLEAN;
> +
> +	/* The flags that *did* change */
> +	changed_flags = (tmp_mp->m_flags ^ mp->m_flags);
> +
> +	error = -EINVAL;
> +
> +	if (tmp_mp->m_qflags != mp->m_qflags)
> +		XFS_BAD_REMOUNT_GOTO(mp, "quota", out);
> +
> +	if (tmp_mp->m_logbufs != mp->m_logbufs ||
> +	    tmp_mp->m_logbsize != mp->m_logbsize)
> +		XFS_BAD_REMOUNT_GOTO(mp, "logbufs/logbsize", out);
> +
> +	if (tmp_mp->m_readio_log != mp->m_readio_log ||
> +	    tmp_mp->m_writeio_log != mp->m_writeio_log)
> +		XFS_BAD_REMOUNT_GOTO(mp, "allocsize/biosize", out);
> +
> +	if ((tmp_mp->m_logname && mp->m_logname &&
> +	     strcmp(tmp_mp->m_logname, mp->m_logname)) ||
> +	    (tmp_mp->m_rtname &&  mp->m_rtname &&
> +	     strcmp(tmp_mp->m_rtname, mp->m_rtname)))
> +		XFS_BAD_REMOUNT_GOTO(mp, "logdev/rtdev", out);
> +

This warning won't trigger if rtname or logname is specified by the
remount and the current mount doesn't have either set (because the
current mp->m_logname == NULL, for example).

I was also wondering why we can't just check these values against the
defaults and complain if they are specified at all at remount time, but
looking back at the commit log it sounds like we have to handle the case
where a mount option string might essentially be copy/pasted from
/proc/mounts and slightly tweaked with a valid change. Does something
actually break if we don't handle that? Though I suppose if it works now
we probably shouldn't break it.

> +	/* Catch-all for anything else */
> +	if (changed_flags & ~ok_flags)
> +		XFS_BAD_REMOUNT_GOTO(mp, "specified", out);
> +
> +	error = 0;
> +out:
> +	xfs_free_fsname(tmp_mp);
> +	kfree(tmp_mp);
>  	return error;
>  }
>  
> @@ -1200,7 +1265,12 @@ xfs_fs_remount(
>  	char			*p;
>  	int			error;
>  
> -	/* First, check for complete junk; i.e. invalid options */
> +	/*
> +	 * Remounting is tricky; we get various combinations
> +	 * of options, both pre-existing and changed, here.
> +	 * This function tries to ensure that what we got
> +	 * is a sane set for remounting, and errors if not.
> +	 */
>  	error = xfs_test_remount_options(sb, mp, options);
>  	if (error)
>  		return error;
> @@ -1228,28 +1298,13 @@ xfs_fs_remount(
>  			break;
>  		default:
>  			/*
> -			 * Logically we would return an error here to prevent
> -			 * users from believing they might have changed
> -			 * mount options using remount which can't be changed.
> -			 *
> -			 * But unfortunately mount(8) adds all options from
> -			 * mtab and fstab to the mount arguments in some cases
> -			 * so we can't blindly reject options, but have to
> -			 * check for each specified option if it actually
> -			 * differs from the currently set option and only
> -			 * reject it if that's the case.
> -			 *
> -			 * Until that is implemented we return success for
> -			 * every remount request, and silently ignore all
> -			 * options that we can't actually change.
> +			 * xfs_test_remount_options really should have errored
> +			 * out on any non-remountable options; anything that got 

Trailing whitespace.

Brian

> +			 * here should be a no-op; a re-statement of existing
> +			 * options. If something slipped through: too bad!
> +			 * We'll just ignore it.
>  			 */
> -#if 0
> -			xfs_info(mp,
> -		"mount option \"%s\" not supported for remount", p);
> -			return -EINVAL;
> -#else
>  			break;
> -#endif
>  		}
>  	}
>  
> 
> 
> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] xfs: test for valid remount options, error if not
  2016-02-10 23:45 ` [PATCH 3/3] xfs: test for valid remount options, error if not Eric Sandeen
  2016-02-15 18:54   ` Brian Foster
@ 2016-02-15 20:25   ` Dave Chinner
  2016-02-15 23:07     ` Eric Sandeen
  1 sibling, 1 reply; 14+ messages in thread
From: Dave Chinner @ 2016-02-15 20:25 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: xfs

On Wed, Feb 10, 2016 at 05:45:33PM -0600, Eric Sandeen wrote:
> This patch attempts to check for a valid set of remount
> options.  As far as I can tell, it's tricky; as the old
> comment says, on remount we may get a long string of
> options from /proc/mounts and/or /etc/mtab, as well
> as options specified on the commandline.  Later options
> may negate previous options, etc.
> 
> At the most basic level, we may be handed a mount option
> which we do not handle on remount, but which may not actually
> be a change from the current mount option set.
> 
> Unfortunately our mount option state is somewhat far flung;
> a combinations of m_flags, and values in various other
> mount structure members; see the showargs function for
> a taste of that.
> 
> So this extends xfs_test_remount_options() to do a full set
> of mount processing of the options remount sees, to arrive
> at a final state, then compares that state to the current
> state, and determines if we can proceed.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> This is lightly tested; mostly just a sanity check to see
> if this approach is a "wtf?" or a "yeah, seems ok."
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 986290c..3d4187c 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -455,7 +455,7 @@ xfs_set_maxicount(xfs_mount_t *mp)
>   * We use smaller I/O sizes when the file system
>   * is being used for NFS service (wsync mount option).
>   */
> -STATIC void
> +void
>  xfs_set_rw_sizes(xfs_mount_t *mp)
>  {
>  	xfs_sb_t	*sbp = &(mp->m_sb);
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index a4e03ab..bee9284 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -335,6 +335,7 @@ extern int	xfs_sb_validate_fsb_count(struct xfs_sb *, __uint64_t);
>  
>  extern int	xfs_dev_is_read_only(struct xfs_mount *, char *);
>  
> +extern void	xfs_set_rw_sizes(xfs_mount_t *);
>  extern void	xfs_set_low_space_thresholds(struct xfs_mount *);
>  
>  int	xfs_zero_extent(struct xfs_inode *ip, xfs_fsblock_t start_fsb,
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index d1cd4fa..50e15d8 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -65,6 +65,8 @@ static struct kset *xfs_kset;		/* top-level xfs sysfs dir */
>  static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs attrs */
>  #endif
>  
> +STATIC int xfs_finish_flags(struct xfs_mount *mp);
> +
>  /*
>   * Table driven mount option parser.
>   */
> @@ -1167,24 +1169,87 @@ xfs_quiesce_attr(
>  	xfs_log_quiesce(mp);
>  }
>  
> +#define XFS_BAD_REMOUNT_GOTO(mp, option, l)	\
> +	{ \
> +		xfs_warn(mp, \
> +		  option " options may not be changed via remount");	\
> +		goto l;	\
> +	}

I think hiding a goto like this is wrong - it forces you to go read
the macro, making the code harder to read and follow. Really, what's
wrong with the simple and obvious:


	if (bad option) {
		bad_option = "bad option string";
		goto out_warn;
	}
	.....

out_warn:
	xfs_warn(mp, "%s options may not be changed via remount",
		 bad_option);
	// free stuff
	return -EINVAL;
}

Yes, I know that this sort of logic flow hiding was done with the
XFS_WANT_CORRUPTED macros, but they were written back in 90s on Irix
when using macros to implement everything were all the rage.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/3 V2] xfs: convert mount option parsing to tokens
  2016-02-10 23:38 ` [PATCH 1/3] xfs: convert mount option parsing to tokens Eric Sandeen
  2016-02-15 18:54   ` Brian Foster
@ 2016-02-15 21:20   ` Eric Sandeen
  2016-02-17 16:54     ` Christoph Hellwig
  2016-02-17 17:19   ` [PATCH 1/3 V3] " Eric Sandeen
  2 siblings, 1 reply; 14+ messages in thread
From: Eric Sandeen @ 2016-02-15 21:20 UTC (permalink / raw)
  To: xfs

This should be a no-op change, just switch to token parsing
like every other respectable filesystem does.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---

V2: Whitespace fixes, remove extraneous break-after-return.

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 59c9b7b..9b2e268 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -65,83 +65,82 @@ static struct kset *xfs_kset;		/* top-level xfs sysfs dir */
 static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs attrs */
 #endif
 
-#define MNTOPT_LOGBUFS	"logbufs"	/* number of XFS log buffers */
-#define MNTOPT_LOGBSIZE	"logbsize"	/* size of XFS log buffers */
-#define MNTOPT_LOGDEV	"logdev"	/* log device */
-#define MNTOPT_RTDEV	"rtdev"		/* realtime I/O device */
-#define MNTOPT_BIOSIZE	"biosize"	/* log2 of preferred buffered io size */
-#define MNTOPT_WSYNC	"wsync"		/* safe-mode nfs compatible mount */
-#define MNTOPT_NOALIGN	"noalign"	/* turn off stripe alignment */
-#define MNTOPT_SWALLOC	"swalloc"	/* turn on stripe width allocation */
-#define MNTOPT_SUNIT	"sunit"		/* data volume stripe unit */
-#define MNTOPT_SWIDTH	"swidth"	/* data volume stripe width */
-#define MNTOPT_NOUUID	"nouuid"	/* ignore filesystem UUID */
-#define MNTOPT_MTPT	"mtpt"		/* filesystem mount point */
-#define MNTOPT_GRPID	"grpid"		/* group-ID from parent directory */
-#define MNTOPT_NOGRPID	"nogrpid"	/* group-ID from current process */
-#define MNTOPT_BSDGROUPS    "bsdgroups"    /* group-ID from parent directory */
-#define MNTOPT_SYSVGROUPS   "sysvgroups"   /* group-ID from current process */
-#define MNTOPT_ALLOCSIZE    "allocsize"    /* preferred allocation size */
-#define MNTOPT_NORECOVERY   "norecovery"   /* don't run XFS recovery */
-#define MNTOPT_BARRIER	"barrier"	/* use writer barriers for log write and
-					 * unwritten extent conversion */
-#define MNTOPT_NOBARRIER "nobarrier"	/* .. disable */
-#define MNTOPT_64BITINODE   "inode64"	/* inodes can be allocated anywhere */
-#define MNTOPT_32BITINODE   "inode32"	/* inode allocation limited to
-					 * XFS_MAXINUMBER_32 */
-#define MNTOPT_IKEEP	"ikeep"		/* do not free empty inode clusters */
-#define MNTOPT_NOIKEEP	"noikeep"	/* free empty inode clusters */
-#define MNTOPT_LARGEIO	   "largeio"	/* report large I/O sizes in stat() */
-#define MNTOPT_NOLARGEIO   "nolargeio"	/* do not report large I/O sizes
-					 * in stat(). */
-#define MNTOPT_ATTR2	"attr2"		/* do use attr2 attribute format */
-#define MNTOPT_NOATTR2	"noattr2"	/* do not use attr2 attribute format */
-#define MNTOPT_FILESTREAM  "filestreams" /* use filestreams allocator */
-#define MNTOPT_QUOTA	"quota"		/* disk quotas (user) */
-#define MNTOPT_NOQUOTA	"noquota"	/* no quotas */
-#define MNTOPT_USRQUOTA	"usrquota"	/* user quota enabled */
-#define MNTOPT_GRPQUOTA	"grpquota"	/* group quota enabled */
-#define MNTOPT_PRJQUOTA	"prjquota"	/* project quota enabled */
-#define MNTOPT_UQUOTA	"uquota"	/* user quota (IRIX variant) */
-#define MNTOPT_GQUOTA	"gquota"	/* group quota (IRIX variant) */
-#define MNTOPT_PQUOTA	"pquota"	/* project quota (IRIX variant) */
-#define MNTOPT_UQUOTANOENF "uqnoenforce"/* user quota limit enforcement */
-#define MNTOPT_GQUOTANOENF "gqnoenforce"/* group quota limit enforcement */
-#define MNTOPT_PQUOTANOENF "pqnoenforce"/* project quota limit enforcement */
-#define MNTOPT_QUOTANOENF  "qnoenforce"	/* same as uqnoenforce */
-#define MNTOPT_DISCARD	   "discard"	/* Discard unused blocks */
-#define MNTOPT_NODISCARD   "nodiscard"	/* Do not discard unused blocks */
-
-#define MNTOPT_DAX	"dax"		/* Enable direct access to bdev pages */
-
 /*
  * Table driven mount option parser.
- *
- * Currently only used for remount, but it will be used for mount
- * in the future, too.
  */
 enum {
-	Opt_barrier,
-	Opt_nobarrier,
-	Opt_inode64,
-	Opt_inode32,
-	Opt_err
+	Opt_logbufs, Opt_logbsize, Opt_logdev, Opt_rtdev, Opt_biosize,
+	Opt_wsync, Opt_noalign, Opt_swalloc, Opt_sunit, Opt_swidth, Opt_nouuid,
+	Opt_mtpt, Opt_grpid, Opt_nogrpid, Opt_bsdgroups, Opt_sysvgroups,
+	Opt_allocsize, Opt_norecovery, Opt_barrier, Opt_nobarrier,
+	Opt_inode64, Opt_inode32, Opt_ikeep, Opt_noikeep,
+	Opt_largeio, Opt_nolargeio, Opt_attr2, Opt_noattr2, Opt_filestreams,
+	Opt_quota, Opt_noquota, Opt_usrquota, Opt_grpquota, Opt_prjquota,
+	Opt_uquota, Opt_gquota, Opt_pquota,
+	Opt_uqnoenforce, Opt_gqnoenforce, Opt_pqnoenforce, Opt_qnoenforce,
+	Opt_discard, Opt_nodiscard, Opt_dax, Opt_err,
 };
 
 static const match_table_t tokens = {
-	{Opt_barrier, "barrier"},
-	{Opt_nobarrier, "nobarrier"},
-	{Opt_inode64, "inode64"},
-	{Opt_inode32, "inode32"},
-	{Opt_err, NULL}
+	{Opt_logbufs,	"logbufs=%u"},	/* number of XFS log buffers */
+	{Opt_logbsize,	"logbsize=%s"},	/* size of XFS log buffers */
+	{Opt_logdev,	"logdev=%s"},	/* log device */
+	{Opt_rtdev,	"rtdev=%s"},	/* realtime I/O device */
+	{Opt_biosize,	"biosize=%u"},	/* log2 of preferred buffered io size */
+	{Opt_wsync,	"wsync"},	/* safe-mode nfs compatible mount */
+	{Opt_noalign,	"noalign"},	/* turn off stripe alignment */
+	{Opt_swalloc,	"swalloc"},	/* turn on stripe width allocation */
+	{Opt_sunit,	"sunit=%u"},	/* data volume stripe unit */
+	{Opt_swidth,	"swidth=%u"},	/* data volume stripe width */
+	{Opt_nouuid,	"nouuid"},	/* ignore filesystem UUID */
+	{Opt_mtpt,	"mtpt"},	/* filesystem mount point */
+	{Opt_grpid,	"grpid"},	/* group-ID from parent directory */
+	{Opt_nogrpid,	"nogrpid"},	/* group-ID from current process */
+	{Opt_bsdgroups,	"bsdgroups"},	/* group-ID from parent directory */
+	{Opt_sysvgroups,"sysvgroups"},	/* group-ID from current process */
+	{Opt_allocsize,	"allocsize=%s"},/* preferred allocation size */
+	{Opt_norecovery,"norecovery"},	/* don't run XFS recovery */
+	{Opt_barrier,	"barrier"},	/* use writer barriers for log write and
+					 * unwritten extent conversion */
+	{Opt_nobarrier,	"nobarrier"},	/* .. disable */
+	{Opt_inode64,	"inode64"},	/* inodes can be allocated anywhere */
+	{Opt_inode32,   "inode32"},	/* inode allocation limited to
+					 * XFS_MAXINUMBER_32 */
+	{Opt_ikeep,	"ikeep"},	/* do not free empty inode clusters */
+	{Opt_noikeep,	"noikeep"},	/* free empty inode clusters */
+	{Opt_largeio,	"largeio"},	/* report large I/O sizes in stat() */
+	{Opt_nolargeio,	"nolargeio"},	/* do not report large I/O sizes
+					 * in stat(). */
+	{Opt_attr2,	"attr2"},	/* do use attr2 attribute format */
+	{Opt_noattr2,	"noattr2"},	/* do not use attr2 attribute format */
+	{Opt_filestreams,"filestreams"},/* use filestreams allocator */
+	{Opt_quota,	"quota"},	/* disk quotas (user) */
+	{Opt_noquota,	"noquota"},	/* no quotas */
+	{Opt_usrquota,	"usrquota"},	/* user quota enabled */
+	{Opt_grpquota,	"grpquota"},	/* group quota enabled */
+	{Opt_prjquota,	"prjquota"},	/* project quota enabled */
+	{Opt_uquota,	"uquota"},	/* user quota (IRIX variant) */
+	{Opt_gquota,	"gquota"},	/* group quota (IRIX variant) */
+	{Opt_pquota,	"pquota"},	/* project quota (IRIX variant) */
+	{Opt_uqnoenforce,"uqnoenforce"},/* user quota limit enforcement */
+	{Opt_gqnoenforce,"gqnoenforce"},/* group quota limit enforcement */
+	{Opt_pqnoenforce,"pqnoenforce"},/* project quota limit enforcement */
+	{Opt_qnoenforce, "qnoenforce"},	/* same as uqnoenforce */
+	{Opt_discard,	"discard"},	/* Discard unused blocks */
+	{Opt_nodiscard,	"nodiscard"},	/* Do not discard unused blocks */
+
+	{Opt_dax,	"dax"},		/* Enable direct access to bdev pages */
+	{Opt_err,	NULL},
 };
 
 
 STATIC int
-suffix_kstrtoint(char *s, unsigned int base, int *res)
+suffix_kstrtoint(const substring_t *s, unsigned int base, int *res)
 {
 	int	last, shift_left_factor = 0, _res;
-	char	*value = s;
+	char	*value = match_strdup(s);
+	int	ret = 0;
+
 
 	last = strlen(value) - 1;
 	if (value[last] == 'K' || value[last] == 'k') {
@@ -157,10 +156,11 @@ suffix_kstrtoint(char *s, unsigned int base, int *res)
 		value[last] = '\0';
 	}
 
-	if (kstrtoint(s, base, &_res))
-		return -EINVAL;
+	if (kstrtoint(value, base, &_res))
+		ret = -EINVAL;
+	kfree(value);
 	*res = _res << shift_left_factor;
-	return 0;
+	return ret;
 }
 
 /*
@@ -176,7 +176,8 @@ xfs_parseargs(
 	char			*options)
 {
 	struct super_block	*sb = mp->m_super;
-	char			*this_char, *value;
+	char			*p;
+	substring_t		args[MAX_OPT_ARGS];
 	int			dsunit = 0;
 	int			dswidth = 0;
 	int			iosize = 0;
@@ -217,152 +218,152 @@ xfs_parseargs(
 	if (!options)
 		goto done;
 
-	while ((this_char = strsep(&options, ",")) != NULL) {
-		if (!*this_char)
+	while ((p = strsep(&options, ",")) != NULL) {
+		int		token;
+
+		if (!*p)
 			continue;
-		if ((value = strchr(this_char, '=')) != NULL)
-			*value++ = 0;
 
-		if (!strcmp(this_char, MNTOPT_LOGBUFS)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
-				return -EINVAL;
-			}
-			if (kstrtoint(value, 10, &mp->m_logbufs))
-				return -EINVAL;
-		} else if (!strcmp(this_char, MNTOPT_LOGBSIZE)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
-				return -EINVAL;
-			}
-			if (suffix_kstrtoint(value, 10, &mp->m_logbsize))
+		token = match_token(p, tokens, args);
+		switch (token) {
+		case Opt_logbufs:
+			if (match_int(args, &mp->m_logbufs))
 				return -EINVAL;
-		} else if (!strcmp(this_char, MNTOPT_LOGDEV)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
+			break;
+		case Opt_logbsize:
+			if (suffix_kstrtoint(args, 10, &mp->m_logbsize))
 				return -EINVAL;
-			}
-			mp->m_logname = kstrndup(value, MAXNAMELEN, GFP_KERNEL);
+			break;
+		case Opt_logdev:
+			mp->m_logname = match_strdup(args);
 			if (!mp->m_logname)
 				return -ENOMEM;
-		} else if (!strcmp(this_char, MNTOPT_MTPT)) {
-			xfs_warn(mp, "%s option not allowed on this system",
-				this_char);
+			break;
+		case Opt_mtpt:
+			xfs_warn(mp, "%s option not allowed on this system", p);
 			return -EINVAL;
-		} else if (!strcmp(this_char, MNTOPT_RTDEV)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
-				return -EINVAL;
-			}
-			mp->m_rtname = kstrndup(value, MAXNAMELEN, GFP_KERNEL);
+		case Opt_rtdev:
+			mp->m_rtname = match_strdup(args);
 			if (!mp->m_rtname)
 				return -ENOMEM;
-		} else if (!strcmp(this_char, MNTOPT_ALLOCSIZE) ||
-			   !strcmp(this_char, MNTOPT_BIOSIZE)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
-				return -EINVAL;
-			}
-			if (suffix_kstrtoint(value, 10, &iosize))
+			break;
+		case Opt_allocsize:
+		case Opt_biosize:
+			if (suffix_kstrtoint(args, 10, &iosize))
 				return -EINVAL;
 			iosizelog = ffs(iosize) - 1;
-		} else if (!strcmp(this_char, MNTOPT_GRPID) ||
-			   !strcmp(this_char, MNTOPT_BSDGROUPS)) {
+			break;
+		case Opt_grpid:
+		case Opt_bsdgroups:
 			mp->m_flags |= XFS_MOUNT_GRPID;
-		} else if (!strcmp(this_char, MNTOPT_NOGRPID) ||
-			   !strcmp(this_char, MNTOPT_SYSVGROUPS)) {
+			break;
+		case Opt_nogrpid:
+		case Opt_sysvgroups:
 			mp->m_flags &= ~XFS_MOUNT_GRPID;
-		} else if (!strcmp(this_char, MNTOPT_WSYNC)) {
+			break;
+		case Opt_wsync:
 			mp->m_flags |= XFS_MOUNT_WSYNC;
-		} else if (!strcmp(this_char, MNTOPT_NORECOVERY)) {
+			break;
+		case Opt_norecovery:
 			mp->m_flags |= XFS_MOUNT_NORECOVERY;
-		} else if (!strcmp(this_char, MNTOPT_NOALIGN)) {
+			break;
+		case Opt_noalign:
 			mp->m_flags |= XFS_MOUNT_NOALIGN;
-		} else if (!strcmp(this_char, MNTOPT_SWALLOC)) {
+			break;
+		case Opt_swalloc:
 			mp->m_flags |= XFS_MOUNT_SWALLOC;
-		} else if (!strcmp(this_char, MNTOPT_SUNIT)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
-				return -EINVAL;
-			}
-			if (kstrtoint(value, 10, &dsunit))
-				return -EINVAL;
-		} else if (!strcmp(this_char, MNTOPT_SWIDTH)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
+			break;
+		case Opt_sunit:
+			if (match_int(args, &dsunit))
 				return -EINVAL;
-			}
-			if (kstrtoint(value, 10, &dswidth))
+			break;
+		case Opt_swidth:
+			if (match_int(args, &dswidth))
 				return -EINVAL;
-		} else if (!strcmp(this_char, MNTOPT_32BITINODE)) {
+			break;
+		case Opt_inode32:
 			mp->m_flags |= XFS_MOUNT_SMALL_INUMS;
-		} else if (!strcmp(this_char, MNTOPT_64BITINODE)) {
+			break;
+		case Opt_inode64:
 			mp->m_flags &= ~XFS_MOUNT_SMALL_INUMS;
-		} else if (!strcmp(this_char, MNTOPT_NOUUID)) {
+			break;
+		case Opt_nouuid:
 			mp->m_flags |= XFS_MOUNT_NOUUID;
-		} else if (!strcmp(this_char, MNTOPT_BARRIER)) {
+			break;
+		case Opt_barrier:
 			mp->m_flags |= XFS_MOUNT_BARRIER;
-		} else if (!strcmp(this_char, MNTOPT_NOBARRIER)) {
+			break;
+		case Opt_nobarrier:
 			mp->m_flags &= ~XFS_MOUNT_BARRIER;
-		} else if (!strcmp(this_char, MNTOPT_IKEEP)) {
+			break;
+		case Opt_ikeep:
 			mp->m_flags |= XFS_MOUNT_IKEEP;
-		} else if (!strcmp(this_char, MNTOPT_NOIKEEP)) {
+			break;
+		case Opt_noikeep:
 			mp->m_flags &= ~XFS_MOUNT_IKEEP;
-		} else if (!strcmp(this_char, MNTOPT_LARGEIO)) {
+			break;
+		case Opt_largeio:
 			mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE;
-		} else if (!strcmp(this_char, MNTOPT_NOLARGEIO)) {
+			break;
+		case Opt_nolargeio:
 			mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE;
-		} else if (!strcmp(this_char, MNTOPT_ATTR2)) {
+			break;
+		case Opt_attr2:
 			mp->m_flags |= XFS_MOUNT_ATTR2;
-		} else if (!strcmp(this_char, MNTOPT_NOATTR2)) {
+			break;
+		case Opt_noattr2:
 			mp->m_flags &= ~XFS_MOUNT_ATTR2;
 			mp->m_flags |= XFS_MOUNT_NOATTR2;
-		} else if (!strcmp(this_char, MNTOPT_FILESTREAM)) {
+			break;
+		case Opt_filestreams:
 			mp->m_flags |= XFS_MOUNT_FILESTREAMS;
-		} else if (!strcmp(this_char, MNTOPT_NOQUOTA)) {
+			break;
+		case Opt_noquota:
 			mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT;
 			mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD;
 			mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE;
-		} else if (!strcmp(this_char, MNTOPT_QUOTA) ||
-			   !strcmp(this_char, MNTOPT_UQUOTA) ||
-			   !strcmp(this_char, MNTOPT_USRQUOTA)) {
+			break;
+		case Opt_quota:
+		case Opt_uquota:
+		case Opt_usrquota:
 			mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE |
 					 XFS_UQUOTA_ENFD);
-		} else if (!strcmp(this_char, MNTOPT_QUOTANOENF) ||
-			   !strcmp(this_char, MNTOPT_UQUOTANOENF)) {
+			break;
+		case Opt_qnoenforce:
+		case Opt_uqnoenforce:
 			mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE);
 			mp->m_qflags &= ~XFS_UQUOTA_ENFD;
-		} else if (!strcmp(this_char, MNTOPT_PQUOTA) ||
-			   !strcmp(this_char, MNTOPT_PRJQUOTA)) {
+			break;
+		case Opt_pquota:
+		case Opt_prjquota:
 			mp->m_qflags |= (XFS_PQUOTA_ACCT | XFS_PQUOTA_ACTIVE |
 					 XFS_PQUOTA_ENFD);
-		} else if (!strcmp(this_char, MNTOPT_PQUOTANOENF)) {
+			break;
+		case Opt_pqnoenforce:
 			mp->m_qflags |= (XFS_PQUOTA_ACCT | XFS_PQUOTA_ACTIVE);
 			mp->m_qflags &= ~XFS_PQUOTA_ENFD;
-		} else if (!strcmp(this_char, MNTOPT_GQUOTA) ||
-			   !strcmp(this_char, MNTOPT_GRPQUOTA)) {
+		case Opt_gquota:
+		case Opt_grpquota:
 			mp->m_qflags |= (XFS_GQUOTA_ACCT | XFS_GQUOTA_ACTIVE |
 					 XFS_GQUOTA_ENFD);
-		} else if (!strcmp(this_char, MNTOPT_GQUOTANOENF)) {
+			break;
+		case Opt_gqnoenforce:
 			mp->m_qflags |= (XFS_GQUOTA_ACCT | XFS_GQUOTA_ACTIVE);
 			mp->m_qflags &= ~XFS_GQUOTA_ENFD;
-		} else if (!strcmp(this_char, MNTOPT_DISCARD)) {
+			break;
+		case Opt_discard:
 			mp->m_flags |= XFS_MOUNT_DISCARD;
-		} else if (!strcmp(this_char, MNTOPT_NODISCARD)) {
+			break;
+		case Opt_nodiscard:
 			mp->m_flags &= ~XFS_MOUNT_DISCARD;
+			break;
 #ifdef CONFIG_FS_DAX
-		} else if (!strcmp(this_char, MNTOPT_DAX)) {
+		case Opt_dax:
 			mp->m_flags |= XFS_MOUNT_DAX;
+			break;
 #endif
-		} else {
-			xfs_warn(mp, "unknown mount option [%s].", this_char);
+		default:
+			xfs_warn(mp, "unknown mount option [%s].", p);
 			return -EINVAL;
 		}
 	}
@@ -461,25 +462,25 @@ xfs_showargs(
 {
 	static struct proc_xfs_info xfs_info_set[] = {
 		/* the few simple ones we can get from the mount struct */
-		{ XFS_MOUNT_IKEEP,		"," MNTOPT_IKEEP },
-		{ XFS_MOUNT_WSYNC,		"," MNTOPT_WSYNC },
-		{ XFS_MOUNT_NOALIGN,		"," MNTOPT_NOALIGN },
-		{ XFS_MOUNT_SWALLOC,		"," MNTOPT_SWALLOC },
-		{ XFS_MOUNT_NOUUID,		"," MNTOPT_NOUUID },
-		{ XFS_MOUNT_NORECOVERY,		"," MNTOPT_NORECOVERY },
-		{ XFS_MOUNT_ATTR2,		"," MNTOPT_ATTR2 },
-		{ XFS_MOUNT_FILESTREAMS,	"," MNTOPT_FILESTREAM },
-		{ XFS_MOUNT_GRPID,		"," MNTOPT_GRPID },
-		{ XFS_MOUNT_DISCARD,		"," MNTOPT_DISCARD },
-		{ XFS_MOUNT_SMALL_INUMS,	"," MNTOPT_32BITINODE },
-		{ XFS_MOUNT_DAX,		"," MNTOPT_DAX },
+		{ XFS_MOUNT_IKEEP,		",ikeep" },
+		{ XFS_MOUNT_WSYNC,		",wsync" },
+		{ XFS_MOUNT_NOALIGN,		",noalign" },
+		{ XFS_MOUNT_SWALLOC,		",swalloc" },
+		{ XFS_MOUNT_NOUUID,		",nouuid" },
+		{ XFS_MOUNT_NORECOVERY,		",norecovery" },
+		{ XFS_MOUNT_ATTR2,		",attr2" },
+		{ XFS_MOUNT_FILESTREAMS,	",filestreams" },
+		{ XFS_MOUNT_GRPID,		",grpid" },
+		{ XFS_MOUNT_DISCARD,		",discard" },
+		{ XFS_MOUNT_SMALL_INUMS,	",inode32" },
+		{ XFS_MOUNT_DAX,		",dax" },
 		{ 0, NULL }
 	};
 	static struct proc_xfs_info xfs_info_unset[] = {
 		/* the few simple ones we can get from the mount struct */
-		{ XFS_MOUNT_COMPAT_IOSIZE,	"," MNTOPT_LARGEIO },
-		{ XFS_MOUNT_BARRIER,		"," MNTOPT_NOBARRIER },
-		{ XFS_MOUNT_SMALL_INUMS,	"," MNTOPT_64BITINODE },
+		{ XFS_MOUNT_COMPAT_IOSIZE,	",largeio" },
+		{ XFS_MOUNT_BARRIER,		",nobarrier" },
+		{ XFS_MOUNT_SMALL_INUMS,	",inode64" },
 		{ 0, NULL }
 	};
 	struct proc_xfs_info	*xfs_infop;
@@ -494,46 +495,46 @@ xfs_showargs(
 	}
 
 	if (mp->m_flags & XFS_MOUNT_DFLT_IOSIZE)
-		seq_printf(m, "," MNTOPT_ALLOCSIZE "=%dk",
+		seq_printf(m, ",allocsize=%dk",
 				(int)(1 << mp->m_writeio_log) >> 10);
 
 	if (mp->m_logbufs > 0)
-		seq_printf(m, "," MNTOPT_LOGBUFS "=%d", mp->m_logbufs);
+		seq_printf(m, ",logbufs=%d", mp->m_logbufs);
 	if (mp->m_logbsize > 0)
-		seq_printf(m, "," MNTOPT_LOGBSIZE "=%dk", mp->m_logbsize >> 10);
+		seq_printf(m, ",logbsize=%dk", mp->m_logbsize >> 10);
 
 	if (mp->m_logname)
-		seq_show_option(m, MNTOPT_LOGDEV, mp->m_logname);
+		seq_show_option(m, "logdev", mp->m_logname);
 	if (mp->m_rtname)
-		seq_show_option(m, MNTOPT_RTDEV, mp->m_rtname);
+		seq_show_option(m, "rtdev", mp->m_rtname);
 
 	if (mp->m_dalign > 0)
-		seq_printf(m, "," MNTOPT_SUNIT "=%d",
+		seq_printf(m, ",sunit=%d",
 				(int)XFS_FSB_TO_BB(mp, mp->m_dalign));
 	if (mp->m_swidth > 0)
-		seq_printf(m, "," MNTOPT_SWIDTH "=%d",
+		seq_printf(m, ",swidth=%d",
 				(int)XFS_FSB_TO_BB(mp, mp->m_swidth));
 
 	if (mp->m_qflags & (XFS_UQUOTA_ACCT|XFS_UQUOTA_ENFD))
-		seq_puts(m, "," MNTOPT_USRQUOTA);
+		seq_puts(m, ",usrquota");
 	else if (mp->m_qflags & XFS_UQUOTA_ACCT)
-		seq_puts(m, "," MNTOPT_UQUOTANOENF);
+		seq_puts(m, ",uqnoenforce");
 
 	if (mp->m_qflags & XFS_PQUOTA_ACCT) {
 		if (mp->m_qflags & XFS_PQUOTA_ENFD)
-			seq_puts(m, "," MNTOPT_PRJQUOTA);
+			seq_puts(m, ",prjquota");
 		else
-			seq_puts(m, "," MNTOPT_PQUOTANOENF);
+			seq_puts(m, ",pqnoenforce");
 	}
 	if (mp->m_qflags & XFS_GQUOTA_ACCT) {
 		if (mp->m_qflags & XFS_GQUOTA_ENFD)
-			seq_puts(m, "," MNTOPT_GRPQUOTA);
+			seq_puts(m, ",grpquota");
 		else
-			seq_puts(m, "," MNTOPT_GQUOTANOENF);
+			seq_puts(m, ",gqnoenforce");
 	}
 
 	if (!(mp->m_qflags & XFS_ALL_QUOTA_ACCT))
-		seq_puts(m, "," MNTOPT_NOQUOTA);
+		seq_puts(m, ",noquota");
 
 	return 0;
 }
@@ -1344,9 +1345,8 @@ xfs_finish_flags(
 	 */
 	if (xfs_sb_version_hascrc(&mp->m_sb) &&
 	    (mp->m_flags & XFS_MOUNT_NOATTR2)) {
-		xfs_warn(mp,
-"Cannot mount a V5 filesystem as %s. %s is always enabled for V5 filesystems.",
-			MNTOPT_NOATTR2, MNTOPT_ATTR2);
+		xfs_warn(mp, "Cannot mount a V5 filesystem as noattr2. "
+			     "attr2 is always enabled for V5 filesystems.");
 		return -EINVAL;
 	}
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] xfs: test for valid remount options, error if not
  2016-02-15 20:25   ` Dave Chinner
@ 2016-02-15 23:07     ` Eric Sandeen
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Sandeen @ 2016-02-15 23:07 UTC (permalink / raw)
  To: xfs



On 2/15/16 2:25 PM, Dave Chinner wrote:
> I think hiding a goto like this is wrong - it forces you to go read
> the macro, making the code harder to read and follow. Really, what's
> wrong with the simple and obvious:
> 
> 
> 	if (bad option) {
> 		bad_option = "bad option string";
> 		goto out_warn;
> 	}
> 	.....
> 
> out_warn:
> 	xfs_warn(mp, "%s options may not be changed via remount",
> 		 bad_option);
> 	// free stuff
> 	return -EINVAL;
> }
> 
> Yes, I know that this sort of logic flow hiding was done with the
> XFS_WANT_CORRUPTED macros, but they were written back in 90s on Irix
> when using macros to implement everything were all the rage.

Yeah, fair point, not sure why I did that ;)  Old habits?

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 2/3 V2] xfs: sanitize remount options
  2016-02-10 23:40 ` [PATCH 2/3] xfs: sanitize remount options Eric Sandeen
  2016-02-15 18:54   ` Brian Foster
@ 2016-02-17  4:29   ` Eric Sandeen
  2016-02-17 16:55     ` Christoph Hellwig
  1 sibling, 1 reply; 14+ messages in thread
From: Eric Sandeen @ 2016-02-17  4:29 UTC (permalink / raw)
  To: xfs

Perform basic sanitization of remount options by
passing the option string and a dummy mount structure
through xfs_parseargs and returning the result.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---

V2: make *sb const in xfs_parseargs, with comment about why
    rename tmp->tmp_mp

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 9b2e268..fe4c14e 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -169,13 +169,17 @@ suffix_kstrtoint(const substring_t *s, unsigned int base, int *res)
  *
  * 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)
 {
-	struct super_block	*sb = mp->m_super;
+	const struct super_block *sb = mp->m_super;
 	char			*p;
 	substring_t		args[MAX_OPT_ARGS];
 	int			dsunit = 0;
@@ -1167,6 +1171,27 @@ xfs_quiesce_attr(
 }
 
 STATIC int
+xfs_test_remount_options(
+	struct super_block	*sb,
+	struct xfs_mount	*mp,
+	char			*options)
+{
+	int			error = 0;
+	struct xfs_mount	*tmp_mp;
+
+	tmp_mp = kmem_zalloc(sizeof(*tmp_mp), KM_MAYFAIL);
+	if (!tmp_mp)
+		return -ENOMEM;
+
+	tmp_mp->m_super = sb;
+	error = xfs_parseargs(tmp_mp, options);
+	xfs_free_fsname(tmp_mp);
+	kfree(tmp_mp);
+
+	return error;
+}
+
+STATIC int
 xfs_fs_remount(
 	struct super_block	*sb,
 	int			*flags,
@@ -1178,6 +1203,11 @@ xfs_fs_remount(
 	char			*p;
 	int			error;
 
+	/* First, check for complete junk; i.e. invalid options */
+	error = xfs_test_remount_options(sb, mp, options);
+	if (error)
+		return error;
+
 	sync_filesystem(sb);
 	while ((p = strsep(&options, ",")) != NULL) {
 		int token;


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3 V2] xfs: convert mount option parsing to tokens
  2016-02-15 21:20   ` [PATCH 1/3 V2] " Eric Sandeen
@ 2016-02-17 16:54     ` Christoph Hellwig
  0 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2016-02-17 16:54 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: xfs

> -suffix_kstrtoint(char *s, unsigned int base, int *res)
> +suffix_kstrtoint(const substring_t *s, unsigned int base, int *res)
>  {
>  	int	last, shift_left_factor = 0, _res;
> -	char	*value = s;
> +	char	*value = match_strdup(s);
> +	int	ret = 0;
> +

Needs to check for a NULL return value.

Otherwise looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3 V2] xfs: sanitize remount options
  2016-02-17  4:29   ` [PATCH 2/3 V2] " Eric Sandeen
@ 2016-02-17 16:55     ` Christoph Hellwig
  0 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2016-02-17 16:55 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: xfs

On Tue, Feb 16, 2016 at 10:29:21PM -0600, Eric Sandeen wrote:
> Perform basic sanitization of remount options by
> passing the option string and a dummy mount structure
> through xfs_parseargs and returning the result.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/3 V3] xfs: convert mount option parsing to tokens
  2016-02-10 23:38 ` [PATCH 1/3] xfs: convert mount option parsing to tokens Eric Sandeen
  2016-02-15 18:54   ` Brian Foster
  2016-02-15 21:20   ` [PATCH 1/3 V2] " Eric Sandeen
@ 2016-02-17 17:19   ` Eric Sandeen
  2 siblings, 0 replies; 14+ messages in thread
From: Eric Sandeen @ 2016-02-17 17:19 UTC (permalink / raw)
  To: xfs

This should be a no-op change, just switch to token parsing
like every other respectable filesystem does.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---

V2: Whitespace fixes, remove extraneous break-after-return (Thanks Brian!)
V3: Check for -ENOMEM from match_strdup (Thanks Christoph!)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 59c9b7b..9e1538d 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -65,83 +65,85 @@ static struct kset *xfs_kset;		/* top-level xfs sysfs dir */
 static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs attrs */
 #endif
 
-#define MNTOPT_LOGBUFS	"logbufs"	/* number of XFS log buffers */
-#define MNTOPT_LOGBSIZE	"logbsize"	/* size of XFS log buffers */
-#define MNTOPT_LOGDEV	"logdev"	/* log device */
-#define MNTOPT_RTDEV	"rtdev"		/* realtime I/O device */
-#define MNTOPT_BIOSIZE	"biosize"	/* log2 of preferred buffered io size */
-#define MNTOPT_WSYNC	"wsync"		/* safe-mode nfs compatible mount */
-#define MNTOPT_NOALIGN	"noalign"	/* turn off stripe alignment */
-#define MNTOPT_SWALLOC	"swalloc"	/* turn on stripe width allocation */
-#define MNTOPT_SUNIT	"sunit"		/* data volume stripe unit */
-#define MNTOPT_SWIDTH	"swidth"	/* data volume stripe width */
-#define MNTOPT_NOUUID	"nouuid"	/* ignore filesystem UUID */
-#define MNTOPT_MTPT	"mtpt"		/* filesystem mount point */
-#define MNTOPT_GRPID	"grpid"		/* group-ID from parent directory */
-#define MNTOPT_NOGRPID	"nogrpid"	/* group-ID from current process */
-#define MNTOPT_BSDGROUPS    "bsdgroups"    /* group-ID from parent directory */
-#define MNTOPT_SYSVGROUPS   "sysvgroups"   /* group-ID from current process */
-#define MNTOPT_ALLOCSIZE    "allocsize"    /* preferred allocation size */
-#define MNTOPT_NORECOVERY   "norecovery"   /* don't run XFS recovery */
-#define MNTOPT_BARRIER	"barrier"	/* use writer barriers for log write and
-					 * unwritten extent conversion */
-#define MNTOPT_NOBARRIER "nobarrier"	/* .. disable */
-#define MNTOPT_64BITINODE   "inode64"	/* inodes can be allocated anywhere */
-#define MNTOPT_32BITINODE   "inode32"	/* inode allocation limited to
-					 * XFS_MAXINUMBER_32 */
-#define MNTOPT_IKEEP	"ikeep"		/* do not free empty inode clusters */
-#define MNTOPT_NOIKEEP	"noikeep"	/* free empty inode clusters */
-#define MNTOPT_LARGEIO	   "largeio"	/* report large I/O sizes in stat() */
-#define MNTOPT_NOLARGEIO   "nolargeio"	/* do not report large I/O sizes
-					 * in stat(). */
-#define MNTOPT_ATTR2	"attr2"		/* do use attr2 attribute format */
-#define MNTOPT_NOATTR2	"noattr2"	/* do not use attr2 attribute format */
-#define MNTOPT_FILESTREAM  "filestreams" /* use filestreams allocator */
-#define MNTOPT_QUOTA	"quota"		/* disk quotas (user) */
-#define MNTOPT_NOQUOTA	"noquota"	/* no quotas */
-#define MNTOPT_USRQUOTA	"usrquota"	/* user quota enabled */
-#define MNTOPT_GRPQUOTA	"grpquota"	/* group quota enabled */
-#define MNTOPT_PRJQUOTA	"prjquota"	/* project quota enabled */
-#define MNTOPT_UQUOTA	"uquota"	/* user quota (IRIX variant) */
-#define MNTOPT_GQUOTA	"gquota"	/* group quota (IRIX variant) */
-#define MNTOPT_PQUOTA	"pquota"	/* project quota (IRIX variant) */
-#define MNTOPT_UQUOTANOENF "uqnoenforce"/* user quota limit enforcement */
-#define MNTOPT_GQUOTANOENF "gqnoenforce"/* group quota limit enforcement */
-#define MNTOPT_PQUOTANOENF "pqnoenforce"/* project quota limit enforcement */
-#define MNTOPT_QUOTANOENF  "qnoenforce"	/* same as uqnoenforce */
-#define MNTOPT_DISCARD	   "discard"	/* Discard unused blocks */
-#define MNTOPT_NODISCARD   "nodiscard"	/* Do not discard unused blocks */
-
-#define MNTOPT_DAX	"dax"		/* Enable direct access to bdev pages */
-
 /*
  * Table driven mount option parser.
- *
- * Currently only used for remount, but it will be used for mount
- * in the future, too.
  */
 enum {
-	Opt_barrier,
-	Opt_nobarrier,
-	Opt_inode64,
-	Opt_inode32,
-	Opt_err
+	Opt_logbufs, Opt_logbsize, Opt_logdev, Opt_rtdev, Opt_biosize,
+	Opt_wsync, Opt_noalign, Opt_swalloc, Opt_sunit, Opt_swidth, Opt_nouuid,
+	Opt_mtpt, Opt_grpid, Opt_nogrpid, Opt_bsdgroups, Opt_sysvgroups,
+	Opt_allocsize, Opt_norecovery, Opt_barrier, Opt_nobarrier,
+	Opt_inode64, Opt_inode32, Opt_ikeep, Opt_noikeep,
+	Opt_largeio, Opt_nolargeio, Opt_attr2, Opt_noattr2, Opt_filestreams,
+	Opt_quota, Opt_noquota, Opt_usrquota, Opt_grpquota, Opt_prjquota,
+	Opt_uquota, Opt_gquota, Opt_pquota,
+	Opt_uqnoenforce, Opt_gqnoenforce, Opt_pqnoenforce, Opt_qnoenforce,
+	Opt_discard, Opt_nodiscard, Opt_dax, Opt_err,
 };
 
 static const match_table_t tokens = {
-	{Opt_barrier, "barrier"},
-	{Opt_nobarrier, "nobarrier"},
-	{Opt_inode64, "inode64"},
-	{Opt_inode32, "inode32"},
-	{Opt_err, NULL}
+	{Opt_logbufs,	"logbufs=%u"},	/* number of XFS log buffers */
+	{Opt_logbsize,	"logbsize=%s"},	/* size of XFS log buffers */
+	{Opt_logdev,	"logdev=%s"},	/* log device */
+	{Opt_rtdev,	"rtdev=%s"},	/* realtime I/O device */
+	{Opt_biosize,	"biosize=%u"},	/* log2 of preferred buffered io size */
+	{Opt_wsync,	"wsync"},	/* safe-mode nfs compatible mount */
+	{Opt_noalign,	"noalign"},	/* turn off stripe alignment */
+	{Opt_swalloc,	"swalloc"},	/* turn on stripe width allocation */
+	{Opt_sunit,	"sunit=%u"},	/* data volume stripe unit */
+	{Opt_swidth,	"swidth=%u"},	/* data volume stripe width */
+	{Opt_nouuid,	"nouuid"},	/* ignore filesystem UUID */
+	{Opt_mtpt,	"mtpt"},	/* filesystem mount point */
+	{Opt_grpid,	"grpid"},	/* group-ID from parent directory */
+	{Opt_nogrpid,	"nogrpid"},	/* group-ID from current process */
+	{Opt_bsdgroups,	"bsdgroups"},	/* group-ID from parent directory */
+	{Opt_sysvgroups,"sysvgroups"},	/* group-ID from current process */
+	{Opt_allocsize,	"allocsize=%s"},/* preferred allocation size */
+	{Opt_norecovery,"norecovery"},	/* don't run XFS recovery */
+	{Opt_barrier,	"barrier"},	/* use writer barriers for log write and
+					 * unwritten extent conversion */
+	{Opt_nobarrier,	"nobarrier"},	/* .. disable */
+	{Opt_inode64,	"inode64"},	/* inodes can be allocated anywhere */
+	{Opt_inode32,   "inode32"},	/* inode allocation limited to
+					 * XFS_MAXINUMBER_32 */
+	{Opt_ikeep,	"ikeep"},	/* do not free empty inode clusters */
+	{Opt_noikeep,	"noikeep"},	/* free empty inode clusters */
+	{Opt_largeio,	"largeio"},	/* report large I/O sizes in stat() */
+	{Opt_nolargeio,	"nolargeio"},	/* do not report large I/O sizes
+					 * in stat(). */
+	{Opt_attr2,	"attr2"},	/* do use attr2 attribute format */
+	{Opt_noattr2,	"noattr2"},	/* do not use attr2 attribute format */
+	{Opt_filestreams,"filestreams"},/* use filestreams allocator */
+	{Opt_quota,	"quota"},	/* disk quotas (user) */
+	{Opt_noquota,	"noquota"},	/* no quotas */
+	{Opt_usrquota,	"usrquota"},	/* user quota enabled */
+	{Opt_grpquota,	"grpquota"},	/* group quota enabled */
+	{Opt_prjquota,	"prjquota"},	/* project quota enabled */
+	{Opt_uquota,	"uquota"},	/* user quota (IRIX variant) */
+	{Opt_gquota,	"gquota"},	/* group quota (IRIX variant) */
+	{Opt_pquota,	"pquota"},	/* project quota (IRIX variant) */
+	{Opt_uqnoenforce,"uqnoenforce"},/* user quota limit enforcement */
+	{Opt_gqnoenforce,"gqnoenforce"},/* group quota limit enforcement */
+	{Opt_pqnoenforce,"pqnoenforce"},/* project quota limit enforcement */
+	{Opt_qnoenforce, "qnoenforce"},	/* same as uqnoenforce */
+	{Opt_discard,	"discard"},	/* Discard unused blocks */
+	{Opt_nodiscard,	"nodiscard"},	/* Do not discard unused blocks */
+
+	{Opt_dax,	"dax"},		/* Enable direct access to bdev pages */
+	{Opt_err,	NULL},
 };
 
 
 STATIC int
-suffix_kstrtoint(char *s, unsigned int base, int *res)
+suffix_kstrtoint(const substring_t *s, unsigned int base, int *res)
 {
 	int	last, shift_left_factor = 0, _res;
-	char	*value = s;
+	char	*value;
+	int	ret = 0;
+
+	value = match_strdup(s);
+	if (!value)
+		return -ENOMEM;
 
 	last = strlen(value) - 1;
 	if (value[last] == 'K' || value[last] == 'k') {
@@ -157,10 +159,11 @@ suffix_kstrtoint(char *s, unsigned int base, int *res)
 		value[last] = '\0';
 	}
 
-	if (kstrtoint(s, base, &_res))
-		return -EINVAL;
+	if (kstrtoint(value, base, &_res))
+		ret = -EINVAL;
+	kfree(value);
 	*res = _res << shift_left_factor;
-	return 0;
+	return ret;
 }
 
 /*
@@ -176,7 +179,8 @@ xfs_parseargs(
 	char			*options)
 {
 	struct super_block	*sb = mp->m_super;
-	char			*this_char, *value;
+	char			*p;
+	substring_t		args[MAX_OPT_ARGS];
 	int			dsunit = 0;
 	int			dswidth = 0;
 	int			iosize = 0;
@@ -217,152 +221,152 @@ xfs_parseargs(
 	if (!options)
 		goto done;
 
-	while ((this_char = strsep(&options, ",")) != NULL) {
-		if (!*this_char)
+	while ((p = strsep(&options, ",")) != NULL) {
+		int		token;
+
+		if (!*p)
 			continue;
-		if ((value = strchr(this_char, '=')) != NULL)
-			*value++ = 0;
 
-		if (!strcmp(this_char, MNTOPT_LOGBUFS)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
-				return -EINVAL;
-			}
-			if (kstrtoint(value, 10, &mp->m_logbufs))
-				return -EINVAL;
-		} else if (!strcmp(this_char, MNTOPT_LOGBSIZE)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
-				return -EINVAL;
-			}
-			if (suffix_kstrtoint(value, 10, &mp->m_logbsize))
+		token = match_token(p, tokens, args);
+		switch (token) {
+		case Opt_logbufs:
+			if (match_int(args, &mp->m_logbufs))
 				return -EINVAL;
-		} else if (!strcmp(this_char, MNTOPT_LOGDEV)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
+			break;
+		case Opt_logbsize:
+			if (suffix_kstrtoint(args, 10, &mp->m_logbsize))
 				return -EINVAL;
-			}
-			mp->m_logname = kstrndup(value, MAXNAMELEN, GFP_KERNEL);
+			break;
+		case Opt_logdev:
+			mp->m_logname = match_strdup(args);
 			if (!mp->m_logname)
 				return -ENOMEM;
-		} else if (!strcmp(this_char, MNTOPT_MTPT)) {
-			xfs_warn(mp, "%s option not allowed on this system",
-				this_char);
+			break;
+		case Opt_mtpt:
+			xfs_warn(mp, "%s option not allowed on this system", p);
 			return -EINVAL;
-		} else if (!strcmp(this_char, MNTOPT_RTDEV)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
-				return -EINVAL;
-			}
-			mp->m_rtname = kstrndup(value, MAXNAMELEN, GFP_KERNEL);
+		case Opt_rtdev:
+			mp->m_rtname = match_strdup(args);
 			if (!mp->m_rtname)
 				return -ENOMEM;
-		} else if (!strcmp(this_char, MNTOPT_ALLOCSIZE) ||
-			   !strcmp(this_char, MNTOPT_BIOSIZE)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
-				return -EINVAL;
-			}
-			if (suffix_kstrtoint(value, 10, &iosize))
+			break;
+		case Opt_allocsize:
+		case Opt_biosize:
+			if (suffix_kstrtoint(args, 10, &iosize))
 				return -EINVAL;
 			iosizelog = ffs(iosize) - 1;
-		} else if (!strcmp(this_char, MNTOPT_GRPID) ||
-			   !strcmp(this_char, MNTOPT_BSDGROUPS)) {
+			break;
+		case Opt_grpid:
+		case Opt_bsdgroups:
 			mp->m_flags |= XFS_MOUNT_GRPID;
-		} else if (!strcmp(this_char, MNTOPT_NOGRPID) ||
-			   !strcmp(this_char, MNTOPT_SYSVGROUPS)) {
+			break;
+		case Opt_nogrpid:
+		case Opt_sysvgroups:
 			mp->m_flags &= ~XFS_MOUNT_GRPID;
-		} else if (!strcmp(this_char, MNTOPT_WSYNC)) {
+			break;
+		case Opt_wsync:
 			mp->m_flags |= XFS_MOUNT_WSYNC;
-		} else if (!strcmp(this_char, MNTOPT_NORECOVERY)) {
+			break;
+		case Opt_norecovery:
 			mp->m_flags |= XFS_MOUNT_NORECOVERY;
-		} else if (!strcmp(this_char, MNTOPT_NOALIGN)) {
+			break;
+		case Opt_noalign:
 			mp->m_flags |= XFS_MOUNT_NOALIGN;
-		} else if (!strcmp(this_char, MNTOPT_SWALLOC)) {
+			break;
+		case Opt_swalloc:
 			mp->m_flags |= XFS_MOUNT_SWALLOC;
-		} else if (!strcmp(this_char, MNTOPT_SUNIT)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
-				return -EINVAL;
-			}
-			if (kstrtoint(value, 10, &dsunit))
-				return -EINVAL;
-		} else if (!strcmp(this_char, MNTOPT_SWIDTH)) {
-			if (!value || !*value) {
-				xfs_warn(mp, "%s option requires an argument",
-					this_char);
+			break;
+		case Opt_sunit:
+			if (match_int(args, &dsunit))
 				return -EINVAL;
-			}
-			if (kstrtoint(value, 10, &dswidth))
+			break;
+		case Opt_swidth:
+			if (match_int(args, &dswidth))
 				return -EINVAL;
-		} else if (!strcmp(this_char, MNTOPT_32BITINODE)) {
+			break;
+		case Opt_inode32:
 			mp->m_flags |= XFS_MOUNT_SMALL_INUMS;
-		} else if (!strcmp(this_char, MNTOPT_64BITINODE)) {
+			break;
+		case Opt_inode64:
 			mp->m_flags &= ~XFS_MOUNT_SMALL_INUMS;
-		} else if (!strcmp(this_char, MNTOPT_NOUUID)) {
+			break;
+		case Opt_nouuid:
 			mp->m_flags |= XFS_MOUNT_NOUUID;
-		} else if (!strcmp(this_char, MNTOPT_BARRIER)) {
+			break;
+		case Opt_barrier:
 			mp->m_flags |= XFS_MOUNT_BARRIER;
-		} else if (!strcmp(this_char, MNTOPT_NOBARRIER)) {
+			break;
+		case Opt_nobarrier:
 			mp->m_flags &= ~XFS_MOUNT_BARRIER;
-		} else if (!strcmp(this_char, MNTOPT_IKEEP)) {
+			break;
+		case Opt_ikeep:
 			mp->m_flags |= XFS_MOUNT_IKEEP;
-		} else if (!strcmp(this_char, MNTOPT_NOIKEEP)) {
+			break;
+		case Opt_noikeep:
 			mp->m_flags &= ~XFS_MOUNT_IKEEP;
-		} else if (!strcmp(this_char, MNTOPT_LARGEIO)) {
+			break;
+		case Opt_largeio:
 			mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE;
-		} else if (!strcmp(this_char, MNTOPT_NOLARGEIO)) {
+			break;
+		case Opt_nolargeio:
 			mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE;
-		} else if (!strcmp(this_char, MNTOPT_ATTR2)) {
+			break;
+		case Opt_attr2:
 			mp->m_flags |= XFS_MOUNT_ATTR2;
-		} else if (!strcmp(this_char, MNTOPT_NOATTR2)) {
+			break;
+		case Opt_noattr2:
 			mp->m_flags &= ~XFS_MOUNT_ATTR2;
 			mp->m_flags |= XFS_MOUNT_NOATTR2;
-		} else if (!strcmp(this_char, MNTOPT_FILESTREAM)) {
+			break;
+		case Opt_filestreams:
 			mp->m_flags |= XFS_MOUNT_FILESTREAMS;
-		} else if (!strcmp(this_char, MNTOPT_NOQUOTA)) {
+			break;
+		case Opt_noquota:
 			mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT;
 			mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD;
 			mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE;
-		} else if (!strcmp(this_char, MNTOPT_QUOTA) ||
-			   !strcmp(this_char, MNTOPT_UQUOTA) ||
-			   !strcmp(this_char, MNTOPT_USRQUOTA)) {
+			break;
+		case Opt_quota:
+		case Opt_uquota:
+		case Opt_usrquota:
 			mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE |
 					 XFS_UQUOTA_ENFD);
-		} else if (!strcmp(this_char, MNTOPT_QUOTANOENF) ||
-			   !strcmp(this_char, MNTOPT_UQUOTANOENF)) {
+			break;
+		case Opt_qnoenforce:
+		case Opt_uqnoenforce:
 			mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE);
 			mp->m_qflags &= ~XFS_UQUOTA_ENFD;
-		} else if (!strcmp(this_char, MNTOPT_PQUOTA) ||
-			   !strcmp(this_char, MNTOPT_PRJQUOTA)) {
+			break;
+		case Opt_pquota:
+		case Opt_prjquota:
 			mp->m_qflags |= (XFS_PQUOTA_ACCT | XFS_PQUOTA_ACTIVE |
 					 XFS_PQUOTA_ENFD);
-		} else if (!strcmp(this_char, MNTOPT_PQUOTANOENF)) {
+			break;
+		case Opt_pqnoenforce:
 			mp->m_qflags |= (XFS_PQUOTA_ACCT | XFS_PQUOTA_ACTIVE);
 			mp->m_qflags &= ~XFS_PQUOTA_ENFD;
-		} else if (!strcmp(this_char, MNTOPT_GQUOTA) ||
-			   !strcmp(this_char, MNTOPT_GRPQUOTA)) {
+		case Opt_gquota:
+		case Opt_grpquota:
 			mp->m_qflags |= (XFS_GQUOTA_ACCT | XFS_GQUOTA_ACTIVE |
 					 XFS_GQUOTA_ENFD);
-		} else if (!strcmp(this_char, MNTOPT_GQUOTANOENF)) {
+			break;
+		case Opt_gqnoenforce:
 			mp->m_qflags |= (XFS_GQUOTA_ACCT | XFS_GQUOTA_ACTIVE);
 			mp->m_qflags &= ~XFS_GQUOTA_ENFD;
-		} else if (!strcmp(this_char, MNTOPT_DISCARD)) {
+			break;
+		case Opt_discard:
 			mp->m_flags |= XFS_MOUNT_DISCARD;
-		} else if (!strcmp(this_char, MNTOPT_NODISCARD)) {
+			break;
+		case Opt_nodiscard:
 			mp->m_flags &= ~XFS_MOUNT_DISCARD;
+			break;
 #ifdef CONFIG_FS_DAX
-		} else if (!strcmp(this_char, MNTOPT_DAX)) {
+		case Opt_dax:
 			mp->m_flags |= XFS_MOUNT_DAX;
+			break;
 #endif
-		} else {
-			xfs_warn(mp, "unknown mount option [%s].", this_char);
+		default:
+			xfs_warn(mp, "unknown mount option [%s].", p);
 			return -EINVAL;
 		}
 	}
@@ -461,25 +465,25 @@ xfs_showargs(
 {
 	static struct proc_xfs_info xfs_info_set[] = {
 		/* the few simple ones we can get from the mount struct */
-		{ XFS_MOUNT_IKEEP,		"," MNTOPT_IKEEP },
-		{ XFS_MOUNT_WSYNC,		"," MNTOPT_WSYNC },
-		{ XFS_MOUNT_NOALIGN,		"," MNTOPT_NOALIGN },
-		{ XFS_MOUNT_SWALLOC,		"," MNTOPT_SWALLOC },
-		{ XFS_MOUNT_NOUUID,		"," MNTOPT_NOUUID },
-		{ XFS_MOUNT_NORECOVERY,		"," MNTOPT_NORECOVERY },
-		{ XFS_MOUNT_ATTR2,		"," MNTOPT_ATTR2 },
-		{ XFS_MOUNT_FILESTREAMS,	"," MNTOPT_FILESTREAM },
-		{ XFS_MOUNT_GRPID,		"," MNTOPT_GRPID },
-		{ XFS_MOUNT_DISCARD,		"," MNTOPT_DISCARD },
-		{ XFS_MOUNT_SMALL_INUMS,	"," MNTOPT_32BITINODE },
-		{ XFS_MOUNT_DAX,		"," MNTOPT_DAX },
+		{ XFS_MOUNT_IKEEP,		",ikeep" },
+		{ XFS_MOUNT_WSYNC,		",wsync" },
+		{ XFS_MOUNT_NOALIGN,		",noalign" },
+		{ XFS_MOUNT_SWALLOC,		",swalloc" },
+		{ XFS_MOUNT_NOUUID,		",nouuid" },
+		{ XFS_MOUNT_NORECOVERY,		",norecovery" },
+		{ XFS_MOUNT_ATTR2,		",attr2" },
+		{ XFS_MOUNT_FILESTREAMS,	",filestreams" },
+		{ XFS_MOUNT_GRPID,		",grpid" },
+		{ XFS_MOUNT_DISCARD,		",discard" },
+		{ XFS_MOUNT_SMALL_INUMS,	",inode32" },
+		{ XFS_MOUNT_DAX,		",dax" },
 		{ 0, NULL }
 	};
 	static struct proc_xfs_info xfs_info_unset[] = {
 		/* the few simple ones we can get from the mount struct */
-		{ XFS_MOUNT_COMPAT_IOSIZE,	"," MNTOPT_LARGEIO },
-		{ XFS_MOUNT_BARRIER,		"," MNTOPT_NOBARRIER },
-		{ XFS_MOUNT_SMALL_INUMS,	"," MNTOPT_64BITINODE },
+		{ XFS_MOUNT_COMPAT_IOSIZE,	",largeio" },
+		{ XFS_MOUNT_BARRIER,		",nobarrier" },
+		{ XFS_MOUNT_SMALL_INUMS,	",inode64" },
 		{ 0, NULL }
 	};
 	struct proc_xfs_info	*xfs_infop;
@@ -494,46 +498,46 @@ xfs_showargs(
 	}
 
 	if (mp->m_flags & XFS_MOUNT_DFLT_IOSIZE)
-		seq_printf(m, "," MNTOPT_ALLOCSIZE "=%dk",
+		seq_printf(m, ",allocsize=%dk",
 				(int)(1 << mp->m_writeio_log) >> 10);
 
 	if (mp->m_logbufs > 0)
-		seq_printf(m, "," MNTOPT_LOGBUFS "=%d", mp->m_logbufs);
+		seq_printf(m, ",logbufs=%d", mp->m_logbufs);
 	if (mp->m_logbsize > 0)
-		seq_printf(m, "," MNTOPT_LOGBSIZE "=%dk", mp->m_logbsize >> 10);
+		seq_printf(m, ",logbsize=%dk", mp->m_logbsize >> 10);
 
 	if (mp->m_logname)
-		seq_show_option(m, MNTOPT_LOGDEV, mp->m_logname);
+		seq_show_option(m, "logdev", mp->m_logname);
 	if (mp->m_rtname)
-		seq_show_option(m, MNTOPT_RTDEV, mp->m_rtname);
+		seq_show_option(m, "rtdev", mp->m_rtname);
 
 	if (mp->m_dalign > 0)
-		seq_printf(m, "," MNTOPT_SUNIT "=%d",
+		seq_printf(m, ",sunit=%d",
 				(int)XFS_FSB_TO_BB(mp, mp->m_dalign));
 	if (mp->m_swidth > 0)
-		seq_printf(m, "," MNTOPT_SWIDTH "=%d",
+		seq_printf(m, ",swidth=%d",
 				(int)XFS_FSB_TO_BB(mp, mp->m_swidth));
 
 	if (mp->m_qflags & (XFS_UQUOTA_ACCT|XFS_UQUOTA_ENFD))
-		seq_puts(m, "," MNTOPT_USRQUOTA);
+		seq_puts(m, ",usrquota");
 	else if (mp->m_qflags & XFS_UQUOTA_ACCT)
-		seq_puts(m, "," MNTOPT_UQUOTANOENF);
+		seq_puts(m, ",uqnoenforce");
 
 	if (mp->m_qflags & XFS_PQUOTA_ACCT) {
 		if (mp->m_qflags & XFS_PQUOTA_ENFD)
-			seq_puts(m, "," MNTOPT_PRJQUOTA);
+			seq_puts(m, ",prjquota");
 		else
-			seq_puts(m, "," MNTOPT_PQUOTANOENF);
+			seq_puts(m, ",pqnoenforce");
 	}
 	if (mp->m_qflags & XFS_GQUOTA_ACCT) {
 		if (mp->m_qflags & XFS_GQUOTA_ENFD)
-			seq_puts(m, "," MNTOPT_GRPQUOTA);
+			seq_puts(m, ",grpquota");
 		else
-			seq_puts(m, "," MNTOPT_GQUOTANOENF);
+			seq_puts(m, ",gqnoenforce");
 	}
 
 	if (!(mp->m_qflags & XFS_ALL_QUOTA_ACCT))
-		seq_puts(m, "," MNTOPT_NOQUOTA);
+		seq_puts(m, ",noquota");
 
 	return 0;
 }
@@ -1344,9 +1348,8 @@ xfs_finish_flags(
 	 */
 	if (xfs_sb_version_hascrc(&mp->m_sb) &&
 	    (mp->m_flags & XFS_MOUNT_NOATTR2)) {
-		xfs_warn(mp,
-"Cannot mount a V5 filesystem as %s. %s is always enabled for V5 filesystems.",
-			MNTOPT_NOATTR2, MNTOPT_ATTR2);
+		xfs_warn(mp, "Cannot mount a V5 filesystem as noattr2. "
+			     "attr2 is always enabled for V5 filesystems.");
 		return -EINVAL;
 	}
 


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2016-02-17 17:19 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-10 23:36 [PATCH 0/3] xfs: mount option handling fixups Eric Sandeen
2016-02-10 23:38 ` [PATCH 1/3] xfs: convert mount option parsing to tokens Eric Sandeen
2016-02-15 18:54   ` Brian Foster
2016-02-15 21:20   ` [PATCH 1/3 V2] " Eric Sandeen
2016-02-17 16:54     ` Christoph Hellwig
2016-02-17 17:19   ` [PATCH 1/3 V3] " Eric Sandeen
2016-02-10 23:40 ` [PATCH 2/3] xfs: sanitize remount options Eric Sandeen
2016-02-15 18:54   ` Brian Foster
2016-02-17  4:29   ` [PATCH 2/3 V2] " Eric Sandeen
2016-02-17 16:55     ` Christoph Hellwig
2016-02-10 23:45 ` [PATCH 3/3] xfs: test for valid remount options, error if not Eric Sandeen
2016-02-15 18:54   ` Brian Foster
2016-02-15 20:25   ` Dave Chinner
2016-02-15 23:07     ` Eric Sandeen

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.