[PATCH 0/3] xfsprogs: misc xfsprogs cleanups

[PATCH 0/3] xfsprogs: misc xfsprogs cleanups

[Eric Sandeen]

A handful of things found while reviewing pending patches...

[PATCH 1/3] mkfs: un-document removed logarithm based CLI options

[Eric Sandeen]

Remove logarithm-based options from usage() and manpage.

Fixes: 70f72d5 "mkfs: remove logarithm based CLI options"
Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
---

diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
index bbbe1c5..4b8c78c 100644
--- a/man/man8/mkfs.xfs.8
+++ b/man/man8/mkfs.xfs.8
@@ -5,7 +5,7 @@ mkfs.xfs \- construct an XFS filesystem
 .B mkfs.xfs
 [
 .B \-b
-.I block_size
+.I block_size_options
 ] [
 .B \-m
 .I global_metadata_options
@@ -33,7 +33,7 @@ mkfs.xfs \- construct an XFS filesystem
 .I realtime_section_options
 ] [
 .B \-s
-.I sector_size
+.I sector_size_options
 ] [
 .B \-L
 .I label
@@ -126,17 +126,14 @@ disable or enable the functionality.
 .BI \-b " block_size_options"
 This option specifies the fundamental block size of the filesystem.
 The valid
-.I block_size_options
-are:
-.BI log= value
-or
+.I block_size_option
+is:
+.RS 1.2i
+.TP
 .BI size= value
-and only one can be supplied.
-The block size is specified either as a base two logarithm value with
-.BR log= ,
-or in bytes with
-.BR size= .
-The default value is 4096 bytes (4 KiB), the minimum is 512, and the
+The filesystem block size is specified with a
+.I value
+in bytes. 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
@@ -147,6 +144,7 @@ Although
 .B mkfs.xfs
 will accept any of these values and create a valid filesystem,
 XFS on Linux can only mount filesystems with pagesize or smaller blocks.
+.RE
 .TP
 .BI \-m " global_metadata_options"
 These options specify metadata format options that either apply to the entire
@@ -419,15 +417,11 @@ The valid
 are:
 .RS 1.2i
 .TP
-.BI size= value " | log=" value " | perblock=" value
+.BI size= value " | perblock=" value
 The inode size is specified either as a
 .I value
 in bytes with
-.BR size= ,
-a base two logarithm
-.I value
-with
-.BR log= ,
+.BR size=
 or as the number fitting in a filesystem block with
 .BR perblock= .
 The minimum (and default)
@@ -648,15 +642,10 @@ These options specify the version and size parameters for the naming
 are:
 .RS 1.2i
 .TP
-.BI size= value " | log=" value
-The block size is specified either as a
-.I value
-in bytes with
-.BR size= ,
-or as a base two logarithm
+.BI size= value
+The directory block size is specified with a
 .I value
-.RB "with " log= .
-The block size must be a power of 2 and cannot be less than the
+in bytes.  The block size must be a power of 2 and cannot be less than the
 filesystem block size.
 The default size
 .I value
@@ -888,15 +877,17 @@ This option disables stripe size detection, enforcing a realtime device with no
 stripe geometry.
 .RE
 .TP
-.BI \-s " sector_size"
+.BI \-s " sector_size_options"
 This option specifies the fundamental sector size of the filesystem.
-The
-.I sector_size
-is specified either as a value in bytes with
+The valid
+.I sector_size_option
+is:
+.RS 1.2i
+.TP
 .BI size= value
-or as a base two logarithm value with
-.BI log= value.
-The default
+The sector size is specified with a
+.I value
+in bytes.  The default
 .I sector_size
 is 512 bytes. The minimum value for sector size is
 512; the maximum is 32768 (32 KiB). The
@@ -907,6 +898,7 @@ filesystem block size.
 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.
+.RE
 .TP
 .BI \-L " label"
 Set the filesystem
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index f3d57cf..1f3494c 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -860,25 +860,24 @@ static void __attribute__((noreturn))
 usage( void )
 {
 	fprintf(stderr, _("Usage: %s\n\
-/* blocksize */		[-b log=n|size=num]\n\
+/* blocksize */		[-b size=num]\n\
 /* metadata */		[-m crc=0|1,finobt=0|1,uuid=xxx,rmapbt=0|1,reflink=0|1]\n\
 /* data subvol */	[-d agcount=n,agsize=n,file,name=xxx,size=num,\n\
 			    (sunit=value,swidth=value|su=num,sw=num|noalign),\n\
-			    sectlog=n|sectsize=num\n\
+			    sectsize=num\n\
 /* force overwrite */	[-f]\n\
 /* inode size */	[-i log=n|perblock=n|size=num,maxpct=n,attr=0|1|2,\n\
 			    projid32bit=0|1,sparse=0|1]\n\
 /* no discard */	[-K]\n\
 /* log subvol */	[-l agnum=n,internal,size=num,logdev=xxx,version=n\n\
-			    sunit=value|su=num,sectlog=n|sectsize=num,\n\
-			    lazy-count=0|1]\n\
+			    sunit=value|su=num,sectsize=num,lazy-count=0|1]\n\
 /* label */		[-L label (maximum 12 characters)]\n\
-/* naming */		[-n log=n|size=num,version=2|ci,ftype=0|1]\n\
+/* naming */		[-n size=num,version=2|ci,ftype=0|1]\n\
 /* no-op info only */	[-N]\n\
 /* prototype file */	[-p fname]\n\
 /* quiet */		[-q]\n\
 /* realtime subvol */	[-r extsize=num,size=num,rtdev=xxx]\n\
-/* sectorsize */	[-s log=n|size=num]\n\
+/* sectorsize */	[-s size=num]\n\
 /* version */		[-V]\n\
 			devicename\n\
 <devicename> is required unless -d name=xxx is given.\n\

[PATCH 2/3] mkfs: pass switch case value directly into getnum/getstr

[Eric Sandeen]

Parsing did does sort of thing:

	case D_AGCOUNT:
		cli->agcount = getnum(value, opts, D_AGCOUNT);

which is just begging for a cut and paste error between the
case value and the enum passed into getnum/getstr.  Pass
"subopt" instead so that it is always consistent with the case.

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

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 1f3494c..035af03 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1400,7 +1400,7 @@ block_opts_parser(
 {
 	switch (subopt) {
 	case B_SIZE:
-		cli->blocksize = getnum(value, opts, B_SIZE);
+		cli->blocksize = getnum(value, opts, subopt);
 		break;
 	default:
 		return -EINVAL;
@@ -1417,52 +1417,52 @@ data_opts_parser(
 {
 	switch (subopt) {
 	case D_AGCOUNT:
-		cli->agcount = getnum(value, opts, D_AGCOUNT);
+		cli->agcount = getnum(value, opts, subopt);
 		break;
 	case D_AGSIZE:
-		cli->agsize = getstr(value, opts, D_AGSIZE);
+		cli->agsize = getstr(value, opts, subopt);
 		break;
 	case D_FILE:
-		cli->xi->disfile = getnum(value, opts, D_FILE);
+		cli->xi->disfile = getnum(value, opts, subopt);
 		break;
 	case D_NAME:
-		cli->xi->dname = getstr(value, opts, D_NAME);
+		cli->xi->dname = getstr(value, opts, subopt);
 		break;
 	case D_SIZE:
-		cli->dsize = getstr(value, opts, D_SIZE);
+		cli->dsize = getstr(value, opts, subopt);
 		break;
 	case D_SUNIT:
-		cli->dsunit = getnum(value, opts, D_SUNIT);
+		cli->dsunit = getnum(value, opts, subopt);
 		break;
 	case D_SWIDTH:
-		cli->dswidth = getnum(value, opts, D_SWIDTH);
+		cli->dswidth = getnum(value, opts, subopt);
 		break;
 	case D_SU:
-		cli->dsu = getstr(value, opts, D_SU);
+		cli->dsu = getstr(value, opts, subopt);
 		break;
 	case D_SW:
-		cli->dsw = getnum(value, opts, D_SW);
+		cli->dsw = getnum(value, opts, subopt);
 		break;
 	case D_NOALIGN:
-		cli->sb_feat.nodalign = getnum(value, opts, D_NOALIGN);
+		cli->sb_feat.nodalign = getnum(value, opts, subopt);
 		break;
 	case D_SECTSIZE:
-		cli->sectorsize = getnum(value, opts, D_SECTSIZE);
+		cli->sectorsize = getnum(value, opts, subopt);
 		break;
 	case D_RTINHERIT:
-		if (getnum(value, opts, D_RTINHERIT))
+		if (getnum(value, opts, subopt))
 			cli->fsx.fsx_xflags |= XFS_DIFLAG_RTINHERIT;
 		break;
 	case D_PROJINHERIT:
-		cli->fsx.fsx_projid = getnum(value, opts, D_PROJINHERIT);
+		cli->fsx.fsx_projid = getnum(value, opts, subopt);
 		cli->fsx.fsx_xflags |= XFS_DIFLAG_PROJINHERIT;
 		break;
 	case D_EXTSZINHERIT:
-		cli->fsx.fsx_extsize = getnum(value, opts, D_EXTSZINHERIT);
+		cli->fsx.fsx_extsize = getnum(value, opts, subopt);
 		cli->fsx.fsx_xflags |= XFS_DIFLAG_EXTSZINHERIT;
 		break;
 	case D_COWEXTSIZE:
-		cli->fsx.fsx_cowextsize = getnum(value, opts, D_COWEXTSIZE);
+		cli->fsx.fsx_cowextsize = getnum(value, opts, subopt);
 		cli->fsx.fsx_xflags |= FS_XFLAG_COWEXTSIZE;
 		break;
 	default:
@@ -1480,25 +1480,25 @@ inode_opts_parser(
 {
 	switch (subopt) {
 	case I_ALIGN:
-		cli->sb_feat.inode_align = getnum(value, opts, I_ALIGN);
+		cli->sb_feat.inode_align = getnum(value, opts, subopt);
 		break;
 	case I_MAXPCT:
-		cli->imaxpct = getnum(value, opts, I_MAXPCT);
+		cli->imaxpct = getnum(value, opts, subopt);
 		break;
 	case I_PERBLOCK:
-		cli->inopblock = getnum(value, opts, I_PERBLOCK);
+		cli->inopblock = getnum(value, opts, subopt);
 		break;
 	case I_SIZE:
-		cli->inodesize = getnum(value, opts, I_SIZE);
+		cli->inodesize = getnum(value, opts, subopt);
 		break;
 	case I_ATTR:
-		cli->sb_feat.attr_version = getnum(value, opts, I_ATTR);
+		cli->sb_feat.attr_version = getnum(value, opts, subopt);
 		break;
 	case I_PROJID32BIT:
-		cli->sb_feat.projid32bit = getnum(value, opts, I_PROJID32BIT);
+		cli->sb_feat.projid32bit = getnum(value, opts, subopt);
 		break;
 	case I_SPINODES:
-		cli->sb_feat.spinodes = getnum(value, opts, I_SPINODES);
+		cli->sb_feat.spinodes = getnum(value, opts, subopt);
 		break;
 	default:
 		return -EINVAL;
@@ -1515,36 +1515,36 @@ log_opts_parser(
 {
 	switch (subopt) {
 	case L_AGNUM:
-		cli->logagno = getnum(value, opts, L_AGNUM);
+		cli->logagno = getnum(value, opts, subopt);
 		break;
 	case L_FILE:
-		cli->xi->lisfile = getnum(value, opts, L_FILE);
+		cli->xi->lisfile = getnum(value, opts, subopt);
 		break;
 	case L_INTERNAL:
-		cli->loginternal = getnum(value, opts, L_INTERNAL);
+		cli->loginternal = getnum(value, opts, subopt);
 		break;
 	case L_SU:
-		cli->lsu = getstr(value, opts, L_SU);
+		cli->lsu = getstr(value, opts, subopt);
 		break;
 	case L_SUNIT:
-		cli->lsunit = getnum(value, opts, L_SUNIT);
+		cli->lsunit = getnum(value, opts, subopt);
 		break;
 	case L_NAME:
 	case L_DEV:
-		cli->xi->logname = getstr(value, opts, L_NAME);
+		cli->xi->logname = getstr(value, opts, subopt);
 		cli->loginternal = 0;
 		break;
 	case L_VERSION:
-		cli->sb_feat.log_version = getnum(value, opts, L_VERSION);
+		cli->sb_feat.log_version = getnum(value, opts, subopt);
 		break;
 	case L_SIZE:
-		cli->logsize = getstr(value, opts, L_SIZE);
+		cli->logsize = getstr(value, opts, subopt);
 		break;
 	case L_SECTSIZE:
-		cli->lsectorsize = getnum(value, opts, L_SECTSIZE);
+		cli->lsectorsize = getnum(value, opts, subopt);
 		break;
 	case L_LAZYSBCNTR:
-		cli->sb_feat.lazy_sb_counters = getnum(value, opts, L_LAZYSBCNTR);
+		cli->sb_feat.lazy_sb_counters = getnum(value, opts, subopt);
 		break;
 	default:
 		return -EINVAL;
@@ -1561,24 +1561,24 @@ meta_opts_parser(
 {
 	switch (subopt) {
 	case M_CRC:
-		cli->sb_feat.crcs_enabled = getnum(value, opts, M_CRC);
+		cli->sb_feat.crcs_enabled = getnum(value, opts, subopt);
 		if (cli->sb_feat.crcs_enabled)
 			cli->sb_feat.dirftype = true;
 		break;
 	case M_FINOBT:
-		cli->sb_feat.finobt = getnum(value, opts, M_FINOBT);
+		cli->sb_feat.finobt = getnum(value, opts, subopt);
 		break;
 	case M_UUID:
 		if (!value || *value == '\0')
-			reqval('m', opts->subopts, M_UUID);
+			reqval('m', opts->subopts, subopt);
 		if (platform_uuid_parse(value, &cli->uuid))
 			illegal(value, "m uuid");
 		break;
 	case M_RMAPBT:
-		cli->sb_feat.rmapbt = getnum(value, opts, M_RMAPBT);
+		cli->sb_feat.rmapbt = getnum(value, opts, subopt);
 		break;
 	case M_REFLINK:
-		cli->sb_feat.reflink = getnum(value, opts, M_REFLINK);
+		cli->sb_feat.reflink = getnum(value, opts, subopt);
 		break;
 	default:
 		return -EINVAL;
@@ -1595,19 +1595,19 @@ naming_opts_parser(
 {
 	switch (subopt) {
 	case N_SIZE:
-		cli->dirblocksize = getstr(value, opts, N_SIZE);
+		cli->dirblocksize = getstr(value, opts, subopt);
 		break;
 	case N_VERSION:
-		value = getstr(value, &nopts, N_VERSION);
+		value = getstr(value, &nopts, subopt);
 		if (!strcasecmp(value, "ci")) {
 			/* ASCII CI mode */
 			cli->sb_feat.nci = true;
 		} else {
-			cli->sb_feat.dir_version = getnum(value, opts, N_VERSION);
+			cli->sb_feat.dir_version = getnum(value, opts, subopt);
 		}
 		break;
 	case N_FTYPE:
-		cli->sb_feat.dirftype = getnum(value, opts, N_FTYPE);
+		cli->sb_feat.dirftype = getnum(value, opts, subopt);
 		break;
 	default:
 		return -EINVAL;
@@ -1624,20 +1624,20 @@ rtdev_opts_parser(
 {
 	switch (subopt) {
 	case R_EXTSIZE:
-		cli->rtextsize = getstr(value, opts, R_EXTSIZE);
+		cli->rtextsize = getstr(value, opts, subopt);
 		break;
 	case R_FILE:
-		cli->xi->risfile = getnum(value, opts, R_FILE);
+		cli->xi->risfile = getnum(value, opts, subopt);
 		break;
 	case R_NAME:
 	case R_DEV:
-		cli->xi->rtname = getstr(value, opts, R_NAME);
+		cli->xi->rtname = getstr(value, opts, subopt);
 		break;
 	case R_SIZE:
-		cli->rtsize = getstr(value, opts, R_SIZE);
+		cli->rtsize = getstr(value, opts, subopt);
 		break;
 	case R_NOALIGN:
-		cli->sb_feat.nortalign = getnum(value, opts, R_NOALIGN);
+		cli->sb_feat.nortalign = getnum(value, opts, subopt);
 		break;
 	default:
 		return -EINVAL;

[PATCH 3/3] mkfs: do not allow both "dev" and "name" subopts for log or realtime

[Eric Sandeen]

Today we can specify i.e. both logdev= and name= ;
it works, with last-parsed-wins behavior:

mkfs.xfs -f -l logdev=/dev/sda1,name=/dev/sda2 /dev/sda3

Make these conflict to resolve ambiguity; do the same for
the realtime subvol.

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

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 035af03..5ae67d5 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -507,6 +507,7 @@ struct opt_params lopts = {
 		},
 		{ .index = L_DEV,
 		  .conflicts = { { &lopts, L_AGNUM },
+				 { &lopts, L_NAME },
 				 { &lopts, L_INTERNAL },
 				 { &lopts, LAST_CONFLICT } },
 		  .defaultval = SUBOPT_NEEDS_VAL,
@@ -529,6 +530,7 @@ struct opt_params lopts = {
 		},
 		{ .index = L_NAME,
 		  .conflicts = { { &lopts, L_AGNUM },
+				 { &lopts, L_DEV },
 				 { &lopts, L_INTERNAL },
 				 { &lopts, LAST_CONFLICT } },
 		  .defaultval = SUBOPT_NEEDS_VAL,
@@ -599,7 +601,8 @@ struct opt_params ropts = {
 		  .defaultval = SUBOPT_NEEDS_VAL,
 		},
 		{ .index = R_DEV,
-		  .conflicts = { { &ropts, LAST_CONFLICT } },
+		  .conflicts = { { &ropts, R_NAME },
+			         { &ropts, LAST_CONFLICT } },
 		  .defaultval = SUBOPT_NEEDS_VAL,
 		},
 		{ .index = R_FILE,
@@ -609,7 +612,8 @@ struct opt_params ropts = {
 		  .conflicts = { { &ropts, LAST_CONFLICT } },
 		},
 		{ .index = R_NAME,
-		  .conflicts = { { &ropts, LAST_CONFLICT } },
+		  .conflicts = { { &ropts, R_DEV },
+			         { &ropts, LAST_CONFLICT } },
 		  .defaultval = SUBOPT_NEEDS_VAL,
 		},
 		{ .index = R_NOALIGN,

Re: [PATCH 1/3] mkfs: un-document removed logarithm based CLI options

[Darrick J. Wong]

On Sun, Dec 24, 2017 at 11:09:01AM -0800, Eric Sandeen wrote:
> Remove logarithm-based options from usage() and manpage.
> 
> Fixes: 70f72d5 "mkfs: remove logarithm based CLI options"
> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>

Looks ok, though /me hates reading manpage sources: :)
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
> index bbbe1c5..4b8c78c 100644
> --- a/man/man8/mkfs.xfs.8
> +++ b/man/man8/mkfs.xfs.8
> @@ -5,7 +5,7 @@ mkfs.xfs \- construct an XFS filesystem
>  .B mkfs.xfs
>  [
>  .B \-b
> -.I block_size
> +.I block_size_options
>  ] [
>  .B \-m
>  .I global_metadata_options
> @@ -33,7 +33,7 @@ mkfs.xfs \- construct an XFS filesystem
>  .I realtime_section_options
>  ] [
>  .B \-s
> -.I sector_size
> +.I sector_size_options
>  ] [
>  .B \-L
>  .I label
> @@ -126,17 +126,14 @@ disable or enable the functionality.
>  .BI \-b " block_size_options"
>  This option specifies the fundamental block size of the filesystem.
>  The valid
> -.I block_size_options
> -are:
> -.BI log= value
> -or
> +.I block_size_option
> +is:
> +.RS 1.2i
> +.TP
>  .BI size= value
> -and only one can be supplied.
> -The block size is specified either as a base two logarithm value with
> -.BR log= ,
> -or in bytes with
> -.BR size= .
> -The default value is 4096 bytes (4 KiB), the minimum is 512, and the
> +The filesystem block size is specified with a
> +.I value
> +in bytes. 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
> @@ -147,6 +144,7 @@ Although
>  .B mkfs.xfs
>  will accept any of these values and create a valid filesystem,
>  XFS on Linux can only mount filesystems with pagesize or smaller blocks.
> +.RE
>  .TP
>  .BI \-m " global_metadata_options"
>  These options specify metadata format options that either apply to the entire
> @@ -419,15 +417,11 @@ The valid
>  are:
>  .RS 1.2i
>  .TP
> -.BI size= value " | log=" value " | perblock=" value
> +.BI size= value " | perblock=" value
>  The inode size is specified either as a
>  .I value
>  in bytes with
> -.BR size= ,
> -a base two logarithm
> -.I value
> -with
> -.BR log= ,
> +.BR size=
>  or as the number fitting in a filesystem block with
>  .BR perblock= .
>  The minimum (and default)
> @@ -648,15 +642,10 @@ These options specify the version and size parameters for the naming
>  are:
>  .RS 1.2i
>  .TP
> -.BI size= value " | log=" value
> -The block size is specified either as a
> -.I value
> -in bytes with
> -.BR size= ,
> -or as a base two logarithm
> +.BI size= value
> +The directory block size is specified with a
>  .I value
> -.RB "with " log= .
> -The block size must be a power of 2 and cannot be less than the
> +in bytes.  The block size must be a power of 2 and cannot be less than the
>  filesystem block size.
>  The default size
>  .I value
> @@ -888,15 +877,17 @@ This option disables stripe size detection, enforcing a realtime device with no
>  stripe geometry.
>  .RE
>  .TP
> -.BI \-s " sector_size"
> +.BI \-s " sector_size_options"
>  This option specifies the fundamental sector size of the filesystem.
> -The
> -.I sector_size
> -is specified either as a value in bytes with
> +The valid
> +.I sector_size_option
> +is:
> +.RS 1.2i
> +.TP
>  .BI size= value
> -or as a base two logarithm value with
> -.BI log= value.
> -The default
> +The sector size is specified with a
> +.I value
> +in bytes.  The default
>  .I sector_size
>  is 512 bytes. The minimum value for sector size is
>  512; the maximum is 32768 (32 KiB). The
> @@ -907,6 +898,7 @@ filesystem block size.
>  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.
> +.RE
>  .TP
>  .BI \-L " label"
>  Set the filesystem
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index f3d57cf..1f3494c 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -860,25 +860,24 @@ static void __attribute__((noreturn))
>  usage( void )
>  {
>  	fprintf(stderr, _("Usage: %s\n\
> -/* blocksize */		[-b log=n|size=num]\n\
> +/* blocksize */		[-b size=num]\n\
>  /* metadata */		[-m crc=0|1,finobt=0|1,uuid=xxx,rmapbt=0|1,reflink=0|1]\n\
>  /* data subvol */	[-d agcount=n,agsize=n,file,name=xxx,size=num,\n\
>  			    (sunit=value,swidth=value|su=num,sw=num|noalign),\n\
> -			    sectlog=n|sectsize=num\n\
> +			    sectsize=num\n\
>  /* force overwrite */	[-f]\n\
>  /* inode size */	[-i log=n|perblock=n|size=num,maxpct=n,attr=0|1|2,\n\
>  			    projid32bit=0|1,sparse=0|1]\n\
>  /* no discard */	[-K]\n\
>  /* log subvol */	[-l agnum=n,internal,size=num,logdev=xxx,version=n\n\
> -			    sunit=value|su=num,sectlog=n|sectsize=num,\n\
> -			    lazy-count=0|1]\n\
> +			    sunit=value|su=num,sectsize=num,lazy-count=0|1]\n\
>  /* label */		[-L label (maximum 12 characters)]\n\
> -/* naming */		[-n log=n|size=num,version=2|ci,ftype=0|1]\n\
> +/* naming */		[-n size=num,version=2|ci,ftype=0|1]\n\
>  /* no-op info only */	[-N]\n\
>  /* prototype file */	[-p fname]\n\
>  /* quiet */		[-q]\n\
>  /* realtime subvol */	[-r extsize=num,size=num,rtdev=xxx]\n\
> -/* sectorsize */	[-s log=n|size=num]\n\
> +/* sectorsize */	[-s size=num]\n\
>  /* version */		[-V]\n\
>  			devicename\n\
>  <devicename> is required unless -d name=xxx is given.\n\
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] mkfs: pass switch case value directly into getnum/getstr

[Darrick J. Wong]

On Sun, Dec 24, 2017 at 11:10:21AM -0800, Eric Sandeen wrote:
> Parsing did does sort of thing:

'Parsing does this sort of thing:'?

> 
> 	case D_AGCOUNT:
> 		cli->agcount = getnum(value, opts, D_AGCOUNT);
> 
> which is just begging for a cut and paste error between the
> case value and the enum passed into getnum/getstr.  Pass
> "subopt" instead so that it is always consistent with the case.
> 
> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>

Otherwise looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 1f3494c..035af03 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -1400,7 +1400,7 @@ block_opts_parser(
>  {
>  	switch (subopt) {
>  	case B_SIZE:
> -		cli->blocksize = getnum(value, opts, B_SIZE);
> +		cli->blocksize = getnum(value, opts, subopt);
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -1417,52 +1417,52 @@ data_opts_parser(
>  {
>  	switch (subopt) {
>  	case D_AGCOUNT:
> -		cli->agcount = getnum(value, opts, D_AGCOUNT);
> +		cli->agcount = getnum(value, opts, subopt);
>  		break;
>  	case D_AGSIZE:
> -		cli->agsize = getstr(value, opts, D_AGSIZE);
> +		cli->agsize = getstr(value, opts, subopt);
>  		break;
>  	case D_FILE:
> -		cli->xi->disfile = getnum(value, opts, D_FILE);
> +		cli->xi->disfile = getnum(value, opts, subopt);
>  		break;
>  	case D_NAME:
> -		cli->xi->dname = getstr(value, opts, D_NAME);
> +		cli->xi->dname = getstr(value, opts, subopt);
>  		break;
>  	case D_SIZE:
> -		cli->dsize = getstr(value, opts, D_SIZE);
> +		cli->dsize = getstr(value, opts, subopt);
>  		break;
>  	case D_SUNIT:
> -		cli->dsunit = getnum(value, opts, D_SUNIT);
> +		cli->dsunit = getnum(value, opts, subopt);
>  		break;
>  	case D_SWIDTH:
> -		cli->dswidth = getnum(value, opts, D_SWIDTH);
> +		cli->dswidth = getnum(value, opts, subopt);
>  		break;
>  	case D_SU:
> -		cli->dsu = getstr(value, opts, D_SU);
> +		cli->dsu = getstr(value, opts, subopt);
>  		break;
>  	case D_SW:
> -		cli->dsw = getnum(value, opts, D_SW);
> +		cli->dsw = getnum(value, opts, subopt);
>  		break;
>  	case D_NOALIGN:
> -		cli->sb_feat.nodalign = getnum(value, opts, D_NOALIGN);
> +		cli->sb_feat.nodalign = getnum(value, opts, subopt);
>  		break;
>  	case D_SECTSIZE:
> -		cli->sectorsize = getnum(value, opts, D_SECTSIZE);
> +		cli->sectorsize = getnum(value, opts, subopt);
>  		break;
>  	case D_RTINHERIT:
> -		if (getnum(value, opts, D_RTINHERIT))
> +		if (getnum(value, opts, subopt))
>  			cli->fsx.fsx_xflags |= XFS_DIFLAG_RTINHERIT;
>  		break;
>  	case D_PROJINHERIT:
> -		cli->fsx.fsx_projid = getnum(value, opts, D_PROJINHERIT);
> +		cli->fsx.fsx_projid = getnum(value, opts, subopt);
>  		cli->fsx.fsx_xflags |= XFS_DIFLAG_PROJINHERIT;
>  		break;
>  	case D_EXTSZINHERIT:
> -		cli->fsx.fsx_extsize = getnum(value, opts, D_EXTSZINHERIT);
> +		cli->fsx.fsx_extsize = getnum(value, opts, subopt);
>  		cli->fsx.fsx_xflags |= XFS_DIFLAG_EXTSZINHERIT;
>  		break;
>  	case D_COWEXTSIZE:
> -		cli->fsx.fsx_cowextsize = getnum(value, opts, D_COWEXTSIZE);
> +		cli->fsx.fsx_cowextsize = getnum(value, opts, subopt);
>  		cli->fsx.fsx_xflags |= FS_XFLAG_COWEXTSIZE;
>  		break;
>  	default:
> @@ -1480,25 +1480,25 @@ inode_opts_parser(
>  {
>  	switch (subopt) {
>  	case I_ALIGN:
> -		cli->sb_feat.inode_align = getnum(value, opts, I_ALIGN);
> +		cli->sb_feat.inode_align = getnum(value, opts, subopt);
>  		break;
>  	case I_MAXPCT:
> -		cli->imaxpct = getnum(value, opts, I_MAXPCT);
> +		cli->imaxpct = getnum(value, opts, subopt);
>  		break;
>  	case I_PERBLOCK:
> -		cli->inopblock = getnum(value, opts, I_PERBLOCK);
> +		cli->inopblock = getnum(value, opts, subopt);
>  		break;
>  	case I_SIZE:
> -		cli->inodesize = getnum(value, opts, I_SIZE);
> +		cli->inodesize = getnum(value, opts, subopt);
>  		break;
>  	case I_ATTR:
> -		cli->sb_feat.attr_version = getnum(value, opts, I_ATTR);
> +		cli->sb_feat.attr_version = getnum(value, opts, subopt);
>  		break;
>  	case I_PROJID32BIT:
> -		cli->sb_feat.projid32bit = getnum(value, opts, I_PROJID32BIT);
> +		cli->sb_feat.projid32bit = getnum(value, opts, subopt);
>  		break;
>  	case I_SPINODES:
> -		cli->sb_feat.spinodes = getnum(value, opts, I_SPINODES);
> +		cli->sb_feat.spinodes = getnum(value, opts, subopt);
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -1515,36 +1515,36 @@ log_opts_parser(
>  {
>  	switch (subopt) {
>  	case L_AGNUM:
> -		cli->logagno = getnum(value, opts, L_AGNUM);
> +		cli->logagno = getnum(value, opts, subopt);
>  		break;
>  	case L_FILE:
> -		cli->xi->lisfile = getnum(value, opts, L_FILE);
> +		cli->xi->lisfile = getnum(value, opts, subopt);
>  		break;
>  	case L_INTERNAL:
> -		cli->loginternal = getnum(value, opts, L_INTERNAL);
> +		cli->loginternal = getnum(value, opts, subopt);
>  		break;
>  	case L_SU:
> -		cli->lsu = getstr(value, opts, L_SU);
> +		cli->lsu = getstr(value, opts, subopt);
>  		break;
>  	case L_SUNIT:
> -		cli->lsunit = getnum(value, opts, L_SUNIT);
> +		cli->lsunit = getnum(value, opts, subopt);
>  		break;
>  	case L_NAME:
>  	case L_DEV:
> -		cli->xi->logname = getstr(value, opts, L_NAME);
> +		cli->xi->logname = getstr(value, opts, subopt);
>  		cli->loginternal = 0;
>  		break;
>  	case L_VERSION:
> -		cli->sb_feat.log_version = getnum(value, opts, L_VERSION);
> +		cli->sb_feat.log_version = getnum(value, opts, subopt);
>  		break;
>  	case L_SIZE:
> -		cli->logsize = getstr(value, opts, L_SIZE);
> +		cli->logsize = getstr(value, opts, subopt);
>  		break;
>  	case L_SECTSIZE:
> -		cli->lsectorsize = getnum(value, opts, L_SECTSIZE);
> +		cli->lsectorsize = getnum(value, opts, subopt);
>  		break;
>  	case L_LAZYSBCNTR:
> -		cli->sb_feat.lazy_sb_counters = getnum(value, opts, L_LAZYSBCNTR);
> +		cli->sb_feat.lazy_sb_counters = getnum(value, opts, subopt);
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -1561,24 +1561,24 @@ meta_opts_parser(
>  {
>  	switch (subopt) {
>  	case M_CRC:
> -		cli->sb_feat.crcs_enabled = getnum(value, opts, M_CRC);
> +		cli->sb_feat.crcs_enabled = getnum(value, opts, subopt);
>  		if (cli->sb_feat.crcs_enabled)
>  			cli->sb_feat.dirftype = true;
>  		break;
>  	case M_FINOBT:
> -		cli->sb_feat.finobt = getnum(value, opts, M_FINOBT);
> +		cli->sb_feat.finobt = getnum(value, opts, subopt);
>  		break;
>  	case M_UUID:
>  		if (!value || *value == '\0')
> -			reqval('m', opts->subopts, M_UUID);
> +			reqval('m', opts->subopts, subopt);
>  		if (platform_uuid_parse(value, &cli->uuid))
>  			illegal(value, "m uuid");
>  		break;
>  	case M_RMAPBT:
> -		cli->sb_feat.rmapbt = getnum(value, opts, M_RMAPBT);
> +		cli->sb_feat.rmapbt = getnum(value, opts, subopt);
>  		break;
>  	case M_REFLINK:
> -		cli->sb_feat.reflink = getnum(value, opts, M_REFLINK);
> +		cli->sb_feat.reflink = getnum(value, opts, subopt);
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -1595,19 +1595,19 @@ naming_opts_parser(
>  {
>  	switch (subopt) {
>  	case N_SIZE:
> -		cli->dirblocksize = getstr(value, opts, N_SIZE);
> +		cli->dirblocksize = getstr(value, opts, subopt);
>  		break;
>  	case N_VERSION:
> -		value = getstr(value, &nopts, N_VERSION);
> +		value = getstr(value, &nopts, subopt);
>  		if (!strcasecmp(value, "ci")) {
>  			/* ASCII CI mode */
>  			cli->sb_feat.nci = true;
>  		} else {
> -			cli->sb_feat.dir_version = getnum(value, opts, N_VERSION);
> +			cli->sb_feat.dir_version = getnum(value, opts, subopt);
>  		}
>  		break;
>  	case N_FTYPE:
> -		cli->sb_feat.dirftype = getnum(value, opts, N_FTYPE);
> +		cli->sb_feat.dirftype = getnum(value, opts, subopt);
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -1624,20 +1624,20 @@ rtdev_opts_parser(
>  {
>  	switch (subopt) {
>  	case R_EXTSIZE:
> -		cli->rtextsize = getstr(value, opts, R_EXTSIZE);
> +		cli->rtextsize = getstr(value, opts, subopt);
>  		break;
>  	case R_FILE:
> -		cli->xi->risfile = getnum(value, opts, R_FILE);
> +		cli->xi->risfile = getnum(value, opts, subopt);
>  		break;
>  	case R_NAME:
>  	case R_DEV:
> -		cli->xi->rtname = getstr(value, opts, R_NAME);
> +		cli->xi->rtname = getstr(value, opts, subopt);
>  		break;
>  	case R_SIZE:
> -		cli->rtsize = getstr(value, opts, R_SIZE);
> +		cli->rtsize = getstr(value, opts, subopt);
>  		break;
>  	case R_NOALIGN:
> -		cli->sb_feat.nortalign = getnum(value, opts, R_NOALIGN);
> +		cli->sb_feat.nortalign = getnum(value, opts, subopt);
>  		break;
>  	default:
>  		return -EINVAL;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] mkfs: do not allow both "dev" and "name" subopts for log or realtime

[Darrick J. Wong]

On Sun, Dec 24, 2017 at 11:12:13AM -0800, Eric Sandeen wrote:
> Today we can specify i.e. both logdev= and name= ;
> it works, with last-parsed-wins behavior:
> 
> mkfs.xfs -f -l logdev=/dev/sda1,name=/dev/sda2 /dev/sda3
> 
> Make these conflict to resolve ambiguity; do the same for
> the realtime subvol.
> 
> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 035af03..5ae67d5 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -507,6 +507,7 @@ struct opt_params lopts = {
>  		},
>  		{ .index = L_DEV,
>  		  .conflicts = { { &lopts, L_AGNUM },
> +				 { &lopts, L_NAME },
>  				 { &lopts, L_INTERNAL },
>  				 { &lopts, LAST_CONFLICT } },
>  		  .defaultval = SUBOPT_NEEDS_VAL,
> @@ -529,6 +530,7 @@ struct opt_params lopts = {
>  		},
>  		{ .index = L_NAME,
>  		  .conflicts = { { &lopts, L_AGNUM },
> +				 { &lopts, L_DEV },
>  				 { &lopts, L_INTERNAL },
>  				 { &lopts, LAST_CONFLICT } },
>  		  .defaultval = SUBOPT_NEEDS_VAL,
> @@ -599,7 +601,8 @@ struct opt_params ropts = {
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = R_DEV,
> -		  .conflicts = { { &ropts, LAST_CONFLICT } },
> +		  .conflicts = { { &ropts, R_NAME },
> +			         { &ropts, LAST_CONFLICT } },
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = R_FILE,
> @@ -609,7 +612,8 @@ struct opt_params ropts = {
>  		  .conflicts = { { &ropts, LAST_CONFLICT } },
>  		},
>  		{ .index = R_NAME,
> -		  .conflicts = { { &ropts, LAST_CONFLICT } },
> +		  .conflicts = { { &ropts, R_DEV },
> +			         { &ropts, LAST_CONFLICT } },
>  		  .defaultval = SUBOPT_NEEDS_VAL,
>  		},
>  		{ .index = R_NOALIGN,
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html