* [RFC, PATCH 00/15] mkfs: sanitise input parameters
@ 2013-11-29 1:43 Dave Chinner
2013-11-29 1:43 ` [PATCH 01/15] xfsprogs: use common code for multi-disk detection Dave Chinner
` (14 more replies)
0 siblings, 15 replies; 42+ messages in thread
From: Dave Chinner @ 2013-11-29 1:43 UTC (permalink / raw)
To: xfs
Hi folks,
This is still a work in progress, but is compelte enough to get
feedback on the general structure. The problem being solved here is
that mkfs does a terrible job of input validation from the command
line, has huge amounts of repeated code in the sub options
processing loops and has many, many unnecessary variable for
tracking simply things like whether a parameter was specified.
This patchset introduces a parameter table structure that is used to
define the parameters and their constraints. Things like minimum and
maximum valid values, default values, conflicting options, etc are
all contained within the table, so all the "policy" is found in a
single place.
This greatly reduces the complexity of the option parsing loop. It
doesn't remove all the complexity (yet) because many of the options
have special cases or more complex conflicts than I've yet added
support for. The idea is, however, that all of the sub-option
parameter setup will eventually end up being implemented as a
generic loop as the parameter structure will hold all the
information about in the input parameters.
To get there, the parameter table still needs more work - it needs
to hold the value/string for the parameter, and we need to reference
those in the code.
The flow on effect of this is that we can remove the many, many
individual variables and start passing the option structures to
functions rather than avoiding using functions because passing so
many variables is messy and nasty. IOWs, it lays the groundwork for
factoring xfs_mkfs.c into something more than a bunch of spagetti...
Anyway, have a look and see what you think about progress so far.
FWIW, the first patch is following up on the multi-disk discussion
Christoph and I had, and the last patch in the series covers all the
issues that arose with "-d file" and treating files like block
devices....
Cheers,
Dave.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 01/15] xfsprogs: use common code for multi-disk detection
2013-11-29 1:43 [RFC, PATCH 00/15] mkfs: sanitise input parameters Dave Chinner
@ 2013-11-29 1:43 ` Dave Chinner
2013-12-02 10:40 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 02/15] mkfs: sanitise ftype parameter values Dave Chinner
` (13 subsequent siblings)
14 siblings, 1 reply; 42+ messages in thread
From: Dave Chinner @ 2013-11-29 1:43 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
Both xfs_repair and mkfs.xfs need to agree on what is a "multidisk:
configuration - mkfs for determining the AG count of the filesystem,
repair for determining how to automatically parallelise it's
execution. This requires a bunch of common defines that both mkfs
and reapir need to share.
In fact, most of the defines in xfs_mkfs.h could be shared with
other programs (i.e. all the defaults mkfs uses) and so it is
simplest to move xfs_mkfs.h to the shared include directory and add
the new defines to it directly.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
include/Makefile | 1 +
include/xfs_mkfs.h | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++
mkfs/Makefile | 2 +-
mkfs/xfs_mkfs.c | 56 +++++++++++++++---------------
mkfs/xfs_mkfs.h | 89 ------------------------------------------------
repair/xfs_repair.c | 54 ++++++++++++++++++++++-------
6 files changed, 171 insertions(+), 129 deletions(-)
create mode 100644 include/xfs_mkfs.h
delete mode 100644 mkfs/xfs_mkfs.h
diff --git a/include/Makefile b/include/Makefile
index 6682b9d..084d72e 100644
--- a/include/Makefile
+++ b/include/Makefile
@@ -38,6 +38,7 @@ QAHFILES = libxfs.h libxlog.h \
xfs_log_format.h \
xfs_log_recover.h \
xfs_metadump.h \
+ xfs_mkfs.h \
xfs_quota_defs.h \
xfs_sb.h \
xfs_shared.h \
diff --git a/include/xfs_mkfs.h b/include/xfs_mkfs.h
new file mode 100644
index 0000000..3388f6d
--- /dev/null
+++ b/include/xfs_mkfs.h
@@ -0,0 +1,98 @@
+/*
+ * Copyright (c) 2000-2001,2004-2005 Silicon Graphics, Inc.
+ * All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+#ifndef __XFS_MKFS_H__
+#define __XFS_MKFS_H__
+
+#define XFS_DFL_SB_VERSION_BITS \
+ (XFS_SB_VERSION_NLINKBIT | \
+ XFS_SB_VERSION_EXTFLGBIT | \
+ XFS_SB_VERSION_DIRV2BIT)
+
+#define XFS_SB_VERSION_MKFS(crc,ia,dia,log2,attr1,sflag,ci,more) (\
+ ((crc)||(ia)||(dia)||(log2)||(attr1)||(sflag)||(ci)||(more)) ? \
+ (((crc) ? XFS_SB_VERSION_5 : XFS_SB_VERSION_4) | \
+ ((ia) ? XFS_SB_VERSION_ALIGNBIT : 0) | \
+ ((dia) ? XFS_SB_VERSION_DALIGNBIT : 0) | \
+ ((log2) ? XFS_SB_VERSION_LOGV2BIT : 0) | \
+ ((attr1) ? XFS_SB_VERSION_ATTRBIT : 0) | \
+ ((sflag) ? XFS_SB_VERSION_SECTORBIT : 0) | \
+ ((ci) ? XFS_SB_VERSION_BORGBIT : 0) | \
+ ((more) ? XFS_SB_VERSION_MOREBITSBIT : 0) | \
+ XFS_DFL_SB_VERSION_BITS | \
+ 0 ) : XFS_SB_VERSION_1 )
+
+#define XFS_SB_VERSION2_MKFS(crc, lazycount, attr2, projid32bit, parent, \
+ ftype) (\
+ ((lazycount) ? XFS_SB_VERSION2_LAZYSBCOUNTBIT : 0) | \
+ ((attr2) ? XFS_SB_VERSION2_ATTR2BIT : 0) | \
+ ((projid32bit) ? XFS_SB_VERSION2_PROJID32BIT : 0) | \
+ ((parent) ? XFS_SB_VERSION2_PARENTBIT : 0) | \
+ ((crc) ? XFS_SB_VERSION2_CRCBIT : 0) | \
+ ((ftype) ? XFS_SB_VERSION2_FTYPE : 0) | \
+ 0 )
+
+#define XFS_DFL_BLOCKSIZE_LOG 12 /* 4096 byte blocks */
+#define XFS_DINODE_DFL_LOG 8 /* 256 byte inodes */
+#define XFS_DINODE_DFL_CRC_LOG 9 /* 512 byte inodes for CRCs */
+#define XFS_MIN_DATA_BLOCKS 100
+#define XFS_MIN_INODE_PERBLOCK 2 /* min inodes per block */
+#define XFS_DFL_IMAXIMUM_PCT 25 /* max % of space for inodes */
+#define XFS_IFLAG_ALIGN 1 /* -i align defaults on */
+#define XFS_MIN_REC_DIRSIZE 12 /* 4096 byte dirblocks (V2) */
+#define XFS_DFL_DIR_VERSION 2 /* default directory version */
+#define XFS_DFL_LOG_SIZE 1000 /* default log size, blocks */
+#define XFS_DFL_LOG_FACTOR 5 /* default log size, factor */
+ /* with max trans reservation */
+#define XFS_MAX_INODE_SIG_BITS 32 /* most significant bits in an
+ * inode number that we'll
+ * accept w/o warnings
+ */
+
+#define XFS_AG_BYTES(bblog) ((long long)BBSIZE << (bblog))
+#define XFS_AG_MIN_BYTES ((XFS_AG_BYTES(15))) /* 16 MB */
+#define XFS_AG_MIN_BLOCKS(blog) ((XFS_AG_BYTES(15)) >> (blog))
+#define XFS_AG_MAX_BLOCKS(blog) ((XFS_AG_BYTES(31) - 1) >> (blog))
+
+#define XFS_MAX_AGNUMBER ((xfs_agnumber_t)(NULLAGNUMBER - 1))
+
+/*
+ * These values define what we consider a "multi-disk" filesystem. That is, a
+ * filesystem that is likely to be made up of multiple devices, and hence have
+ * some level of parallelism avoid to it at the IO level.
+ */
+#define XFS_MULTIDISK_AGLOG 5 /* 32 AGs */
+#define XFS_NOMULTIDISK_AGLOG 2 /* 4 AGs */
+#define XFS_MULTIDISK_AGCOUNT (1 << XFS_MULTIDISK_AGLOG)
+
+
+/* xfs_mkfs.c */
+extern int isdigits (char *str);
+extern long long cvtnum (unsigned int blocksize,
+ unsigned int sectorsize, char *s);
+
+/* proto.c */
+extern char *setup_proto (char *fname);
+extern void parse_proto (xfs_mount_t *mp, struct fsxattr *fsx, char **pp);
+extern void res_failed (int err);
+
+/* maxtrres.c */
+extern int max_trans_res (int crcs_enabled, int dirversion,
+ int sectorlog, int blocklog, int inodelog, int dirblocklog,
+ int logversion, int log_sunit);
+
+#endif /* __XFS_MKFS_H__ */
diff --git a/mkfs/Makefile b/mkfs/Makefile
index 75da633..9dd3d3a 100644
--- a/mkfs/Makefile
+++ b/mkfs/Makefile
@@ -8,7 +8,7 @@ include $(TOPDIR)/include/builddefs
LTCOMMAND = mkfs.xfs
FSTYP = fstyp
-HFILES = xfs_mkfs.h
+HFILES =
CFILES = maxtrres.c proto.c xfs_mkfs.c
ifeq ($(ENABLE_BLKID),yes)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index d82128c..cc74535 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -24,7 +24,7 @@
#include <disk/fstyp.h>
#include <disk/volume.h>
#endif
-#include "xfs_mkfs.h"
+#include <xfs/xfs_mkfs.h>
/*
* Device topology information.
@@ -659,43 +659,45 @@ calc_default_ag_geometry(
}
/*
- * For the remainder we choose an AG size based on the
- * number of data blocks available, trying to keep the
- * number of AGs relatively small (especially compared
- * to the original algorithm). AG count is calculated
- * based on the preferred AG size, not vice-versa - the
- * count can be increased by growfs, so prefer to use
- * smaller counts at mkfs time.
- *
- * For a single underlying storage device between 128MB
- * and 4TB in size, just use 4 AGs, otherwise scale up
- * smoothly between min/max AG sizes.
+ * For a single underlying storage device between 128MB and 4TB in size
+ * just use 4 AGs and scale up smoothly between min/max AG sizes.
*/
-
- if (!multidisk && dblocks >= MEGABYTES(128, blocklog)) {
+ if (!multidisk) {
if (dblocks >= TERABYTES(4, blocklog)) {
blocks = XFS_AG_MAX_BLOCKS(blocklog);
goto done;
+ } else if (dblocks >= MEGABYTES(128, blocklog)) {
+ shift = XFS_NOMULTIDISK_AGLOG;
+ goto calc_blocks;
}
- shift = 2;
- } else if (dblocks > GIGABYTES(512, blocklog))
- shift = 5;
- else if (dblocks > GIGABYTES(8, blocklog))
- shift = 4;
- else if (dblocks >= MEGABYTES(128, blocklog))
- shift = 3;
- else if (dblocks >= MEGABYTES(64, blocklog))
- shift = 2;
- else if (dblocks >= MEGABYTES(32, blocklog))
- shift = 1;
- else
- shift = 0;
+ }
+
+ /*
+ * For the multidisk configs we choose an AG count based on the number
+ * of data blocks available, trying to keep the number of AGs higher
+ * than the single disk configurations. This makes the assumption that
+ * larger filesystems have more parallelism available to them.
+ */
+ shift = XFS_MULTIDISK_AGLOG;
+ if (dblocks < GIGABYTES(512, blocklog))
+ shift--;
+ if (dblocks < GIGABYTES(8, blocklog))
+ shift--;
+ if (dblocks < MEGABYTES(128, blocklog))
+ shift--;
+ if (dblocks < MEGABYTES(64, blocklog))
+ shift--;
+ if (dblocks < MEGABYTES(32, blocklog))
+ shift--;
+
/*
* If dblocks is not evenly divisible by the number of
* desired AGs, round "blocks" up so we don't lose the
* last bit of the filesystem. The same principle applies
* to the AG count, so we don't lose the last AG!
*/
+calc_blocks:
+ ASSERT(shift >= 0 && shift <= XFS_MULTIDISK_AGLOG);
blocks = dblocks >> shift;
if (dblocks & xfs_mask32lo(shift)) {
if (blocks < XFS_AG_MAX_BLOCKS(blocklog))
diff --git a/mkfs/xfs_mkfs.h b/mkfs/xfs_mkfs.h
deleted file mode 100644
index 9df5f37..0000000
--- a/mkfs/xfs_mkfs.h
+++ /dev/null
@@ -1,89 +0,0 @@
-/*
- * Copyright (c) 2000-2001,2004-2005 Silicon Graphics, Inc.
- * All Rights Reserved.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it would be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write the Free Software Foundation,
- * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
- */
-#ifndef __XFS_MKFS_H__
-#define __XFS_MKFS_H__
-
-#define XFS_DFL_SB_VERSION_BITS \
- (XFS_SB_VERSION_NLINKBIT | \
- XFS_SB_VERSION_EXTFLGBIT | \
- XFS_SB_VERSION_DIRV2BIT)
-
-#define XFS_SB_VERSION_MKFS(crc,ia,dia,log2,attr1,sflag,ci,more) (\
- ((crc)||(ia)||(dia)||(log2)||(attr1)||(sflag)||(ci)||(more)) ? \
- (((crc) ? XFS_SB_VERSION_5 : XFS_SB_VERSION_4) | \
- ((ia) ? XFS_SB_VERSION_ALIGNBIT : 0) | \
- ((dia) ? XFS_SB_VERSION_DALIGNBIT : 0) | \
- ((log2) ? XFS_SB_VERSION_LOGV2BIT : 0) | \
- ((attr1) ? XFS_SB_VERSION_ATTRBIT : 0) | \
- ((sflag) ? XFS_SB_VERSION_SECTORBIT : 0) | \
- ((ci) ? XFS_SB_VERSION_BORGBIT : 0) | \
- ((more) ? XFS_SB_VERSION_MOREBITSBIT : 0) | \
- XFS_DFL_SB_VERSION_BITS | \
- 0 ) : XFS_SB_VERSION_1 )
-
-#define XFS_SB_VERSION2_MKFS(crc, lazycount, attr2, projid32bit, parent, \
- ftype) (\
- ((lazycount) ? XFS_SB_VERSION2_LAZYSBCOUNTBIT : 0) | \
- ((attr2) ? XFS_SB_VERSION2_ATTR2BIT : 0) | \
- ((projid32bit) ? XFS_SB_VERSION2_PROJID32BIT : 0) | \
- ((parent) ? XFS_SB_VERSION2_PARENTBIT : 0) | \
- ((crc) ? XFS_SB_VERSION2_CRCBIT : 0) | \
- ((ftype) ? XFS_SB_VERSION2_FTYPE : 0) | \
- 0 )
-
-#define XFS_DFL_BLOCKSIZE_LOG 12 /* 4096 byte blocks */
-#define XFS_DINODE_DFL_LOG 8 /* 256 byte inodes */
-#define XFS_DINODE_DFL_CRC_LOG 9 /* 512 byte inodes for CRCs */
-#define XFS_MIN_DATA_BLOCKS 100
-#define XFS_MIN_INODE_PERBLOCK 2 /* min inodes per block */
-#define XFS_DFL_IMAXIMUM_PCT 25 /* max % of space for inodes */
-#define XFS_IFLAG_ALIGN 1 /* -i align defaults on */
-#define XFS_MIN_REC_DIRSIZE 12 /* 4096 byte dirblocks (V2) */
-#define XFS_DFL_DIR_VERSION 2 /* default directory version */
-#define XFS_DFL_LOG_SIZE 1000 /* default log size, blocks */
-#define XFS_DFL_LOG_FACTOR 5 /* default log size, factor */
- /* with max trans reservation */
-#define XFS_MAX_INODE_SIG_BITS 32 /* most significant bits in an
- * inode number that we'll
- * accept w/o warnings
- */
-
-#define XFS_AG_BYTES(bblog) ((long long)BBSIZE << (bblog))
-#define XFS_AG_MIN_BYTES ((XFS_AG_BYTES(15))) /* 16 MB */
-#define XFS_AG_MIN_BLOCKS(blog) ((XFS_AG_BYTES(15)) >> (blog))
-#define XFS_AG_MAX_BLOCKS(blog) ((XFS_AG_BYTES(31) - 1) >> (blog))
-
-#define XFS_MAX_AGNUMBER ((xfs_agnumber_t)(NULLAGNUMBER - 1))
-
-
-/* xfs_mkfs.c */
-extern int isdigits (char *str);
-extern long long cvtnum (unsigned int blocksize,
- unsigned int sectorsize, char *s);
-
-/* proto.c */
-extern char *setup_proto (char *fname);
-extern void parse_proto (xfs_mount_t *mp, struct fsxattr *fsx, char **pp);
-extern void res_failed (int err);
-
-/* maxtrres.c */
-extern int max_trans_res (int crcs_enabled, int dirversion,
- int sectorlog, int blocklog, int inodelog, int dirblocklog,
- int logversion, int log_sunit);
-
-#endif /* __XFS_MKFS_H__ */
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index a863337..7cfeb11 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -18,6 +18,7 @@
#include <xfs/libxlog.h>
#include <sys/resource.h>
+#include <xfs/xfs_mkfs.h>
#include "avl.h"
#include "avl64.h"
#include "globals.h"
@@ -519,6 +520,33 @@ _("sb realtime summary inode %" PRIu64 " %sinconsistent with calculated value %u
}
+/*
+ * mkfs increases the AG count for "multidisk" configurations, we want
+ * to target these for an increase in thread count. Hence check the superlock
+ * geometry information to determine if mkfs considered this a multidisk
+ * configuration.
+ */
+static bool
+is_multidisk_filesystem(
+ struct xfs_mount *mp)
+{
+ struct xfs_sb *sbp = &mp->m_sb;
+
+ /* High agcount filesystems are always considered "multidisk" */
+ if (sbp->sb_agcount >= XFS_MULTIDISK_AGCOUNT)
+ return true;
+
+ /*
+ * If it doesn't have a sunit/swidth, mkfs didn't consider it a
+ * multi-disk array, so we don't either.
+ */
+ if (!sbp->sb_unit)
+ return false;
+
+ ASSERT(sbp->sb_width);
+ return true;
+}
+
int
main(int argc, char **argv)
{
@@ -617,19 +645,21 @@ main(int argc, char **argv)
/*
* Automatic striding for high agcount filesystems.
*
- * More AGs indicates that the filesystem is either large or can handle
- * more IO parallelism. Either way, we should try to process multiple
- * AGs at a time in such a configuration to try to saturate the
- * underlying storage and speed the repair process. Only do this if
- * prefetching is enabled.
- *
- * Given mkfs defaults for 16AGs for "multidisk" configurations, we want
- * to target these for an increase in thread count. Hence a stride value
- * of 15 is chosen to ensure we get at least 2 AGs being scanned at once
- * on such filesystems.
+ * Multidisk filesystems can handle more IO parallelism so we should try
+ * to process multiple AGs at a time in such a configuration to try to
+ * saturate the underlying storage and speed the repair process. Only do
+ * this if prefetching is enabled.
*/
- if (!ag_stride && glob_agcount >= 16 && do_prefetch)
- ag_stride = 15;
+ if (!ag_stride && do_prefetch && is_multidisk_filesystem(mp)) {
+ /*
+ * For small agcount multidisk systems, just double the
+ * parallelism. For larger AG count filesystems (32 and above)
+ * use more parallelism, and linearly increase the parallelism
+ * with the number of AGs.
+ */
+ ag_stride = glob_agcount;
+ ag_stride = min(glob_agcount, XFS_MULTIDISK_AGCOUNT / 2) - 1;
+ }
if (ag_stride) {
thread_count = (glob_agcount + ag_stride - 1) / ag_stride;
--
1.8.4.rc3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 02/15] mkfs: sanitise ftype parameter values.
2013-11-29 1:43 [RFC, PATCH 00/15] mkfs: sanitise input parameters Dave Chinner
2013-11-29 1:43 ` [PATCH 01/15] xfsprogs: use common code for multi-disk detection Dave Chinner
@ 2013-11-29 1:43 ` Dave Chinner
2013-12-02 10:40 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 03/15] mkfs: Sanitise the superblock feature macros Dave Chinner
` (12 subsequent siblings)
14 siblings, 1 reply; 42+ messages in thread
From: Dave Chinner @ 2013-11-29 1:43 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
Because passing "-n ftype=2" should fail.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
mkfs/xfs_mkfs.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index cc74535..3145205 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1550,12 +1550,15 @@ _("cannot specify both crc and ftype\n"));
reqval('n', nopts, N_FTYPE);
if (nftype)
respec('n', nopts, N_FTYPE);
- dirftype = atoi(value);
+ c = atoi(value);
+ if (c < 0 || c > 1)
+ illegal(value, "n ftype");
if (crcs_enabled) {
fprintf(stderr,
_("cannot specify both crc and ftype\n"));
usage();
}
+ dirftype = c;
nftype = 1;
break;
default:
--
1.8.4.rc3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 03/15] mkfs: Sanitise the superblock feature macros
2013-11-29 1:43 [RFC, PATCH 00/15] mkfs: sanitise input parameters Dave Chinner
2013-11-29 1:43 ` [PATCH 01/15] xfsprogs: use common code for multi-disk detection Dave Chinner
2013-11-29 1:43 ` [PATCH 02/15] mkfs: sanitise ftype parameter values Dave Chinner
@ 2013-11-29 1:43 ` Dave Chinner
2013-12-02 10:43 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 04/15] mkfs: validate all input values Dave Chinner
` (11 subsequent siblings)
14 siblings, 1 reply; 42+ messages in thread
From: Dave Chinner @ 2013-11-29 1:43 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
They are horrible macros that simply obfuscate the code, so
let's factor the code and make them nice functions.
To do this, add a sb_feat_args structure that carries around the
variables rather than a strange assortment of variables. This means
all the default can be clearly defined in a structure
initialisation, and dependent feature bits are easy to check.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
include/xfs_mkfs.h | 25 +-----
mkfs/xfs_mkfs.c | 227 ++++++++++++++++++++++++++++++++++-------------------
2 files changed, 146 insertions(+), 106 deletions(-)
diff --git a/include/xfs_mkfs.h b/include/xfs_mkfs.h
index 3388f6d..3af9cb1 100644
--- a/include/xfs_mkfs.h
+++ b/include/xfs_mkfs.h
@@ -23,36 +23,13 @@
XFS_SB_VERSION_EXTFLGBIT | \
XFS_SB_VERSION_DIRV2BIT)
-#define XFS_SB_VERSION_MKFS(crc,ia,dia,log2,attr1,sflag,ci,more) (\
- ((crc)||(ia)||(dia)||(log2)||(attr1)||(sflag)||(ci)||(more)) ? \
- (((crc) ? XFS_SB_VERSION_5 : XFS_SB_VERSION_4) | \
- ((ia) ? XFS_SB_VERSION_ALIGNBIT : 0) | \
- ((dia) ? XFS_SB_VERSION_DALIGNBIT : 0) | \
- ((log2) ? XFS_SB_VERSION_LOGV2BIT : 0) | \
- ((attr1) ? XFS_SB_VERSION_ATTRBIT : 0) | \
- ((sflag) ? XFS_SB_VERSION_SECTORBIT : 0) | \
- ((ci) ? XFS_SB_VERSION_BORGBIT : 0) | \
- ((more) ? XFS_SB_VERSION_MOREBITSBIT : 0) | \
- XFS_DFL_SB_VERSION_BITS | \
- 0 ) : XFS_SB_VERSION_1 )
-
-#define XFS_SB_VERSION2_MKFS(crc, lazycount, attr2, projid32bit, parent, \
- ftype) (\
- ((lazycount) ? XFS_SB_VERSION2_LAZYSBCOUNTBIT : 0) | \
- ((attr2) ? XFS_SB_VERSION2_ATTR2BIT : 0) | \
- ((projid32bit) ? XFS_SB_VERSION2_PROJID32BIT : 0) | \
- ((parent) ? XFS_SB_VERSION2_PARENTBIT : 0) | \
- ((crc) ? XFS_SB_VERSION2_CRCBIT : 0) | \
- ((ftype) ? XFS_SB_VERSION2_FTYPE : 0) | \
- 0 )
-
#define XFS_DFL_BLOCKSIZE_LOG 12 /* 4096 byte blocks */
#define XFS_DINODE_DFL_LOG 8 /* 256 byte inodes */
#define XFS_DINODE_DFL_CRC_LOG 9 /* 512 byte inodes for CRCs */
#define XFS_MIN_DATA_BLOCKS 100
#define XFS_MIN_INODE_PERBLOCK 2 /* min inodes per block */
#define XFS_DFL_IMAXIMUM_PCT 25 /* max % of space for inodes */
-#define XFS_IFLAG_ALIGN 1 /* -i align defaults on */
+#define XFS_IFLAG_ALIGN true /* -i align defaults on */
#define XFS_MIN_REC_DIRSIZE 12 /* 4096 byte dirblocks (V2) */
#define XFS_DFL_DIR_VERSION 2 /* default directory version */
#define XFS_DFL_LOG_SIZE 1000 /* default log size, blocks */
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 3145205..caa0631 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -862,6 +862,86 @@ discard_blocks(dev_t dev, __uint64_t nsectors)
platform_discard_blocks(fd, 0, nsectors << 9);
}
+struct sb_feat_args {
+ int log_version;
+ int attr_version;
+ int dir_version;
+ bool inode_align;
+ bool nci;
+ bool lazy_sb_counters;
+ bool projid16bit;
+ bool crcs_enabled;
+ bool dirftype;
+ bool parent_pointers;
+};
+
+static void
+sb_set_features(
+ struct xfs_sb *sbp,
+ struct sb_feat_args *fp,
+ int sectsize,
+ int lsectsize,
+ int dsunit)
+{
+
+ sbp->sb_versionnum = XFS_DFL_SB_VERSION_BITS;
+ if (fp->crcs_enabled)
+ sbp->sb_versionnum |= XFS_SB_VERSION_5;
+ else
+ sbp->sb_versionnum |= XFS_SB_VERSION_4;
+
+ if (fp->inode_align)
+ sbp->sb_versionnum |= XFS_SB_VERSION_ALIGNBIT;
+ if (dsunit)
+ sbp->sb_versionnum |= XFS_SB_VERSION_DALIGNBIT;
+ if (fp->log_version == 2)
+ sbp->sb_versionnum |= XFS_SB_VERSION_LOGV2BIT;
+ if (fp->attr_version == 1)
+ sbp->sb_versionnum |= XFS_SB_VERSION_ATTRBIT;
+ if (sectsize > BBSIZE || lsectsize > BBSIZE)
+ sbp->sb_versionnum |= XFS_SB_VERSION_SECTORBIT;
+ if (fp->nci)
+ sbp->sb_versionnum |= XFS_SB_VERSION_BORGBIT;
+
+
+ sbp->sb_features2 = 0;
+ if (fp->lazy_sb_counters)
+ sbp->sb_features2 |= XFS_SB_VERSION2_LAZYSBCOUNTBIT;
+ if (!fp->projid16bit)
+ sbp->sb_features2 |= XFS_SB_VERSION2_PROJID32BIT;
+ if (fp->parent_pointers)
+ sbp->sb_features2 |= XFS_SB_VERSION2_PARENTBIT;
+ if (fp->crcs_enabled)
+ sbp->sb_features2 |= XFS_SB_VERSION2_CRCBIT;
+ if (fp->attr_version == 2)
+ sbp->sb_features2 |= XFS_SB_VERSION2_ATTR2BIT;
+
+ /* v5 superblocks have their own feature bit for dirftype */
+ if (fp->dirftype && !fp->crcs_enabled)
+ sbp->sb_features2 |= XFS_SB_VERSION2_FTYPE;
+
+ /* update whether extended features are in use */
+ if (sbp->sb_features2 != 0)
+ sbp->sb_versionnum |= XFS_SB_VERSION_MOREBITSBIT;
+
+ /*
+ * Due to a structure alignment issue, sb_features2 ended up in one
+ * of two locations, the second "incorrect" location represented by
+ * the sb_bad_features2 field. To avoid older kernels mounting
+ * filesystems they shouldn't, set both field to the same value.
+ */
+ sbp->sb_bad_features2 = sbp->sb_features2;
+
+ if (!fp->crcs_enabled)
+ return;
+
+ /* default features for v5 filesystems */
+ sbp->sb_features_compat = 0;
+ sbp->sb_features_ro_compat = 0;
+ sbp->sb_features_incompat = XFS_SB_FEAT_INCOMPAT_FTYPE;
+ sbp->sb_features_log_incompat = 0;
+}
+
int
main(
int argc,
@@ -873,8 +953,6 @@ main(
xfs_agnumber_t agno;
__uint64_t agsize;
xfs_alloc_rec_t *arec;
- int attrversion;
- int projid16bit;
struct xfs_btree_block *block;
int blflag;
int blocklog;
@@ -889,8 +967,6 @@ main(
char *dfile;
int dirblocklog;
int dirblocksize;
- int dirftype;
- int dirversion;
char *dsize;
int dsu;
int dsw;
@@ -898,7 +974,6 @@ main(
int dswidth;
int force_overwrite;
struct fsxattr fsx;
- int iaflag;
int ilflag;
int imaxpct;
int imflag;
@@ -918,7 +993,6 @@ main(
int loginternal;
char *logsize;
xfs_dfsbno_t logstart;
- int logversion;
int lvflag;
int lsflag;
int lsectorlog;
@@ -938,7 +1012,6 @@ main(
int nftype;
int nsflag;
int nvflag;
- int nci;
int Nflag;
int discard = 1;
char *p;
@@ -962,16 +1035,24 @@ main(
int worst_freelist;
libxfs_init_t xi;
struct fs_topology ft;
- int lazy_sb_counters;
- int crcs_enabled;
+ struct sb_feat_args sb_feat = {
+ .log_version = 2,
+ .attr_version = 2,
+ .dir_version = XFS_DFL_DIR_VERSION,
+ .inode_align = XFS_IFLAG_ALIGN,
+ .nci = false,
+ .lazy_sb_counters = true,
+ .projid16bit = false,
+ .crcs_enabled = false,
+ .dirftype = false,
+ .parent_pointers = false,
+ };
progname = basename(argv[0]);
setlocale(LC_ALL, "");
bindtextdomain(PACKAGE, LOCALEDIR);
textdomain(PACKAGE);
- attrversion = 2;
- projid16bit = 0;
blflag = bsflag = slflag = ssflag = lslflag = lssflag = 0;
blocklog = blocksize = 0;
sectorlog = lsectorlog = XFS_MIN_SECTORSIZE_LOG;
@@ -980,23 +1061,18 @@ main(
ilflag = imflag = ipflag = isflag = 0;
liflag = laflag = lsflag = ldflag = lvflag = 0;
loginternal = 1;
- logversion = 2;
logagno = logblocks = rtblocks = rtextblocks = 0;
- Nflag = nlflag = nsflag = nvflag = nci = 0;
- nftype = dirftype = 0; /* inode type information in the dir */
+ Nflag = nlflag = nsflag = nvflag = 0;
+ nftype = 0;
dirblocklog = dirblocksize = 0;
- dirversion = XFS_DFL_DIR_VERSION;
qflag = 0;
imaxpct = inodelog = inopblock = isize = 0;
- iaflag = XFS_IFLAG_ALIGN;
dfile = logfile = rtfile = NULL;
dsize = logsize = rtsize = rtextsize = protofile = NULL;
dsu = dsw = dsunit = dswidth = lalign = lsu = lsunit = 0;
nodsflag = norsflag = 0;
force_overwrite = 0;
worst_freelist = 0;
- lazy_sb_counters = 1;
- crcs_enabled = 0;
memset(&fsx, 0, sizeof(fsx));
memset(&xi, 0, sizeof(xi));
@@ -1235,10 +1311,11 @@ main(
switch (getsubopt(&p, (constpp)iopts, &value)) {
case I_ALIGN:
if (!value || *value == '\0')
- value = "1";
- iaflag = atoi(value);
- if (iaflag < 0 || iaflag > 1)
+ break;
+ c = atoi(value);
+ if (c < 0 || c > 1)
illegal(value, "i align");
+ sb_feat.inode_align = c ? true : false;
break;
case I_LOG:
if (!value || *value == '\0')
@@ -1308,7 +1385,7 @@ main(
c = atoi(value);
if (c < 0 || c > 2)
illegal(value, "i attr");
- attrversion = c;
+ sb_feat.attr_version = c;
break;
case I_PROJID32BIT:
if (!value || *value == '\0')
@@ -1316,7 +1393,7 @@ main(
c = atoi(value);
if (c < 0 || c > 1)
illegal(value, "i projid32bit");
- projid16bit = c ? 0 : 1;
+ sb_feat.projid16bit = c ? false : true;
break;
default:
unknown('i', value);
@@ -1406,9 +1483,10 @@ main(
reqval('l', lopts, L_VERSION);
if (lvflag)
respec('l', lopts, L_VERSION);
- logversion = atoi(value);
- if (logversion < 1 || logversion > 2)
+ c = atoi(value);
+ if (c < 1 || c > 2)
illegal(value, "l version");
+ sb_feat.log_version = c;
lvflag = 1;
break;
case L_SIZE:
@@ -1457,7 +1535,8 @@ main(
c = atoi(value);
if (c < 0 || c > 1)
illegal(value, "l lazy-count");
- lazy_sb_counters = c;
+ sb_feat.lazy_sb_counters = c ? true
+ : false;
break;
default:
unknown('l', value);
@@ -1481,12 +1560,14 @@ main(
c = atoi(value);
if (c < 0 || c > 1)
illegal(value, "m crc");
- crcs_enabled = c;
- if (nftype && crcs_enabled) {
+ if (c && nftype) {
fprintf(stderr,
_("cannot specify both crc and ftype\n"));
usage();
}
+ sb_feat.crcs_enabled = c ? true : false;
+ if (c)
+ sb_feat.dirftype = true;
break;
default:
unknown('m', value);
@@ -1536,12 +1617,14 @@ _("cannot specify both crc and ftype\n"));
if (nvflag)
respec('n', nopts, N_VERSION);
if (!strcasecmp(value, "ci")) {
- nci = 1; /* ASCII CI mode */
+ /* ASCII CI mode */
+ sb_feat.nci = true;
} else {
- dirversion = atoi(value);
- if (dirversion != 2)
+ c = atoi(value);
+ if (c != 2)
illegal(value,
"n version");
+ sb_feat.dir_version = c;
}
nvflag = 1;
break;
@@ -1553,12 +1636,12 @@ _("cannot specify both crc and ftype\n"));
c = atoi(value);
if (c < 0 || c > 1)
illegal(value, "n ftype");
- if (crcs_enabled) {
+ if (sb_feat.crcs_enabled) {
fprintf(stderr,
_("cannot specify both crc and ftype\n"));
usage();
}
- dirftype = c;
+ sb_feat.dirftype = c ? true : false;
nftype = 1;
break;
default:
@@ -1778,7 +1861,7 @@ _("block size %d cannot be smaller than logical sector size %d\n"),
usage();
} else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
lsu = blocksize;
- logversion = 2;
+ sb_feat.log_version = 2;
}
/*
@@ -1786,7 +1869,7 @@ _("block size %d cannot be smaller than logical sector size %d\n"),
* no longer optional for CRC enabled filesystems. Catch them up front
* here before doing anything else.
*/
- if (crcs_enabled) {
+ if (sb_feat.crcs_enabled) {
/* minimum inode size is 512 bytes, ipflag checked later */
if ((isflag || ilflag) && inodelog < XFS_DINODE_DFL_CRC_LOG) {
fprintf(stderr,
@@ -1796,28 +1879,28 @@ _("Minimum inode size for CRCs is %d bytes\n"),
}
/* inodes always aligned */
- if (iaflag != 1) {
+ if (!sb_feat.inode_align) {
fprintf(stderr,
_("Inodes always aligned for CRC enabled filesytems\n"));
usage();
}
/* lazy sb counters always on */
- if (lazy_sb_counters != 1) {
+ if (!sb_feat.lazy_sb_counters) {
fprintf(stderr,
_("Lazy superblock counted always enabled for CRC enabled filesytems\n"));
usage();
}
/* version 2 logs always on */
- if (logversion != 2) {
+ if (sb_feat.log_version != 2) {
fprintf(stderr,
_("V2 logs always enabled for CRC enabled filesytems\n"));
usage();
}
/* attr2 always on */
- if (attrversion != 2) {
+ if (sb_feat.attr_version != 2) {
fprintf(stderr,
_("V2 attribute format always enabled on CRC enabled filesytems\n"));
usage();
@@ -1825,7 +1908,7 @@ _("V2 attribute format always enabled on CRC enabled filesytems\n"));
/* 32 bit project quota always on */
/* attr2 always on */
- if (projid16bit == 1) {
+ if (sb_feat.projid16bit) {
fprintf(stderr,
_("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
usage();
@@ -1879,11 +1962,11 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
inodelog = blocklog - libxfs_highbit32(inopblock);
isize = 1 << inodelog;
} else if (!ilflag && !isflag) {
- inodelog = crcs_enabled ? XFS_DINODE_DFL_CRC_LOG
- : XFS_DINODE_DFL_LOG;
+ inodelog = sb_feat.crcs_enabled ? XFS_DINODE_DFL_CRC_LOG
+ : XFS_DINODE_DFL_LOG;
isize = 1 << inodelog;
}
- if (crcs_enabled && inodelog < XFS_DINODE_DFL_CRC_LOG) {
+ if (sb_feat.crcs_enabled && inodelog < XFS_DINODE_DFL_CRC_LOG) {
fprintf(stderr,
_("Minimum inode size for CRCs is %d bytes\n"),
1 << XFS_DINODE_DFL_CRC_LOG);
@@ -2015,10 +2098,10 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
}
/* if lsu or lsunit was specified, automatically use v2 logs */
- if ((lsu || lsunit) && logversion == 1) {
+ if ((lsu || lsunit) && sb_feat.log_version == 1) {
fprintf(stderr,
_("log stripe unit specified, using v2 logs\n"));
- logversion = 2;
+ sb_feat.log_version = 2;
}
calc_stripe_factors(dsu, dsw, sectorsize, lsu, lsectorsize,
@@ -2333,12 +2416,12 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
}
/* convert from 512 byte blocks to fs blocks */
lsunit = DTOBT(lsunit);
- } else if (logversion == 2 && loginternal && dsunit) {
+ } else if (sb_feat.log_version == 2 && loginternal && dsunit) {
/* lsunit and dsunit now in fs blocks */
lsunit = dsunit;
}
- if (logversion == 2 && (lsunit * blocksize) > 256 * 1024) {
+ if (sb_feat.log_version == 2 && (lsunit * blocksize) > 256 * 1024) {
fprintf(stderr,
_("log stripe unit (%d bytes) is too large (maximum is 256KiB)\n"),
(lsunit * blocksize));
@@ -2346,9 +2429,9 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
fprintf(stderr, _("log stripe unit adjusted to 32KiB\n"));
}
- min_logblocks = max_trans_res(crcs_enabled, dirversion,
+ min_logblocks = max_trans_res(sb_feat.crcs_enabled, sb_feat.dir_version,
sectorlog, blocklog, inodelog, dirblocklog,
- logversion, lsunit);
+ sb_feat.log_version, lsunit);
ASSERT(min_logblocks);
min_logblocks = MAX(XFS_MIN_LOG_BLOCKS, min_logblocks);
if (!logsize && dblocks >= (1024*1024*1024) >> blocklog)
@@ -2462,14 +2545,6 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
}
validate_log_size(logblocks, blocklog, min_logblocks);
- /*
- * dirent filetype field always enabled on v5 superblocks
- */
- if (crcs_enabled) {
- sbp->sb_features_incompat = XFS_SB_FEAT_INCOMPAT_FTYPE;
- dirftype = 1;
- }
-
if (!qflag || Nflag) {
printf(_(
"meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
@@ -2482,13 +2557,16 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
" =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
"realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
dfile, isize, (long long)agcount, (long long)agsize,
- "", sectorsize, attrversion, !projid16bit,
- "", crcs_enabled,
+ "", sectorsize, sb_feat.attr_version,
+ !sb_feat.projid16bit,
+ "", sb_feat.crcs_enabled,
"", blocksize, (long long)dblocks, imaxpct,
"", dsunit, dswidth,
- dirversion, dirblocksize, nci, dirftype,
+ sb_feat.dir_version, dirblocksize, sb_feat.nci,
+ sb_feat.dirftype,
logfile, 1 << blocklog, (long long)logblocks,
- logversion, "", lsectorsize, lsunit, lazy_sb_counters,
+ sb_feat.log_version, "", lsectorsize, lsunit,
+ sb_feat.lazy_sb_counters,
rtfile, rtextblocks << blocklog,
(long long)rtblocks, (long long)rtextents);
if (Nflag)
@@ -2531,17 +2609,17 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
sbp->sb_unit = dsunit;
sbp->sb_width = dswidth;
sbp->sb_dirblklog = dirblocklog - blocklog;
- if (logversion == 2) { /* This is stored in bytes */
+ if (sb_feat.log_version == 2) { /* This is stored in bytes */
lsunit = (lsunit == 0) ? 1 : XFS_FSB_TO_B(mp, lsunit);
sbp->sb_logsunit = lsunit;
} else
sbp->sb_logsunit = 0;
- if (iaflag) {
+ if (sb_feat.inode_align) {
int cluster_size = XFS_INODE_BIG_CLUSTER_SIZE;
- if (crcs_enabled)
+ if (sb_feat.crcs_enabled)
cluster_size *= isize / XFS_DINODE_MIN_SIZE;
sbp->sb_inoalignmt = cluster_size >> blocklog;
- iaflag = sbp->sb_inoalignmt != 0;
+ sb_feat.inode_align = sbp->sb_inoalignmt != 0;
} else
sbp->sb_inoalignmt = 0;
if (lsectorsize != BBSIZE || sectorsize != BBSIZE) {
@@ -2552,22 +2630,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
sbp->sb_logsectsize = 0;
}
- sbp->sb_features2 = XFS_SB_VERSION2_MKFS(crcs_enabled, lazy_sb_counters,
- attrversion == 2, !projid16bit, 0,
- (!crcs_enabled && dirftype));
- sbp->sb_versionnum = XFS_SB_VERSION_MKFS(crcs_enabled, iaflag,
- dsunit != 0,
- logversion == 2, attrversion == 1,
- (sectorsize != BBSIZE ||
- lsectorsize != BBSIZE),
- nci, sbp->sb_features2 != 0);
- /*
- * Due to a structure alignment issue, sb_features2 ended up in one
- * of two locations, the second "incorrect" location represented by
- * the sb_bad_features2 field. To avoid older kernels mounting
- * filesystems they shouldn't, set both field to the same value.
- */
- sbp->sb_bad_features2 = sbp->sb_features2;
+ sb_set_features(&mp->m_sb, &sb_feat, sectorsize, lsectorsize, dsunit);
if (force_overwrite)
zero_old_xfs_structures(&xi, sbp);
@@ -2623,7 +2686,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
libxfs_log_clear(mp->m_logdev_targp,
XFS_FSB_TO_DADDR(mp, logstart),
(xfs_extlen_t)XFS_FSB_TO_BB(mp, logblocks),
- &sbp->sb_uuid, logversion, lsunit, XLOG_FMT);
+ &sbp->sb_uuid, sb_feat.log_version, lsunit, XLOG_FMT);
mp = libxfs_mount(mp, sbp, xi.ddev, xi.logdev, xi.rtdev, 0);
if (mp == NULL) {
--
1.8.4.rc3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 04/15] mkfs: validate all input values
2013-11-29 1:43 [RFC, PATCH 00/15] mkfs: sanitise input parameters Dave Chinner
` (2 preceding siblings ...)
2013-11-29 1:43 ` [PATCH 03/15] mkfs: Sanitise the superblock feature macros Dave Chinner
@ 2013-11-29 1:43 ` Dave Chinner
2013-12-02 17:04 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 05/15] mkfs: factor boolean option parsing Dave Chinner
` (10 subsequent siblings)
14 siblings, 1 reply; 42+ messages in thread
From: Dave Chinner @ 2013-11-29 1:43 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
Right now, mkfs does a poor job of input validation of values. Many
parameters do not check for trailing garbage and so will pass
obviously invalid values as OK. Some don't even detect completely
invalid values, leaving it for other checks later on to fail due to
a bad value conversion - these tend to rely on atoi() implicitly
returning a sane value when it is passed garbage, and atoi gives no
guarantee of the return value when passed garbage.
Clean all this up by passing all strings that need to be converted
into values into a common function that is called regardless of
whether unit conversion is needed or not. Further, make sure every
conversion is checked for a valid result, and abort the moment an
invalid value is detected.
Get rid of the silly "isdigits(), cvtnum()" calls which don't use
any of the conversion capabilities of cvtnum() because we've already
ensured that there are no conversion units in the string via the
isdigits() call. These can simply be replaced by a standard
strtoll() call followed by checking for no trailing bytes.
Finally, the block size of the filesystem is not known until all
the options have been parsed and we can determine if the default is
to be used. This means any parameter that relies on using conversion
from filesystem block size (the "NNNb" format) requires the block
size to first be specified on the command line so it is known.
Similarly, we make the same rule for specifying counts in sectors.
This is a change from the existing behaviour that assumes sectors
are 512 bytes unless otherwise changed on the command line. This,
unfortunately, leads to complete silliness where you can specify the
sector size as a count of sectors. It also means that you can do
some conversions with 512 byte sector sizes, and others with
whatever was specified on the command line, meaning the mkfs
behaviour changes depending in where in the command line the sector
size is changed....
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
include/xfs_mkfs.h | 7 +-
man/man8/mkfs.xfs.8 | 26 ++++++-
mkfs/proto.c | 36 ++++-----
mkfs/xfs_mkfs.c | 215 +++++++++++++++++++++++++++-------------------------
4 files changed, 153 insertions(+), 131 deletions(-)
diff --git a/include/xfs_mkfs.h b/include/xfs_mkfs.h
index 3af9cb1..996b690 100644
--- a/include/xfs_mkfs.h
+++ b/include/xfs_mkfs.h
@@ -56,11 +56,8 @@
#define XFS_NOMULTIDISK_AGLOG 2 /* 4 AGs */
#define XFS_MULTIDISK_AGCOUNT (1 << XFS_MULTIDISK_AGLOG)
-
-/* xfs_mkfs.c */
-extern int isdigits (char *str);
-extern long long cvtnum (unsigned int blocksize,
- unsigned int sectorsize, char *s);
+extern long long getnum(const char *str, unsigned int blocksize,
+ unsigned int sectorsize, bool convert);
/* proto.c */
extern char *setup_proto (char *fname);
diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
index 8184e10..b9c2c13 100644
--- a/man/man8/mkfs.xfs.8
+++ b/man/man8/mkfs.xfs.8
@@ -61,11 +61,11 @@ SCSI disk, use:
.PP
The metadata log can be placed on another device to reduce the number
of disk seeks. To create a filesystem on the first partition on the
-first SCSI disk with a 10000 block log located on the first partition
+first SCSI disk with a 10MiB log located on the first partition
on the second SCSI disk, use:
.RS
.HP
-.B mkfs.xfs\ \-l\ logdev=/dev/sdb1,size=10000b /dev/sda1
+.B mkfs.xfs\ \-l\ logdev=/dev/sdb1,size=10m /dev/sda1
.RE
.PP
Each of the
@@ -75,9 +75,9 @@ suboptions if multiple suboptions apply to the same option.
Equivalently, each main option can be given multiple times with
different suboptions.
For example,
-.B \-l internal,size=10000b
+.B \-l internal,size=10m
and
-.B \-l internal \-l size=10000b
+.B \-l internal \-l size=10m
are equivalent.
.PP
In the descriptions below, sizes are given in sectors, bytes, blocks,
@@ -106,6 +106,15 @@ option below).
.HP
.BR e "\ \-\ multiply by one exabyte (1,048,576 terabytes)."
.PD
+.RE
+.PP
+When specifying parameters in units of sectors or filesystem blocks, the
+.B \-s
+option or the
+.B \-b
+option first needs to be added to the command line.
+Failure to specify the size of the units will result in illegal value errors
+when parameters are quantified in those units.
.SH OPTIONS
.TP
.BI \-b " block_size_options"
@@ -123,6 +132,11 @@ or in bytes with
.BR size= .
The default value is 4096 bytes (4 KiB), the minimum is 512, and the
maximum is 65536 (64 KiB).
+.IP
+To specify any options on the command line in units of filesystem blocks, this
+option must be specified first so that the filesystem block size is
+applied consistently to all options.
+.IP
XFS on Linux currently only supports pagesize or smaller blocks.
.TP
.BI \-d " data_section_options"
@@ -741,6 +755,10 @@ is 512 bytes. The minimum value for sector size is
.I sector_size
must be a power of 2 size and cannot be made larger than the
filesystem block size.
+.IP
+To specify any options on the command line in units of sectors, this
+option must be specified first so that the sector size is
+applied consistently to all options.
.TP
.BI \-L " label"
Set the filesystem
diff --git a/mkfs/proto.c b/mkfs/proto.c
index 4cc0df6..da5115b 100644
--- a/mkfs/proto.c
+++ b/mkfs/proto.c
@@ -23,7 +23,6 @@
/*
* Prototypes for internal functions.
*/
-static long getnum(char **pp);
static char *getstr(char **pp);
static void fail(char *msg, int i);
static void getres(xfs_trans_t *tp, uint blocks);
@@ -77,21 +76,11 @@ setup_proto(
* Skip past the stuff there for compatibility, a string and 2 numbers.
*/
(void)getstr(&buf); /* boot image name */
- (void)getnum(&buf); /* block count */
- (void)getnum(&buf); /* inode count */
+ (void)getnum(getstr(&buf), 0, 0, false); /* block count */
+ (void)getnum(getstr(&buf), 0, 0, false); /* inode count */
return buf;
}
-static long
-getnum(
- char **pp)
-{
- char *s;
-
- s = getstr(pp);
- return atol(s);
-}
-
static void
fail(
char *msg,
@@ -434,8 +423,8 @@ parseproto(
val = val * 8 + mstr[i] - '0';
}
mode |= val;
- creds.cr_uid = (int)getnum(pp);
- creds.cr_gid = (int)getnum(pp);
+ creds.cr_uid = getnum(getstr(pp), 0, 0, false);
+ creds.cr_gid = getnum(getstr(pp), 0, 0, false);
xname.name = (uchar_t *)name;
xname.len = name ? strlen(name) : 0;
tp = libxfs_trans_alloc(mp, 0);
@@ -459,7 +448,14 @@ parseproto(
case IF_RESERVED: /* pre-allocated space only */
value = getstr(pp);
- llen = cvtnum(mp->m_sb.sb_blocksize, mp->m_sb.sb_sectsize, value);
+ llen = getnum(value, mp->m_sb.sb_blocksize,
+ mp->m_sb.sb_sectsize, true);
+ if (llen < 0) {
+ fprintf(stderr,
+ _("%s: Bad value %s for proto file %s\n"),
+ progname, value, name);
+ exit(1);
+ }
getres(tp, XFS_B_TO_FSB(mp, llen));
error = libxfs_inode_alloc(&tp, pip, mode|S_IFREG, 1, 0,
@@ -482,8 +478,8 @@ parseproto(
case IF_BLOCK:
getres(tp, 0);
- majdev = (int)getnum(pp);
- mindev = (int)getnum(pp);
+ majdev = getnum(getstr(pp), 0, 0, false);
+ mindev = getnum(getstr(pp), 0, 0, false);
error = libxfs_inode_alloc(&tp, pip, mode|S_IFBLK, 1,
IRIX_MKDEV(majdev, mindev), &creds, fsxp, &ip);
if (error) {
@@ -497,8 +493,8 @@ parseproto(
case IF_CHAR:
getres(tp, 0);
- majdev = (int)getnum(pp);
- mindev = (int)getnum(pp);
+ majdev = getnum(getstr(pp), 0, 0, false);
+ mindev = getnum(getstr(pp), 0, 0, false);
error = libxfs_inode_alloc(&tp, pip, mode|S_IFCHR, 1,
IRIX_MKDEV(majdev, mindev), &creds, fsxp, &ip);
if (error)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index caa0631..1cc2bcb 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -49,6 +49,9 @@ static void respec(char opt, char *tab[], int idx);
static void unknown(char opt, char *s);
static int ispow2(unsigned int i);
+static long long cvtnum(unsigned int blocksize,
+ unsigned int sectorsize, const char *s);
+
/*
* option tables for getsubopt calls
*/
@@ -942,6 +945,28 @@ sb_set_features(
sbp->sb_features_log_incompat = 0;
}
+long long
+getnum(
+ const char *str,
+ unsigned int blocksize,
+ unsigned int sectorsize,
+ bool convert)
+{
+ long long i;
+ char *sp;
+
+ if (convert)
+ return cvtnum(blocksize, sectorsize, str);
+
+ i = strtoll(str, &sp, 0);
+ if (i == 0 && sp == str)
+ return -1LL;
+ if (*sp != '\0')
+ return -1LL; /* trailing garbage */
+ return i;
+}
+
+
int
main(
int argc,
@@ -1055,8 +1080,8 @@ main(
blflag = bsflag = slflag = ssflag = lslflag = lssflag = 0;
blocklog = blocksize = 0;
- sectorlog = lsectorlog = XFS_MIN_SECTORSIZE_LOG;
- sectorsize = lsectorsize = XFS_MIN_SECTORSIZE;
+ sectorlog = lsectorlog = 0;
+ sectorsize = lsectorsize = 0;
agsize = daflag = dasize = dblocks = 0;
ilflag = imflag = ipflag = isflag = 0;
liflag = laflag = lsflag = ldflag = lvflag = 0;
@@ -1099,7 +1124,7 @@ main(
if (bsflag)
conflict('b', bopts, B_SIZE,
B_LOG);
- blocklog = atoi(value);
+ blocklog = getnum(value, 0, 0, false);
if (blocklog <= 0)
illegal(value, "b log");
blocksize = 1 << blocklog;
@@ -1113,8 +1138,8 @@ main(
if (blflag)
conflict('b', bopts, B_LOG,
B_SIZE);
- blocksize = cvtnum(
- blocksize, sectorsize, value);
+ blocksize = getnum(value, blocksize,
+ sectorsize, true);
if (blocksize <= 0 ||
!ispow2(blocksize))
illegal(value, "b size");
@@ -1137,8 +1162,7 @@ main(
reqval('d', dopts, D_AGCOUNT);
if (daflag)
respec('d', dopts, D_AGCOUNT);
- agcount = (__uint64_t)
- strtoul(value, NULL, 10);
+ agcount = getnum(value, 0, 0, false);
if ((__int64_t)agcount <= 0)
illegal(value, "d agcount");
daflag = 1;
@@ -1148,14 +1172,16 @@ main(
reqval('d', dopts, D_AGSIZE);
if (dasize)
respec('d', dopts, D_AGSIZE);
- agsize = cvtnum(
- blocksize, sectorsize, value);
+ agsize = getnum(value, blocksize,
+ sectorsize, true);
+ if ((__int64_t)agsize <= 0)
+ illegal(value, "d agsize");
dasize = 1;
break;
case D_FILE:
if (!value || *value == '\0')
value = "1";
- xi.disfile = atoi(value);
+ xi.disfile = getnum(value, 0, 0, false);
if (xi.disfile < 0 || xi.disfile > 1)
illegal(value, "d file");
if (xi.disfile && !Nflag)
@@ -1183,13 +1209,9 @@ main(
if (nodsflag)
conflict('d', dopts, D_NOALIGN,
D_SUNIT);
- if (!isdigits(value)) {
- fprintf(stderr,
- _("%s: Specify data sunit in 512-byte blocks, no unit suffix\n"),
- progname);
- exit(1);
- }
- dsunit = cvtnum(0, 0, value);
+ dsunit = getnum(value, 0, 0, false);
+ if (dsunit < 0)
+ illegal(value, "d sunit");
break;
case D_SWIDTH:
if (!value || *value == '\0')
@@ -1199,13 +1221,9 @@ main(
if (nodsflag)
conflict('d', dopts, D_NOALIGN,
D_SWIDTH);
- if (!isdigits(value)) {
- fprintf(stderr,
- _("%s: Specify data swidth in 512-byte blocks, no unit suffix\n"),
- progname);
- exit(1);
- }
- dswidth = cvtnum(0, 0, value);
+ dswidth = getnum(value, 0, 0, false);
+ if (dswidth < 0)
+ illegal(value, "d swidth");
break;
case D_SU:
if (!value || *value == '\0')
@@ -1215,8 +1233,10 @@ main(
if (nodsflag)
conflict('d', dopts, D_NOALIGN,
D_SU);
- dsu = cvtnum(
- blocksize, sectorsize, value);
+ dsu = getnum(value, blocksize,
+ sectorsize, true);
+ if (dsu < 0)
+ illegal(value, "d su");
break;
case D_SW:
if (!value || *value == '\0')
@@ -1226,13 +1246,9 @@ main(
if (nodsflag)
conflict('d', dopts, D_NOALIGN,
D_SW);
- if (!isdigits(value)) {
- fprintf(stderr,
- _("%s: Specify data sw as multiple of su, no unit suffix\n"),
- progname);
- exit(1);
- }
- dsw = cvtnum(0, 0, value);
+ dsw = getnum(value, 0, 0, false);
+ if (dsw < 0)
+ illegal(value, "d sw");
break;
case D_NOALIGN:
if (dsu)
@@ -1257,7 +1273,7 @@ main(
if (ssflag)
conflict('d', dopts, D_SECTSIZE,
D_SECTLOG);
- sectorlog = atoi(value);
+ sectorlog = getnum(value, 0, 0, false);
if (sectorlog <= 0)
illegal(value, "d sectlog");
sectorsize = 1 << sectorlog;
@@ -1271,8 +1287,8 @@ main(
if (slflag)
conflict('d', dopts, D_SECTLOG,
D_SECTSIZE);
- sectorsize = cvtnum(
- blocksize, sectorsize, value);
+ sectorsize = getnum(value, blocksize,
+ sectorsize, true);
if (sectorsize <= 0 ||
!ispow2(sectorsize))
illegal(value, "d sectsize");
@@ -1312,7 +1328,7 @@ main(
case I_ALIGN:
if (!value || *value == '\0')
break;
- c = atoi(value);
+ c = getnum(value, 0, 0, false);
if (c < 0 || c > 1)
illegal(value, "i align");
sb_feat.inode_align = c ? true : false;
@@ -1328,7 +1344,7 @@ main(
if (isflag)
conflict('i', iopts, I_SIZE,
I_LOG);
- inodelog = atoi(value);
+ inodelog = getnum(value, 0, 0, false);
if (inodelog <= 0)
illegal(value, "i log");
isize = 1 << inodelog;
@@ -1339,7 +1355,7 @@ main(
reqval('i', iopts, I_MAXPCT);
if (imflag)
respec('i', iopts, I_MAXPCT);
- imaxpct = atoi(value);
+ imaxpct = getnum(value, 0, 0, false);
if (imaxpct < 0 || imaxpct > 100)
illegal(value, "i maxpct");
imflag = 1;
@@ -1355,7 +1371,7 @@ main(
if (isflag)
conflict('i', iopts, I_SIZE,
I_PERBLOCK);
- inopblock = atoi(value);
+ inopblock = getnum(value, 0, 0, false);
if (inopblock <
XFS_MIN_INODE_PERBLOCK ||
!ispow2(inopblock))
@@ -1373,7 +1389,7 @@ main(
I_SIZE);
if (isflag)
respec('i', iopts, I_SIZE);
- isize = cvtnum(0, 0, value);
+ isize = getnum(value, 0, 0, true);
if (isize <= 0 || !ispow2(isize))
illegal(value, "i size");
inodelog = libxfs_highbit32(isize);
@@ -1382,7 +1398,7 @@ main(
case I_ATTR:
if (!value || *value == '\0')
reqval('i', iopts, I_ATTR);
- c = atoi(value);
+ c = getnum(value, 0, 0, false);
if (c < 0 || c > 2)
illegal(value, "i attr");
sb_feat.attr_version = c;
@@ -1390,7 +1406,7 @@ main(
case I_PROJID32BIT:
if (!value || *value == '\0')
value = "0";
- c = atoi(value);
+ c = getnum(value, 0, 0, false);
if (c < 0 || c > 1)
illegal(value, "i projid32bit");
sb_feat.projid16bit = c ? false : true;
@@ -1413,7 +1429,9 @@ main(
respec('l', lopts, L_AGNUM);
if (ldflag)
conflict('l', lopts, L_AGNUM, L_DEV);
- logagno = atoi(value);
+ logagno = getnum(value, 0, 0, false);
+ if (logagno < 0)
+ illegal(value, "l agno");
laflag = 1;
break;
case L_FILE:
@@ -1422,7 +1440,7 @@ main(
if (loginternal)
conflict('l', lopts, L_INTERNAL,
L_FILE);
- xi.lisfile = atoi(value);
+ xi.lisfile = getnum(value, 0, 0, false);
if (xi.lisfile < 0 || xi.lisfile > 1)
illegal(value, "l file");
if (xi.lisfile)
@@ -1438,7 +1456,7 @@ main(
L_INTERNAL);
if (liflag)
respec('l', lopts, L_INTERNAL);
- loginternal = atoi(value);
+ loginternal = getnum(value, 0, 0, false);
if (loginternal < 0 || loginternal > 1)
illegal(value, "l internal");
liflag = 1;
@@ -1448,20 +1466,19 @@ main(
reqval('l', lopts, L_SU);
if (lsu)
respec('l', lopts, L_SU);
- lsu = cvtnum(
- blocksize, sectorsize, value);
+ lsu = getnum(value, blocksize,
+ sectorsize, true);
+ if (lsu < 0)
+ illegal(value, "l su");
break;
case L_SUNIT:
if (!value || *value == '\0')
reqval('l', lopts, L_SUNIT);
if (lsunit)
respec('l', lopts, L_SUNIT);
- if (!isdigits(value)) {
- fprintf(stderr,
- _("Specify log sunit in 512-byte blocks, no size suffix\n"));
- usage();
- }
- lsunit = cvtnum(0, 0, value);
+ lsunit = getnum(value, 0, 0, false);
+ if (lsunit < 0)
+ illegal(value, "l sunit");
break;
case L_NAME:
case L_DEV:
@@ -1483,7 +1500,7 @@ main(
reqval('l', lopts, L_VERSION);
if (lvflag)
respec('l', lopts, L_VERSION);
- c = atoi(value);
+ c = getnum(value, 0, 0, false);
if (c < 1 || c > 2)
illegal(value, "l version");
sb_feat.log_version = c;
@@ -1505,7 +1522,7 @@ main(
if (lssflag)
conflict('l', lopts, L_SECTSIZE,
L_SECTLOG);
- lsectorlog = atoi(value);
+ lsectorlog = getnum(value, 0, 0, false);
if (lsectorlog <= 0)
illegal(value, "l sectlog");
lsectorsize = 1 << lsectorlog;
@@ -1519,8 +1536,8 @@ main(
if (lslflag)
conflict('l', lopts, L_SECTLOG,
L_SECTSIZE);
- lsectorsize = cvtnum(
- blocksize, sectorsize, value);
+ lsectorsize = getnum(value, blocksize,
+ sectorsize, true);
if (lsectorsize <= 0 ||
!ispow2(lsectorsize))
illegal(value, "l sectsize");
@@ -1532,7 +1549,7 @@ main(
if (!value || *value == '\0')
reqval('l', lopts,
L_LAZYSBCNTR);
- c = atoi(value);
+ c = getnum(value, 0, 0, false);
if (c < 0 || c > 1)
illegal(value, "l lazy-count");
sb_feat.lazy_sb_counters = c ? true
@@ -1557,7 +1574,7 @@ main(
case M_CRC:
if (!value || *value == '\0')
reqval('m', mopts, M_CRC);
- c = atoi(value);
+ c = getnum(value, 0, 0, false);
if (c < 0 || c > 1)
illegal(value, "m crc");
if (c && nftype) {
@@ -1588,7 +1605,7 @@ _("cannot specify both crc and ftype\n"));
if (nsflag)
conflict('n', nopts, N_SIZE,
N_LOG);
- dirblocklog = atoi(value);
+ dirblocklog = getnum(value, 0, 0, false);
if (dirblocklog <= 0)
illegal(value, "n log");
dirblocksize = 1 << dirblocklog;
@@ -1602,8 +1619,8 @@ _("cannot specify both crc and ftype\n"));
if (nlflag)
conflict('n', nopts, N_LOG,
N_SIZE);
- dirblocksize = cvtnum(
- blocksize, sectorsize, value);
+ dirblocksize = getnum(value, blocksize,
+ sectorsize, true);
if (dirblocksize <= 0 ||
!ispow2(dirblocksize))
illegal(value, "n size");
@@ -1620,7 +1637,7 @@ _("cannot specify both crc and ftype\n"));
/* ASCII CI mode */
sb_feat.nci = true;
} else {
- c = atoi(value);
+ c = getnum(value, 0, 0, false);
if (c != 2)
illegal(value,
"n version");
@@ -1633,7 +1650,7 @@ _("cannot specify both crc and ftype\n"));
reqval('n', nopts, N_FTYPE);
if (nftype)
respec('n', nopts, N_FTYPE);
- c = atoi(value);
+ c = getnum(value, 0, 0, false);
if (c < 0 || c > 1)
illegal(value, "n ftype");
if (sb_feat.crcs_enabled) {
@@ -1679,7 +1696,7 @@ _("cannot specify both crc and ftype\n"));
case R_FILE:
if (!value || *value == '\0')
value = "1";
- xi.risfile = atoi(value);
+ xi.risfile = getnum(value, 0, 0, false);
if (xi.risfile < 0 || xi.risfile > 1)
illegal(value, "r file");
if (xi.risfile)
@@ -1723,7 +1740,7 @@ _("cannot specify both crc and ftype\n"));
if (ssflag || lssflag)
conflict('s', sopts, S_SECTSIZE,
S_SECTLOG);
- sectorlog = atoi(value);
+ sectorlog = getnum(value, 0, 0, false);
if (sectorlog <= 0)
illegal(value, "s sectlog");
lsectorlog = sectorlog;
@@ -1740,8 +1757,8 @@ _("cannot specify both crc and ftype\n"));
if (slflag || lslflag)
conflict('s', sopts, S_SECTLOG,
S_SECTSIZE);
- sectorsize = cvtnum(
- blocksize, sectorsize, value);
+ sectorsize = getnum(value, blocksize,
+ sectorsize, true);
if (sectorsize <= 0 ||
!ispow2(sectorsize))
illegal(value, "s sectsize");
@@ -1791,6 +1808,15 @@ _("cannot specify both crc and ftype\n"));
usage();
}
+ if (!slflag && !ssflag) {
+ sectorlog = XFS_MIN_SECTORSIZE_LOG;
+ sectorsize = XFS_MIN_SECTORSIZE;
+ }
+ if (!lslflag && !lssflag) {
+ lsectorlog = sectorlog;
+ lsectorsize = sectorsize;
+ }
+
memset(&ft, 0, sizeof(ft));
get_topology(&xi, &ft, force_overwrite);
@@ -1944,7 +1970,9 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
if (dsize) {
__uint64_t dbytes;
- dbytes = cvtnum(blocksize, sectorsize, dsize);
+ dbytes = getnum(dsize, blocksize, sectorsize, true);
+ if ((__int64_t)dbytes < 0)
+ illegal(dsize, "d size");
if (dbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
_("illegal data length %lld, not a multiple of %d\n"),
@@ -1981,7 +2009,9 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
if (logsize) {
__uint64_t logbytes;
- logbytes = cvtnum(blocksize, sectorsize, logsize);
+ logbytes = getnum(logsize, blocksize, sectorsize, true);
+ if ((__int64_t)logbytes < 0)
+ illegal(logsize, "l size");
if (logbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
_("illegal log length %lld, not a multiple of %d\n"),
@@ -2003,7 +2033,9 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
if (rtsize) {
__uint64_t rtbytes;
- rtbytes = cvtnum(blocksize, sectorsize, rtsize);
+ rtbytes = getnum(rtsize, blocksize, sectorsize, true);
+ if ((__int64_t)rtbytes < 0)
+ illegal(rtsize, "r size");
if (rtbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
_("illegal rt length %lld, not a multiple of %d\n"),
@@ -2023,7 +2055,9 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
if (rtextsize) {
__uint64_t rtextbytes;
- rtextbytes = cvtnum(blocksize, sectorsize, rtextsize);
+ rtextbytes = getnum(rtextsize, blocksize, sectorsize, true);
+ if ((__int64_t)rtextbytes < 0)
+ illegal(rtsize, "r extsize");
if (rtextbytes % blocksize) {
fprintf(stderr,
_("illegal rt extent size %lld, not a multiple of %d\n"),
@@ -3086,28 +3120,11 @@ unknown(
usage();
}
-/*
- * isdigits -- returns 1 if string contains nothing but [0-9], 0 otherwise
- */
-int
-isdigits(
- char *str)
-{
- int i;
- int n = strlen(str);
-
- for (i = 0; i < n; i++) {
- if (!isdigit((int)str[i]))
- return 0;
- }
- return 1;
-}
-
long long
cvtnum(
unsigned int blocksize,
unsigned int sectorsize,
- char *s)
+ const char *s)
{
long long i;
char *sp;
@@ -3118,17 +3135,11 @@ cvtnum(
if (*sp == '\0')
return i;
- if (*sp == 'b' && sp[1] == '\0') {
- if (blocksize)
- return i * blocksize;
- fprintf(stderr, _("blocksize not available yet.\n"));
- usage();
- }
- if (*sp == 's' && sp[1] == '\0') {
- if (sectorsize)
- return i * sectorsize;
- return i * BBSIZE;
- }
+ if (*sp == 'b' && sp[1] == '\0')
+ return i * blocksize;
+ if (*sp == 's' && sp[1] == '\0')
+ return i * sectorsize;
+
if (*sp == 'k' && sp[1] == '\0')
return 1024LL * i;
if (*sp == 'm' && sp[1] == '\0')
--
1.8.4.rc3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 05/15] mkfs: factor boolean option parsing
2013-11-29 1:43 [RFC, PATCH 00/15] mkfs: sanitise input parameters Dave Chinner
` (3 preceding siblings ...)
2013-11-29 1:43 ` [PATCH 04/15] mkfs: validate all input values Dave Chinner
@ 2013-11-29 1:43 ` Dave Chinner
2013-12-02 10:46 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 06/15] mkfs: validate logarithmic parameters sanely Dave Chinner
` (9 subsequent siblings)
14 siblings, 1 reply; 42+ messages in thread
From: Dave Chinner @ 2013-11-29 1:43 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
Many of the options passed to mkfs have boolean options (0 or 1) and
all hand roll the same code and validity checks. Factor these out
into a common function.
Note that the lazy-count option is now changed to match other
booleans in that if you don't specify a value, it reverts to the
default value (on) rather than throwing an error. Similarly the -m
crc and -n ftype options default to off rather than throwing an
error.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
mkfs/xfs_mkfs.c | 101 +++++++++++++++++++++++---------------------------------
1 file changed, 42 insertions(+), 59 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 1cc2bcb..126cbea 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -42,7 +42,7 @@ struct fs_topology {
* Prototypes for internal functions.
*/
static void conflict(char opt, char *tab[], int oldidx, int newidx);
-static void illegal(char *value, char *opt);
+static void illegal(const char *value, const char *opt);
static __attribute__((noreturn)) void usage (void);
static __attribute__((noreturn)) void reqval(char opt, char *tab[], int idx);
static void respec(char opt, char *tab[], int idx);
@@ -966,6 +966,21 @@ getnum(
return i;
}
+static bool
+getbool(
+ const char *str,
+ const char *illegal_str,
+ bool default_val)
+{
+ long long c;
+
+ if (!str || *str == '\0')
+ return default_val;
+ c = getnum(str, 0, 0, false);
+ if (c < 0 || c > 1)
+ illegal(str, illegal_str);
+ return c ? true : false;
+}
int
main(
@@ -1179,11 +1194,8 @@ main(
dasize = 1;
break;
case D_FILE:
- if (!value || *value == '\0')
- value = "1";
- xi.disfile = getnum(value, 0, 0, false);
- if (xi.disfile < 0 || xi.disfile > 1)
- illegal(value, "d file");
+ xi.disfile = getbool(value, "d file",
+ true);
if (xi.disfile && !Nflag)
xi.dcreat = 1;
break;
@@ -1326,12 +1338,8 @@ main(
switch (getsubopt(&p, (constpp)iopts, &value)) {
case I_ALIGN:
- if (!value || *value == '\0')
- break;
- c = getnum(value, 0, 0, false);
- if (c < 0 || c > 1)
- illegal(value, "i align");
- sb_feat.inode_align = c ? true : false;
+ sb_feat.inode_align = getbool(
+ value, "i align", true);
break;
case I_LOG:
if (!value || *value == '\0')
@@ -1404,12 +1412,8 @@ main(
sb_feat.attr_version = c;
break;
case I_PROJID32BIT:
- if (!value || *value == '\0')
- value = "0";
- c = getnum(value, 0, 0, false);
- if (c < 0 || c > 1)
- illegal(value, "i projid32bit");
- sb_feat.projid16bit = c ? false : true;
+ sb_feat.projid16bit = !getbool(value,
+ "i projid32bit", false);
break;
default:
unknown('i', value);
@@ -1435,20 +1439,15 @@ main(
laflag = 1;
break;
case L_FILE:
- if (!value || *value == '\0')
- value = "1";
if (loginternal)
conflict('l', lopts, L_INTERNAL,
L_FILE);
- xi.lisfile = getnum(value, 0, 0, false);
- if (xi.lisfile < 0 || xi.lisfile > 1)
- illegal(value, "l file");
+ xi.lisfile = getbool(value, "l file",
+ true);
if (xi.lisfile)
xi.lcreat = 1;
break;
case L_INTERNAL:
- if (!value || *value == '\0')
- value = "1";
if (ldflag)
conflict('l', lopts, L_INTERNAL, L_DEV);
if (xi.lisfile)
@@ -1456,9 +1455,9 @@ main(
L_INTERNAL);
if (liflag)
respec('l', lopts, L_INTERNAL);
- loginternal = getnum(value, 0, 0, false);
- if (loginternal < 0 || loginternal > 1)
- illegal(value, "l internal");
+
+ loginternal = getbool(value,
+ "l internal", true);
liflag = 1;
break;
case L_SU:
@@ -1546,14 +1545,9 @@ main(
lssflag = 1;
break;
case L_LAZYSBCNTR:
- if (!value || *value == '\0')
- reqval('l', lopts,
- L_LAZYSBCNTR);
- c = getnum(value, 0, 0, false);
- if (c < 0 || c > 1)
- illegal(value, "l lazy-count");
- sb_feat.lazy_sb_counters = c ? true
- : false;
+ sb_feat.lazy_sb_counters = getbool(
+ value, "l lazy-count",
+ true);
break;
default:
unknown('l', value);
@@ -1572,18 +1566,14 @@ main(
switch (getsubopt(&p, (constpp)mopts, &value)) {
case M_CRC:
- if (!value || *value == '\0')
- reqval('m', mopts, M_CRC);
- c = getnum(value, 0, 0, false);
- if (c < 0 || c > 1)
- illegal(value, "m crc");
- if (c && nftype) {
+ sb_feat.crcs_enabled = getbool(
+ value, "m crc", false);
+ if (sb_feat.crcs_enabled && nftype) {
fprintf(stderr,
-_("cannot specify both crc and ftype\n"));
+_("cannot specify both -m crc=1 and -n ftype\n"));
usage();
}
- sb_feat.crcs_enabled = c ? true : false;
- if (c)
+ if (sb_feat.crcs_enabled)
sb_feat.dirftype = true;
break;
default:
@@ -1646,19 +1636,15 @@ _("cannot specify both crc and ftype\n"));
nvflag = 1;
break;
case N_FTYPE:
- if (!value || *value == '\0')
- reqval('n', nopts, N_FTYPE);
if (nftype)
respec('n', nopts, N_FTYPE);
- c = getnum(value, 0, 0, false);
- if (c < 0 || c > 1)
- illegal(value, "n ftype");
if (sb_feat.crcs_enabled) {
fprintf(stderr,
-_("cannot specify both crc and ftype\n"));
+_("cannot specify both -m crc=1 and -n ftype\n"));
usage();
}
- sb_feat.dirftype = c ? true : false;
+ sb_feat.dirftype = getbool(value,
+ "n ftype", false);
nftype = 1;
break;
default:
@@ -1694,11 +1680,8 @@ _("cannot specify both crc and ftype\n"));
rtextsize = value;
break;
case R_FILE:
- if (!value || *value == '\0')
- value = "1";
- xi.risfile = getnum(value, 0, 0, false);
- if (xi.risfile < 0 || xi.risfile > 1)
- illegal(value, "r file");
+ xi.risfile = getbool(value,
+ "r file", true);
if (xi.risfile)
xi.rcreat = 1;
break;
@@ -3074,8 +3057,8 @@ conflict(
static void
illegal(
- char *value,
- char *opt)
+ const char *value,
+ const char *opt)
{
fprintf(stderr, _("Illegal value %s for -%s option\n"), value, opt);
usage();
--
1.8.4.rc3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 06/15] mkfs: validate logarithmic parameters sanely
2013-11-29 1:43 [RFC, PATCH 00/15] mkfs: sanitise input parameters Dave Chinner
` (4 preceding siblings ...)
2013-11-29 1:43 ` [PATCH 05/15] mkfs: factor boolean option parsing Dave Chinner
@ 2013-11-29 1:43 ` Dave Chinner
2013-12-02 17:06 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 07/15] mkfs: structify input parameter passing Dave Chinner
` (8 subsequent siblings)
14 siblings, 1 reply; 42+ messages in thread
From: Dave Chinner @ 2013-11-29 1:43 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
Testing logarithmic paramters like "-n log=<num>" shows that we do a
terrible job of validating such input. e.g.:
# mkfs.xfs -f -n log=456858480 /dev/vda
.....
naming =version 2 bsize=65536 ascii-ci=0 ftype=0
....
Yeah, I just asked for a block size of 2^456858480, and it didn't
get rejected. Great, isn't it?
So, factor out the parsing of logarithmic parameters, and pass in
the maximum valid value that they can take. These maximum values
might not be completely accurate (e.g. block/sector sizes will
affect the eventual valid maximum) but we can get rid of all the
overflows and stupidities before we get to fine-grained validity
checking later in mkfs once things like block and sector sizes have
been finalised.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
mkfs/xfs_mkfs.c | 79 +++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 51 insertions(+), 28 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 126cbea..fbb2a35 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -982,6 +982,27 @@ getbool(
return c ? true : false;
}
+static int
+getnum_checked(
+ const char *str,
+ long long min_val,
+ long long max_val,
+ const char *illegal_str,
+ char reqval_char,
+ char *reqval_opts[],
+ int reqval_optind)
+{
+ long long c;
+
+ if (!str || *str == '\0')
+ reqval(reqval_char, reqval_opts, reqval_optind);
+
+ c = getnum(str, 0, 0, false);
+ if (c < min_val || c > max_val)
+ illegal(str, illegal_str);
+ return c;
+}
+
int
main(
int argc,
@@ -1132,16 +1153,16 @@ main(
switch (getsubopt(&p, (constpp)bopts, &value)) {
case B_LOG:
- if (!value || *value == '\0')
- reqval('b', bopts, B_LOG);
if (blflag)
respec('b', bopts, B_LOG);
if (bsflag)
conflict('b', bopts, B_SIZE,
B_LOG);
- blocklog = getnum(value, 0, 0, false);
- if (blocklog <= 0)
- illegal(value, "b log");
+ blocklog = getnum_checked(value,
+ XFS_MIN_BLOCKSIZE_LOG,
+ XFS_MAX_BLOCKSIZE_LOG,
+ "b log", 'b', bopts,
+ B_LOG);
blocksize = 1 << blocklog;
blflag = 1;
break;
@@ -1278,16 +1299,16 @@ main(
nodsflag = 1;
break;
case D_SECTLOG:
- if (!value || *value == '\0')
- reqval('d', dopts, D_SECTLOG);
if (slflag)
respec('d', dopts, D_SECTLOG);
if (ssflag)
conflict('d', dopts, D_SECTSIZE,
D_SECTLOG);
- sectorlog = getnum(value, 0, 0, false);
- if (sectorlog <= 0)
- illegal(value, "d sectlog");
+ sectorlog = getnum_checked(value,
+ XFS_MIN_SECTORSIZE_LOG,
+ XFS_MAX_SECTORSIZE_LOG,
+ "d sectlog", 'd', dopts,
+ D_SECTLOG);
sectorsize = 1 << sectorlog;
slflag = 1;
break;
@@ -1352,9 +1373,11 @@ main(
if (isflag)
conflict('i', iopts, I_SIZE,
I_LOG);
- inodelog = getnum(value, 0, 0, false);
- if (inodelog <= 0)
- illegal(value, "i log");
+ inodelog = getnum_checked(value,
+ XFS_DINODE_MIN_LOG,
+ XFS_DINODE_MAX_LOG,
+ "i log", 'i', iopts,
+ I_LOG);
isize = 1 << inodelog;
ilflag = 1;
break;
@@ -1514,16 +1537,16 @@ main(
lsflag = 1;
break;
case L_SECTLOG:
- if (!value || *value == '\0')
- reqval('l', lopts, L_SECTLOG);
if (lslflag)
respec('l', lopts, L_SECTLOG);
if (lssflag)
conflict('l', lopts, L_SECTSIZE,
L_SECTLOG);
- lsectorlog = getnum(value, 0, 0, false);
- if (lsectorlog <= 0)
- illegal(value, "l sectlog");
+ lsectorlog = getnum_checked(value,
+ XFS_MIN_SECTORSIZE_LOG,
+ XFS_MAX_SECTORSIZE_LOG,
+ "l sectlog", 'l', lopts,
+ L_SECTLOG);
lsectorsize = 1 << lsectorlog;
lslflag = 1;
break;
@@ -1588,16 +1611,16 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
switch (getsubopt(&p, (constpp)nopts, &value)) {
case N_LOG:
- if (!value || *value == '\0')
- reqval('n', nopts, N_LOG);
if (nlflag)
respec('n', nopts, N_LOG);
if (nsflag)
conflict('n', nopts, N_SIZE,
N_LOG);
- dirblocklog = getnum(value, 0, 0, false);
- if (dirblocklog <= 0)
- illegal(value, "n log");
+ dirblocklog = getnum_checked(value,
+ XFS_MIN_REC_DIRSIZE,
+ XFS_MAX_BLOCKSIZE_LOG,
+ "n log", 'n', nopts,
+ N_LOG);
dirblocksize = 1 << dirblocklog;
nlflag = 1;
break;
@@ -1716,16 +1739,16 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
switch (getsubopt(&p, (constpp)sopts, &value)) {
case S_LOG:
case S_SECTLOG:
- if (!value || *value == '\0')
- reqval('s', sopts, S_SECTLOG);
if (slflag || lslflag)
respec('s', sopts, S_SECTLOG);
if (ssflag || lssflag)
conflict('s', sopts, S_SECTSIZE,
S_SECTLOG);
- sectorlog = getnum(value, 0, 0, false);
- if (sectorlog <= 0)
- illegal(value, "s sectlog");
+ sectorlog = getnum_checked(value,
+ XFS_MIN_SECTORSIZE_LOG,
+ XFS_MAX_SECTORSIZE_LOG,
+ "s sectlog", 's', sopts,
+ S_SECTLOG);
lsectorlog = sectorlog;
sectorsize = 1 << sectorlog;
lsectorsize = sectorsize;
--
1.8.4.rc3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 07/15] mkfs: structify input parameter passing
2013-11-29 1:43 [RFC, PATCH 00/15] mkfs: sanitise input parameters Dave Chinner
` (5 preceding siblings ...)
2013-11-29 1:43 ` [PATCH 06/15] mkfs: validate logarithmic parameters sanely Dave Chinner
@ 2013-11-29 1:43 ` Dave Chinner
2013-12-02 17:11 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 08/15] mkfs: getbool is redundant Dave Chinner
` (7 subsequent siblings)
14 siblings, 1 reply; 42+ messages in thread
From: Dave Chinner @ 2013-11-29 1:43 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
Passing large number of parameters around to number conversion
functions is painful. Add a structure to encapsulate the constant
parameters that are passed, and convert getnum_checked to use it.
This is the first real step towards a table driven option parser.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
mkfs/xfs_mkfs.c | 602 ++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 392 insertions(+), 210 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index fbb2a35..5842cc7 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -52,141 +52,318 @@ static int ispow2(unsigned int i);
static long long cvtnum(unsigned int blocksize,
unsigned int sectorsize, const char *s);
-/*
- * option tables for getsubopt calls
- */
-char *bopts[] = {
+#define MAX_SUBOPTS 16
+struct opt_params {
+ const char name;
+ const char *subopts[MAX_SUBOPTS];
+ struct subopt_param {
+ int index;
+ long long minval;
+ long long maxval;
+ } subopt_params[MAX_SUBOPTS];
+};
+
+const struct opt_params bopts = {
+ .name = 'b',
+ .subopts = {
#define B_LOG 0
- "log",
+ "log",
#define B_SIZE 1
- "size",
- NULL
+ "size",
+ NULL
+ },
+ .subopt_params = {
+ { .index = B_LOG,
+ .minval = XFS_MIN_BLOCKSIZE_LOG,
+ .maxval = XFS_MAX_BLOCKSIZE_LOG,
+ },
+ { .index = B_SIZE,
+ .minval = XFS_MIN_BLOCKSIZE,
+ .maxval = XFS_MAX_BLOCKSIZE,
+ },
+ },
};
-char *dopts[] = {
+const struct opt_params dopts = {
+ .name = 'd',
+ .subopts = {
#define D_AGCOUNT 0
- "agcount",
+ "agcount",
#define D_FILE 1
- "file",
+ "file",
#define D_NAME 2
- "name",
+ "name",
#define D_SIZE 3
- "size",
+ "size",
#define D_SUNIT 4
- "sunit",
+ "sunit",
#define D_SWIDTH 5
- "swidth",
+ "swidth",
#define D_AGSIZE 6
- "agsize",
+ "agsize",
#define D_SU 7
- "su",
+ "su",
#define D_SW 8
- "sw",
+ "sw",
#define D_SECTLOG 9
- "sectlog",
+ "sectlog",
#define D_SECTSIZE 10
- "sectsize",
+ "sectsize",
#define D_NOALIGN 11
- "noalign",
+ "noalign",
#define D_RTINHERIT 12
- "rtinherit",
+ "rtinherit",
#define D_PROJINHERIT 13
- "projinherit",
+ "projinherit",
#define D_EXTSZINHERIT 14
- "extszinherit",
- NULL
+ "extszinherit",
+ NULL
+ },
+ .subopt_params = {
+ { .index = D_AGCOUNT,
+ },
+ { .index = D_FILE,
+ },
+ { .index = D_NAME,
+ },
+ { .index = D_SIZE,
+ },
+ { .index = D_SUNIT,
+ },
+ { .index = D_SWIDTH,
+ },
+ { .index = D_AGSIZE,
+ },
+ { .index = D_SU,
+ },
+ { .index = D_SW,
+ },
+ { .index = D_SECTLOG,
+ .minval = XFS_MIN_SECTORSIZE_LOG,
+ .maxval = XFS_MAX_SECTORSIZE_LOG,
+ },
+ { .index = D_SECTSIZE,
+ .minval = XFS_MIN_SECTORSIZE,
+ .maxval = XFS_MAX_SECTORSIZE,
+ },
+ { .index = D_NOALIGN,
+ },
+ { .index = D_RTINHERIT,
+ },
+ { .index = D_PROJINHERIT,
+ },
+ { .index = D_EXTSZINHERIT,
+ },
+ },
};
-char *iopts[] = {
+
+const struct opt_params iopts = {
+ .name = 'i',
+ .subopts = {
#define I_ALIGN 0
- "align",
+ "align",
#define I_LOG 1
- "log",
+ "log",
#define I_MAXPCT 2
- "maxpct",
+ "maxpct",
#define I_PERBLOCK 3
- "perblock",
+ "perblock",
#define I_SIZE 4
- "size",
+ "size",
#define I_ATTR 5
- "attr",
+ "attr",
#define I_PROJID32BIT 6
- "projid32bit",
- NULL
+ "projid32bit",
+ NULL
+ },
+ .subopt_params = {
+ { .index = I_ALIGN,
+ },
+ { .index = I_LOG,
+ .minval = XFS_DINODE_MIN_LOG,
+ .maxval = XFS_DINODE_MAX_LOG,
+ },
+ { .index = I_MAXPCT,
+ },
+ { .index = I_PERBLOCK,
+ },
+ { .index = I_SIZE,
+ },
+ { .index = I_ATTR,
+ },
+ { .index = I_PROJID32BIT,
+ },
+ },
};
-char *lopts[] = {
+const struct opt_params lopts = {
+ .name = 'l',
+ .subopts = {
#define L_AGNUM 0
- "agnum",
+ "agnum",
#define L_INTERNAL 1
- "internal",
+ "internal",
#define L_SIZE 2
- "size",
+ "size",
#define L_VERSION 3
- "version",
+ "version",
#define L_SUNIT 4
- "sunit",
+ "sunit",
#define L_SU 5
- "su",
+ "su",
#define L_DEV 6
- "logdev",
+ "logdev",
#define L_SECTLOG 7
- "sectlog",
+ "sectlog",
#define L_SECTSIZE 8
- "sectsize",
+ "sectsize",
#define L_FILE 9
- "file",
+ "file",
#define L_NAME 10
- "name",
+ "name",
#define L_LAZYSBCNTR 11
- "lazy-count",
- NULL
+ "lazy-count",
+ NULL
+ },
+ .subopt_params = {
+ { .index = L_AGNUM,
+ },
+ { .index = L_INTERNAL,
+ },
+ { .index = L_SIZE,
+ },
+ { .index = L_VERSION,
+ },
+ { .index = L_SUNIT,
+ },
+ { .index = L_SU,
+ },
+ { .index = L_DEV,
+ },
+ { .index = L_SECTLOG,
+ .minval = XFS_MIN_SECTORSIZE_LOG,
+ .maxval = XFS_MAX_SECTORSIZE_LOG,
+ },
+ { .index = L_SECTSIZE,
+ .minval = XFS_MIN_SECTORSIZE,
+ .maxval = XFS_MAX_SECTORSIZE,
+ },
+ { .index = L_FILE,
+ },
+ { .index = L_NAME,
+ },
+ { .index = L_LAZYSBCNTR,
+ },
+ },
};
-char *nopts[] = {
+const struct opt_params nopts = {
+ .name = 'n',
+ .subopts = {
#define N_LOG 0
- "log",
+ "log",
#define N_SIZE 1
- "size",
+ "size",
#define N_VERSION 2
- "version",
+ "version",
#define N_FTYPE 3
- "ftype",
+ "ftype",
NULL,
+ },
+ .subopt_params = {
+ { .index = N_LOG,
+ .minval = XFS_MIN_REC_DIRSIZE,
+ .maxval = XFS_MAX_BLOCKSIZE_LOG,
+ },
+ { .index = N_SIZE,
+ .minval = 1 << XFS_MIN_REC_DIRSIZE,
+ .maxval = XFS_MAX_BLOCKSIZE,
+ },
+ { .index = N_VERSION,
+ },
+ { .index = N_FTYPE,
+ },
+ },
};
-char *ropts[] = {
+const struct opt_params ropts = {
+ .name = 'r',
+ .subopts = {
#define R_EXTSIZE 0
- "extsize",
+ "extsize",
#define R_SIZE 1
- "size",
+ "size",
#define R_DEV 2
- "rtdev",
+ "rtdev",
#define R_FILE 3
- "file",
+ "file",
#define R_NAME 4
- "name",
+ "name",
#define R_NOALIGN 5
- "noalign",
- NULL
+ "noalign",
+ NULL
+ },
+ .subopt_params = {
+ { .index = R_EXTSIZE,
+ },
+ { .index = R_SIZE,
+ },
+ { .index = R_DEV,
+ },
+ { .index = R_FILE,
+ },
+ { .index = R_NAME,
+ },
+ { .index = R_NOALIGN,
+ },
+ },
};
-char *sopts[] = {
+const struct opt_params sopts = {
+ .name = 's',
+ .subopts = {
#define S_LOG 0
- "log",
+ "log",
#define S_SECTLOG 1
- "sectlog",
+ "sectlog",
#define S_SIZE 2
- "size",
+ "size",
#define S_SECTSIZE 3
- "sectsize",
- NULL
+ "sectsize",
+ NULL
+ },
+ .subopt_params = {
+ { .index = S_LOG,
+ .minval = XFS_MIN_SECTORSIZE_LOG,
+ .maxval = XFS_MAX_SECTORSIZE_LOG,
+ },
+ { .index = S_SECTLOG,
+ .minval = XFS_MIN_SECTORSIZE_LOG,
+ .maxval = XFS_MAX_SECTORSIZE_LOG,
+ },
+ { .index = S_SIZE,
+ .minval = XFS_MIN_SECTORSIZE,
+ .maxval = XFS_MAX_SECTORSIZE,
+ },
+ { .index = S_SECTSIZE,
+ .minval = XFS_MIN_SECTORSIZE,
+ .maxval = XFS_MAX_SECTORSIZE,
+ },
+ },
};
-char *mopts[] = {
+const struct opt_params mopts = {
+ .name = 'm',
+ .subopts = {
#define M_CRC 0
- "crc",
- NULL
+ "crc",
+ NULL
+ },
+ .subopt_params = {
+ { .index = M_CRC,
+ },
+ },
};
#define TERABYTES(count, blog) ((__uint64_t)(count) << (40 - (blog)))
@@ -982,24 +1159,33 @@ getbool(
return c ? true : false;
}
+static __attribute__((noreturn)) void
+illegal_option(
+ const char *value,
+ const struct opt_params *opts,
+ int index)
+{
+ fprintf(stderr,
+ _("Illegal value %s for -%c %s option\n"),
+ value, opts->name, opts->subopts[index]);
+ usage();
+}
+
static int
getnum_checked(
const char *str,
- long long min_val,
- long long max_val,
- const char *illegal_str,
- char reqval_char,
- char *reqval_opts[],
- int reqval_optind)
+ const struct opt_params *opts,
+ int index)
{
long long c;
if (!str || *str == '\0')
- reqval(reqval_char, reqval_opts, reqval_optind);
+ reqval(opts->name, (char **)opts->subopts, index);
c = getnum(str, 0, 0, false);
- if (c < min_val || c > max_val)
- illegal(str, illegal_str);
+ if (c < opts->subopt_params[index].minval ||
+ c > opts->subopt_params[index].maxval)
+ illegal_option(str, opts, index);
return c;
}
@@ -1149,30 +1335,29 @@ main(
case 'b':
p = optarg;
while (*p != '\0') {
+ char **subopts = (char **)bopts.subopts;
char *value;
- switch (getsubopt(&p, (constpp)bopts, &value)) {
+ switch (getsubopt(&p, (constpp)subopts,
+ &value)) {
case B_LOG:
if (blflag)
- respec('b', bopts, B_LOG);
+ respec('b', subopts, B_LOG);
if (bsflag)
- conflict('b', bopts, B_SIZE,
+ conflict('b', subopts, B_SIZE,
B_LOG);
- blocklog = getnum_checked(value,
- XFS_MIN_BLOCKSIZE_LOG,
- XFS_MAX_BLOCKSIZE_LOG,
- "b log", 'b', bopts,
- B_LOG);
+ blocklog = getnum_checked(value, &bopts,
+ B_LOG);
blocksize = 1 << blocklog;
blflag = 1;
break;
case B_SIZE:
if (!value || *value == '\0')
- reqval('b', bopts, B_SIZE);
+ reqval('b', subopts, B_SIZE);
if (bsflag)
- respec('b', bopts, B_SIZE);
+ respec('b', subopts, B_SIZE);
if (blflag)
- conflict('b', bopts, B_LOG,
+ conflict('b', subopts, B_LOG,
B_SIZE);
blocksize = getnum(value, blocksize,
sectorsize, true);
@@ -1190,14 +1375,16 @@ main(
case 'd':
p = optarg;
while (*p != '\0') {
+ char **subopts = (char **)dopts.subopts;
char *value;
- switch (getsubopt(&p, (constpp)dopts, &value)) {
+ switch (getsubopt(&p, (constpp)subopts,
+ &value)) {
case D_AGCOUNT:
if (!value || *value == '\0')
- reqval('d', dopts, D_AGCOUNT);
+ reqval('d', subopts, D_AGCOUNT);
if (daflag)
- respec('d', dopts, D_AGCOUNT);
+ respec('d', subopts, D_AGCOUNT);
agcount = getnum(value, 0, 0, false);
if ((__int64_t)agcount <= 0)
illegal(value, "d agcount");
@@ -1205,9 +1392,9 @@ main(
break;
case D_AGSIZE:
if (!value || *value == '\0')
- reqval('d', dopts, D_AGSIZE);
+ reqval('d', subopts, D_AGSIZE);
if (dasize)
- respec('d', dopts, D_AGSIZE);
+ respec('d', subopts, D_AGSIZE);
agsize = getnum(value, blocksize,
sectorsize, true);
if ((__int64_t)agsize <= 0)
@@ -1222,25 +1409,25 @@ main(
break;
case D_NAME:
if (!value || *value == '\0')
- reqval('d', dopts, D_NAME);
+ reqval('d', subopts, D_NAME);
if (xi.dname)
- respec('d', dopts, D_NAME);
+ respec('d', subopts, D_NAME);
xi.dname = value;
break;
case D_SIZE:
if (!value || *value == '\0')
- reqval('d', dopts, D_SIZE);
+ reqval('d', subopts, D_SIZE);
if (dsize)
- respec('d', dopts, D_SIZE);
+ respec('d', subopts, D_SIZE);
dsize = value;
break;
case D_SUNIT:
if (!value || *value == '\0')
- reqval('d', dopts, D_SUNIT);
+ reqval('d', subopts, D_SUNIT);
if (dsunit)
- respec('d', dopts, D_SUNIT);
+ respec('d', subopts, D_SUNIT);
if (nodsflag)
- conflict('d', dopts, D_NOALIGN,
+ conflict('d', subopts, D_NOALIGN,
D_SUNIT);
dsunit = getnum(value, 0, 0, false);
if (dsunit < 0)
@@ -1248,11 +1435,11 @@ main(
break;
case D_SWIDTH:
if (!value || *value == '\0')
- reqval('d', dopts, D_SWIDTH);
+ reqval('d', subopts, D_SWIDTH);
if (dswidth)
- respec('d', dopts, D_SWIDTH);
+ respec('d', subopts, D_SWIDTH);
if (nodsflag)
- conflict('d', dopts, D_NOALIGN,
+ conflict('d', subopts, D_NOALIGN,
D_SWIDTH);
dswidth = getnum(value, 0, 0, false);
if (dswidth < 0)
@@ -1260,11 +1447,11 @@ main(
break;
case D_SU:
if (!value || *value == '\0')
- reqval('d', dopts, D_SU);
+ reqval('d', subopts, D_SU);
if (dsu)
- respec('d', dopts, D_SU);
+ respec('d', subopts, D_SU);
if (nodsflag)
- conflict('d', dopts, D_NOALIGN,
+ conflict('d', subopts, D_NOALIGN,
D_SU);
dsu = getnum(value, blocksize,
sectorsize, true);
@@ -1273,11 +1460,11 @@ main(
break;
case D_SW:
if (!value || *value == '\0')
- reqval('d', dopts, D_SW);
+ reqval('d', subopts, D_SW);
if (dsw)
- respec('d', dopts, D_SW);
+ respec('d', subopts, D_SW);
if (nodsflag)
- conflict('d', dopts, D_NOALIGN,
+ conflict('d', subopts, D_NOALIGN,
D_SW);
dsw = getnum(value, 0, 0, false);
if (dsw < 0)
@@ -1285,40 +1472,37 @@ main(
break;
case D_NOALIGN:
if (dsu)
- conflict('d', dopts, D_SU,
+ conflict('d', subopts, D_SU,
D_NOALIGN);
if (dsunit)
- conflict('d', dopts, D_SUNIT,
+ conflict('d', subopts, D_SUNIT,
D_NOALIGN);
if (dsw)
- conflict('d', dopts, D_SW,
+ conflict('d', subopts, D_SW,
D_NOALIGN);
if (dswidth)
- conflict('d', dopts, D_SWIDTH,
+ conflict('d', subopts, D_SWIDTH,
D_NOALIGN);
nodsflag = 1;
break;
case D_SECTLOG:
if (slflag)
- respec('d', dopts, D_SECTLOG);
+ respec('d', subopts, D_SECTLOG);
if (ssflag)
- conflict('d', dopts, D_SECTSIZE,
+ conflict('d', subopts, D_SECTSIZE,
D_SECTLOG);
- sectorlog = getnum_checked(value,
- XFS_MIN_SECTORSIZE_LOG,
- XFS_MAX_SECTORSIZE_LOG,
- "d sectlog", 'd', dopts,
- D_SECTLOG);
+ sectorlog = getnum_checked(value, &dopts,
+ D_SECTLOG);
sectorsize = 1 << sectorlog;
slflag = 1;
break;
case D_SECTSIZE:
if (!value || *value == '\0')
- reqval('d', dopts, D_SECTSIZE);
+ reqval('d', subopts, D_SECTSIZE);
if (ssflag)
- respec('d', dopts, D_SECTSIZE);
+ respec('d', subopts, D_SECTSIZE);
if (slflag)
- conflict('d', dopts, D_SECTLOG,
+ conflict('d', subopts, D_SECTLOG,
D_SECTSIZE);
sectorsize = getnum(value, blocksize,
sectorsize, true);
@@ -1335,14 +1519,14 @@ main(
break;
case D_PROJINHERIT:
if (!value || *value == '\0')
- reqval('d', dopts, D_PROJINHERIT);
+ reqval('d', subopts, D_PROJINHERIT);
fsx.fsx_projid = atoi(value);
fsx.fsx_xflags |= \
XFS_DIFLAG_PROJINHERIT;
break;
case D_EXTSZINHERIT:
if (!value || *value == '\0')
- reqval('d', dopts, D_EXTSZINHERIT);
+ reqval('d', subopts, D_EXTSZINHERIT);
fsx.fsx_extsize = atoi(value);
fsx.fsx_xflags |= \
XFS_DIFLAG_EXTSZINHERIT;
@@ -1355,37 +1539,34 @@ main(
case 'i':
p = optarg;
while (*p != '\0') {
+ char **subopts = (char **)iopts.subopts;
char *value;
- switch (getsubopt(&p, (constpp)iopts, &value)) {
+ switch (getsubopt(&p, (constpp)subopts,
+ &value)) {
case I_ALIGN:
sb_feat.inode_align = getbool(
value, "i align", true);
break;
case I_LOG:
- if (!value || *value == '\0')
- reqval('i', iopts, I_LOG);
if (ilflag)
- respec('i', iopts, I_LOG);
+ respec('i', subopts, I_LOG);
if (ipflag)
- conflict('i', iopts, I_PERBLOCK,
+ conflict('i', subopts, I_PERBLOCK,
I_LOG);
if (isflag)
- conflict('i', iopts, I_SIZE,
+ conflict('i', subopts, I_SIZE,
I_LOG);
- inodelog = getnum_checked(value,
- XFS_DINODE_MIN_LOG,
- XFS_DINODE_MAX_LOG,
- "i log", 'i', iopts,
- I_LOG);
+ inodelog = getnum_checked(value, &iopts,
+ I_LOG);
isize = 1 << inodelog;
ilflag = 1;
break;
case I_MAXPCT:
if (!value || *value == '\0')
- reqval('i', iopts, I_MAXPCT);
+ reqval('i', subopts, I_MAXPCT);
if (imflag)
- respec('i', iopts, I_MAXPCT);
+ respec('i', subopts, I_MAXPCT);
imaxpct = getnum(value, 0, 0, false);
if (imaxpct < 0 || imaxpct > 100)
illegal(value, "i maxpct");
@@ -1393,14 +1574,14 @@ main(
break;
case I_PERBLOCK:
if (!value || *value == '\0')
- reqval('i', iopts, I_PERBLOCK);
+ reqval('i', subopts, I_PERBLOCK);
if (ilflag)
- conflict('i', iopts, I_LOG,
+ conflict('i', subopts, I_LOG,
I_PERBLOCK);
if (ipflag)
- respec('i', iopts, I_PERBLOCK);
+ respec('i', subopts, I_PERBLOCK);
if (isflag)
- conflict('i', iopts, I_SIZE,
+ conflict('i', subopts, I_SIZE,
I_PERBLOCK);
inopblock = getnum(value, 0, 0, false);
if (inopblock <
@@ -1411,15 +1592,15 @@ main(
break;
case I_SIZE:
if (!value || *value == '\0')
- reqval('i', iopts, I_SIZE);
+ reqval('i', subopts, I_SIZE);
if (ilflag)
- conflict('i', iopts, I_LOG,
+ conflict('i', subopts, I_LOG,
I_SIZE);
if (ipflag)
- conflict('i', iopts, I_PERBLOCK,
+ conflict('i', subopts, I_PERBLOCK,
I_SIZE);
if (isflag)
- respec('i', iopts, I_SIZE);
+ respec('i', subopts, I_SIZE);
isize = getnum(value, 0, 0, true);
if (isize <= 0 || !ispow2(isize))
illegal(value, "i size");
@@ -1428,7 +1609,7 @@ main(
break;
case I_ATTR:
if (!value || *value == '\0')
- reqval('i', iopts, I_ATTR);
+ reqval('i', subopts, I_ATTR);
c = getnum(value, 0, 0, false);
if (c < 0 || c > 2)
illegal(value, "i attr");
@@ -1446,16 +1627,18 @@ main(
case 'l':
p = optarg;
while (*p != '\0') {
+ char **subopts = (char **)lopts.subopts;
char *value;
- switch (getsubopt(&p, (constpp)lopts, &value)) {
+ switch (getsubopt(&p, (constpp)subopts,
+ &value)) {
case L_AGNUM:
if (!value || *value == '\0')
- reqval('l', lopts, L_AGNUM);
+ reqval('l', subopts, L_AGNUM);
if (laflag)
- respec('l', lopts, L_AGNUM);
+ respec('l', subopts, L_AGNUM);
if (ldflag)
- conflict('l', lopts, L_AGNUM, L_DEV);
+ conflict('l', subopts, L_AGNUM, L_DEV);
logagno = getnum(value, 0, 0, false);
if (logagno < 0)
illegal(value, "l agno");
@@ -1463,7 +1646,7 @@ main(
break;
case L_FILE:
if (loginternal)
- conflict('l', lopts, L_INTERNAL,
+ conflict('l', subopts, L_INTERNAL,
L_FILE);
xi.lisfile = getbool(value, "l file",
true);
@@ -1472,12 +1655,12 @@ main(
break;
case L_INTERNAL:
if (ldflag)
- conflict('l', lopts, L_INTERNAL, L_DEV);
+ conflict('l', subopts, L_INTERNAL, L_DEV);
if (xi.lisfile)
- conflict('l', lopts, L_FILE,
+ conflict('l', subopts, L_FILE,
L_INTERNAL);
if (liflag)
- respec('l', lopts, L_INTERNAL);
+ respec('l', subopts, L_INTERNAL);
loginternal = getbool(value,
"l internal", true);
@@ -1485,9 +1668,9 @@ main(
break;
case L_SU:
if (!value || *value == '\0')
- reqval('l', lopts, L_SU);
+ reqval('l', subopts, L_SU);
if (lsu)
- respec('l', lopts, L_SU);
+ respec('l', subopts, L_SU);
lsu = getnum(value, blocksize,
sectorsize, true);
if (lsu < 0)
@@ -1495,9 +1678,9 @@ main(
break;
case L_SUNIT:
if (!value || *value == '\0')
- reqval('l', lopts, L_SUNIT);
+ reqval('l', subopts, L_SUNIT);
if (lsunit)
- respec('l', lopts, L_SUNIT);
+ respec('l', subopts, L_SUNIT);
lsunit = getnum(value, 0, 0, false);
if (lsunit < 0)
illegal(value, "l sunit");
@@ -1505,13 +1688,13 @@ main(
case L_NAME:
case L_DEV:
if (laflag)
- conflict('l', lopts, L_AGNUM, L_DEV);
+ conflict('l', subopts, L_AGNUM, L_DEV);
if (liflag)
- conflict('l', lopts, L_INTERNAL, L_DEV);
+ conflict('l', subopts, L_INTERNAL, L_DEV);
if (!value || *value == '\0')
- reqval('l', lopts, L_NAME);
+ reqval('l', subopts, L_NAME);
if (xi.logname)
- respec('l', lopts, L_NAME);
+ respec('l', subopts, L_NAME);
ldflag = 1;
loginternal = 0;
logfile = value;
@@ -1519,9 +1702,9 @@ main(
break;
case L_VERSION:
if (!value || *value == '\0')
- reqval('l', lopts, L_VERSION);
+ reqval('l', subopts, L_VERSION);
if (lvflag)
- respec('l', lopts, L_VERSION);
+ respec('l', subopts, L_VERSION);
c = getnum(value, 0, 0, false);
if (c < 1 || c > 2)
illegal(value, "l version");
@@ -1530,33 +1713,30 @@ main(
break;
case L_SIZE:
if (!value || *value == '\0')
- reqval('l', lopts, L_SIZE);
+ reqval('l', subopts, L_SIZE);
if (logsize)
- respec('l', lopts, L_SIZE);
+ respec('l', subopts, L_SIZE);
logsize = value;
lsflag = 1;
break;
case L_SECTLOG:
if (lslflag)
- respec('l', lopts, L_SECTLOG);
+ respec('l', subopts, L_SECTLOG);
if (lssflag)
- conflict('l', lopts, L_SECTSIZE,
+ conflict('l', subopts, L_SECTSIZE,
L_SECTLOG);
lsectorlog = getnum_checked(value,
- XFS_MIN_SECTORSIZE_LOG,
- XFS_MAX_SECTORSIZE_LOG,
- "l sectlog", 'l', lopts,
- L_SECTLOG);
+ &lopts, L_SECTLOG);
lsectorsize = 1 << lsectorlog;
lslflag = 1;
break;
case L_SECTSIZE:
if (!value || *value == '\0')
- reqval('l', lopts, L_SECTSIZE);
+ reqval('l', subopts, L_SECTSIZE);
if (lssflag)
- respec('l', lopts, L_SECTSIZE);
+ respec('l', subopts, L_SECTSIZE);
if (lslflag)
- conflict('l', lopts, L_SECTLOG,
+ conflict('l', subopts, L_SECTLOG,
L_SECTSIZE);
lsectorsize = getnum(value, blocksize,
sectorsize, true);
@@ -1585,9 +1765,11 @@ main(
case 'm':
p = optarg;
while (*p != '\0') {
+ char **subopts = (char **)mopts.subopts;
char *value;
- switch (getsubopt(&p, (constpp)mopts, &value)) {
+ switch (getsubopt(&p, (constpp)subopts,
+ &value)) {
case M_CRC:
sb_feat.crcs_enabled = getbool(
value, "m crc", false);
@@ -1607,30 +1789,29 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
case 'n':
p = optarg;
while (*p != '\0') {
+ char **subopts = (char **)nopts.subopts;
char *value;
- switch (getsubopt(&p, (constpp)nopts, &value)) {
+ switch (getsubopt(&p, (constpp)subopts,
+ &value)) {
case N_LOG:
if (nlflag)
- respec('n', nopts, N_LOG);
+ respec('n', subopts, N_LOG);
if (nsflag)
- conflict('n', nopts, N_SIZE,
+ conflict('n', subopts, N_SIZE,
N_LOG);
dirblocklog = getnum_checked(value,
- XFS_MIN_REC_DIRSIZE,
- XFS_MAX_BLOCKSIZE_LOG,
- "n log", 'n', nopts,
- N_LOG);
+ &nopts, N_LOG);
dirblocksize = 1 << dirblocklog;
nlflag = 1;
break;
case N_SIZE:
if (!value || *value == '\0')
- reqval('n', nopts, N_SIZE);
+ reqval('n', subopts, N_SIZE);
if (nsflag)
- respec('n', nopts, N_SIZE);
+ respec('n', subopts, N_SIZE);
if (nlflag)
- conflict('n', nopts, N_LOG,
+ conflict('n', subopts, N_LOG,
N_SIZE);
dirblocksize = getnum(value, blocksize,
sectorsize, true);
@@ -1643,9 +1824,9 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
break;
case N_VERSION:
if (!value || *value == '\0')
- reqval('n', nopts, N_VERSION);
+ reqval('n', subopts, N_VERSION);
if (nvflag)
- respec('n', nopts, N_VERSION);
+ respec('n', subopts, N_VERSION);
if (!strcasecmp(value, "ci")) {
/* ASCII CI mode */
sb_feat.nci = true;
@@ -1660,7 +1841,7 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
break;
case N_FTYPE:
if (nftype)
- respec('n', nopts, N_FTYPE);
+ respec('n', subopts, N_FTYPE);
if (sb_feat.crcs_enabled) {
fprintf(stderr,
_("cannot specify both -m crc=1 and -n ftype\n"));
@@ -1692,14 +1873,16 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
case 'r':
p = optarg;
while (*p != '\0') {
+ char **subopts = (char **)ropts.subopts;
char *value;
- switch (getsubopt(&p, (constpp)ropts, &value)) {
+ switch (getsubopt(&p, (constpp)subopts,
+ &value)) {
case R_EXTSIZE:
if (!value || *value == '\0')
- reqval('r', ropts, R_EXTSIZE);
+ reqval('r', subopts, R_EXTSIZE);
if (rtextsize)
- respec('r', ropts, R_EXTSIZE);
+ respec('r', subopts, R_EXTSIZE);
rtextsize = value;
break;
case R_FILE:
@@ -1711,16 +1894,16 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
case R_NAME:
case R_DEV:
if (!value || *value == '\0')
- reqval('r', ropts, R_NAME);
+ reqval('r', subopts, R_NAME);
if (xi.rtname)
- respec('r', ropts, R_NAME);
+ respec('r', subopts, R_NAME);
xi.rtname = value;
break;
case R_SIZE:
if (!value || *value == '\0')
- reqval('r', ropts, R_SIZE);
+ reqval('r', subopts, R_SIZE);
if (rtsize)
- respec('r', ropts, R_SIZE);
+ respec('r', subopts, R_SIZE);
rtsize = value;
break;
case R_NOALIGN:
@@ -1734,21 +1917,20 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
case 's':
p = optarg;
while (*p != '\0') {
+ char **subopts = (char **)sopts.subopts;
char *value;
- switch (getsubopt(&p, (constpp)sopts, &value)) {
+ switch (getsubopt(&p, (constpp)subopts,
+ &value)) {
case S_LOG:
case S_SECTLOG:
if (slflag || lslflag)
- respec('s', sopts, S_SECTLOG);
+ respec('s', subopts, S_SECTLOG);
if (ssflag || lssflag)
- conflict('s', sopts, S_SECTSIZE,
- S_SECTLOG);
- sectorlog = getnum_checked(value,
- XFS_MIN_SECTORSIZE_LOG,
- XFS_MAX_SECTORSIZE_LOG,
- "s sectlog", 's', sopts,
- S_SECTLOG);
+ conflict('s', subopts,
+ S_SECTSIZE, S_SECTLOG);
+ sectorlog = getnum_checked(value, &sopts,
+ S_SECTLOG);
lsectorlog = sectorlog;
sectorsize = 1 << sectorlog;
lsectorsize = sectorsize;
@@ -1757,11 +1939,11 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
case S_SIZE:
case S_SECTSIZE:
if (!value || *value == '\0')
- reqval('s', sopts, S_SECTSIZE);
+ reqval('s', subopts, S_SECTSIZE);
if (ssflag || lssflag)
- respec('s', sopts, S_SECTSIZE);
+ respec('s', subopts, S_SECTSIZE);
if (slflag || lslflag)
- conflict('s', sopts, S_SECTLOG,
+ conflict('s', subopts, S_SECTLOG,
S_SECTSIZE);
sectorsize = getnum(value, blocksize,
sectorsize, true);
--
1.8.4.rc3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 08/15] mkfs: getbool is redundant
2013-11-29 1:43 [RFC, PATCH 00/15] mkfs: sanitise input parameters Dave Chinner
` (6 preceding siblings ...)
2013-11-29 1:43 ` [PATCH 07/15] mkfs: structify input parameter passing Dave Chinner
@ 2013-11-29 1:43 ` Dave Chinner
2013-12-02 17:12 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 09/15] mkfs: use getnum_checked for all ranged parameters Dave Chinner
` (6 subsequent siblings)
14 siblings, 1 reply; 42+ messages in thread
From: Dave Chinner @ 2013-11-29 1:43 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
getbool() can be replaced with getnum_checked with appropriate
min/max values set for the boolean variables.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
mkfs/xfs_mkfs.c | 147 ++++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 106 insertions(+), 41 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 5842cc7..564f2c1 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -53,6 +53,7 @@ static long long cvtnum(unsigned int blocksize,
unsigned int sectorsize, const char *s);
#define MAX_SUBOPTS 16
+#define SUBOPT_NEEDS_VAL (-1LL)
struct opt_params {
const char name;
const char *subopts[MAX_SUBOPTS];
@@ -60,6 +61,7 @@ struct opt_params {
int index;
long long minval;
long long maxval;
+ long long defaultval;
} subopt_params[MAX_SUBOPTS];
};
@@ -76,10 +78,12 @@ const struct opt_params bopts = {
{ .index = B_LOG,
.minval = XFS_MIN_BLOCKSIZE_LOG,
.maxval = XFS_MAX_BLOCKSIZE_LOG,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = B_SIZE,
.minval = XFS_MIN_BLOCKSIZE,
.maxval = XFS_MAX_BLOCKSIZE,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
},
};
@@ -121,38 +125,55 @@ const struct opt_params dopts = {
},
.subopt_params = {
{ .index = D_AGCOUNT,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_FILE,
+ .minval = 0,
+ .maxval = 1,
+ .defaultval = 1,
},
{ .index = D_NAME,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SIZE,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SUNIT,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SWIDTH,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_AGSIZE,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SU,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SW,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SECTLOG,
.minval = XFS_MIN_SECTORSIZE_LOG,
.maxval = XFS_MAX_SECTORSIZE_LOG,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SECTSIZE,
.minval = XFS_MIN_SECTORSIZE,
.maxval = XFS_MAX_SECTORSIZE,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_NOALIGN,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_RTINHERIT,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_PROJINHERIT,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_EXTSZINHERIT,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
},
};
@@ -179,20 +200,31 @@ const struct opt_params iopts = {
},
.subopt_params = {
{ .index = I_ALIGN,
+ .minval = 0,
+ .maxval = 1,
+ .defaultval = 1,
},
{ .index = I_LOG,
.minval = XFS_DINODE_MIN_LOG,
.maxval = XFS_DINODE_MAX_LOG,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = I_MAXPCT,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = I_PERBLOCK,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = I_SIZE,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = I_ATTR,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = I_PROJID32BIT,
+ .minval = 0,
+ .maxval = 1,
+ .defaultval = 1,
},
},
};
@@ -228,32 +260,50 @@ const struct opt_params lopts = {
},
.subopt_params = {
{ .index = L_AGNUM,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_INTERNAL,
+ .minval = 0,
+ .maxval = 1,
+ .defaultval = 1,
},
{ .index = L_SIZE,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_VERSION,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_SUNIT,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_SU,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_DEV,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_SECTLOG,
.minval = XFS_MIN_SECTORSIZE_LOG,
.maxval = XFS_MAX_SECTORSIZE_LOG,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_SECTSIZE,
.minval = XFS_MIN_SECTORSIZE,
.maxval = XFS_MAX_SECTORSIZE,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_FILE,
+ .minval = 0,
+ .maxval = 1,
+ .defaultval = 1,
},
{ .index = L_NAME,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_LAZYSBCNTR,
+ .minval = 0,
+ .maxval = 1,
+ .defaultval = 1,
},
},
};
@@ -275,14 +325,20 @@ const struct opt_params nopts = {
{ .index = N_LOG,
.minval = XFS_MIN_REC_DIRSIZE,
.maxval = XFS_MAX_BLOCKSIZE_LOG,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = N_SIZE,
.minval = 1 << XFS_MIN_REC_DIRSIZE,
.maxval = XFS_MAX_BLOCKSIZE,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = N_VERSION,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = N_FTYPE,
+ .minval = 0,
+ .maxval = 1,
+ .defaultval = 0,
},
},
};
@@ -306,16 +362,22 @@ const struct opt_params ropts = {
},
.subopt_params = {
{ .index = R_EXTSIZE,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = R_SIZE,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = R_DEV,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = R_FILE,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = R_NAME,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = R_NOALIGN,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
},
};
@@ -337,18 +399,22 @@ const struct opt_params sopts = {
{ .index = S_LOG,
.minval = XFS_MIN_SECTORSIZE_LOG,
.maxval = XFS_MAX_SECTORSIZE_LOG,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = S_SECTLOG,
.minval = XFS_MIN_SECTORSIZE_LOG,
.maxval = XFS_MAX_SECTORSIZE_LOG,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = S_SIZE,
.minval = XFS_MIN_SECTORSIZE,
.maxval = XFS_MAX_SECTORSIZE,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = S_SECTSIZE,
.minval = XFS_MIN_SECTORSIZE,
.maxval = XFS_MAX_SECTORSIZE,
+ .defaultval = SUBOPT_NEEDS_VAL,
},
},
};
@@ -362,6 +428,9 @@ const struct opt_params mopts = {
},
.subopt_params = {
{ .index = M_CRC,
+ .minval = 0,
+ .maxval = 1,
+ .defaultval = 0,
},
},
};
@@ -1143,22 +1212,6 @@ getnum(
return i;
}
-static bool
-getbool(
- const char *str,
- const char *illegal_str,
- bool default_val)
-{
- long long c;
-
- if (!str || *str == '\0')
- return default_val;
- c = getnum(str, 0, 0, false);
- if (c < 0 || c > 1)
- illegal(str, illegal_str);
- return c ? true : false;
-}
-
static __attribute__((noreturn)) void
illegal_option(
const char *value,
@@ -1173,18 +1226,28 @@ illegal_option(
static int
getnum_checked(
- const char *str,
+ const char *str,
const struct opt_params *opts,
- int index)
+ int index)
{
- long long c;
+ const struct subopt_param *sp = &opts->subopt_params[index];
+ long long c;
- if (!str || *str == '\0')
+ if (sp->index != index) {
+ fprintf(stderr,
+ ("Developer screwed up option parsing (%d/%d)! Please report!\n"),
+ sp->index, index);
reqval(opts->name, (char **)opts->subopts, index);
+ }
+
+ if (!str || *str == '\0') {
+ if (sp->defaultval == SUBOPT_NEEDS_VAL)
+ reqval(opts->name, (char **)opts->subopts, index);
+ return sp->defaultval;
+ }
c = getnum(str, 0, 0, false);
- if (c < opts->subopt_params[index].minval ||
- c > opts->subopt_params[index].maxval)
+ if (c < sp->minval || c > sp->maxval)
illegal_option(str, opts, index);
return c;
}
@@ -1402,8 +1465,8 @@ main(
dasize = 1;
break;
case D_FILE:
- xi.disfile = getbool(value, "d file",
- true);
+ xi.disfile = getnum_checked(value,
+ &dopts, D_FILE);
if (xi.disfile && !Nflag)
xi.dcreat = 1;
break;
@@ -1545,8 +1608,8 @@ main(
switch (getsubopt(&p, (constpp)subopts,
&value)) {
case I_ALIGN:
- sb_feat.inode_align = getbool(
- value, "i align", true);
+ sb_feat.inode_align = getnum_checked(
+ value, &iopts, I_ALIGN);
break;
case I_LOG:
if (ilflag)
@@ -1616,8 +1679,9 @@ main(
sb_feat.attr_version = c;
break;
case I_PROJID32BIT:
- sb_feat.projid16bit = !getbool(value,
- "i projid32bit", false);
+ sb_feat.projid16bit =
+ !getnum_checked(value, &iopts,
+ I_PROJID32BIT);
break;
default:
unknown('i', value);
@@ -1648,8 +1712,8 @@ main(
if (loginternal)
conflict('l', subopts, L_INTERNAL,
L_FILE);
- xi.lisfile = getbool(value, "l file",
- true);
+ xi.lisfile = getnum_checked(value,
+ &lopts, L_FILE);
if (xi.lisfile)
xi.lcreat = 1;
break;
@@ -1662,8 +1726,8 @@ main(
if (liflag)
respec('l', subopts, L_INTERNAL);
- loginternal = getbool(value,
- "l internal", true);
+ loginternal = getnum_checked(value,
+ &lopts, L_INTERNAL);
liflag = 1;
break;
case L_SU:
@@ -1748,9 +1812,9 @@ main(
lssflag = 1;
break;
case L_LAZYSBCNTR:
- sb_feat.lazy_sb_counters = getbool(
- value, "l lazy-count",
- true);
+ sb_feat.lazy_sb_counters =
+ getnum_checked(value, &lopts,
+ L_LAZYSBCNTR);
break;
default:
unknown('l', value);
@@ -1771,8 +1835,9 @@ main(
switch (getsubopt(&p, (constpp)subopts,
&value)) {
case M_CRC:
- sb_feat.crcs_enabled = getbool(
- value, "m crc", false);
+ sb_feat.crcs_enabled =
+ getnum_checked(value, &mopts,
+ M_CRC);
if (sb_feat.crcs_enabled && nftype) {
fprintf(stderr,
_("cannot specify both -m crc=1 and -n ftype\n"));
@@ -1847,8 +1912,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
_("cannot specify both -m crc=1 and -n ftype\n"));
usage();
}
- sb_feat.dirftype = getbool(value,
- "n ftype", false);
+ sb_feat.dirftype = getnum_checked(value,
+ &nopts, N_FTYPE);
nftype = 1;
break;
default:
@@ -1886,8 +1951,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
rtextsize = value;
break;
case R_FILE:
- xi.risfile = getbool(value,
- "r file", true);
+ xi.risfile = getnum_checked(value,
+ &ropts, R_FILE);
if (xi.risfile)
xi.rcreat = 1;
break;
--
1.8.4.rc3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 09/15] mkfs: use getnum_checked for all ranged parameters
2013-11-29 1:43 [RFC, PATCH 00/15] mkfs: sanitise input parameters Dave Chinner
` (7 preceding siblings ...)
2013-11-29 1:43 ` [PATCH 08/15] mkfs: getbool is redundant Dave Chinner
@ 2013-11-29 1:43 ` Dave Chinner
2013-12-02 17:14 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 10/15] mkfs: add respecification detection to generic parsing Dave Chinner
` (5 subsequent siblings)
14 siblings, 1 reply; 42+ messages in thread
From: Dave Chinner @ 2013-11-29 1:43 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
Now that getnum_checked can handle min/max checking, use this for
all parameters that take straight numbers and don't require unit
conversions.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
include/xfs_mkfs.h | 5 +-
mkfs/xfs_mkfs.c | 152 +++++++++++++++++++++++++++--------------------------
2 files changed, 81 insertions(+), 76 deletions(-)
diff --git a/include/xfs_mkfs.h b/include/xfs_mkfs.h
index 996b690..f5639b0 100644
--- a/include/xfs_mkfs.h
+++ b/include/xfs_mkfs.h
@@ -42,8 +42,9 @@
#define XFS_AG_BYTES(bblog) ((long long)BBSIZE << (bblog))
#define XFS_AG_MIN_BYTES ((XFS_AG_BYTES(15))) /* 16 MB */
-#define XFS_AG_MIN_BLOCKS(blog) ((XFS_AG_BYTES(15)) >> (blog))
-#define XFS_AG_MAX_BLOCKS(blog) ((XFS_AG_BYTES(31) - 1) >> (blog))
+#define XFS_AG_MAX_BYTES ((XFS_AG_BYTES(31))) /* 1 TB */
+#define XFS_AG_MIN_BLOCKS(blog) (XFS_AG_MIN_BYTES >> (blog))
+#define XFS_AG_MAX_BLOCKS(blog) ((XFS_AG_MAX_BYTES - 1) >> (blog))
#define XFS_MAX_AGNUMBER ((xfs_agnumber_t)(NULLAGNUMBER - 1))
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 564f2c1..2f51f5b 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -125,6 +125,8 @@ const struct opt_params dopts = {
},
.subopt_params = {
{ .index = D_AGCOUNT,
+ .minval = 1,
+ .maxval = XFS_MAX_AGNUMBER,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_FILE,
@@ -139,18 +141,26 @@ const struct opt_params dopts = {
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SUNIT,
+ .minval = 0,
+ .maxval = UINT_MAX,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SWIDTH,
+ .minval = 0,
+ .maxval = UINT_MAX,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_AGSIZE,
+ .minval = XFS_AG_MIN_BYTES,
+ .maxval = XFS_AG_MAX_BYTES,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SU,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SW,
+ .minval = 0,
+ .maxval = UINT_MAX,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SECTLOG,
@@ -164,15 +174,23 @@ const struct opt_params dopts = {
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_NOALIGN,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .minval = 1,
+ .maxval = 1,
+ .defaultval = 1,
},
{ .index = D_RTINHERIT,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .minval = 1,
+ .maxval = 1,
+ .defaultval = 1,
},
{ .index = D_PROJINHERIT,
+ .minval = 0,
+ .maxval = UINT_MAX,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_EXTSZINHERIT,
+ .minval = 0,
+ .maxval = UINT_MAX,
.defaultval = SUBOPT_NEEDS_VAL,
},
},
@@ -210,15 +228,23 @@ const struct opt_params iopts = {
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = I_MAXPCT,
+ .minval = 0,
+ .maxval = 100,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = I_PERBLOCK,
+ .minval = XFS_MIN_INODE_PERBLOCK,
+ .maxval = XFS_MAX_BLOCKSIZE / XFS_DINODE_MIN_SIZE,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = I_SIZE,
+ .minval = XFS_DINODE_MIN_SIZE,
+ .maxval = XFS_DINODE_MAX_SIZE,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = I_ATTR,
+ .minval = 0,
+ .maxval = 2,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = I_PROJID32BIT,
@@ -260,6 +286,8 @@ const struct opt_params lopts = {
},
.subopt_params = {
{ .index = L_AGNUM,
+ .minval = 0,
+ .maxval = UINT_MAX,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_INTERNAL,
@@ -271,9 +299,13 @@ const struct opt_params lopts = {
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_VERSION,
+ .minval = 1,
+ .maxval = 2,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_SUNIT,
+ .minval = BTOBB(XLOG_MIN_RECORD_BSIZE),
+ .maxval = BTOBB(XLOG_MAX_RECORD_BSIZE),
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_SU,
@@ -333,6 +365,8 @@ const struct opt_params nopts = {
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = N_VERSION,
+ .minval = 2,
+ .maxval = 2,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = N_FTYPE,
@@ -1444,13 +1478,11 @@ main(
switch (getsubopt(&p, (constpp)subopts,
&value)) {
case D_AGCOUNT:
- if (!value || *value == '\0')
- reqval('d', subopts, D_AGCOUNT);
if (daflag)
respec('d', subopts, D_AGCOUNT);
- agcount = getnum(value, 0, 0, false);
- if ((__int64_t)agcount <= 0)
- illegal(value, "d agcount");
+
+ agcount = getnum_checked(value, &dopts,
+ D_AGCOUNT);
daflag = 1;
break;
case D_AGSIZE:
@@ -1485,28 +1517,22 @@ main(
dsize = value;
break;
case D_SUNIT:
- if (!value || *value == '\0')
- reqval('d', subopts, D_SUNIT);
if (dsunit)
respec('d', subopts, D_SUNIT);
if (nodsflag)
conflict('d', subopts, D_NOALIGN,
D_SUNIT);
- dsunit = getnum(value, 0, 0, false);
- if (dsunit < 0)
- illegal(value, "d sunit");
+ dsunit = getnum_checked(value, &dopts,
+ D_SUNIT);
break;
case D_SWIDTH:
- if (!value || *value == '\0')
- reqval('d', subopts, D_SWIDTH);
if (dswidth)
respec('d', subopts, D_SWIDTH);
if (nodsflag)
conflict('d', subopts, D_NOALIGN,
D_SWIDTH);
- dswidth = getnum(value, 0, 0, false);
- if (dswidth < 0)
- illegal(value, "d swidth");
+ dswidth = getnum_checked(value, &dopts,
+ D_SWIDTH);
break;
case D_SU:
if (!value || *value == '\0')
@@ -1522,16 +1548,13 @@ main(
illegal(value, "d su");
break;
case D_SW:
- if (!value || *value == '\0')
- reqval('d', subopts, D_SW);
if (dsw)
respec('d', subopts, D_SW);
if (nodsflag)
conflict('d', subopts, D_NOALIGN,
D_SW);
- dsw = getnum(value, 0, 0, false);
- if (dsw < 0)
- illegal(value, "d sw");
+ dsw = getnum_checked(value, &dopts,
+ D_SW);
break;
case D_NOALIGN:
if (dsu)
@@ -1577,21 +1600,22 @@ main(
ssflag = 1;
break;
case D_RTINHERIT:
- fsx.fsx_xflags |= \
- XFS_DIFLAG_RTINHERIT;
+ c = getnum_checked(value, &dopts,
+ D_RTINHERIT);
+ if (c)
+ fsx.fsx_xflags |=
+ XFS_DIFLAG_RTINHERIT;
break;
case D_PROJINHERIT:
- if (!value || *value == '\0')
- reqval('d', subopts, D_PROJINHERIT);
- fsx.fsx_projid = atoi(value);
- fsx.fsx_xflags |= \
+ fsx.fsx_projid = getnum_checked(value,
+ &dopts, D_PROJINHERIT);
+ fsx.fsx_xflags |=
XFS_DIFLAG_PROJINHERIT;
break;
case D_EXTSZINHERIT:
- if (!value || *value == '\0')
- reqval('d', subopts, D_EXTSZINHERIT);
- fsx.fsx_extsize = atoi(value);
- fsx.fsx_xflags |= \
+ fsx.fsx_extsize = getnum_checked(value,
+ &dopts, D_EXTSZINHERIT);
+ fsx.fsx_xflags |=
XFS_DIFLAG_EXTSZINHERIT;
break;
default:
@@ -1626,18 +1650,13 @@ main(
ilflag = 1;
break;
case I_MAXPCT:
- if (!value || *value == '\0')
- reqval('i', subopts, I_MAXPCT);
if (imflag)
respec('i', subopts, I_MAXPCT);
- imaxpct = getnum(value, 0, 0, false);
- if (imaxpct < 0 || imaxpct > 100)
- illegal(value, "i maxpct");
+ imaxpct = getnum_checked(value, &iopts,
+ I_MAXPCT);
imflag = 1;
break;
case I_PERBLOCK:
- if (!value || *value == '\0')
- reqval('i', subopts, I_PERBLOCK);
if (ilflag)
conflict('i', subopts, I_LOG,
I_PERBLOCK);
@@ -1646,16 +1665,13 @@ main(
if (isflag)
conflict('i', subopts, I_SIZE,
I_PERBLOCK);
- inopblock = getnum(value, 0, 0, false);
- if (inopblock <
- XFS_MIN_INODE_PERBLOCK ||
- !ispow2(inopblock))
+ inopblock = getnum_checked(value, &iopts,
+ I_PERBLOCK);
+ if (!ispow2(inopblock))
illegal(value, "i perblock");
ipflag = 1;
break;
case I_SIZE:
- if (!value || *value == '\0')
- reqval('i', subopts, I_SIZE);
if (ilflag)
conflict('i', subopts, I_LOG,
I_SIZE);
@@ -1664,19 +1680,17 @@ main(
I_SIZE);
if (isflag)
respec('i', subopts, I_SIZE);
- isize = getnum(value, 0, 0, true);
- if (isize <= 0 || !ispow2(isize))
+ isize = getnum_checked(value, &iopts,
+ I_SIZE);
+ if (!ispow2(isize))
illegal(value, "i size");
inodelog = libxfs_highbit32(isize);
isflag = 1;
break;
case I_ATTR:
- if (!value || *value == '\0')
- reqval('i', subopts, I_ATTR);
- c = getnum(value, 0, 0, false);
- if (c < 0 || c > 2)
- illegal(value, "i attr");
- sb_feat.attr_version = c;
+ sb_feat.attr_version =
+ getnum_checked(value, &iopts,
+ I_ATTR);
break;
case I_PROJID32BIT:
sb_feat.projid16bit =
@@ -1697,15 +1711,12 @@ main(
switch (getsubopt(&p, (constpp)subopts,
&value)) {
case L_AGNUM:
- if (!value || *value == '\0')
- reqval('l', subopts, L_AGNUM);
if (laflag)
respec('l', subopts, L_AGNUM);
if (ldflag)
conflict('l', subopts, L_AGNUM, L_DEV);
- logagno = getnum(value, 0, 0, false);
- if (logagno < 0)
- illegal(value, "l agno");
+ logagno = getnum_checked(value, &lopts,
+ L_AGNUM);
laflag = 1;
break;
case L_FILE:
@@ -1741,13 +1752,10 @@ main(
illegal(value, "l su");
break;
case L_SUNIT:
- if (!value || *value == '\0')
- reqval('l', subopts, L_SUNIT);
if (lsunit)
respec('l', subopts, L_SUNIT);
- lsunit = getnum(value, 0, 0, false);
- if (lsunit < 0)
- illegal(value, "l sunit");
+ lsunit = getnum_checked(value, &lopts,
+ L_SUNIT);
break;
case L_NAME:
case L_DEV:
@@ -1765,14 +1773,11 @@ main(
xi.logname = value;
break;
case L_VERSION:
- if (!value || *value == '\0')
- reqval('l', subopts, L_VERSION);
if (lvflag)
respec('l', subopts, L_VERSION);
- c = getnum(value, 0, 0, false);
- if (c < 1 || c > 2)
- illegal(value, "l version");
- sb_feat.log_version = c;
+ sb_feat.log_version =
+ getnum_checked(value, &lopts,
+ L_VERSION);
lvflag = 1;
break;
case L_SIZE:
@@ -1896,11 +1901,10 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
/* ASCII CI mode */
sb_feat.nci = true;
} else {
- c = getnum(value, 0, 0, false);
- if (c != 2)
- illegal(value,
- "n version");
- sb_feat.dir_version = c;
+ sb_feat.dir_version =
+ getnum_checked(value,
+ &nopts,
+ N_VERSION);
}
nvflag = 1;
break;
--
1.8.4.rc3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 10/15] mkfs: add respecification detection to generic parsing
2013-11-29 1:43 [RFC, PATCH 00/15] mkfs: sanitise input parameters Dave Chinner
` (8 preceding siblings ...)
2013-11-29 1:43 ` [PATCH 09/15] mkfs: use getnum_checked for all ranged parameters Dave Chinner
@ 2013-11-29 1:43 ` Dave Chinner
2013-12-02 17:15 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 11/15] mkfs: table based parsing for converted parameters Dave Chinner
` (4 subsequent siblings)
14 siblings, 1 reply; 42+ messages in thread
From: Dave Chinner @ 2013-11-29 1:43 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
Add flags to the generic input parameter tables so that
respecification can be detected in a generic manner.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
mkfs/xfs_mkfs.c | 64 +++++++++++++++------------------------------------------
1 file changed, 17 insertions(+), 47 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 2f51f5b..af9b9c4 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -57,15 +57,17 @@ static long long cvtnum(unsigned int blocksize,
struct opt_params {
const char name;
const char *subopts[MAX_SUBOPTS];
+
struct subopt_param {
int index;
+ bool seen;
long long minval;
long long maxval;
long long defaultval;
} subopt_params[MAX_SUBOPTS];
};
-const struct opt_params bopts = {
+struct opt_params bopts = {
.name = 'b',
.subopts = {
#define B_LOG 0
@@ -88,7 +90,7 @@ const struct opt_params bopts = {
},
};
-const struct opt_params dopts = {
+struct opt_params dopts = {
.name = 'd',
.subopts = {
#define D_AGCOUNT 0
@@ -197,7 +199,7 @@ const struct opt_params dopts = {
};
-const struct opt_params iopts = {
+struct opt_params iopts = {
.name = 'i',
.subopts = {
#define I_ALIGN 0
@@ -255,7 +257,7 @@ const struct opt_params iopts = {
},
};
-const struct opt_params lopts = {
+struct opt_params lopts = {
.name = 'l',
.subopts = {
#define L_AGNUM 0
@@ -340,7 +342,7 @@ const struct opt_params lopts = {
},
};
-const struct opt_params nopts = {
+struct opt_params nopts = {
.name = 'n',
.subopts = {
#define N_LOG 0
@@ -377,7 +379,7 @@ const struct opt_params nopts = {
},
};
-const struct opt_params ropts = {
+struct opt_params ropts = {
.name = 'r',
.subopts = {
#define R_EXTSIZE 0
@@ -416,7 +418,7 @@ const struct opt_params ropts = {
},
};
-const struct opt_params sopts = {
+struct opt_params sopts = {
.name = 's',
.subopts = {
#define S_LOG 0
@@ -453,7 +455,7 @@ const struct opt_params sopts = {
},
};
-const struct opt_params mopts = {
+struct opt_params mopts = {
.name = 'm',
.subopts = {
#define M_CRC 0
@@ -1261,10 +1263,10 @@ illegal_option(
static int
getnum_checked(
const char *str,
- const struct opt_params *opts,
+ struct opt_params *opts,
int index)
{
- const struct subopt_param *sp = &opts->subopt_params[index];
+ struct subopt_param *sp = &opts->subopt_params[index];
long long c;
if (sp->index != index) {
@@ -1274,6 +1276,11 @@ getnum_checked(
reqval(opts->name, (char **)opts->subopts, index);
}
+ /* check for respecification of the option */
+ if (sp->seen)
+ respec(opts->name, (char **)opts->subopts, index);
+ sp->seen = true;
+
if (!str || *str == '\0') {
if (sp->defaultval == SUBOPT_NEEDS_VAL)
reqval(opts->name, (char **)opts->subopts, index);
@@ -1438,8 +1445,6 @@ main(
switch (getsubopt(&p, (constpp)subopts,
&value)) {
case B_LOG:
- if (blflag)
- respec('b', subopts, B_LOG);
if (bsflag)
conflict('b', subopts, B_SIZE,
B_LOG);
@@ -1478,9 +1483,6 @@ main(
switch (getsubopt(&p, (constpp)subopts,
&value)) {
case D_AGCOUNT:
- if (daflag)
- respec('d', subopts, D_AGCOUNT);
-
agcount = getnum_checked(value, &dopts,
D_AGCOUNT);
daflag = 1;
@@ -1517,8 +1519,6 @@ main(
dsize = value;
break;
case D_SUNIT:
- if (dsunit)
- respec('d', subopts, D_SUNIT);
if (nodsflag)
conflict('d', subopts, D_NOALIGN,
D_SUNIT);
@@ -1526,8 +1526,6 @@ main(
D_SUNIT);
break;
case D_SWIDTH:
- if (dswidth)
- respec('d', subopts, D_SWIDTH);
if (nodsflag)
conflict('d', subopts, D_NOALIGN,
D_SWIDTH);
@@ -1548,8 +1546,6 @@ main(
illegal(value, "d su");
break;
case D_SW:
- if (dsw)
- respec('d', subopts, D_SW);
if (nodsflag)
conflict('d', subopts, D_NOALIGN,
D_SW);
@@ -1572,8 +1568,6 @@ main(
nodsflag = 1;
break;
case D_SECTLOG:
- if (slflag)
- respec('d', subopts, D_SECTLOG);
if (ssflag)
conflict('d', subopts, D_SECTSIZE,
D_SECTLOG);
@@ -1636,8 +1630,6 @@ main(
value, &iopts, I_ALIGN);
break;
case I_LOG:
- if (ilflag)
- respec('i', subopts, I_LOG);
if (ipflag)
conflict('i', subopts, I_PERBLOCK,
I_LOG);
@@ -1650,8 +1642,6 @@ main(
ilflag = 1;
break;
case I_MAXPCT:
- if (imflag)
- respec('i', subopts, I_MAXPCT);
imaxpct = getnum_checked(value, &iopts,
I_MAXPCT);
imflag = 1;
@@ -1660,8 +1650,6 @@ main(
if (ilflag)
conflict('i', subopts, I_LOG,
I_PERBLOCK);
- if (ipflag)
- respec('i', subopts, I_PERBLOCK);
if (isflag)
conflict('i', subopts, I_SIZE,
I_PERBLOCK);
@@ -1678,8 +1666,6 @@ main(
if (ipflag)
conflict('i', subopts, I_PERBLOCK,
I_SIZE);
- if (isflag)
- respec('i', subopts, I_SIZE);
isize = getnum_checked(value, &iopts,
I_SIZE);
if (!ispow2(isize))
@@ -1711,8 +1697,6 @@ main(
switch (getsubopt(&p, (constpp)subopts,
&value)) {
case L_AGNUM:
- if (laflag)
- respec('l', subopts, L_AGNUM);
if (ldflag)
conflict('l', subopts, L_AGNUM, L_DEV);
logagno = getnum_checked(value, &lopts,
@@ -1734,8 +1718,6 @@ main(
if (xi.lisfile)
conflict('l', subopts, L_FILE,
L_INTERNAL);
- if (liflag)
- respec('l', subopts, L_INTERNAL);
loginternal = getnum_checked(value,
&lopts, L_INTERNAL);
@@ -1752,8 +1734,6 @@ main(
illegal(value, "l su");
break;
case L_SUNIT:
- if (lsunit)
- respec('l', subopts, L_SUNIT);
lsunit = getnum_checked(value, &lopts,
L_SUNIT);
break;
@@ -1773,8 +1753,6 @@ main(
xi.logname = value;
break;
case L_VERSION:
- if (lvflag)
- respec('l', subopts, L_VERSION);
sb_feat.log_version =
getnum_checked(value, &lopts,
L_VERSION);
@@ -1789,8 +1767,6 @@ main(
lsflag = 1;
break;
case L_SECTLOG:
- if (lslflag)
- respec('l', subopts, L_SECTLOG);
if (lssflag)
conflict('l', subopts, L_SECTSIZE,
L_SECTLOG);
@@ -1865,8 +1841,6 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
switch (getsubopt(&p, (constpp)subopts,
&value)) {
case N_LOG:
- if (nlflag)
- respec('n', subopts, N_LOG);
if (nsflag)
conflict('n', subopts, N_SIZE,
N_LOG);
@@ -1909,8 +1883,6 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
nvflag = 1;
break;
case N_FTYPE:
- if (nftype)
- respec('n', subopts, N_FTYPE);
if (sb_feat.crcs_enabled) {
fprintf(stderr,
_("cannot specify both -m crc=1 and -n ftype\n"));
@@ -1993,8 +1965,6 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
&value)) {
case S_LOG:
case S_SECTLOG:
- if (slflag || lslflag)
- respec('s', subopts, S_SECTLOG);
if (ssflag || lssflag)
conflict('s', subopts,
S_SECTSIZE, S_SECTLOG);
--
1.8.4.rc3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 11/15] mkfs: table based parsing for converted parameters
2013-11-29 1:43 [RFC, PATCH 00/15] mkfs: sanitise input parameters Dave Chinner
` (9 preceding siblings ...)
2013-11-29 1:43 ` [PATCH 10/15] mkfs: add respecification detection to generic parsing Dave Chinner
@ 2013-11-29 1:43 ` Dave Chinner
2013-12-02 17:17 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 12/15] mkfs: merge getnum Dave Chinner
` (3 subsequent siblings)
14 siblings, 1 reply; 42+ messages in thread
From: Dave Chinner @ 2013-11-29 1:43 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
All the parameters that can be passed as block or sector sizes need
to be passed the block and sector sizes that they should be using
for conversion. For parameter parsing, it is always the same two
variables, so to make things easy just declare them as global
variables so we can avoid needing to pass them to getnum_checked().
We also need to mark these parameters are requiring conversion so
that we don't need to pass this information manually to
getnum_checked(). Further, some of these options are required to
have a power of 2 value, so add optional checking for that as well.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
mkfs/xfs_mkfs.c | 171 ++++++++++++++++++++++----------------------------------
1 file changed, 68 insertions(+), 103 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index af9b9c4..e7bee06 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -52,6 +52,13 @@ static int ispow2(unsigned int i);
static long long cvtnum(unsigned int blocksize,
unsigned int sectorsize, const char *s);
+/*
+ * The configured block and sector sizes are defined as global variables so
+ * that they don't need to be passed to functions that require them.
+ */
+unsigned int blocksize;
+unsigned int sectorsize;
+
#define MAX_SUBOPTS 16
#define SUBOPT_NEEDS_VAL (-1LL)
struct opt_params {
@@ -61,6 +68,8 @@ struct opt_params {
struct subopt_param {
int index;
bool seen;
+ bool convert;
+ bool is_power_2;
long long minval;
long long maxval;
long long defaultval;
@@ -83,6 +92,8 @@ struct opt_params bopts = {
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = B_SIZE,
+ .convert = true,
+ .is_power_2 = true,
.minval = XFS_MIN_BLOCKSIZE,
.maxval = XFS_MAX_BLOCKSIZE,
.defaultval = SUBOPT_NEEDS_VAL,
@@ -140,6 +151,9 @@ struct opt_params dopts = {
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SIZE,
+ .convert = true,
+ .minval = XFS_AG_MIN_BYTES,
+ .maxval = LLONG_MAX,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SUNIT,
@@ -153,11 +167,15 @@ struct opt_params dopts = {
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_AGSIZE,
+ .convert = true,
.minval = XFS_AG_MIN_BYTES,
.maxval = XFS_AG_MAX_BYTES,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SU,
+ .convert = true,
+ .minval = 0,
+ .maxval = UINT_MAX,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SW,
@@ -171,6 +189,8 @@ struct opt_params dopts = {
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SECTSIZE,
+ .convert = true,
+ .is_power_2 = true,
.minval = XFS_MIN_SECTORSIZE,
.maxval = XFS_MAX_SECTORSIZE,
.defaultval = SUBOPT_NEEDS_VAL,
@@ -235,11 +255,13 @@ struct opt_params iopts = {
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = I_PERBLOCK,
+ .is_power_2 = true,
.minval = XFS_MIN_INODE_PERBLOCK,
.maxval = XFS_MAX_BLOCKSIZE / XFS_DINODE_MIN_SIZE,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = I_SIZE,
+ .is_power_2 = true,
.minval = XFS_DINODE_MIN_SIZE,
.maxval = XFS_DINODE_MAX_SIZE,
.defaultval = SUBOPT_NEEDS_VAL,
@@ -298,6 +320,9 @@ struct opt_params lopts = {
.defaultval = 1,
},
{ .index = L_SIZE,
+ .convert = true,
+ .minval = 2 * 1024 * 1024LL, /* XXX: XFS_MIN_LOG_BYTES */
+ .maxval = XFS_MAX_LOG_BYTES,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_VERSION,
@@ -311,6 +336,9 @@ struct opt_params lopts = {
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_SU,
+ .convert = true,
+ .minval = XLOG_MIN_RECORD_BSIZE,
+ .maxval = XLOG_MAX_RECORD_BSIZE,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_DEV,
@@ -322,6 +350,8 @@ struct opt_params lopts = {
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_SECTSIZE,
+ .convert = true,
+ .is_power_2 = true,
.minval = XFS_MIN_SECTORSIZE,
.maxval = XFS_MAX_SECTORSIZE,
.defaultval = SUBOPT_NEEDS_VAL,
@@ -362,6 +392,8 @@ struct opt_params nopts = {
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = N_SIZE,
+ .convert = true,
+ .is_power_2 = true,
.minval = 1 << XFS_MIN_REC_DIRSIZE,
.maxval = XFS_MAX_BLOCKSIZE,
.defaultval = SUBOPT_NEEDS_VAL,
@@ -398,9 +430,15 @@ struct opt_params ropts = {
},
.subopt_params = {
{ .index = R_EXTSIZE,
+ .convert = true,
+ .minval = XFS_MIN_RTEXTSIZE,
+ .maxval = XFS_MAX_RTEXTSIZE,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = R_SIZE,
+ .convert = true,
+ .minval = 0,
+ .maxval = LLONG_MAX,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = R_DEV,
@@ -443,11 +481,15 @@ struct opt_params sopts = {
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = S_SIZE,
+ .convert = true,
+ .is_power_2 = true,
.minval = XFS_MIN_SECTORSIZE,
.maxval = XFS_MAX_SECTORSIZE,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = S_SECTSIZE,
+ .convert = true,
+ .is_power_2 = true,
.minval = XFS_MIN_SECTORSIZE,
.maxval = XFS_MAX_SECTORSIZE,
.defaultval = SUBOPT_NEEDS_VAL,
@@ -1230,15 +1272,15 @@ sb_set_features(
long long
getnum(
const char *str,
- unsigned int blocksize,
- unsigned int sectorsize,
+ unsigned int blksize,
+ unsigned int sectsize,
bool convert)
{
long long i;
char *sp;
if (convert)
- return cvtnum(blocksize, sectorsize, str);
+ return cvtnum(blksize, sectsize, str);
i = strtoll(str, &sp, 0);
if (i == 0 && sp == str)
@@ -1287,9 +1329,11 @@ getnum_checked(
return sp->defaultval;
}
- c = getnum(str, 0, 0, false);
+ c = getnum(str, blocksize, sectorsize, sp->convert);
if (c < sp->minval || c > sp->maxval)
illegal_option(str, opts, index);
+ if (sp->is_power_2 && !ispow2(c))
+ illegal_option(str, opts, index);
return c;
}
@@ -1307,7 +1351,6 @@ main(
struct xfs_btree_block *block;
int blflag;
int blocklog;
- unsigned int blocksize;
int bsflag;
int bsize;
xfs_buf_t *buf;
@@ -1377,7 +1420,6 @@ main(
char *rtsize;
xfs_sb_t *sbp;
int sectorlog;
- unsigned int sectorsize;
__uint64_t sector_mask;
int slflag;
int ssflag;
@@ -1454,18 +1496,11 @@ main(
blflag = 1;
break;
case B_SIZE:
- if (!value || *value == '\0')
- reqval('b', subopts, B_SIZE);
- if (bsflag)
- respec('b', subopts, B_SIZE);
if (blflag)
conflict('b', subopts, B_LOG,
B_SIZE);
- blocksize = getnum(value, blocksize,
- sectorsize, true);
- if (blocksize <= 0 ||
- !ispow2(blocksize))
- illegal(value, "b size");
+ blocksize = getnum_checked(value, &bopts,
+ B_SIZE);
blocklog = libxfs_highbit32(blocksize);
bsflag = 1;
break;
@@ -1488,14 +1523,8 @@ main(
daflag = 1;
break;
case D_AGSIZE:
- if (!value || *value == '\0')
- reqval('d', subopts, D_AGSIZE);
- if (dasize)
- respec('d', subopts, D_AGSIZE);
- agsize = getnum(value, blocksize,
- sectorsize, true);
- if ((__int64_t)agsize <= 0)
- illegal(value, "d agsize");
+ agsize = getnum_checked(value, &dopts,
+ D_AGSIZE);
dasize = 1;
break;
case D_FILE:
@@ -1533,17 +1562,11 @@ main(
D_SWIDTH);
break;
case D_SU:
- if (!value || *value == '\0')
- reqval('d', subopts, D_SU);
- if (dsu)
- respec('d', subopts, D_SU);
if (nodsflag)
conflict('d', subopts, D_NOALIGN,
D_SU);
- dsu = getnum(value, blocksize,
- sectorsize, true);
- if (dsu < 0)
- illegal(value, "d su");
+ dsu = getnum_checked(value, &dopts,
+ D_SU);
break;
case D_SW:
if (nodsflag)
@@ -1577,18 +1600,11 @@ main(
slflag = 1;
break;
case D_SECTSIZE:
- if (!value || *value == '\0')
- reqval('d', subopts, D_SECTSIZE);
- if (ssflag)
- respec('d', subopts, D_SECTSIZE);
if (slflag)
conflict('d', subopts, D_SECTLOG,
D_SECTSIZE);
- sectorsize = getnum(value, blocksize,
- sectorsize, true);
- if (sectorsize <= 0 ||
- !ispow2(sectorsize))
- illegal(value, "d sectsize");
+ sectorsize = getnum_checked(value,
+ &dopts, D_SECTSIZE);
sectorlog =
libxfs_highbit32(sectorsize);
ssflag = 1;
@@ -1655,8 +1671,6 @@ main(
I_PERBLOCK);
inopblock = getnum_checked(value, &iopts,
I_PERBLOCK);
- if (!ispow2(inopblock))
- illegal(value, "i perblock");
ipflag = 1;
break;
case I_SIZE:
@@ -1668,8 +1682,6 @@ main(
I_SIZE);
isize = getnum_checked(value, &iopts,
I_SIZE);
- if (!ispow2(isize))
- illegal(value, "i size");
inodelog = libxfs_highbit32(isize);
isflag = 1;
break;
@@ -1724,14 +1736,8 @@ main(
liflag = 1;
break;
case L_SU:
- if (!value || *value == '\0')
- reqval('l', subopts, L_SU);
- if (lsu)
- respec('l', subopts, L_SU);
- lsu = getnum(value, blocksize,
- sectorsize, true);
- if (lsu < 0)
- illegal(value, "l su");
+ lsu = getnum_checked(value, &lopts,
+ L_SU);
break;
case L_SUNIT:
lsunit = getnum_checked(value, &lopts,
@@ -1776,18 +1782,11 @@ main(
lslflag = 1;
break;
case L_SECTSIZE:
- if (!value || *value == '\0')
- reqval('l', subopts, L_SECTSIZE);
- if (lssflag)
- respec('l', subopts, L_SECTSIZE);
if (lslflag)
conflict('l', subopts, L_SECTLOG,
L_SECTSIZE);
- lsectorsize = getnum(value, blocksize,
- sectorsize, true);
- if (lsectorsize <= 0 ||
- !ispow2(lsectorsize))
- illegal(value, "l sectsize");
+ lsectorsize = getnum_checked(value,
+ &lopts, L_SECTSIZE);
lsectorlog =
libxfs_highbit32(lsectorsize);
lssflag = 1;
@@ -1850,18 +1849,11 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
nlflag = 1;
break;
case N_SIZE:
- if (!value || *value == '\0')
- reqval('n', subopts, N_SIZE);
- if (nsflag)
- respec('n', subopts, N_SIZE);
if (nlflag)
conflict('n', subopts, N_LOG,
N_SIZE);
- dirblocksize = getnum(value, blocksize,
- sectorsize, true);
- if (dirblocksize <= 0 ||
- !ispow2(dirblocksize))
- illegal(value, "n size");
+ dirblocksize = getnum_checked(value,
+ &nopts, N_SIZE);
dirblocklog =
libxfs_highbit32(dirblocksize);
nsflag = 1;
@@ -1977,18 +1969,11 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
break;
case S_SIZE:
case S_SECTSIZE:
- if (!value || *value == '\0')
- reqval('s', subopts, S_SECTSIZE);
- if (ssflag || lssflag)
- respec('s', subopts, S_SECTSIZE);
if (slflag || lslflag)
conflict('s', subopts, S_SECTLOG,
S_SECTSIZE);
- sectorsize = getnum(value, blocksize,
- sectorsize, true);
- if (sectorsize <= 0 ||
- !ispow2(sectorsize))
- illegal(value, "s sectsize");
+ sectorsize = getnum_checked(value,
+ &sopts, S_SECTSIZE);
lsectorsize = sectorsize;
sectorlog =
libxfs_highbit32(sectorsize);
@@ -2197,9 +2182,7 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
if (dsize) {
__uint64_t dbytes;
- dbytes = getnum(dsize, blocksize, sectorsize, true);
- if ((__int64_t)dbytes < 0)
- illegal(dsize, "d size");
+ dbytes = getnum_checked(dsize, &dopts, D_SIZE);
if (dbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
_("illegal data length %lld, not a multiple of %d\n"),
@@ -2236,9 +2219,7 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
if (logsize) {
__uint64_t logbytes;
- logbytes = getnum(logsize, blocksize, sectorsize, true);
- if ((__int64_t)logbytes < 0)
- illegal(logsize, "l size");
+ logbytes = getnum_checked(logsize, &lopts, L_SIZE);
if (logbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
_("illegal log length %lld, not a multiple of %d\n"),
@@ -2260,9 +2241,7 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
if (rtsize) {
__uint64_t rtbytes;
- rtbytes = getnum(rtsize, blocksize, sectorsize, true);
- if ((__int64_t)rtbytes < 0)
- illegal(rtsize, "r size");
+ rtbytes = getnum_checked(rtsize, &ropts, R_SIZE);
if (rtbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
_("illegal rt length %lld, not a multiple of %d\n"),
@@ -2282,27 +2261,13 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
if (rtextsize) {
__uint64_t rtextbytes;
- rtextbytes = getnum(rtextsize, blocksize, sectorsize, true);
- if ((__int64_t)rtextbytes < 0)
- illegal(rtsize, "r extsize");
+ rtextbytes = getnum_checked(rtextsize, &ropts, R_EXTSIZE);
if (rtextbytes % blocksize) {
fprintf(stderr,
_("illegal rt extent size %lld, not a multiple of %d\n"),
(long long)rtextbytes, blocksize);
usage();
}
- if (rtextbytes > XFS_MAX_RTEXTSIZE) {
- fprintf(stderr,
- _("rt extent size %s too large, maximum %d\n"),
- rtextsize, XFS_MAX_RTEXTSIZE);
- usage();
- }
- if (rtextbytes < XFS_MIN_RTEXTSIZE) {
- fprintf(stderr,
- _("rt extent size %s too small, minimum %d\n"),
- rtextsize, XFS_MIN_RTEXTSIZE);
- usage();
- }
rtextblocks = (xfs_extlen_t)(rtextbytes >> blocklog);
} else {
/*
--
1.8.4.rc3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 12/15] mkfs: merge getnum
2013-11-29 1:43 [RFC, PATCH 00/15] mkfs: sanitise input parameters Dave Chinner
` (10 preceding siblings ...)
2013-11-29 1:43 ` [PATCH 11/15] mkfs: table based parsing for converted parameters Dave Chinner
@ 2013-11-29 1:43 ` Dave Chinner
2013-12-02 17:22 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 13/15] mkfs: encode conflicts into parsing table Dave Chinner
` (2 subsequent siblings)
14 siblings, 1 reply; 42+ messages in thread
From: Dave Chinner @ 2013-11-29 1:43 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
getnum() is now only called by getnum_checked(). Move the two
together into a single getnum() function and change all the callers
back to getnum().
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
include/xfs_mkfs.h | 4 +-
mkfs/proto.c | 20 ++++++
mkfs/xfs_mkfs.c | 202 ++++++++++++++++++++++++-----------------------------
3 files changed, 113 insertions(+), 113 deletions(-)
diff --git a/include/xfs_mkfs.h b/include/xfs_mkfs.h
index f5639b0..a4312e1 100644
--- a/include/xfs_mkfs.h
+++ b/include/xfs_mkfs.h
@@ -57,8 +57,8 @@
#define XFS_NOMULTIDISK_AGLOG 2 /* 4 AGs */
#define XFS_MULTIDISK_AGCOUNT (1 << XFS_MULTIDISK_AGLOG)
-extern long long getnum(const char *str, unsigned int blocksize,
- unsigned int sectorsize, bool convert);
+extern long long cvtnum(unsigned int blksize, unsigned int sectsize,
+ const char *str);
/* proto.c */
extern char *setup_proto (char *fname);
diff --git a/mkfs/proto.c b/mkfs/proto.c
index da5115b..0477d65 100644
--- a/mkfs/proto.c
+++ b/mkfs/proto.c
@@ -43,6 +43,26 @@ static long filesize(int fd);
((uint)(MKFS_BLOCKRES_INODE + XFS_DA_NODE_MAXDEPTH + \
(XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK) - 1) + (rb)))
+static long long
+getnum(
+ const char *str,
+ unsigned int blksize,
+ unsigned int sectsize,
+ bool convert)
+{
+ long long i;
+ char *sp;
+
+ if (convert)
+ return cvtnum(blksize, sectsize, str);
+
+ i = strtoll(str, &sp, 0);
+ if (i == 0 && sp == str)
+ return -1LL;
+ if (*sp != '\0')
+ return -1LL; /* trailing garbage */
+ return i;
+}
char *
setup_proto(
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index e7bee06..827c300 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -49,9 +49,6 @@ static void respec(char opt, char *tab[], int idx);
static void unknown(char opt, char *s);
static int ispow2(unsigned int i);
-static long long cvtnum(unsigned int blocksize,
- unsigned int sectorsize, const char *s);
-
/*
* The configured block and sector sizes are defined as global variables so
* that they don't need to be passed to functions that require them.
@@ -1269,27 +1266,6 @@ sb_set_features(
sbp->sb_features_log_incompat = 0;
}
-long long
-getnum(
- const char *str,
- unsigned int blksize,
- unsigned int sectsize,
- bool convert)
-{
- long long i;
- char *sp;
-
- if (convert)
- return cvtnum(blksize, sectsize, str);
-
- i = strtoll(str, &sp, 0);
- if (i == 0 && sp == str)
- return -1LL;
- if (*sp != '\0')
- return -1LL; /* trailing garbage */
- return i;
-}
-
static __attribute__((noreturn)) void
illegal_option(
const char *value,
@@ -1302,8 +1278,8 @@ illegal_option(
usage();
}
-static int
-getnum_checked(
+static long long
+getnum(
const char *str,
struct opt_params *opts,
int index)
@@ -1323,13 +1299,32 @@ getnum_checked(
respec(opts->name, (char **)opts->subopts, index);
sp->seen = true;
+ /* empty strings might just return a default value */
if (!str || *str == '\0') {
if (sp->defaultval == SUBOPT_NEEDS_VAL)
reqval(opts->name, (char **)opts->subopts, index);
return sp->defaultval;
}
- c = getnum(str, blocksize, sectorsize, sp->convert);
+ /*
+ * Some values are pure numbers, others can have suffixes that define
+ * the units of the number. Those get passed to cvtnum(), otherwise we
+ * convert it ourselves to guarantee there is no trailing garbage in the
+ * number.
+ */
+ if (sp->convert)
+ c = cvtnum(blocksize, sectorsize, str);
+ else {
+ char *sp;
+
+ c = strtoll(str, &sp, 0);
+ if (c == 0 && sp == str)
+ illegal_option(str, opts, index);
+ if (*sp != '\0')
+ illegal_option(str, opts, index);
+ }
+
+ /* Validity check the result. */
if (c < sp->minval || c > sp->maxval)
illegal_option(str, opts, index);
if (sp->is_power_2 && !ispow2(c))
@@ -1490,8 +1485,7 @@ main(
if (bsflag)
conflict('b', subopts, B_SIZE,
B_LOG);
- blocklog = getnum_checked(value, &bopts,
- B_LOG);
+ blocklog = getnum(value, &bopts, B_LOG);
blocksize = 1 << blocklog;
blflag = 1;
break;
@@ -1499,8 +1493,8 @@ main(
if (blflag)
conflict('b', subopts, B_LOG,
B_SIZE);
- blocksize = getnum_checked(value, &bopts,
- B_SIZE);
+ blocksize = getnum(value, &bopts,
+ B_SIZE);
blocklog = libxfs_highbit32(blocksize);
bsflag = 1;
break;
@@ -1518,18 +1512,17 @@ main(
switch (getsubopt(&p, (constpp)subopts,
&value)) {
case D_AGCOUNT:
- agcount = getnum_checked(value, &dopts,
- D_AGCOUNT);
+ agcount = getnum(value, &dopts,
+ D_AGCOUNT);
daflag = 1;
break;
case D_AGSIZE:
- agsize = getnum_checked(value, &dopts,
- D_AGSIZE);
+ agsize = getnum(value, &dopts, D_AGSIZE);
dasize = 1;
break;
case D_FILE:
- xi.disfile = getnum_checked(value,
- &dopts, D_FILE);
+ xi.disfile = getnum(value, &dopts,
+ D_FILE);
if (xi.disfile && !Nflag)
xi.dcreat = 1;
break;
@@ -1551,29 +1544,26 @@ main(
if (nodsflag)
conflict('d', subopts, D_NOALIGN,
D_SUNIT);
- dsunit = getnum_checked(value, &dopts,
- D_SUNIT);
+ dsunit = getnum(value, &dopts, D_SUNIT);
break;
case D_SWIDTH:
if (nodsflag)
conflict('d', subopts, D_NOALIGN,
D_SWIDTH);
- dswidth = getnum_checked(value, &dopts,
- D_SWIDTH);
+ dswidth = getnum(value, &dopts,
+ D_SWIDTH);
break;
case D_SU:
if (nodsflag)
conflict('d', subopts, D_NOALIGN,
D_SU);
- dsu = getnum_checked(value, &dopts,
- D_SU);
+ dsu = getnum(value, &dopts, D_SU);
break;
case D_SW:
if (nodsflag)
conflict('d', subopts, D_NOALIGN,
D_SW);
- dsw = getnum_checked(value, &dopts,
- D_SW);
+ dsw = getnum(value, &dopts, D_SW);
break;
case D_NOALIGN:
if (dsu)
@@ -1594,8 +1584,8 @@ main(
if (ssflag)
conflict('d', subopts, D_SECTSIZE,
D_SECTLOG);
- sectorlog = getnum_checked(value, &dopts,
- D_SECTLOG);
+ sectorlog = getnum(value, &dopts,
+ D_SECTLOG);
sectorsize = 1 << sectorlog;
slflag = 1;
break;
@@ -1603,28 +1593,27 @@ main(
if (slflag)
conflict('d', subopts, D_SECTLOG,
D_SECTSIZE);
- sectorsize = getnum_checked(value,
- &dopts, D_SECTSIZE);
+ sectorsize = getnum(value, &dopts,
+ D_SECTSIZE);
sectorlog =
libxfs_highbit32(sectorsize);
ssflag = 1;
break;
case D_RTINHERIT:
- c = getnum_checked(value, &dopts,
- D_RTINHERIT);
+ c = getnum(value, &dopts, D_RTINHERIT);
if (c)
fsx.fsx_xflags |=
XFS_DIFLAG_RTINHERIT;
break;
case D_PROJINHERIT:
- fsx.fsx_projid = getnum_checked(value,
- &dopts, D_PROJINHERIT);
+ fsx.fsx_projid = getnum(value, &dopts,
+ D_PROJINHERIT);
fsx.fsx_xflags |=
XFS_DIFLAG_PROJINHERIT;
break;
case D_EXTSZINHERIT:
- fsx.fsx_extsize = getnum_checked(value,
- &dopts, D_EXTSZINHERIT);
+ fsx.fsx_extsize = getnum(value, &dopts,
+ D_EXTSZINHERIT);
fsx.fsx_xflags |=
XFS_DIFLAG_EXTSZINHERIT;
break;
@@ -1642,8 +1631,8 @@ main(
switch (getsubopt(&p, (constpp)subopts,
&value)) {
case I_ALIGN:
- sb_feat.inode_align = getnum_checked(
- value, &iopts, I_ALIGN);
+ sb_feat.inode_align = getnum(value,
+ &iopts, I_ALIGN);
break;
case I_LOG:
if (ipflag)
@@ -1652,14 +1641,13 @@ main(
if (isflag)
conflict('i', subopts, I_SIZE,
I_LOG);
- inodelog = getnum_checked(value, &iopts,
- I_LOG);
+ inodelog = getnum(value, &iopts, I_LOG);
isize = 1 << inodelog;
ilflag = 1;
break;
case I_MAXPCT:
- imaxpct = getnum_checked(value, &iopts,
- I_MAXPCT);
+ imaxpct = getnum(value, &iopts,
+ I_MAXPCT);
imflag = 1;
break;
case I_PERBLOCK:
@@ -1669,8 +1657,8 @@ main(
if (isflag)
conflict('i', subopts, I_SIZE,
I_PERBLOCK);
- inopblock = getnum_checked(value, &iopts,
- I_PERBLOCK);
+ inopblock = getnum(value, &iopts,
+ I_PERBLOCK);
ipflag = 1;
break;
case I_SIZE:
@@ -1680,20 +1668,18 @@ main(
if (ipflag)
conflict('i', subopts, I_PERBLOCK,
I_SIZE);
- isize = getnum_checked(value, &iopts,
- I_SIZE);
+ isize = getnum(value, &iopts, I_SIZE);
inodelog = libxfs_highbit32(isize);
isflag = 1;
break;
case I_ATTR:
sb_feat.attr_version =
- getnum_checked(value, &iopts,
- I_ATTR);
+ getnum(value, &iopts, I_ATTR);
break;
case I_PROJID32BIT:
sb_feat.projid16bit =
- !getnum_checked(value, &iopts,
- I_PROJID32BIT);
+ !getnum(value, &iopts,
+ I_PROJID32BIT);
break;
default:
unknown('i', value);
@@ -1711,16 +1697,15 @@ main(
case L_AGNUM:
if (ldflag)
conflict('l', subopts, L_AGNUM, L_DEV);
- logagno = getnum_checked(value, &lopts,
- L_AGNUM);
+ logagno = getnum(value, &lopts, L_AGNUM);
laflag = 1;
break;
case L_FILE:
if (loginternal)
conflict('l', subopts, L_INTERNAL,
L_FILE);
- xi.lisfile = getnum_checked(value,
- &lopts, L_FILE);
+ xi.lisfile = getnum(value, &lopts,
+ L_FILE);
if (xi.lisfile)
xi.lcreat = 1;
break;
@@ -1731,17 +1716,15 @@ main(
conflict('l', subopts, L_FILE,
L_INTERNAL);
- loginternal = getnum_checked(value,
- &lopts, L_INTERNAL);
+ loginternal = getnum(value, &lopts,
+ L_INTERNAL);
liflag = 1;
break;
case L_SU:
- lsu = getnum_checked(value, &lopts,
- L_SU);
+ lsu = getnum(value, &lopts, L_SU);
break;
case L_SUNIT:
- lsunit = getnum_checked(value, &lopts,
- L_SUNIT);
+ lsunit = getnum(value, &lopts, L_SUNIT);
break;
case L_NAME:
case L_DEV:
@@ -1760,8 +1743,7 @@ main(
break;
case L_VERSION:
sb_feat.log_version =
- getnum_checked(value, &lopts,
- L_VERSION);
+ getnum(value, &lopts, L_VERSION);
lvflag = 1;
break;
case L_SIZE:
@@ -1776,8 +1758,8 @@ main(
if (lssflag)
conflict('l', subopts, L_SECTSIZE,
L_SECTLOG);
- lsectorlog = getnum_checked(value,
- &lopts, L_SECTLOG);
+ lsectorlog = getnum(value, &lopts,
+ L_SECTLOG);
lsectorsize = 1 << lsectorlog;
lslflag = 1;
break;
@@ -1785,15 +1767,15 @@ main(
if (lslflag)
conflict('l', subopts, L_SECTLOG,
L_SECTSIZE);
- lsectorsize = getnum_checked(value,
- &lopts, L_SECTSIZE);
+ lsectorsize = getnum(value, &lopts,
+ L_SECTSIZE);
lsectorlog =
libxfs_highbit32(lsectorsize);
lssflag = 1;
break;
case L_LAZYSBCNTR:
sb_feat.lazy_sb_counters =
- getnum_checked(value, &lopts,
+ getnum(value, &lopts,
L_LAZYSBCNTR);
break;
default:
@@ -1816,8 +1798,7 @@ main(
&value)) {
case M_CRC:
sb_feat.crcs_enabled =
- getnum_checked(value, &mopts,
- M_CRC);
+ getnum(value, &mopts, M_CRC);
if (sb_feat.crcs_enabled && nftype) {
fprintf(stderr,
_("cannot specify both -m crc=1 and -n ftype\n"));
@@ -1843,8 +1824,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
if (nsflag)
conflict('n', subopts, N_SIZE,
N_LOG);
- dirblocklog = getnum_checked(value,
- &nopts, N_LOG);
+ dirblocklog = getnum(value, &nopts,
+ N_LOG);
dirblocksize = 1 << dirblocklog;
nlflag = 1;
break;
@@ -1852,8 +1833,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
if (nlflag)
conflict('n', subopts, N_LOG,
N_SIZE);
- dirblocksize = getnum_checked(value,
- &nopts, N_SIZE);
+ dirblocksize = getnum(value, &nopts,
+ N_SIZE);
dirblocklog =
libxfs_highbit32(dirblocksize);
nsflag = 1;
@@ -1868,9 +1849,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
sb_feat.nci = true;
} else {
sb_feat.dir_version =
- getnum_checked(value,
- &nopts,
- N_VERSION);
+ getnum(value, &nopts,
+ N_VERSION);
}
nvflag = 1;
break;
@@ -1880,8 +1860,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
_("cannot specify both -m crc=1 and -n ftype\n"));
usage();
}
- sb_feat.dirftype = getnum_checked(value,
- &nopts, N_FTYPE);
+ sb_feat.dirftype = getnum(value, &nopts,
+ N_FTYPE);
nftype = 1;
break;
default:
@@ -1919,8 +1899,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
rtextsize = value;
break;
case R_FILE:
- xi.risfile = getnum_checked(value,
- &ropts, R_FILE);
+ xi.risfile = getnum(value, &ropts,
+ R_FILE);
if (xi.risfile)
xi.rcreat = 1;
break;
@@ -1960,8 +1940,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
if (ssflag || lssflag)
conflict('s', subopts,
S_SECTSIZE, S_SECTLOG);
- sectorlog = getnum_checked(value, &sopts,
- S_SECTLOG);
+ sectorlog = getnum(value, &sopts,
+ S_SECTLOG);
lsectorlog = sectorlog;
sectorsize = 1 << sectorlog;
lsectorsize = sectorsize;
@@ -1972,8 +1952,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
if (slflag || lslflag)
conflict('s', subopts, S_SECTLOG,
S_SECTSIZE);
- sectorsize = getnum_checked(value,
- &sopts, S_SECTSIZE);
+ sectorsize = getnum(value, &sopts,
+ S_SECTSIZE);
lsectorsize = sectorsize;
sectorlog =
libxfs_highbit32(sectorsize);
@@ -2182,7 +2162,7 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
if (dsize) {
__uint64_t dbytes;
- dbytes = getnum_checked(dsize, &dopts, D_SIZE);
+ dbytes = getnum(dsize, &dopts, D_SIZE);
if (dbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
_("illegal data length %lld, not a multiple of %d\n"),
@@ -2219,7 +2199,7 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
if (logsize) {
__uint64_t logbytes;
- logbytes = getnum_checked(logsize, &lopts, L_SIZE);
+ logbytes = getnum(logsize, &lopts, L_SIZE);
if (logbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
_("illegal log length %lld, not a multiple of %d\n"),
@@ -2241,7 +2221,7 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
if (rtsize) {
__uint64_t rtbytes;
- rtbytes = getnum_checked(rtsize, &ropts, R_SIZE);
+ rtbytes = getnum(rtsize, &ropts, R_SIZE);
if (rtbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
_("illegal rt length %lld, not a multiple of %d\n"),
@@ -2261,7 +2241,7 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
if (rtextsize) {
__uint64_t rtextbytes;
- rtextbytes = getnum_checked(rtextsize, &ropts, R_EXTSIZE);
+ rtextbytes = getnum(rtextsize, &ropts, R_EXTSIZE);
if (rtextbytes % blocksize) {
fprintf(stderr,
_("illegal rt extent size %lld, not a multiple of %d\n"),
@@ -3314,8 +3294,8 @@ unknown(
long long
cvtnum(
- unsigned int blocksize,
- unsigned int sectorsize,
+ unsigned int blksize,
+ unsigned int sectsize,
const char *s)
{
long long i;
@@ -3328,9 +3308,9 @@ cvtnum(
return i;
if (*sp == 'b' && sp[1] == '\0')
- return i * blocksize;
+ return i * blksize;
if (*sp == 's' && sp[1] == '\0')
- return i * sectorsize;
+ return i * sectsize;
if (*sp == 'k' && sp[1] == '\0')
return 1024LL * i;
--
1.8.4.rc3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 13/15] mkfs: encode conflicts into parsing table
2013-11-29 1:43 [RFC, PATCH 00/15] mkfs: sanitise input parameters Dave Chinner
` (11 preceding siblings ...)
2013-11-29 1:43 ` [PATCH 12/15] mkfs: merge getnum Dave Chinner
@ 2013-11-29 1:43 ` Dave Chinner
2013-12-02 17:23 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 14/15] mkfs: add string options to generic parsing Dave Chinner
2013-11-29 1:43 ` [PATCH 15/15] mkfs: don't treat files as though they are block devices Dave Chinner
14 siblings, 1 reply; 42+ messages in thread
From: Dave Chinner @ 2013-11-29 1:43 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
Many options conflict, so we need to specify which options conflict
with each other in a generic manner. We already have a "seen"
variable used for respecification detection, so we can also use this
code conflict detection. Hence add a "conflicts" array to the sub
options parameter definition.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
mkfs/xfs_mkfs.c | 246 ++++++++++++++++++++++++++++----------------------------
1 file changed, 123 insertions(+), 123 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 827c300..8395fa4 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -58,6 +58,9 @@ unsigned int sectorsize;
#define MAX_SUBOPTS 16
#define SUBOPT_NEEDS_VAL (-1LL)
+
+#define MAX_CONFLICTS 8
+#define LAST_CONFLICT (-1)
struct opt_params {
const char name;
const char *subopts[MAX_SUBOPTS];
@@ -67,6 +70,7 @@ struct opt_params {
bool seen;
bool convert;
bool is_power_2;
+ int conflicts[MAX_CONFLICTS];
long long minval;
long long maxval;
long long defaultval;
@@ -84,6 +88,8 @@ struct opt_params bopts = {
},
.subopt_params = {
{ .index = B_LOG,
+ .conflicts = { B_SIZE,
+ LAST_CONFLICT },
.minval = XFS_MIN_BLOCKSIZE_LOG,
.maxval = XFS_MAX_BLOCKSIZE_LOG,
.defaultval = SUBOPT_NEEDS_VAL,
@@ -91,6 +97,8 @@ struct opt_params bopts = {
{ .index = B_SIZE,
.convert = true,
.is_power_2 = true,
+ .conflicts = { B_LOG,
+ LAST_CONFLICT },
.minval = XFS_MIN_BLOCKSIZE,
.maxval = XFS_MAX_BLOCKSIZE,
.defaultval = SUBOPT_NEEDS_VAL,
@@ -135,57 +143,84 @@ struct opt_params dopts = {
},
.subopt_params = {
{ .index = D_AGCOUNT,
+ .conflicts = { D_AGSIZE,
+ LAST_CONFLICT },
.minval = 1,
.maxval = XFS_MAX_AGNUMBER,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_FILE,
+ .conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 1,
.defaultval = 1,
},
{ .index = D_NAME,
+ .conflicts = { LAST_CONFLICT },
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SIZE,
+ .conflicts = { LAST_CONFLICT },
.convert = true,
.minval = XFS_AG_MIN_BYTES,
.maxval = LLONG_MAX,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SUNIT,
+ .conflicts = { D_NOALIGN,
+ D_SU,
+ D_SW,
+ LAST_CONFLICT },
.minval = 0,
.maxval = UINT_MAX,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SWIDTH,
+ .conflicts = { D_NOALIGN,
+ D_SU,
+ D_SW,
+ LAST_CONFLICT },
.minval = 0,
.maxval = UINT_MAX,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_AGSIZE,
+ .conflicts = { D_AGCOUNT,
+ LAST_CONFLICT },
.convert = true,
.minval = XFS_AG_MIN_BYTES,
.maxval = XFS_AG_MAX_BYTES,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SU,
+ .conflicts = { D_NOALIGN,
+ D_SUNIT,
+ D_SWIDTH,
+ LAST_CONFLICT },
.convert = true,
.minval = 0,
.maxval = UINT_MAX,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SW,
+ .conflicts = { D_NOALIGN,
+ D_SUNIT,
+ D_SWIDTH,
+ LAST_CONFLICT },
.minval = 0,
.maxval = UINT_MAX,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SECTLOG,
+ .conflicts = { D_SECTSIZE,
+ LAST_CONFLICT },
.minval = XFS_MIN_SECTORSIZE_LOG,
.maxval = XFS_MAX_SECTORSIZE_LOG,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SECTSIZE,
+ .conflicts = { D_SECTLOG,
+ LAST_CONFLICT },
.convert = true,
.is_power_2 = true,
.minval = XFS_MIN_SECTORSIZE,
@@ -193,21 +228,29 @@ struct opt_params dopts = {
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_NOALIGN,
+ .conflicts = { D_SU,
+ D_SW,
+ D_SUNIT,
+ D_SWIDTH,
+ LAST_CONFLICT },
.minval = 1,
.maxval = 1,
.defaultval = 1,
},
{ .index = D_RTINHERIT,
+ .conflicts = { LAST_CONFLICT },
.minval = 1,
.maxval = 1,
.defaultval = 1,
},
{ .index = D_PROJINHERIT,
+ .conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = UINT_MAX,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = D_EXTSZINHERIT,
+ .conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = UINT_MAX,
.defaultval = SUBOPT_NEEDS_VAL,
@@ -237,38 +280,51 @@ struct opt_params iopts = {
},
.subopt_params = {
{ .index = I_ALIGN,
+ .conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 1,
.defaultval = 1,
},
{ .index = I_LOG,
+ .conflicts = { I_PERBLOCK,
+ I_SIZE,
+ LAST_CONFLICT },
.minval = XFS_DINODE_MIN_LOG,
.maxval = XFS_DINODE_MAX_LOG,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = I_MAXPCT,
+ .conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 100,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = I_PERBLOCK,
+ .conflicts = { I_LOG,
+ I_SIZE,
+ LAST_CONFLICT },
.is_power_2 = true,
.minval = XFS_MIN_INODE_PERBLOCK,
.maxval = XFS_MAX_BLOCKSIZE / XFS_DINODE_MIN_SIZE,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = I_SIZE,
+ .conflicts = { I_PERBLOCK,
+ I_LOG,
+ LAST_CONFLICT },
.is_power_2 = true,
.minval = XFS_DINODE_MIN_SIZE,
.maxval = XFS_DINODE_MAX_SIZE,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = I_ATTR,
+ .conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 2,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = I_PROJID32BIT,
+ .conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 1,
.defaultval = 1,
@@ -307,46 +363,64 @@ struct opt_params lopts = {
},
.subopt_params = {
{ .index = L_AGNUM,
+ .conflicts = { L_DEV,
+ LAST_CONFLICT },
.minval = 0,
.maxval = UINT_MAX,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_INTERNAL,
+ .conflicts = { L_FILE,
+ L_DEV,
+ LAST_CONFLICT },
.minval = 0,
.maxval = 1,
.defaultval = 1,
},
{ .index = L_SIZE,
+ .conflicts = { LAST_CONFLICT },
.convert = true,
.minval = 2 * 1024 * 1024LL, /* XXX: XFS_MIN_LOG_BYTES */
.maxval = XFS_MAX_LOG_BYTES,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_VERSION,
+ .conflicts = { LAST_CONFLICT },
.minval = 1,
.maxval = 2,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_SUNIT,
+ .conflicts = { L_SU,
+ LAST_CONFLICT },
.minval = BTOBB(XLOG_MIN_RECORD_BSIZE),
.maxval = BTOBB(XLOG_MAX_RECORD_BSIZE),
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_SU,
+ .conflicts = { L_SUNIT,
+ LAST_CONFLICT },
.convert = true,
.minval = XLOG_MIN_RECORD_BSIZE,
.maxval = XLOG_MAX_RECORD_BSIZE,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_DEV,
+ .conflicts = { L_AGNUM,
+ L_INTERNAL,
+ LAST_CONFLICT },
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_SECTLOG,
+ .conflicts = { L_SECTSIZE,
+ LAST_CONFLICT },
.minval = XFS_MIN_SECTORSIZE_LOG,
.maxval = XFS_MAX_SECTORSIZE_LOG,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_SECTSIZE,
+ .conflicts = { L_SECTLOG,
+ LAST_CONFLICT },
.convert = true,
.is_power_2 = true,
.minval = XFS_MIN_SECTORSIZE,
@@ -354,14 +428,20 @@ struct opt_params lopts = {
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_FILE,
+ .conflicts = { L_INTERNAL,
+ LAST_CONFLICT },
.minval = 0,
.maxval = 1,
.defaultval = 1,
},
{ .index = L_NAME,
+ .conflicts = { L_AGNUM,
+ L_INTERNAL,
+ LAST_CONFLICT },
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = L_LAZYSBCNTR,
+ .conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 1,
.defaultval = 1,
@@ -384,11 +464,15 @@ struct opt_params nopts = {
},
.subopt_params = {
{ .index = N_LOG,
+ .conflicts = { N_SIZE,
+ LAST_CONFLICT },
.minval = XFS_MIN_REC_DIRSIZE,
.maxval = XFS_MAX_BLOCKSIZE_LOG,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = N_SIZE,
+ .conflicts = { N_LOG,
+ LAST_CONFLICT },
.convert = true,
.is_power_2 = true,
.minval = 1 << XFS_MIN_REC_DIRSIZE,
@@ -396,11 +480,13 @@ struct opt_params nopts = {
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = N_VERSION,
+ .conflicts = { LAST_CONFLICT },
.minval = 2,
.maxval = 2,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = N_FTYPE,
+ .conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 1,
.defaultval = 0,
@@ -427,27 +513,33 @@ struct opt_params ropts = {
},
.subopt_params = {
{ .index = R_EXTSIZE,
+ .conflicts = { LAST_CONFLICT },
.convert = true,
.minval = XFS_MIN_RTEXTSIZE,
.maxval = XFS_MAX_RTEXTSIZE,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = R_SIZE,
+ .conflicts = { LAST_CONFLICT },
.convert = true,
.minval = 0,
.maxval = LLONG_MAX,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = R_DEV,
+ .conflicts = { LAST_CONFLICT },
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = R_FILE,
+ .conflicts = { LAST_CONFLICT },
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = R_NAME,
+ .conflicts = { LAST_CONFLICT },
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = R_NOALIGN,
+ .conflicts = { LAST_CONFLICT },
.defaultval = SUBOPT_NEEDS_VAL,
},
},
@@ -468,16 +560,25 @@ struct opt_params sopts = {
},
.subopt_params = {
{ .index = S_LOG,
+ .conflicts = { S_SIZE,
+ S_SECTSIZE,
+ LAST_CONFLICT },
.minval = XFS_MIN_SECTORSIZE_LOG,
.maxval = XFS_MAX_SECTORSIZE_LOG,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = S_SECTLOG,
+ .conflicts = { S_SIZE,
+ S_SECTSIZE,
+ LAST_CONFLICT },
.minval = XFS_MIN_SECTORSIZE_LOG,
.maxval = XFS_MAX_SECTORSIZE_LOG,
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = S_SIZE,
+ .conflicts = { S_LOG,
+ S_SECTLOG,
+ LAST_CONFLICT },
.convert = true,
.is_power_2 = true,
.minval = XFS_MIN_SECTORSIZE,
@@ -485,6 +586,9 @@ struct opt_params sopts = {
.defaultval = SUBOPT_NEEDS_VAL,
},
{ .index = S_SECTSIZE,
+ .conflicts = { S_LOG,
+ S_SECTLOG,
+ LAST_CONFLICT },
.convert = true,
.is_power_2 = true,
.minval = XFS_MIN_SECTORSIZE,
@@ -503,6 +607,7 @@ struct opt_params mopts = {
},
.subopt_params = {
{ .index = M_CRC,
+ .conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 1,
.defaultval = 0,
@@ -546,30 +651,14 @@ calc_stripe_factors(
int *lsunit)
{
/* Handle data sunit/swidth options */
- if (*dsunit || *dswidth) {
- if (dsu || dsw) {
- fprintf(stderr,
- _("data su/sw must not be used in "
- "conjunction with data sunit/swidth\n"));
- usage();
- }
-
- if ((*dsunit && !*dswidth) || (!*dsunit && *dswidth)) {
- fprintf(stderr,
- _("both data sunit and data swidth options "
- "must be specified\n"));
- usage();
- }
+ if ((*dsunit && !*dswidth) || (!*dsunit && *dswidth)) {
+ fprintf(stderr,
+ _("both data sunit and data swidth options "
+ "must be specified\n"));
+ usage();
}
if (dsu || dsw) {
- if (*dsunit || *dswidth) {
- fprintf(stderr,
- _("data sunit/swidth must not be used in "
- "conjunction with data su/sw\n"));
- usage();
- }
-
if ((dsu && !dsw) || (!dsu && dsw)) {
fprintf(stderr,
_("both data su and data sw options "
@@ -597,24 +686,8 @@ calc_stripe_factors(
/* Handle log sunit options */
- if (*lsunit) {
- if (lsu) {
- fprintf(stderr,
- _("log su should not be used in "
- "conjunction with log sunit\n"));
- usage();
- }
- }
-
- if (lsu) {
- if (*lsunit) {
- fprintf(stderr,
- _("log sunit should not be used in "
- "conjunction with log su\n"));
- usage();
- }
+ if (lsu)
*lsunit = (int)BTOBBT(lsu);
- }
}
#ifdef ENABLE_BLKID
@@ -1299,6 +1372,14 @@ getnum(
respec(opts->name, (char **)opts->subopts, index);
sp->seen = true;
+ /* check for conflicts with the option */
+ for (c = 0; c < MAX_CONFLICTS; c++) {
+ if (sp->conflicts[c] == LAST_CONFLICT)
+ break;
+ if (opts->subopt_params[c].seen)
+ conflict(opts->name, (char **)opts->subopts, c, index);
+ }
+
/* empty strings might just return a default value */
if (!str || *str == '\0') {
if (sp->defaultval == SUBOPT_NEEDS_VAL)
@@ -1482,17 +1563,11 @@ main(
switch (getsubopt(&p, (constpp)subopts,
&value)) {
case B_LOG:
- if (bsflag)
- conflict('b', subopts, B_SIZE,
- B_LOG);
blocklog = getnum(value, &bopts, B_LOG);
blocksize = 1 << blocklog;
blflag = 1;
break;
case B_SIZE:
- if (blflag)
- conflict('b', subopts, B_LOG,
- B_SIZE);
blocksize = getnum(value, &bopts,
B_SIZE);
blocklog = libxfs_highbit32(blocksize);
@@ -1541,58 +1616,29 @@ main(
dsize = value;
break;
case D_SUNIT:
- if (nodsflag)
- conflict('d', subopts, D_NOALIGN,
- D_SUNIT);
dsunit = getnum(value, &dopts, D_SUNIT);
break;
case D_SWIDTH:
- if (nodsflag)
- conflict('d', subopts, D_NOALIGN,
- D_SWIDTH);
dswidth = getnum(value, &dopts,
D_SWIDTH);
break;
case D_SU:
- if (nodsflag)
- conflict('d', subopts, D_NOALIGN,
- D_SU);
dsu = getnum(value, &dopts, D_SU);
break;
case D_SW:
- if (nodsflag)
- conflict('d', subopts, D_NOALIGN,
- D_SW);
dsw = getnum(value, &dopts, D_SW);
break;
case D_NOALIGN:
- if (dsu)
- conflict('d', subopts, D_SU,
- D_NOALIGN);
- if (dsunit)
- conflict('d', subopts, D_SUNIT,
- D_NOALIGN);
- if (dsw)
- conflict('d', subopts, D_SW,
- D_NOALIGN);
- if (dswidth)
- conflict('d', subopts, D_SWIDTH,
- D_NOALIGN);
- nodsflag = 1;
+ nodsflag = getnum(value, &dopts,
+ D_NOALIGN);
break;
case D_SECTLOG:
- if (ssflag)
- conflict('d', subopts, D_SECTSIZE,
- D_SECTLOG);
sectorlog = getnum(value, &dopts,
D_SECTLOG);
sectorsize = 1 << sectorlog;
slflag = 1;
break;
case D_SECTSIZE:
- if (slflag)
- conflict('d', subopts, D_SECTLOG,
- D_SECTSIZE);
sectorsize = getnum(value, &dopts,
D_SECTSIZE);
sectorlog =
@@ -1635,12 +1681,6 @@ main(
&iopts, I_ALIGN);
break;
case I_LOG:
- if (ipflag)
- conflict('i', subopts, I_PERBLOCK,
- I_LOG);
- if (isflag)
- conflict('i', subopts, I_SIZE,
- I_LOG);
inodelog = getnum(value, &iopts, I_LOG);
isize = 1 << inodelog;
ilflag = 1;
@@ -1651,23 +1691,11 @@ main(
imflag = 1;
break;
case I_PERBLOCK:
- if (ilflag)
- conflict('i', subopts, I_LOG,
- I_PERBLOCK);
- if (isflag)
- conflict('i', subopts, I_SIZE,
- I_PERBLOCK);
inopblock = getnum(value, &iopts,
I_PERBLOCK);
ipflag = 1;
break;
case I_SIZE:
- if (ilflag)
- conflict('i', subopts, I_LOG,
- I_SIZE);
- if (ipflag)
- conflict('i', subopts, I_PERBLOCK,
- I_SIZE);
isize = getnum(value, &iopts, I_SIZE);
inodelog = libxfs_highbit32(isize);
isflag = 1;
@@ -1695,27 +1723,16 @@ main(
switch (getsubopt(&p, (constpp)subopts,
&value)) {
case L_AGNUM:
- if (ldflag)
- conflict('l', subopts, L_AGNUM, L_DEV);
logagno = getnum(value, &lopts, L_AGNUM);
laflag = 1;
break;
case L_FILE:
- if (loginternal)
- conflict('l', subopts, L_INTERNAL,
- L_FILE);
xi.lisfile = getnum(value, &lopts,
L_FILE);
if (xi.lisfile)
xi.lcreat = 1;
break;
case L_INTERNAL:
- if (ldflag)
- conflict('l', subopts, L_INTERNAL, L_DEV);
- if (xi.lisfile)
- conflict('l', subopts, L_FILE,
- L_INTERNAL);
-
loginternal = getnum(value, &lopts,
L_INTERNAL);
liflag = 1;
@@ -1755,18 +1772,12 @@ main(
lsflag = 1;
break;
case L_SECTLOG:
- if (lssflag)
- conflict('l', subopts, L_SECTSIZE,
- L_SECTLOG);
lsectorlog = getnum(value, &lopts,
L_SECTLOG);
lsectorsize = 1 << lsectorlog;
lslflag = 1;
break;
case L_SECTSIZE:
- if (lslflag)
- conflict('l', subopts, L_SECTLOG,
- L_SECTSIZE);
lsectorsize = getnum(value, &lopts,
L_SECTSIZE);
lsectorlog =
@@ -1821,18 +1832,12 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
switch (getsubopt(&p, (constpp)subopts,
&value)) {
case N_LOG:
- if (nsflag)
- conflict('n', subopts, N_SIZE,
- N_LOG);
dirblocklog = getnum(value, &nopts,
N_LOG);
dirblocksize = 1 << dirblocklog;
nlflag = 1;
break;
case N_SIZE:
- if (nlflag)
- conflict('n', subopts, N_LOG,
- N_SIZE);
dirblocksize = getnum(value, &nopts,
N_SIZE);
dirblocklog =
@@ -1937,7 +1942,7 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
&value)) {
case S_LOG:
case S_SECTLOG:
- if (ssflag || lssflag)
+ if (lssflag)
conflict('s', subopts,
S_SECTSIZE, S_SECTLOG);
sectorlog = getnum(value, &sopts,
@@ -1949,7 +1954,7 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
break;
case S_SIZE:
case S_SECTSIZE:
- if (slflag || lslflag)
+ if (lslflag)
conflict('s', subopts, S_SECTLOG,
S_SECTSIZE);
sectorsize = getnum(value, &sopts,
@@ -2148,11 +2153,6 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
dirblocksize = 1 << dirblocklog;
}
- if (daflag && dasize) {
- fprintf(stderr,
- _("both -d agcount= and agsize= specified, use one or the other\n"));
- usage();
- }
if (xi.disfile && (!dsize || !xi.dname)) {
fprintf(stderr,
--
1.8.4.rc3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 14/15] mkfs: add string options to generic parsing
2013-11-29 1:43 [RFC, PATCH 00/15] mkfs: sanitise input parameters Dave Chinner
` (12 preceding siblings ...)
2013-11-29 1:43 ` [PATCH 13/15] mkfs: encode conflicts into parsing table Dave Chinner
@ 2013-11-29 1:43 ` Dave Chinner
2013-11-29 1:43 ` [PATCH 15/15] mkfs: don't treat files as though they are block devices Dave Chinner
14 siblings, 0 replies; 42+ messages in thread
From: Dave Chinner @ 2013-11-29 1:43 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
So that string options are correctly detected for conflicts and
respecification, add a getstr() function that modifies the option
tables appropriately.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
mkfs/xfs_mkfs.c | 129 +++++++++++++++++++++++++++++++-------------------------
1 file changed, 71 insertions(+), 58 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 8395fa4..88dd53e 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -68,6 +68,7 @@ struct opt_params {
struct subopt_param {
int index;
bool seen;
+ bool str_seen;
bool convert;
bool is_power_2;
int conflicts[MAX_CONFLICTS];
@@ -1351,14 +1352,15 @@ illegal_option(
usage();
}
-static long long
-getnum(
+static void
+check_opt(
const char *str,
struct opt_params *opts,
- int index)
+ int index,
+ bool str_seen)
{
- struct subopt_param *sp = &opts->subopt_params[index];
- long long c;
+ struct subopt_param *sp = &opts->subopt_params[index];
+ int i;
if (sp->index != index) {
fprintf(stderr,
@@ -1367,19 +1369,44 @@ getnum(
reqval(opts->name, (char **)opts->subopts, index);
}
- /* check for respecification of the option */
- if (sp->seen)
- respec(opts->name, (char **)opts->subopts, index);
- sp->seen = true;
+ /*
+ * Check for respecification of the option. This is more complex than it
+ * seems because some options are parsed twice - once as a string during
+ * input parsing, then later the string is passed to getnum for
+ * conversion into a number and bounds checking. Hence the two variables
+ * used to track the different uses based on the @str parameter passed
+ * to us.
+ */
+ if (!str_seen) {
+ if (sp->seen)
+ respec(opts->name, (char **)opts->subopts, index);
+ sp->seen = true;
+ } else {
+ if (sp->str_seen)
+ respec(opts->name, (char **)opts->subopts, index);
+ sp->str_seen = true;
+ }
/* check for conflicts with the option */
- for (c = 0; c < MAX_CONFLICTS; c++) {
- if (sp->conflicts[c] == LAST_CONFLICT)
+ for (i = 0; i < MAX_CONFLICTS; i++) {
+ if (sp->conflicts[i] == LAST_CONFLICT)
break;
- if (opts->subopt_params[c].seen)
- conflict(opts->name, (char **)opts->subopts, c, index);
+ if (opts->subopt_params[i].seen ||
+ opts->subopt_params[i].str_seen)
+ conflict(opts->name, (char **)opts->subopts, i, index);
}
+}
+
+static long long
+getnum(
+ const char *str,
+ struct opt_params *opts,
+ int index)
+{
+ struct subopt_param *sp = &opts->subopt_params[index];
+ long long c;
+ check_opt(str, opts, index, false);
/* empty strings might just return a default value */
if (!str || *str == '\0') {
if (sp->defaultval == SUBOPT_NEEDS_VAL)
@@ -1413,6 +1440,26 @@ getnum(
return c;
}
+/*
+ * Option is a string - do all the option table work, and check there
+ * is actually an option string. Otherwise we don't do anything with the string
+ * here - validation will be done later when the string is converted to a value
+ * or used as a file/device path.
+ */
+static char *
+getstr(
+ char *str,
+ struct opt_params *opts,
+ int index)
+{
+ check_opt(str, opts, index, true);
+
+ /* empty strings for string options are not valid */
+ if (!str || *str == '\0')
+ reqval(opts->name, (char **)opts->subopts, index);
+ return str;
+}
+
int
main(
int argc,
@@ -1602,18 +1649,10 @@ main(
xi.dcreat = 1;
break;
case D_NAME:
- if (!value || *value == '\0')
- reqval('d', subopts, D_NAME);
- if (xi.dname)
- respec('d', subopts, D_NAME);
- xi.dname = value;
+ xi.dname = getstr(value, &dopts, D_NAME);
break;
case D_SIZE:
- if (!value || *value == '\0')
- reqval('d', subopts, D_SIZE);
- if (dsize)
- respec('d', subopts, D_SIZE);
- dsize = value;
+ dsize = getstr(value, &dopts, D_SIZE);
break;
case D_SUNIT:
dsunit = getnum(value, &dopts, D_SUNIT);
@@ -1745,18 +1784,10 @@ main(
break;
case L_NAME:
case L_DEV:
- if (laflag)
- conflict('l', subopts, L_AGNUM, L_DEV);
- if (liflag)
- conflict('l', subopts, L_INTERNAL, L_DEV);
- if (!value || *value == '\0')
- reqval('l', subopts, L_NAME);
- if (xi.logname)
- respec('l', subopts, L_NAME);
+ logfile = getstr(value, &lopts, L_NAME);
+ xi.logname = logfile;
ldflag = 1;
loginternal = 0;
- logfile = value;
- xi.logname = value;
break;
case L_VERSION:
sb_feat.log_version =
@@ -1764,12 +1795,7 @@ main(
lvflag = 1;
break;
case L_SIZE:
- if (!value || *value == '\0')
- reqval('l', subopts, L_SIZE);
- if (logsize)
- respec('l', subopts, L_SIZE);
- logsize = value;
- lsflag = 1;
+ logsize = getstr(value, &lopts, L_SIZE);
break;
case L_SECTLOG:
lsectorlog = getnum(value, &lopts,
@@ -1845,10 +1871,7 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
nsflag = 1;
break;
case N_VERSION:
- if (!value || *value == '\0')
- reqval('n', subopts, N_VERSION);
- if (nvflag)
- respec('n', subopts, N_VERSION);
+ value = getstr(value, &nopts, N_VERSION);
if (!strcasecmp(value, "ci")) {
/* ASCII CI mode */
sb_feat.nci = true;
@@ -1897,11 +1920,8 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
switch (getsubopt(&p, (constpp)subopts,
&value)) {
case R_EXTSIZE:
- if (!value || *value == '\0')
- reqval('r', subopts, R_EXTSIZE);
- if (rtextsize)
- respec('r', subopts, R_EXTSIZE);
- rtextsize = value;
+ rtextsize = getstr(value, &ropts,
+ R_EXTSIZE);
break;
case R_FILE:
xi.risfile = getnum(value, &ropts,
@@ -1911,18 +1931,11 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
break;
case R_NAME:
case R_DEV:
- if (!value || *value == '\0')
- reqval('r', subopts, R_NAME);
- if (xi.rtname)
- respec('r', subopts, R_NAME);
- xi.rtname = value;
+ xi.rtname = getstr(value, &ropts,
+ R_NAME);
break;
case R_SIZE:
- if (!value || *value == '\0')
- reqval('r', subopts, R_SIZE);
- if (rtsize)
- respec('r', subopts, R_SIZE);
- rtsize = value;
+ rtsize = getstr(value, &ropts, R_SIZE);
break;
case R_NOALIGN:
norsflag = 1;
--
1.8.4.rc3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 15/15] mkfs: don't treat files as though they are block devices
2013-11-29 1:43 [RFC, PATCH 00/15] mkfs: sanitise input parameters Dave Chinner
` (13 preceding siblings ...)
2013-11-29 1:43 ` [PATCH 14/15] mkfs: add string options to generic parsing Dave Chinner
@ 2013-11-29 1:43 ` Dave Chinner
2013-12-02 17:24 ` Christoph Hellwig
14 siblings, 1 reply; 42+ messages in thread
From: Dave Chinner @ 2013-11-29 1:43 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
If the device is actually a file, and "-d file" is not specified,
mkfs will try to treat it as a block device and get stuff wrong.
Image files don't necessarily have the same sector sizes as the
block device or filesystem underlying the image file, nor should we
be issuing discard ioctls on image files.
To fix this sanely, only require "-d file" if the device name is
invalid to trigger creation of the file. Otherwise, use stat() to
determine if the device is a file or block device and deal with that
appropriately by setting the "isfile" variables and turning off
direct IO. Then ensure that we check the "isfile" options before
doing things that are specific to block devices.
Other file/blockdev issues fixed:
- use getstr to detect specifying the data device name
twice.
- check file/size/name parameters before anything else.
- overwrite checks need to be done before the image file is
opened and potentially truncated.
- blkid_get_topology() should not be called for image files,
so warn when it is called that way.
- zero_old_xfs_structures() emits a spurious error:
"existing superblock read failed: Success"
when it is run on a truncated image file. Don't warn if we
see this problem on an image file.
- Don't issue discards on image files.
- Use fsync() for image files, not BLKFLSBUF in
platform_flush_device() for Linux.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
libxfs/linux.c | 11 +++-
mkfs/xfs_mkfs.c | 167 +++++++++++++++++++++++++++++++++++++++-----------------
2 files changed, 126 insertions(+), 52 deletions(-)
diff --git a/libxfs/linux.c b/libxfs/linux.c
index 2e07d54..2fd529c 100644
--- a/libxfs/linux.c
+++ b/libxfs/linux.c
@@ -123,7 +123,16 @@ platform_set_blocksize(int fd, char *path, dev_t device, int blocksize, int fata
void
platform_flush_device(int fd, dev_t device)
{
- if (major(device) != RAMDISK_MAJOR)
+ struct stat64 st;
+ if (major(device) == RAMDISK_MAJOR)
+ return;
+
+ if (fstat64(fd, &st) < 0)
+ return;
+
+ if (S_ISREG(st.st_mode))
+ fsync(fd);
+ else
ioctl(fd, BLKFLSBUF, 0);
}
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 88dd53e..c1e3e9d 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -701,7 +701,7 @@ calc_stripe_factors(
*/
static int
check_overwrite(
- char *device)
+ const char *device)
{
const char *type;
blkid_probe pr = NULL;
@@ -718,7 +718,7 @@ check_overwrite(
fd = open(device, O_RDONLY);
if (fd < 0)
goto out;
- platform_findsizes(device, fd, &size, &bsz);
+ platform_findsizes((char *)device, fd, &size, &bsz);
close(fd);
/* nothing to overwrite on a 0-length device */
@@ -765,7 +765,6 @@ check_overwrite(
"according to blkid\n"), progname, device);
}
ret = 1;
-
out:
if (pr)
blkid_free_probe(pr);
@@ -791,8 +790,12 @@ static void blkid_get_topology(
struct stat statbuf;
/* can't get topology info from a file */
- if (!stat(device, &statbuf) && S_ISREG(statbuf.st_mode))
+ if (!stat(device, &statbuf) && S_ISREG(statbuf.st_mode)) {
+ fprintf(stderr,
+ _("%s: Warning: trying to probe topology of a file %s!\n"),
+ progname, device);
return;
+ }
pr = blkid_new_probe_from_filename(device);
if (!pr)
@@ -870,7 +873,7 @@ static void get_topology(
#else /* ENABLE_BLKID */
static int
check_overwrite(
- char *device)
+ const char *device)
{
char *type;
@@ -927,6 +930,75 @@ static void get_topology(
#endif /* ENABLE_BLKID */
static void
+check_device_type(
+ const char *name,
+ int *isfile,
+ bool no_size,
+ bool no_name,
+ int *create,
+ bool force_overwrite,
+ const char *optname)
+{
+ struct stat64 statbuf;
+
+ if (*isfile && (no_size || no_name)) {
+ fprintf(stderr,
+ _("if -%s file then -%s name and -%s size are required\n"),
+ optname, optname, optname);
+ usage();
+ }
+
+ if (stat64(name, &statbuf)) {
+ if (errno == ENOENT && *isfile) {
+ if (create)
+ *create = 1;
+ return;
+ }
+
+ fprintf(stderr,
+ _("Error accessing specified device %s: %s\n"),
+ name, strerror(errno));
+ usage();
+ return;
+ }
+
+ if (!force_overwrite && check_overwrite(name)) {
+ fprintf(stderr,
+ _("%s: Use the -f option to force overwrite.\n"),
+ progname);
+ exit(1);
+ }
+
+ /*
+ * We only want to completely truncate and recreate an existing file if
+ * we were specifically told it was a file. Set the create flag only in
+ * this case to trigger that behaviour.
+ */
+ if (S_ISREG(statbuf.st_mode)) {
+ if (!*isfile)
+ *isfile = 1;
+ else if (create)
+ *create = 1;
+ return;
+ }
+
+ if (S_ISBLK(statbuf.st_mode)) {
+ if (*isfile) {
+ fprintf(stderr,
+ _("specified \"-%s file\" on a block device %s\n"),
+ optname, name);
+ usage();
+ }
+ return;
+ }
+
+ fprintf(stderr,
+ _("specified device %s not a file or block device\n"),
+ name);
+ usage();
+}
+
+static void
fixup_log_stripe_unit(
int lsflag,
int sunit,
@@ -1203,11 +1275,19 @@ zero_old_xfs_structures(
}
memset(buf, 0, new_sb->sb_sectsize);
- if (pread(xi->dfd, buf, new_sb->sb_sectsize, 0) != new_sb->sb_sectsize) {
- fprintf(stderr, _("existing superblock read failed: %s\n"),
- strerror(errno));
- free(buf);
- return;
+ /*
+ * If we are creating an image file, it might be of zero length at this
+ * point in time. Hence reading the existing superblock is going to
+ * return zero bytes. It's not a failure we need to warn about in this
+ * case.
+ */
+ off = pread(xi->dfd, buf, new_sb->sb_sectsize, 0);
+ if (off != new_sb->sb_sectsize) {
+ if (!xi->disfile)
+ fprintf(stderr,
+ _("error reading existing superblock: %s\n"),
+ strerror(errno));
+ goto done;
}
libxfs_sb_from_disk(&sb, buf);
@@ -1645,8 +1725,6 @@ main(
case D_FILE:
xi.disfile = getnum(value, &dopts,
D_FILE);
- if (xi.disfile && !Nflag)
- xi.dcreat = 1;
break;
case D_NAME:
xi.dname = getstr(value, &dopts, D_NAME);
@@ -1768,8 +1846,6 @@ main(
case L_FILE:
xi.lisfile = getnum(value, &lopts,
L_FILE);
- if (xi.lisfile)
- xi.lcreat = 1;
break;
case L_INTERNAL:
loginternal = getnum(value, &lopts,
@@ -1926,8 +2002,6 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
case R_FILE:
xi.risfile = getnum(value, &ropts,
R_FILE);
- if (xi.risfile)
- xi.rcreat = 1;
break;
case R_NAME:
case R_DEV:
@@ -1994,13 +2068,7 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
fprintf(stderr, _("extra arguments\n"));
usage();
} else if (argc - optind == 1) {
- dfile = xi.volname = argv[optind];
- if (xi.dname) {
- fprintf(stderr,
- _("cannot specify both %s and -d name=%s\n"),
- xi.volname, xi.dname);
- usage();
- }
+ dfile = xi.volname = getstr(argv[optind], &dopts, D_NAME);
} else
dfile = xi.dname;
@@ -2027,6 +2095,26 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
lsectorsize = sectorsize;
}
+ /*
+ * Before anything else, verify that we are correctly operating on
+ * files or block devices and set the control parameters correctly.
+ * Explicitly disable direct IO for image files so we don't error out on
+ * sector size mismatches between the new filesystem and the underlying
+ * host filesystem.
+ */
+ check_device_type(dfile, &xi.disfile, !dsize, !xi.dname,
+ Nflag ? NULL : &xi.dcreat, force_overwrite, "d");
+ if (!loginternal)
+ check_device_type(xi.logname, &xi.lisfile, !logsize, !xi.logname,
+ Nflag ? NULL : &xi.lcreat,
+ force_overwrite, "l");
+ if (xi.rtname)
+ check_device_type(xi.rtname, &xi.risfile, !rtsize, !xi.rtname,
+ Nflag ? NULL : &xi.rcreat,
+ force_overwrite, "r");
+ if (xi.disfile || xi.lisfile || xi.risfile)
+ xi.isdirect = 0;
+
memset(&ft, 0, sizeof(ft));
get_topology(&xi, &ft, force_overwrite);
@@ -2167,11 +2255,6 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
}
- if (xi.disfile && (!dsize || !xi.dname)) {
- fprintf(stderr,
- _("if -d file then -d name and -d size are required\n"));
- usage();
- }
if (dsize) {
__uint64_t dbytes;
@@ -2204,11 +2287,6 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
usage();
}
- if (xi.lisfile && (!logsize || !xi.logname)) {
- fprintf(stderr,
- _("if -l file then -l name and -l size are required\n"));
- usage();
- }
if (logsize) {
__uint64_t logbytes;
@@ -2226,11 +2304,6 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
(long long)logbytes, blocksize,
(long long)(logblocks << blocklog));
}
- if (xi.risfile && (!rtsize || !xi.rtname)) {
- fprintf(stderr,
- _("if -r file then -r name and -r size are required\n"));
- usage();
- }
if (rtsize) {
__uint64_t rtbytes;
@@ -2354,22 +2427,14 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
xi.rtsize &= sector_mask;
xi.logBBsize &= (__uint64_t)-1 << (MAX(lsectorlog, 10) - BBSHIFT);
- if (!force_overwrite) {
- if (check_overwrite(dfile) ||
- check_overwrite(logfile) ||
- check_overwrite(xi.rtname)) {
- fprintf(stderr,
- _("%s: Use the -f option to force overwrite.\n"),
- progname);
- exit(1);
- }
- }
+ /* don't do discards on print-only runs or on files */
if (discard && !Nflag) {
- discard_blocks(xi.ddev, xi.dsize);
- if (xi.rtdev)
+ if (!xi.disfile)
+ discard_blocks(xi.ddev, xi.dsize);
+ if (xi.rtdev && !xi.risfile)
discard_blocks(xi.rtdev, xi.rtsize);
- if (xi.logdev && xi.logdev != xi.ddev)
+ if (xi.logdev && xi.logdev != xi.ddev && !xi.lisfile)
discard_blocks(xi.logdev, xi.logBBsize);
}
--
1.8.4.rc3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 01/15] xfsprogs: use common code for multi-disk detection
2013-11-29 1:43 ` [PATCH 01/15] xfsprogs: use common code for multi-disk detection Dave Chinner
@ 2013-12-02 10:40 ` Christoph Hellwig
2013-12-02 22:49 ` Dave Chinner
0 siblings, 1 reply; 42+ messages in thread
From: Christoph Hellwig @ 2013-12-02 10:40 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Fri, Nov 29, 2013 at 12:43:36PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Both xfs_repair and mkfs.xfs need to agree on what is a "multidisk:
> configuration - mkfs for determining the AG count of the filesystem,
> repair for determining how to automatically parallelise it's
> execution. This requires a bunch of common defines that both mkfs
> and reapir need to share.
>
> In fact, most of the defines in xfs_mkfs.h could be shared with
> other programs (i.e. all the defaults mkfs uses) and so it is
> simplest to move xfs_mkfs.h to the shared include directory and add
> the new defines to it directly.
I have to say I do not like making the mkfs header public at all.
There's things like local prototypes in there that shouldn't be in
include/, and the name is wrong, too. include/xfs_geom.h maybe?
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 02/15] mkfs: sanitise ftype parameter values.
2013-11-29 1:43 ` [PATCH 02/15] mkfs: sanitise ftype parameter values Dave Chinner
@ 2013-12-02 10:40 ` Christoph Hellwig
0 siblings, 0 replies; 42+ messages in thread
From: Christoph Hellwig @ 2013-12-02 10:40 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
Looks good,
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] 42+ messages in thread
* Re: [PATCH 03/15] mkfs: Sanitise the superblock feature macros
2013-11-29 1:43 ` [PATCH 03/15] mkfs: Sanitise the superblock feature macros Dave Chinner
@ 2013-12-02 10:43 ` Christoph Hellwig
2013-12-02 22:50 ` Dave Chinner
0 siblings, 1 reply; 42+ messages in thread
From: Christoph Hellwig @ 2013-12-02 10:43 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Fri, Nov 29, 2013 at 12:43:38PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> They are horrible macros that simply obfuscate the code, so
> let's factor the code and make them nice functions.
>
> To do this, add a sb_feat_args structure that carries around the
> variables rather than a strange assortment of variables. This means
> all the default can be clearly defined in a structure
> initialisation, and dependent feature bits are easy to check.
Nice clean,
Reviewed-by: Christoph Hellwig <hch@lst.de>
But one minor nitpick:
> + bool projid16bit;
Given that 32-bit projids are the newer feature I'd make them the
flag instead of inverting it, which is how all the other flags work.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 05/15] mkfs: factor boolean option parsing
2013-11-29 1:43 ` [PATCH 05/15] mkfs: factor boolean option parsing Dave Chinner
@ 2013-12-02 10:46 ` Christoph Hellwig
2013-12-02 23:13 ` Dave Chinner
0 siblings, 1 reply; 42+ messages in thread
From: Christoph Hellwig @ 2013-12-02 10:46 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Fri, Nov 29, 2013 at 12:43:40PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Many of the options passed to mkfs have boolean options (0 or 1) and
> all hand roll the same code and validity checks. Factor these out
> into a common function.
>
> Note that the lazy-count option is now changed to match other
> booleans in that if you don't specify a value, it reverts to the
> default value (on) rather than throwing an error. Similarly the -m
> crc and -n ftype options default to off rather than throwing an
> error.
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
Unrelated question that came up when reading through this patch:
should we start deprecating some options that have long been the
default, like lazy-count or attrv1?
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 04/15] mkfs: validate all input values
2013-11-29 1:43 ` [PATCH 04/15] mkfs: validate all input values Dave Chinner
@ 2013-12-02 17:04 ` Christoph Hellwig
2013-12-02 23:12 ` Dave Chinner
0 siblings, 1 reply; 42+ messages in thread
From: Christoph Hellwig @ 2013-12-02 17:04 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 2637 bytes --]
On Fri, Nov 29, 2013 at 12:43:39PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Right now, mkfs does a poor job of input validation of values. Many
> parameters do not check for trailing garbage and so will pass
> obviously invalid values as OK. Some don't even detect completely
> invalid values, leaving it for other checks later on to fail due to
> a bad value conversion - these tend to rely on atoi() implicitly
> returning a sane value when it is passed garbage, and atoi gives no
> guarantee of the return value when passed garbage.
Would be useful to have a test case for some of these garbage values..
> Finally, the block size of the filesystem is not known until all
> the options have been parsed and we can determine if the default is
> to be used. This means any parameter that relies on using conversion
> from filesystem block size (the "NNNb" format) requires the block
> size to first be specified on the command line so it is known.
>
> Similarly, we make the same rule for specifying counts in sectors.
> This is a change from the existing behaviour that assumes sectors
> are 512 bytes unless otherwise changed on the command line. This,
> unfortunately, leads to complete silliness where you can specify the
> sector size as a count of sectors. It also means that you can do
> some conversions with 512 byte sector sizes, and others with
> whatever was specified on the command line, meaning the mkfs
> behaviour changes depending in where in the command line the sector
> size is changed....
I wonder if this might break some existing uses. The whole notion of
512byte sectors is so ingrained in most people that this doesn't sound
as stupid as it is.
Maybe just warn about that particular case for now instead of outright
rejecting it?
> + creds.cr_uid = getnum(getstr(pp), 0, 0, false);
> + creds.cr_gid = getnum(getstr(pp), 0, 0, false);
Not that I really care deeply, but requiring uids to be numeric seems a
little silly. Maybe we should put accepting user and groups names on a
beginners todo list somewhere.
> +long long
> +getnum(
> + const char *str,
> + unsigned int blocksize,
> + unsigned int sectorsize,
> + bool convert)
> +{
> + long long i;
> + char *sp;
> +
> + if (convert)
> + return cvtnum(blocksize, sectorsize, str);
> +
> + i = strtoll(str, &sp, 0);
> + if (i == 0 && sp == str)
> + return -1LL;
> + if (*sp != '\0')
> + return -1LL; /* trailing garbage */
> + return i;
> +}
So this function does two totally different things based on the last
parameter? Unless the answers is one of the next patches will fix it
¿ thyink it should be split.
[-- Attachment #2: Type: text/plain, Size: 121 bytes --]
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 06/15] mkfs: validate logarithmic parameters sanely
2013-11-29 1:43 ` [PATCH 06/15] mkfs: validate logarithmic parameters sanely Dave Chinner
@ 2013-12-02 17:06 ` Christoph Hellwig
2013-12-02 23:14 ` Dave Chinner
2013-12-03 1:34 ` Michael L. Semon
0 siblings, 2 replies; 42+ messages in thread
From: Christoph Hellwig @ 2013-12-02 17:06 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Fri, Nov 29, 2013 at 12:43:41PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Testing logarithmic paramters like "-n log=<num>" shows that we do a
> terrible job of validating such input. e.g.:
>
> # mkfs.xfs -f -n log=456858480 /dev/vda
> .....
> naming =version 2 bsize=65536 ascii-ci=0 ftype=0
> ....
>
> Yeah, I just asked for a block size of 2^456858480, and it didn't
> get rejected. Great, isn't it?
>
> So, factor out the parsing of logarithmic parameters, and pass in
> the maximum valid value that they can take. These maximum values
> might not be completely accurate (e.g. block/sector sizes will
> affect the eventual valid maximum) but we can get rid of all the
> overflows and stupidities before we get to fine-grained validity
> checking later in mkfs once things like block and sector sizes have
> been finalised.
Btw, is there any good reason not to deprecate the logarithmic
parameters? I can't see why anyone would want to use them, but I see
lots of potential for confusion (happened to myself in the past).
The patch itself looks good:
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] 42+ messages in thread
* Re: [PATCH 07/15] mkfs: structify input parameter passing
2013-11-29 1:43 ` [PATCH 07/15] mkfs: structify input parameter passing Dave Chinner
@ 2013-12-02 17:11 ` Christoph Hellwig
2013-12-02 23:16 ` Dave Chinner
0 siblings, 1 reply; 42+ messages in thread
From: Christoph Hellwig @ 2013-12-02 17:11 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
> + const char name;
> + const char *subopts[MAX_SUBOPTS];
> + struct subopt_param {
> + int index;
> + long long minval;
> + long long maxval;
> + } subopt_params[MAX_SUBOPTS];
> +};
Any reason to have a separate array for subopts instead of
moving it into struct subopt_param?
Except for that I really like the approach!
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] 42+ messages in thread
* Re: [PATCH 08/15] mkfs: getbool is redundant
2013-11-29 1:43 ` [PATCH 08/15] mkfs: getbool is redundant Dave Chinner
@ 2013-12-02 17:12 ` Christoph Hellwig
0 siblings, 0 replies; 42+ messages in thread
From: Christoph Hellwig @ 2013-12-02 17:12 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Fri, Nov 29, 2013 at 12:43:43PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> getbool() can be replaced with getnum_checked with appropriate
> min/max values set for the boolean variables.
Looks good,
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] 42+ messages in thread
* Re: [PATCH 09/15] mkfs: use getnum_checked for all ranged parameters
2013-11-29 1:43 ` [PATCH 09/15] mkfs: use getnum_checked for all ranged parameters Dave Chinner
@ 2013-12-02 17:14 ` Christoph Hellwig
0 siblings, 0 replies; 42+ messages in thread
From: Christoph Hellwig @ 2013-12-02 17:14 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
Looks good,
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] 42+ messages in thread
* Re: [PATCH 10/15] mkfs: add respecification detection to generic parsing
2013-11-29 1:43 ` [PATCH 10/15] mkfs: add respecification detection to generic parsing Dave Chinner
@ 2013-12-02 17:15 ` Christoph Hellwig
0 siblings, 0 replies; 42+ messages in thread
From: Christoph Hellwig @ 2013-12-02 17:15 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
> -const struct opt_params bopts = {
> +struct opt_params bopts = {
Maybe you shouldn't have added these consts in the first place :)
Otherwise looks good,
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] 42+ messages in thread
* Re: [PATCH 11/15] mkfs: table based parsing for converted parameters
2013-11-29 1:43 ` [PATCH 11/15] mkfs: table based parsing for converted parameters Dave Chinner
@ 2013-12-02 17:17 ` Christoph Hellwig
0 siblings, 0 replies; 42+ messages in thread
From: Christoph Hellwig @ 2013-12-02 17:17 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Fri, Nov 29, 2013 at 12:43:46PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> All the parameters that can be passed as block or sector sizes need
> to be passed the block and sector sizes that they should be using
> for conversion. For parameter parsing, it is always the same two
> variables, so to make things easy just declare them as global
> variables so we can avoid needing to pass them to getnum_checked().
>
> We also need to mark these parameters are requiring conversion so
> that we don't need to pass this information manually to
> getnum_checked(). Further, some of these options are required to
> have a power of 2 value, so add optional checking for that as well.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
Looks good,
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] 42+ messages in thread
* Re: [PATCH 12/15] mkfs: merge getnum
2013-11-29 1:43 ` [PATCH 12/15] mkfs: merge getnum Dave Chinner
@ 2013-12-02 17:22 ` Christoph Hellwig
2013-12-02 23:20 ` Dave Chinner
0 siblings, 1 reply; 42+ messages in thread
From: Christoph Hellwig @ 2013-12-02 17:22 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Fri, Nov 29, 2013 at 12:43:47PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> getnum() is now only called by getnum_checked(). Move the two
> together into a single getnum() function and change all the callers
> back to getnum().
So we now have two different getnums in mkfs now. Maybe the one in
proto.c should have a different name?
> +static long long
> +getnum(
> + const char *str,
> + unsigned int blksize,
> + unsigned int sectsize,
> + bool convert)
> +{
> + long long i;
> + char *sp;
> +
> + if (convert)
> + return cvtnum(blksize, sectsize, str);
Also the whole if convert is true sillyness lives on here. The caller
that wants cvtnum should just call it directly.
> + else {
> + char *sp;
> +
> + c = strtoll(str, &sp, 0);
> + if (c == 0 && sp == str)
> + illegal_option(str, opts, index);
> + if (*sp != '\0')
> + illegal_option(str, opts, index);
> + }
And given that the strtoll wrapping code is the same for both getnums
I suspect we shoud just have a mkfs_strtoll that gets called here,
and directly by the proto.c callers.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 13/15] mkfs: encode conflicts into parsing table
2013-11-29 1:43 ` [PATCH 13/15] mkfs: encode conflicts into parsing table Dave Chinner
@ 2013-12-02 17:23 ` Christoph Hellwig
0 siblings, 0 replies; 42+ messages in thread
From: Christoph Hellwig @ 2013-12-02 17:23 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
Looks good,
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] 42+ messages in thread
* Re: [PATCH 15/15] mkfs: don't treat files as though they are block devices
2013-11-29 1:43 ` [PATCH 15/15] mkfs: don't treat files as though they are block devices Dave Chinner
@ 2013-12-02 17:24 ` Christoph Hellwig
2013-12-02 23:21 ` Dave Chinner
0 siblings, 1 reply; 42+ messages in thread
From: Christoph Hellwig @ 2013-12-02 17:24 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
> void
> platform_flush_device(int fd, dev_t device)
> {
> - if (major(device) != RAMDISK_MAJOR)
> + struct stat64 st;
> + if (major(device) == RAMDISK_MAJOR)
> + return;
> +
> + if (fstat64(fd, &st) < 0)
> + return;
> +
> + if (S_ISREG(st.st_mode))
> + fsync(fd);
> + else
> ioctl(fd, BLKFLSBUF, 0);
> }
Given that fsync does the right thing for device on Linux aswell
I'd suggest we make this function call it all the time and get rid
of all the ramdisk magic.
Should probabbly be a a separate patch.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 01/15] xfsprogs: use common code for multi-disk detection
2013-12-02 10:40 ` Christoph Hellwig
@ 2013-12-02 22:49 ` Dave Chinner
0 siblings, 0 replies; 42+ messages in thread
From: Dave Chinner @ 2013-12-02 22:49 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Dec 02, 2013 at 02:40:16AM -0800, Christoph Hellwig wrote:
> On Fri, Nov 29, 2013 at 12:43:36PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > Both xfs_repair and mkfs.xfs need to agree on what is a "multidisk:
> > configuration - mkfs for determining the AG count of the filesystem,
> > repair for determining how to automatically parallelise it's
> > execution. This requires a bunch of common defines that both mkfs
> > and reapir need to share.
> >
> > In fact, most of the defines in xfs_mkfs.h could be shared with
> > other programs (i.e. all the defaults mkfs uses) and so it is
> > simplest to move xfs_mkfs.h to the shared include directory and add
> > the new defines to it directly.
>
> I have to say I do not like making the mkfs header public at all.
> There's things like local prototypes in there that shouldn't be in
> include/, and the name is wrong, too. include/xfs_geom.h maybe?
Yeah, I know it's a bit messy - I thought that as the series went on
all those prototypes would go away, but only some of them have. I
like the idea of xfs_geom.h, though, because that encapsulates what
we are trying to shared here...
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] 42+ messages in thread
* Re: [PATCH 03/15] mkfs: Sanitise the superblock feature macros
2013-12-02 10:43 ` Christoph Hellwig
@ 2013-12-02 22:50 ` Dave Chinner
0 siblings, 0 replies; 42+ messages in thread
From: Dave Chinner @ 2013-12-02 22:50 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Dec 02, 2013 at 02:43:02AM -0800, Christoph Hellwig wrote:
> On Fri, Nov 29, 2013 at 12:43:38PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > They are horrible macros that simply obfuscate the code, so
> > let's factor the code and make them nice functions.
> >
> > To do this, add a sb_feat_args structure that carries around the
> > variables rather than a strange assortment of variables. This means
> > all the default can be clearly defined in a structure
> > initialisation, and dependent feature bits are easy to check.
>
> Nice clean,
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> But one minor nitpick:
>
> > + bool projid16bit;
>
> Given that 32-bit projids are the newer feature I'd make them the
> flag instead of inverting it, which is how all the other flags work.
Fair enough - I just converted the existing variables to a
structure. I'll fix that up.
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] 42+ messages in thread
* Re: [PATCH 04/15] mkfs: validate all input values
2013-12-02 17:04 ` Christoph Hellwig
@ 2013-12-02 23:12 ` Dave Chinner
2013-12-03 9:42 ` Christoph Hellwig
0 siblings, 1 reply; 42+ messages in thread
From: Dave Chinner @ 2013-12-02 23:12 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 4327 bytes --]
On Mon, Dec 02, 2013 at 09:04:20AM -0800, Christoph Hellwig wrote:
> On Fri, Nov 29, 2013 at 12:43:39PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > Right now, mkfs does a poor job of input validation of values. Many
> > parameters do not check for trailing garbage and so will pass
> > obviously invalid values as OK. Some don't even detect completely
> > invalid values, leaving it for other checks later on to fail due to
> > a bad value conversion - these tend to rely on atoi() implicitly
> > returning a sane value when it is passed garbage, and atoi gives no
> > guarantee of the return value when passed garbage.
>
> Would be useful to have a test case for some of these garbage values..
Yes, I need to write a script that tests a large number of
valid/invalid command line options to make sure I haven't broken
random stuff. The conflicts part is the fun bit...
But, in general, stuff like this is what I'm trying to prevent:
# mkfs.xfs -d agcount=32fdsglkjg /dev/vda
will take that as a valid parameter with a value of 32....
> > Finally, the block size of the filesystem is not known until all
> > the options have been parsed and we can determine if the default is
> > to be used. This means any parameter that relies on using conversion
> > from filesystem block size (the "NNNb" format) requires the block
> > size to first be specified on the command line so it is known.
> >
> > Similarly, we make the same rule for specifying counts in sectors.
> > This is a change from the existing behaviour that assumes sectors
> > are 512 bytes unless otherwise changed on the command line. This,
> > unfortunately, leads to complete silliness where you can specify the
> > sector size as a count of sectors. It also means that you can do
> > some conversions with 512 byte sector sizes, and others with
> > whatever was specified on the command line, meaning the mkfs
> > behaviour changes depending in where in the command line the sector
> > size is changed....
>
> I wonder if this might break some existing uses. The whole notion of
> 512byte sectors is so ingrained in most people that this doesn't sound
> as stupid as it is.
>
> Maybe just warn about that particular case for now instead of outright
> rejecting it?
How does this make sense, though?
# mkfs.xfs -s size=4s /dev/vda
Specifying the sector size in *sectors* is currently considered a
valid thing to do. That's insane and fundamentally broken, because
this
# mkfs.xfs -b size=4s -s size=2s /dev/vda
results in the block size conversion using a 512 byte sector size,
and everything else using a 1024 byte sector size for conversions.
e.g:
# mkfs.xfs -b size=4s -s size=2s -n size=2s /dev/vda
results in a block size of 2k (4*512) and a directory block size of
2k (2*1024). i.e. the result of unit conversion is dependent on
where the sector size is specified on the command line!
> > + creds.cr_uid = getnum(getstr(pp), 0, 0, false);
> > + creds.cr_gid = getnum(getstr(pp), 0, 0, false);
>
> Not that I really care deeply, but requiring uids to be numeric seems a
> little silly. Maybe we should put accepting user and groups names on a
> beginners todo list somewhere.
Yup, seems like a goo idea to support that...
>
> > +long long
> > +getnum(
> > + const char *str,
> > + unsigned int blocksize,
> > + unsigned int sectorsize,
> > + bool convert)
> > +{
> > + long long i;
> > + char *sp;
> > +
> > + if (convert)
> > + return cvtnum(blocksize, sectorsize, str);
> > +
> > + i = strtoll(str, &sp, 0);
> > + if (i == 0 && sp == str)
> > + return -1LL;
> > + if (*sp != '\0')
> > + return -1LL; /* trailing garbage */
> > + return i;
> > +}
>
> So this function does two totally different things based on the last
> parameter? Unless the answers is one of the next patches will fix it
> ¿ thyink it should be split.
That function grows a lot more checking of the values - min/max
checking, conflict/respec checking, etc as everything gets converted
to struct based checking.
What I intended to do was remove cvtnum() altogether as it is now a
direct copy of the cvtnum function in libxcmd/input.c::cvtnum() and
just link against libxcmd. I haven't got that far yet - I might just
move cvtnum into getnum() and be done with it....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
[-- Attachment #2: Type: text/plain, Size: 121 bytes --]
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 05/15] mkfs: factor boolean option parsing
2013-12-02 10:46 ` Christoph Hellwig
@ 2013-12-02 23:13 ` Dave Chinner
0 siblings, 0 replies; 42+ messages in thread
From: Dave Chinner @ 2013-12-02 23:13 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Dec 02, 2013 at 02:46:31AM -0800, Christoph Hellwig wrote:
> On Fri, Nov 29, 2013 at 12:43:40PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > Many of the options passed to mkfs have boolean options (0 or 1) and
> > all hand roll the same code and validity checks. Factor these out
> > into a common function.
> >
> > Note that the lazy-count option is now changed to match other
> > booleans in that if you don't specify a value, it reverts to the
> > default value (on) rather than throwing an error. Similarly the -m
> > crc and -n ftype options default to off rather than throwing an
> > error.
>
> Looks good,
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> Unrelated question that came up when reading through this patch:
>
> should we start deprecating some options that have long been the
> default, like lazy-count or attrv1?
Yes, we probably should. I'll put that at the end of the series when
it's just a trivial case of adding a flag to the relevant options
and adding a check and warning....
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] 42+ messages in thread
* Re: [PATCH 06/15] mkfs: validate logarithmic parameters sanely
2013-12-02 17:06 ` Christoph Hellwig
@ 2013-12-02 23:14 ` Dave Chinner
2013-12-03 1:34 ` Michael L. Semon
1 sibling, 0 replies; 42+ messages in thread
From: Dave Chinner @ 2013-12-02 23:14 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Dec 02, 2013 at 09:06:01AM -0800, Christoph Hellwig wrote:
> On Fri, Nov 29, 2013 at 12:43:41PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > Testing logarithmic paramters like "-n log=<num>" shows that we do a
> > terrible job of validating such input. e.g.:
> >
> > # mkfs.xfs -f -n log=456858480 /dev/vda
> > .....
> > naming =version 2 bsize=65536 ascii-ci=0 ftype=0
> > ....
> >
> > Yeah, I just asked for a block size of 2^456858480, and it didn't
> > get rejected. Great, isn't it?
> >
> > So, factor out the parsing of logarithmic parameters, and pass in
> > the maximum valid value that they can take. These maximum values
> > might not be completely accurate (e.g. block/sector sizes will
> > affect the eventual valid maximum) but we can get rid of all the
> > overflows and stupidities before we get to fine-grained validity
> > checking later in mkfs once things like block and sector sizes have
> > been finalised.
>
> Btw, is there any good reason not to deprecate the logarithmic
> parameters? I can't see why anyone would want to use them, but I see
> lots of potential for confusion (happened to myself in the past).
Yup, I can't see a good reason for keeping them. Indeed, we could
just add a conversion identifier to indicate the value is in a power
of 2 and have cvtnum() do the conversion for us...
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] 42+ messages in thread
* Re: [PATCH 07/15] mkfs: structify input parameter passing
2013-12-02 17:11 ` Christoph Hellwig
@ 2013-12-02 23:16 ` Dave Chinner
0 siblings, 0 replies; 42+ messages in thread
From: Dave Chinner @ 2013-12-02 23:16 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Dec 02, 2013 at 09:11:03AM -0800, Christoph Hellwig wrote:
> > + const char name;
> > + const char *subopts[MAX_SUBOPTS];
> > + struct subopt_param {
> > + int index;
> > + long long minval;
> > + long long maxval;
> > + } subopt_params[MAX_SUBOPTS];
> > +};
>
> Any reason to have a separate array for subopts instead of
> moving it into struct subopt_param?
getsubopt() requires a array of tokens for it's parsing, hence the
separation of the subopts names and the related parameters.
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] 42+ messages in thread
* Re: [PATCH 12/15] mkfs: merge getnum
2013-12-02 17:22 ` Christoph Hellwig
@ 2013-12-02 23:20 ` Dave Chinner
0 siblings, 0 replies; 42+ messages in thread
From: Dave Chinner @ 2013-12-02 23:20 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Dec 02, 2013 at 09:22:33AM -0800, Christoph Hellwig wrote:
> On Fri, Nov 29, 2013 at 12:43:47PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > getnum() is now only called by getnum_checked(). Move the two
> > together into a single getnum() function and change all the callers
> > back to getnum().
>
> So we now have two different getnums in mkfs now. Maybe the one in
> proto.c should have a different name?
Probably should.
>
> > +static long long
> > +getnum(
> > + const char *str,
> > + unsigned int blksize,
> > + unsigned int sectsize,
> > + bool convert)
> > +{
> > + long long i;
> > + char *sp;
> > +
> > + if (convert)
> > + return cvtnum(blksize, sectsize, str);
>
> Also the whole if convert is true sillyness lives on here. The caller
> that wants cvtnum should just call it directly.
Yes, but soon it doesn't just return the value directly ;)
> > + else {
> > + char *sp;
> > +
> > + c = strtoll(str, &sp, 0);
> > + if (c == 0 && sp == str)
> > + illegal_option(str, opts, index);
> > + if (*sp != '\0')
> > + illegal_option(str, opts, index);
> > + }
>
> And given that the strtoll wrapping code is the same for both getnums
> I suspect we shoud just have a mkfs_strtoll that gets called here,
> and directly by the proto.c callers.
I'll have a look at doing that once everything else falls out.
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] 42+ messages in thread
* Re: [PATCH 15/15] mkfs: don't treat files as though they are block devices
2013-12-02 17:24 ` Christoph Hellwig
@ 2013-12-02 23:21 ` Dave Chinner
0 siblings, 0 replies; 42+ messages in thread
From: Dave Chinner @ 2013-12-02 23:21 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Dec 02, 2013 at 09:24:41AM -0800, Christoph Hellwig wrote:
> > void
> > platform_flush_device(int fd, dev_t device)
> > {
> > - if (major(device) != RAMDISK_MAJOR)
> > + struct stat64 st;
> > + if (major(device) == RAMDISK_MAJOR)
> > + return;
> > +
> > + if (fstat64(fd, &st) < 0)
> > + return;
> > +
> > + if (S_ISREG(st.st_mode))
> > + fsync(fd);
> > + else
> > ioctl(fd, BLKFLSBUF, 0);
> > }
>
> Given that fsync does the right thing for device on Linux aswell
> I'd suggest we make this function call it all the time and get rid
> of all the ramdisk magic.
>
> Should probabbly be a a separate patch.
Yeah, I just shovelled all the fixes to problems I found into this
patch, so it needs to be split up a bit more....
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] 42+ messages in thread
* Re: [PATCH 06/15] mkfs: validate logarithmic parameters sanely
2013-12-02 17:06 ` Christoph Hellwig
2013-12-02 23:14 ` Dave Chinner
@ 2013-12-03 1:34 ` Michael L. Semon
1 sibling, 0 replies; 42+ messages in thread
From: Michael L. Semon @ 2013-12-03 1:34 UTC (permalink / raw)
To: Christoph Hellwig, Dave Chinner; +Cc: xfs
On 12/02/2013 12:06 PM, Christoph Hellwig wrote:
> On Fri, Nov 29, 2013 at 12:43:41PM +1100, Dave Chinner wrote:
>> From: Dave Chinner <dchinner@redhat.com>
>>
>> Testing logarithmic paramters like "-n log=<num>" shows that we do a
>> terrible job of validating such input. e.g.:
>>
>> # mkfs.xfs -f -n log=456858480 /dev/vda
>> .....
>> naming =version 2 bsize=65536 ascii-ci=0 ftype=0
>> ....
>>
>> Yeah, I just asked for a block size of 2^456858480, and it didn't
>> get rejected. Great, isn't it?
>>
>> So, factor out the parsing of logarithmic parameters, and pass in
>> the maximum valid value that they can take. These maximum values
>> might not be completely accurate (e.g. block/sector sizes will
>> affect the eventual valid maximum) but we can get rid of all the
>> overflows and stupidities before we get to fine-grained validity
>> checking later in mkfs once things like block and sector sizes have
>> been finalised.
>
> Btw, is there any good reason not to deprecate the logarithmic
> parameters? I can't see why anyone would want to use them, but I see
> lots of potential for confusion (happened to myself in the past).
>
> The patch itself looks good:
I use log= almost exclusively. The habit comes from using ntpd for
many years. An ntp.conf line like this...
server ntp.example.org minpoll 4 maxpoll 10
...means "poll server 'ntp.example.org' no fewer than once every 16s,
no greater than once every 1024s." For XFS, I remember the numbers
9, 10, and 11, dropping the 12 because it's the default. At least
for block sizes, v5 XFS has me dropping the 9 as well. There are
many places in computers to remember 1024 and 2048, and they're just
more readily in mind as 10 and 11. Personal preference.
Feel free to deprecate it, though. The change back to non-logarithmic
notation isn't going to be a problem. I was just putting in my two
cents on the matter.
Thanks!
Michael
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 04/15] mkfs: validate all input values
2013-12-02 23:12 ` Dave Chinner
@ 2013-12-03 9:42 ` Christoph Hellwig
2013-12-11 4:27 ` Jeff Liu
0 siblings, 1 reply; 42+ messages in thread
From: Christoph Hellwig @ 2013-12-03 9:42 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On Tue, Dec 03, 2013 at 10:12:02AM +1100, Dave Chinner wrote:
> How does this make sense, though?
>
> # mkfs.xfs -s size=4s /dev/vda
>
> Specifying the sector size in *sectors* is currently considered a
> valid thing to do. That's insane and fundamentally broken, because
> this
>
> # mkfs.xfs -b size=4s -s size=2s /dev/vda
>
> results in the block size conversion using a 512 byte sector size,
> and everything else using a 1024 byte sector size for conversions.
> e.g:
>
> # mkfs.xfs -b size=4s -s size=2s -n size=2s /dev/vda
>
> results in a block size of 2k (4*512) and a directory block size of
> 2k (2*1024). i.e. the result of unit conversion is dependent on
> where the sector size is specified on the command line!
True. Guess we should indeed just outright rejecting it. I was more
concerned about using the sector size before defined for other
parameters, but given how seldomly we specify it on the command line
anyway we're probably better off just using the normal table based
validation.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 04/15] mkfs: validate all input values
2013-12-03 9:42 ` Christoph Hellwig
@ 2013-12-11 4:27 ` Jeff Liu
2013-12-11 23:57 ` Dave Chinner
0 siblings, 1 reply; 42+ messages in thread
From: Jeff Liu @ 2013-12-11 4:27 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
Hi, Dave,
While test this patch, I wonder if we should also validate non-supported
data block size combine with the system page size or not, as we do such
kind of checkup for non-supported inode size in mkfs...
I can simply trigger scary corruption error with backtraces on 4K page
size machine via: mkfs.xfs -f -b size=8192 /dev/xxx; mount /dev/xxx /xfs
Also, here is patch inspired by Eric's previous fix for non-xfs mount probes.
Thanks,
-Jeff
From: Jie Liu <jeff.liu@oracle.com>
Subject: xfs: don't emit corruption noise on non-supported mount options
There is no need to issue the scary corruption error and backtrace
which were shown as following if we get ENOSYS due to mount with
non-supported options, e.g, mkfs.xfs -f -b size=8192 /dev/sda7
XFS (sda7): File system with blocksize 8192 bytes. Only pagesize (4096) or less will currently work.
.........
XFS (sda7): Internal error xfs_sb_read_verify at line 630 of file fs/xfs/xfs_sb.c
Workqueue: xfslogd xfs_buf_iodone_work [xfs]
Call Trace:
[<ffffffff8171412b>] dump_stack+0x45/0x56
[<ffffffffa0a63c7b>] xfs_error_report+0x3b/0x40 [xfs]
[<ffffffffa0a609e5>] ? xfs_buf_iodone_work+0xa5/0xd0 [xfs]
[<ffffffffa0a63cd5>] xfs_corruption_error+0x55/0x80 [xfs]
[<ffffffffa0ac9883>] xfs_sb_read_verify+0x143/0x150 [xfs]
[<ffffffffa0a609e5>] ? xfs_buf_iodone_work+0xa5/0xd0 [xfs]
[<ffffffffa0a609e5>] xfs_buf_iodone_work+0xa5/0xd0 [xfs]
[<ffffffff81080582>] process_one_work+0x182/0x450
[<ffffffff81081341>] worker_thread+0x121/0x410
[<ffffffff81081220>] ? rescuer_thread+0x3e0/0x3e0
[<ffffffff81087ff2>] kthread+0xd2/0xf0
[<ffffffff81087f20>] ? kthread_create_on_node+0x190/0x190
[<ffffffff81724c3c>] ret_from_fork+0x7c/0xb0
[<ffffffff81087f20>] ? kthread_create_on_node+0x190/0x190
XFS (sda7): Corruption detected. Unmount and run xfs_repair
XFS (sda7): SB validate failed with error 38.
This is inspired by another similar fix from Eric Sandeen:
[ commit 31625f28a "xfs: don't emit corruption noise on fs probes" ]
Signed-off-by: Jie Liu <jeff.liu@oracle.com>
---
fs/xfs/xfs_sb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
index b7c9aea..47e69c8 100644
--- a/fs/xfs/xfs_sb.c
+++ b/fs/xfs/xfs_sb.c
@@ -625,7 +625,7 @@ xfs_sb_read_verify(
out_error:
if (error) {
- if (error != EWRONGFS)
+ if (error != EWRONGFS && error != ENOSYS)
XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
mp, bp->b_addr);
xfs_buf_ioerror(bp, error);
--
1.8.3.2
On 12/03 2013 17:42 PM, Christoph Hellwig wrote:
> On Tue, Dec 03, 2013 at 10:12:02AM +1100, Dave Chinner wrote:
>> How does this make sense, though?
>>
>> # mkfs.xfs -s size=4s /dev/vda
>>
>> Specifying the sector size in *sectors* is currently considered a
>> valid thing to do. That's insane and fundamentally broken, because
>> this
>>
>> # mkfs.xfs -b size=4s -s size=2s /dev/vda
>>
>> results in the block size conversion using a 512 byte sector size,
>> and everything else using a 1024 byte sector size for conversions.
>> e.g:
>>
>> # mkfs.xfs -b size=4s -s size=2s -n size=2s /dev/vda
>>
>> results in a block size of 2k (4*512) and a directory block size of
>> 2k (2*1024). i.e. the result of unit conversion is dependent on
>> where the sector size is specified on the command line!
>
> True. Guess we should indeed just outright rejecting it. I was more
> concerned about using the sector size before defined for other
> parameters, but given how seldomly we specify it on the command line
> anyway we're probably better off just using the normal table based
> validation.
>
> _______________________________________________
> 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 related [flat|nested] 42+ messages in thread
* Re: [PATCH 04/15] mkfs: validate all input values
2013-12-11 4:27 ` Jeff Liu
@ 2013-12-11 23:57 ` Dave Chinner
0 siblings, 0 replies; 42+ messages in thread
From: Dave Chinner @ 2013-12-11 23:57 UTC (permalink / raw)
To: Jeff Liu; +Cc: Christoph Hellwig, xfs
On Wed, Dec 11, 2013 at 12:27:40PM +0800, Jeff Liu wrote:
> Hi, Dave,
>
> While test this patch, I wonder if we should also validate non-supported
> data block size combine with the system page size or not, as we do such
> kind of checkup for non-supported inode size in mkfs...
>
> I can simply trigger scary corruption error with backtraces on 4K page
> size machine via: mkfs.xfs -f -b size=8192 /dev/xxx; mount /dev/xxx /xfs
That's the same case as a single bit error, which is somethign we
should catch and warn loudly about. So, no, I don't think we should
change it for this reason.
That said, we do need to improve the verfiers to be able to separate
CRC validation errors from corruption detected by the verifier. This
means we'll need to rework the boiler-plate error handling in all
the verifiers and we should probably address the verbosity issue
of these corruption warnings at that point in time....
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] 42+ messages in thread
end of thread, other threads:[~2013-12-11 23:57 UTC | newest]
Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-29 1:43 [RFC, PATCH 00/15] mkfs: sanitise input parameters Dave Chinner
2013-11-29 1:43 ` [PATCH 01/15] xfsprogs: use common code for multi-disk detection Dave Chinner
2013-12-02 10:40 ` Christoph Hellwig
2013-12-02 22:49 ` Dave Chinner
2013-11-29 1:43 ` [PATCH 02/15] mkfs: sanitise ftype parameter values Dave Chinner
2013-12-02 10:40 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 03/15] mkfs: Sanitise the superblock feature macros Dave Chinner
2013-12-02 10:43 ` Christoph Hellwig
2013-12-02 22:50 ` Dave Chinner
2013-11-29 1:43 ` [PATCH 04/15] mkfs: validate all input values Dave Chinner
2013-12-02 17:04 ` Christoph Hellwig
2013-12-02 23:12 ` Dave Chinner
2013-12-03 9:42 ` Christoph Hellwig
2013-12-11 4:27 ` Jeff Liu
2013-12-11 23:57 ` Dave Chinner
2013-11-29 1:43 ` [PATCH 05/15] mkfs: factor boolean option parsing Dave Chinner
2013-12-02 10:46 ` Christoph Hellwig
2013-12-02 23:13 ` Dave Chinner
2013-11-29 1:43 ` [PATCH 06/15] mkfs: validate logarithmic parameters sanely Dave Chinner
2013-12-02 17:06 ` Christoph Hellwig
2013-12-02 23:14 ` Dave Chinner
2013-12-03 1:34 ` Michael L. Semon
2013-11-29 1:43 ` [PATCH 07/15] mkfs: structify input parameter passing Dave Chinner
2013-12-02 17:11 ` Christoph Hellwig
2013-12-02 23:16 ` Dave Chinner
2013-11-29 1:43 ` [PATCH 08/15] mkfs: getbool is redundant Dave Chinner
2013-12-02 17:12 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 09/15] mkfs: use getnum_checked for all ranged parameters Dave Chinner
2013-12-02 17:14 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 10/15] mkfs: add respecification detection to generic parsing Dave Chinner
2013-12-02 17:15 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 11/15] mkfs: table based parsing for converted parameters Dave Chinner
2013-12-02 17:17 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 12/15] mkfs: merge getnum Dave Chinner
2013-12-02 17:22 ` Christoph Hellwig
2013-12-02 23:20 ` Dave Chinner
2013-11-29 1:43 ` [PATCH 13/15] mkfs: encode conflicts into parsing table Dave Chinner
2013-12-02 17:23 ` Christoph Hellwig
2013-11-29 1:43 ` [PATCH 14/15] mkfs: add string options to generic parsing Dave Chinner
2013-11-29 1:43 ` [PATCH 15/15] mkfs: don't treat files as though they are block devices Dave Chinner
2013-12-02 17:24 ` Christoph Hellwig
2013-12-02 23:21 ` Dave Chinner
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.