* [PATCH 0/7] mkfs: save user input into opts table
@ 2017-07-20 9:29 Jan Tulak
2017-07-20 9:29 ` [PATCH 1/7] mkfs: Save raw user input field to the opts struct Jan Tulak
` (6 more replies)
0 siblings, 7 replies; 39+ messages in thread
From: Jan Tulak @ 2017-07-20 9:29 UTC (permalink / raw)
To: linux-xfs; +Cc: Jan Tulak
Hi guys
This is a respin of https://www.spinics.net/lists/linux-xfs/msg06158.html, but
I decided to split that set in two smaller ones. This first one implements the
infrastructure, but does not replace standalone variables with the opts table
fields, that happens in the other set.
To reiterate: The main goal of this set is to save user input into the opts
struct, instead of various ad-hoc variables. I'm adding the set and get
functions to do this and applying them where appropriate. Once all the values
are in a single place, we can expand the automated validation and use it e.g.
for loading up optional config file like Luis wants to do.
This set is now is based on for-next and not on my type-chaning set (which
will come some time later). Then there are some fixes in specific commits --
all issues you raised should be addressed.
You can see the tree for both sets at:
https://github.com/jtulak/xfsprogs-dev/tree/setters
Cheers,
Jan
Jan Tulak (7):
mkfs: Save raw user input field to the opts struct
mkfs: rename defaultval to flagval in opts
mkfs: remove intermediate getstr followed by getnum
mkfs: merge tables for opts parsing into one table
mkfs: move getnum within the file
mkfs: extend opt_params with a value field
mkfs: save user input values into opts
mkfs/xfs_mkfs.c | 1773 ++++++++++++++++++++++++++++++-------------------------
1 file changed, 970 insertions(+), 803 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 1/7] mkfs: Save raw user input field to the opts struct
2017-07-20 9:29 [PATCH 0/7] mkfs: save user input into opts table Jan Tulak
@ 2017-07-20 9:29 ` Jan Tulak
2017-07-27 16:27 ` Luis R. Rodriguez
2017-07-20 9:29 ` [PATCH 2/7] mkfs: rename defaultval to flagval in opts Jan Tulak
` (5 subsequent siblings)
6 siblings, 1 reply; 39+ messages in thread
From: Jan Tulak @ 2017-07-20 9:29 UTC (permalink / raw)
To: linux-xfs; +Cc: Jan Tulak
Save exactly what the user gave us for every option. This way, we will
never lose the information if we need it to print back an issue.
(Just add the infrastructure now, used in the next patches.)
Signed-off-by: Jan Tulak <jtulak@redhat.com>
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
mkfs/xfs_mkfs.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index a69190b9..4b030101 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -107,6 +107,11 @@ unsigned int sectorsize;
* sets what is used with simple specifying the subopt (-d file).
* A special SUBOPT_NEEDS_VAL can be used to require a user-given
* value in any case.
+ *
+ * raw_input INTERNAL
+ * Filled raw string from the user, so we never lose that information e.g.
+ * to print it back in case of an issue.
+ *
*/
struct opt_params {
const char name;
@@ -122,6 +127,7 @@ struct opt_params {
long long minval;
long long maxval;
long long defaultval;
+ const char *raw_input;
} subopt_params[MAX_SUBOPTS];
};
@@ -729,6 +735,18 @@ struct opt_params mopts = {
*/
#define WHACK_SIZE (128 * 1024)
+static inline void
+set_conf_raw(struct opt_params *opt, int subopt, const char *value)
+{
+ opt->subopt_params[subopt].raw_input = value;
+}
+
+static inline const char *
+get_conf_raw(const struct opt_params *opt, int subopt)
+{
+ return opt->subopt_params[subopt].raw_input;
+}
+
/*
* Convert lsu to lsunit for 512 bytes blocks and check validity of the values.
*/
--
2.11.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 2/7] mkfs: rename defaultval to flagval in opts
2017-07-20 9:29 [PATCH 0/7] mkfs: save user input into opts table Jan Tulak
2017-07-20 9:29 ` [PATCH 1/7] mkfs: Save raw user input field to the opts struct Jan Tulak
@ 2017-07-20 9:29 ` Jan Tulak
2017-07-20 9:29 ` [PATCH 3/7] mkfs: remove intermediate getstr followed by getnum Jan Tulak
` (4 subsequent siblings)
6 siblings, 0 replies; 39+ messages in thread
From: Jan Tulak @ 2017-07-20 9:29 UTC (permalink / raw)
To: linux-xfs; +Cc: Jan Tulak
The old name 'defaultval' was misleading - it is not the default value,
but the value the option has when used as a flag by an user.
Signed-off-by: Jan Tulak <jtulak@redhat.com>
Reviewed-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
mkfs/xfs_mkfs.c | 120 ++++++++++++++++++++++++++++----------------------------
1 file changed, 60 insertions(+), 60 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 4b030101..f2f6468e 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -101,7 +101,7 @@ unsigned int sectorsize;
* to zero. But if one value is different: minval=0 and maxval=1,
* then it is OK.)
*
- * defaultval MANDATORY
+ * flagval MANDATORY
* The value used if user specifies the subopt, but no value.
* If the subopt accepts some values (-d file=[1|0]), then this
* sets what is used with simple specifying the subopt (-d file).
@@ -126,7 +126,7 @@ struct opt_params {
int conflicts[MAX_CONFLICTS];
long long minval;
long long maxval;
- long long defaultval;
+ long long flagval;
const char *raw_input;
} subopt_params[MAX_SUBOPTS];
};
@@ -146,7 +146,7 @@ struct opt_params bopts = {
LAST_CONFLICT },
.minval = XFS_MIN_BLOCKSIZE_LOG,
.maxval = XFS_MAX_BLOCKSIZE_LOG,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = B_SIZE,
.convert = true,
@@ -155,7 +155,7 @@ struct opt_params bopts = {
LAST_CONFLICT },
.minval = XFS_MIN_BLOCKSIZE,
.maxval = XFS_MAX_BLOCKSIZE,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
},
};
@@ -201,24 +201,24 @@ struct opt_params dopts = {
LAST_CONFLICT },
.minval = 1,
.maxval = XFS_MAX_AGNUMBER,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = D_FILE,
.conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 1,
- .defaultval = 1,
+ .flagval = 1,
},
{ .index = D_NAME,
.conflicts = { LAST_CONFLICT },
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SIZE,
.conflicts = { LAST_CONFLICT },
.convert = true,
.minval = XFS_AG_MIN_BYTES,
.maxval = LLONG_MAX,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SUNIT,
.conflicts = { D_NOALIGN,
@@ -227,7 +227,7 @@ struct opt_params dopts = {
LAST_CONFLICT },
.minval = 0,
.maxval = UINT_MAX,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SWIDTH,
.conflicts = { D_NOALIGN,
@@ -236,7 +236,7 @@ struct opt_params dopts = {
LAST_CONFLICT },
.minval = 0,
.maxval = UINT_MAX,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = D_AGSIZE,
.conflicts = { D_AGCOUNT,
@@ -244,7 +244,7 @@ struct opt_params dopts = {
.convert = true,
.minval = XFS_AG_MIN_BYTES,
.maxval = XFS_AG_MAX_BYTES,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SU,
.conflicts = { D_NOALIGN,
@@ -254,7 +254,7 @@ struct opt_params dopts = {
.convert = true,
.minval = 0,
.maxval = UINT_MAX,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SW,
.conflicts = { D_NOALIGN,
@@ -263,14 +263,14 @@ struct opt_params dopts = {
LAST_CONFLICT },
.minval = 0,
.maxval = UINT_MAX,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = 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,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = D_SECTSIZE,
.conflicts = { D_SECTLOG,
@@ -279,7 +279,7 @@ struct opt_params dopts = {
.is_power_2 = true,
.minval = XFS_MIN_SECTORSIZE,
.maxval = XFS_MAX_SECTORSIZE,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = D_NOALIGN,
.conflicts = { D_SU,
@@ -289,25 +289,25 @@ struct opt_params dopts = {
LAST_CONFLICT },
.minval = 0,
.maxval = 1,
- .defaultval = 1,
+ .flagval = 1,
},
{ .index = D_RTINHERIT,
.conflicts = { LAST_CONFLICT },
.minval = 1,
.maxval = 1,
- .defaultval = 1,
+ .flagval = 1,
},
{ .index = D_PROJINHERIT,
.conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = UINT_MAX,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = D_EXTSZINHERIT,
.conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = UINT_MAX,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
},
};
@@ -339,7 +339,7 @@ struct opt_params iopts = {
.conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 1,
- .defaultval = 1,
+ .flagval = 1,
},
{ .index = I_LOG,
.conflicts = { I_PERBLOCK,
@@ -347,13 +347,13 @@ struct opt_params iopts = {
LAST_CONFLICT },
.minval = XFS_DINODE_MIN_LOG,
.maxval = XFS_DINODE_MAX_LOG,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = I_MAXPCT,
.conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 100,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = I_PERBLOCK,
.conflicts = { I_LOG,
@@ -362,7 +362,7 @@ struct opt_params iopts = {
.is_power_2 = true,
.minval = XFS_MIN_INODE_PERBLOCK,
.maxval = XFS_MAX_BLOCKSIZE / XFS_DINODE_MIN_SIZE,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = I_SIZE,
.conflicts = { I_PERBLOCK,
@@ -371,25 +371,25 @@ struct opt_params iopts = {
.is_power_2 = true,
.minval = XFS_DINODE_MIN_SIZE,
.maxval = XFS_DINODE_MAX_SIZE,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = I_ATTR,
.conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 2,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = I_PROJID32BIT,
.conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 1,
- .defaultval = 1,
+ .flagval = 1,
},
{ .index = I_SPINODES,
.conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 1,
- .defaultval = 1,
+ .flagval = 1,
},
},
};
@@ -429,7 +429,7 @@ struct opt_params lopts = {
LAST_CONFLICT },
.minval = 0,
.maxval = UINT_MAX,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = L_INTERNAL,
.conflicts = { L_FILE,
@@ -437,27 +437,27 @@ struct opt_params lopts = {
LAST_CONFLICT },
.minval = 0,
.maxval = 1,
- .defaultval = 1,
+ .flagval = 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,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = L_VERSION,
.conflicts = { LAST_CONFLICT },
.minval = 1,
.maxval = 2,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = L_SUNIT,
.conflicts = { L_SU,
LAST_CONFLICT },
.minval = 1,
.maxval = BTOBB(XLOG_MAX_RECORD_BSIZE),
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = L_SU,
.conflicts = { L_SUNIT,
@@ -465,20 +465,20 @@ struct opt_params lopts = {
.convert = true,
.minval = BBTOB(1),
.maxval = XLOG_MAX_RECORD_BSIZE,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = L_DEV,
.conflicts = { L_AGNUM,
L_INTERNAL,
LAST_CONFLICT },
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = 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,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = L_SECTSIZE,
.conflicts = { L_SECTLOG,
@@ -487,26 +487,26 @@ struct opt_params lopts = {
.is_power_2 = true,
.minval = XFS_MIN_SECTORSIZE,
.maxval = XFS_MAX_SECTORSIZE,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = L_FILE,
.conflicts = { L_INTERNAL,
LAST_CONFLICT },
.minval = 0,
.maxval = 1,
- .defaultval = 1,
+ .flagval = 1,
},
{ .index = L_NAME,
.conflicts = { L_AGNUM,
L_INTERNAL,
LAST_CONFLICT },
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = L_LAZYSBCNTR,
.conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 1,
- .defaultval = 1,
+ .flagval = 1,
},
},
};
@@ -530,7 +530,7 @@ struct opt_params nopts = {
LAST_CONFLICT },
.minval = XFS_MIN_REC_DIRSIZE,
.maxval = XFS_MAX_BLOCKSIZE_LOG,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = N_SIZE,
.conflicts = { N_LOG,
@@ -539,19 +539,19 @@ struct opt_params nopts = {
.is_power_2 = true,
.minval = 1 << XFS_MIN_REC_DIRSIZE,
.maxval = XFS_MAX_BLOCKSIZE,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = N_VERSION,
.conflicts = { LAST_CONFLICT },
.minval = 2,
.maxval = 2,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = N_FTYPE,
.conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 1,
- .defaultval = 1,
+ .flagval = 1,
},
},
};
@@ -579,33 +579,33 @@ struct opt_params ropts = {
.convert = true,
.minval = XFS_MIN_RTEXTSIZE,
.maxval = XFS_MAX_RTEXTSIZE,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = R_SIZE,
.conflicts = { LAST_CONFLICT },
.convert = true,
.minval = 0,
.maxval = LLONG_MAX,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = R_DEV,
.conflicts = { LAST_CONFLICT },
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = R_FILE,
.minval = 0,
.maxval = 1,
- .defaultval = 1,
+ .flagval = 1,
.conflicts = { LAST_CONFLICT },
},
{ .index = R_NAME,
.conflicts = { LAST_CONFLICT },
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = R_NOALIGN,
.minval = 0,
.maxval = 1,
- .defaultval = 1,
+ .flagval = 1,
.conflicts = { LAST_CONFLICT },
},
},
@@ -631,7 +631,7 @@ struct opt_params sopts = {
LAST_CONFLICT },
.minval = XFS_MIN_SECTORSIZE_LOG,
.maxval = XFS_MAX_SECTORSIZE_LOG,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = S_SECTLOG,
.conflicts = { S_SIZE,
@@ -639,7 +639,7 @@ struct opt_params sopts = {
LAST_CONFLICT },
.minval = XFS_MIN_SECTORSIZE_LOG,
.maxval = XFS_MAX_SECTORSIZE_LOG,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = S_SIZE,
.conflicts = { S_LOG,
@@ -649,7 +649,7 @@ struct opt_params sopts = {
.is_power_2 = true,
.minval = XFS_MIN_SECTORSIZE,
.maxval = XFS_MAX_SECTORSIZE,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = S_SECTSIZE,
.conflicts = { S_LOG,
@@ -659,7 +659,7 @@ struct opt_params sopts = {
.is_power_2 = true,
.minval = XFS_MIN_SECTORSIZE,
.maxval = XFS_MAX_SECTORSIZE,
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
},
};
@@ -684,29 +684,29 @@ struct opt_params mopts = {
.conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 1,
- .defaultval = 1,
+ .flagval = 1,
},
{ .index = M_FINOBT,
.conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 1,
- .defaultval = 1,
+ .flagval = 1,
},
{ .index = M_UUID,
.conflicts = { LAST_CONFLICT },
- .defaultval = SUBOPT_NEEDS_VAL,
+ .flagval = SUBOPT_NEEDS_VAL,
},
{ .index = M_RMAPBT,
.conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 1,
- .defaultval = 1,
+ .flagval = 1,
},
{ .index = M_REFLINK,
.conflicts = { LAST_CONFLICT },
.minval = 0,
.maxval = 1,
- .defaultval = 1,
+ .flagval = 1,
},
},
};
@@ -1347,9 +1347,9 @@ getnum(
check_opt(opts, index, false);
/* empty strings might just return a default value */
if (!str || *str == '\0') {
- if (sp->defaultval == SUBOPT_NEEDS_VAL)
+ if (sp->flagval == SUBOPT_NEEDS_VAL)
reqval(opts->name, (char **)opts->subopts, index);
- return sp->defaultval;
+ return sp->flagval;
}
if (sp->minval == 0 && sp->maxval == 0) {
--
2.11.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 3/7] mkfs: remove intermediate getstr followed by getnum
2017-07-20 9:29 [PATCH 0/7] mkfs: save user input into opts table Jan Tulak
2017-07-20 9:29 ` [PATCH 1/7] mkfs: Save raw user input field to the opts struct Jan Tulak
2017-07-20 9:29 ` [PATCH 2/7] mkfs: rename defaultval to flagval in opts Jan Tulak
@ 2017-07-20 9:29 ` Jan Tulak
2017-07-20 15:54 ` Darrick J. Wong
2017-07-21 12:24 ` [PATCH 3/7 v2] " Jan Tulak
2017-07-20 9:29 ` [PATCH 4/7] mkfs: merge tables for opts parsing into one table Jan Tulak
` (3 subsequent siblings)
6 siblings, 2 replies; 39+ messages in thread
From: Jan Tulak @ 2017-07-20 9:29 UTC (permalink / raw)
To: linux-xfs; +Cc: Jan Tulak
Some options loaded a number as a string with getstr and converted it to
number with getnum later in the code, without any reason for this
approach. (They were probably forgotten in some past cleaning.)
This patch changes them to skip the string and use getnum directly in
the main option-parsing loop, as do all the other numerical options.
And as we now have two variables of the same type for the same value,
merge them together. (e.g. former string dsize and number dbytes).
Signed-off-by: Jan Tulak <jtulak@redhat.com>
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
In reply to Eric's comment, so it doesn't pop up again:
This patch has to be applied after "mkfs: Save raw user input ...",
because otherwise we would temporary lose the access to strings
with user-given values and thus wouldn't be able to report them in
case of any issue.
---
mkfs/xfs_mkfs.c | 90 ++++++++++++++++++++++++++-------------------------------
1 file changed, 41 insertions(+), 49 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index f2f6468e..127f14c3 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1345,6 +1345,7 @@ getnum(
long long c;
check_opt(opts, index, false);
+ set_conf_raw(opts, index, str);
/* empty strings might just return a default value */
if (!str || *str == '\0') {
if (sp->flagval == SUBOPT_NEEDS_VAL)
@@ -1432,12 +1433,12 @@ main(
char *dfile;
int dirblocklog;
int dirblocksize;
- char *dsize;
+ int dbytes;
int dsu;
int dsw;
int dsunit;
int dswidth;
- int force_overwrite;
+ bool force_overwrite;
struct fsxattr fsx;
int ilflag;
int imaxpct;
@@ -1456,7 +1457,7 @@ main(
xfs_rfsblock_t logblocks;
char *logfile;
int loginternal;
- char *logsize;
+ int logbytes;
xfs_fsblock_t logstart;
int lvflag;
int lsflag;
@@ -1485,11 +1486,11 @@ main(
char *protostring;
int qflag;
xfs_rfsblock_t rtblocks;
+ int rtbytes;
xfs_extlen_t rtextblocks;
xfs_rtblock_t rtextents;
- char *rtextsize;
+ int rtextbytes;
char *rtfile;
- char *rtsize;
xfs_sb_t *sbp;
int sectorlog;
__uint64_t sector_mask;
@@ -1537,7 +1538,8 @@ main(
qflag = 0;
imaxpct = inodelog = inopblock = isize = 0;
dfile = logfile = rtfile = NULL;
- dsize = logsize = rtsize = rtextsize = protofile = NULL;
+ protofile = NULL;
+ rtbytes = rtextbytes = logbytes = dbytes = 0;
dsu = dsw = dsunit = dswidth = lalign = lsu = lsunit = 0;
nodsflag = norsflag = 0;
force_overwrite = 0;
@@ -1601,7 +1603,7 @@ main(
xi.dname = getstr(value, &dopts, D_NAME);
break;
case D_SIZE:
- dsize = getstr(value, &dopts, D_SIZE);
+ dbytes = getnum(value, &dopts, D_SIZE);
break;
case D_SUNIT:
dsunit = getnum(value, &dopts, D_SUNIT);
@@ -1746,7 +1748,7 @@ main(
lvflag = 1;
break;
case L_SIZE:
- logsize = getstr(value, &lopts, L_SIZE);
+ logbytes = getnum(value, &lopts, L_SIZE);
break;
case L_SECTLOG:
lsectorlog = getnum(value, &lopts,
@@ -1875,8 +1877,7 @@ main(
switch (getsubopt(&p, subopts, &value)) {
case R_EXTSIZE:
- rtextsize = getstr(value, &ropts,
- R_EXTSIZE);
+ rtextbytes = getnum(value, &ropts, R_EXTSIZE);
break;
case R_FILE:
xi.risfile = getnum(value, &ropts,
@@ -1888,7 +1889,7 @@ main(
R_NAME);
break;
case R_SIZE:
- rtsize = getstr(value, &ropts, R_SIZE);
+ rtbytes = getnum(value, &ropts, R_SIZE);
break;
case R_NOALIGN:
norsflag = getnum(value, &ropts,
@@ -1991,14 +1992,14 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
* sector size mismatches between the new filesystem and the underlying
* host filesystem.
*/
- check_device_type(dfile, &xi.disfile, !dsize, !dfile,
+ check_device_type(dfile, &xi.disfile, !dbytes, !dfile,
Nflag ? NULL : &xi.dcreat, force_overwrite, "d");
if (!loginternal)
- check_device_type(xi.logname, &xi.lisfile, !logsize, !xi.logname,
- Nflag ? NULL : &xi.lcreat,
+ check_device_type(xi.logname, &xi.lisfile, !logbytes,
+ !xi.logname, Nflag ? NULL : &xi.lcreat,
force_overwrite, "l");
if (xi.rtname)
- check_device_type(xi.rtname, &xi.risfile, !rtsize, !xi.rtname,
+ check_device_type(xi.rtname, &xi.risfile, !rtbytes, !xi.rtname,
Nflag ? NULL : &xi.rcreat,
force_overwrite, "r");
if (xi.disfile || xi.lisfile || xi.risfile)
@@ -2179,10 +2180,7 @@ _("rmapbt not supported with realtime devices\n"));
}
- if (dsize) {
- __uint64_t dbytes;
-
- dbytes = getnum(dsize, &dopts, D_SIZE);
+ if (dbytes) {
if (dbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
_("illegal data length %lld, not a multiple of %d\n"),
@@ -2211,10 +2209,7 @@ _("rmapbt not supported with realtime devices\n"));
usage();
}
- if (logsize) {
- __uint64_t logbytes;
-
- logbytes = getnum(logsize, &lopts, L_SIZE);
+ if (logbytes) {
if (logbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
_("illegal log length %lld, not a multiple of %d\n"),
@@ -2228,10 +2223,7 @@ _("rmapbt not supported with realtime devices\n"));
(long long)logbytes, blocksize,
(long long)(logblocks << blocklog));
}
- if (rtsize) {
- __uint64_t rtbytes;
-
- rtbytes = getnum(rtsize, &ropts, R_SIZE);
+ if (rtbytes) {
if (rtbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
_("illegal rt length %lld, not a multiple of %d\n"),
@@ -2248,10 +2240,7 @@ _("rmapbt not supported with realtime devices\n"));
/*
* If specified, check rt extent size against its constraints.
*/
- if (rtextsize) {
- __uint64_t rtextbytes;
-
- rtextbytes = getnum(rtextsize, &ropts, R_EXTSIZE);
+ if (rtextbytes) {
if (rtextbytes % blocksize) {
fprintf(stderr,
_("illegal rt extent size %lld, not a multiple of %d\n"),
@@ -2268,7 +2257,7 @@ _("rmapbt not supported with realtime devices\n"));
__uint64_t rswidth;
__uint64_t rtextbytes;
- if (!norsflag && !xi.risfile && !(!rtsize && xi.disfile))
+ if (!norsflag && !xi.risfile && !(!rtbytes && xi.disfile))
rswidth = ft.rtswidth;
else
rswidth = 0;
@@ -2379,15 +2368,16 @@ _("rmapbt not supported with realtime devices\n"));
rtfile = _("volume rt");
else if (!xi.rtdev)
rtfile = _("none");
- if (dsize && xi.dsize > 0 && dblocks > DTOBT(xi.dsize)) {
+ if (dbytes && xi.dsize > 0 && dblocks > DTOBT(xi.dsize)) {
fprintf(stderr,
_("size %s specified for data subvolume is too large, "
"maximum is %lld blocks\n"),
- dsize, (long long)DTOBT(xi.dsize));
+ get_conf_raw(&dopts, D_SIZE),
+ (long long)DTOBT(xi.dsize));
usage();
- } else if (!dsize && xi.dsize > 0)
+ } else if (!dbytes && xi.dsize > 0)
dblocks = DTOBT(xi.dsize);
- else if (!dsize) {
+ else if (!dbytes) {
fprintf(stderr, _("can't get size of data subvolume\n"));
usage();
}
@@ -2420,22 +2410,23 @@ reported by the device (%u).\n"),
reported by the device (%u).\n"),
lsectorsize, xi.lbsize);
}
- if (rtsize && xi.rtsize > 0 && xi.rtbsize > sectorsize) {
+ if (rtbytes && xi.rtsize > 0 && xi.rtbsize > sectorsize) {
fprintf(stderr, _(
"Warning: the realtime subvolume sector size %u is less than the sector size\n\
reported by the device (%u).\n"),
sectorsize, xi.rtbsize);
}
- if (rtsize && xi.rtsize > 0 && rtblocks > DTOBT(xi.rtsize)) {
+ if (rtbytes && xi.rtsize > 0 && rtblocks > DTOBT(xi.rtsize)) {
fprintf(stderr,
_("size %s specified for rt subvolume is too large, "
"maximum is %lld blocks\n"),
- rtsize, (long long)DTOBT(xi.rtsize));
+ get_conf_raw(&ropts, R_SIZE),
+ (long long)DTOBT(xi.rtsize));
usage();
- } else if (!rtsize && xi.rtsize > 0)
+ } else if (!rtbytes && xi.rtsize > 0)
rtblocks = DTOBT(xi.rtsize);
- else if (rtsize && !xi.rtdev) {
+ else if (rtbytes && !xi.rtdev) {
fprintf(stderr,
_("size specified for non-existent rt subvolume\n"));
usage();
@@ -2641,26 +2632,27 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
sb_feat.inode_align);
ASSERT(min_logblocks);
min_logblocks = MAX(XFS_MIN_LOG_BLOCKS, min_logblocks);
- if (!logsize && dblocks >= (1024*1024*1024) >> blocklog)
+ if (!logbytes && dblocks >= (1024*1024*1024) >> blocklog)
min_logblocks = MAX(min_logblocks, XFS_MIN_LOG_BYTES>>blocklog);
- if (logsize && xi.logBBsize > 0 && logblocks > DTOBT(xi.logBBsize)) {
+ if (logbytes && xi.logBBsize > 0 && logblocks > DTOBT(xi.logBBsize)) {
fprintf(stderr,
_("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
- logsize, (long long)DTOBT(xi.logBBsize));
+ get_conf_raw(&lopts, L_SIZE),
+ (long long)DTOBT(xi.logBBsize));
usage();
- } else if (!logsize && xi.logBBsize > 0) {
+ } else if (!logbytes && xi.logBBsize > 0) {
logblocks = DTOBT(xi.logBBsize);
- } else if (logsize && !xi.logdev && !loginternal) {
+ } else if (logbytes && !xi.logdev && !loginternal) {
fprintf(stderr,
_("size specified for non-existent log subvolume\n"));
usage();
- } else if (loginternal && logsize && logblocks >= dblocks) {
+ } else if (loginternal && logbytes && logblocks >= dblocks) {
fprintf(stderr, _("size %lld too large for internal log\n"),
(long long)logblocks);
usage();
} else if (!loginternal && !xi.logdev) {
logblocks = 0;
- } else if (loginternal && !logsize) {
+ } else if (loginternal && !logbytes) {
if (dblocks < GIGABYTES(1, blocklog)) {
/* tiny filesystems get minimum sized logs. */
@@ -2724,7 +2716,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
* Readjust the log size to fit within an AG if it was sized
* automatically.
*/
- if (!logsize) {
+ if (!logbytes) {
logblocks = MIN(logblocks,
libxfs_alloc_ag_max_usable(mp));
--
2.11.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 4/7] mkfs: merge tables for opts parsing into one table
2017-07-20 9:29 [PATCH 0/7] mkfs: save user input into opts table Jan Tulak
` (2 preceding siblings ...)
2017-07-20 9:29 ` [PATCH 3/7] mkfs: remove intermediate getstr followed by getnum Jan Tulak
@ 2017-07-20 9:29 ` Jan Tulak
2017-07-20 9:29 ` [PATCH 5/7] mkfs: move getnum within the file Jan Tulak
` (2 subsequent siblings)
6 siblings, 0 replies; 39+ messages in thread
From: Jan Tulak @ 2017-07-20 9:29 UTC (permalink / raw)
To: linux-xfs; +Cc: Jan Tulak
Merge separate instances of opt_params into one indexable table. Git
makes this patch look complicated, but it does not change
values or structure of anything else. It only moves all the "struct
opt_params dopts = {...}", changes indentation for these substructures
and replaces their usage (dopts -> opts[OPT_D]).
The reason for this is to be able to address all options from any single one,
even across OPT_X. Right now, we can do automated conflict checks only
within one OPT_X, but after this, it is possible to extend the conflict
declaration to other options as well.
Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
Change:
* updated description
---
mkfs/xfs_mkfs.c | 1316 +++++++++++++++++++++++++++++--------------------------
1 file changed, 683 insertions(+), 633 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 127f14c3..f10cb15a 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -39,6 +39,7 @@ static int ispow2(unsigned int i);
unsigned int blocksize;
unsigned int sectorsize;
+#define MAX_OPTS 16
#define MAX_SUBOPTS 16
#define SUBOPT_NEEDS_VAL (-1LL)
#define MAX_CONFLICTS 8
@@ -49,6 +50,10 @@ unsigned int sectorsize;
*
* Description of the structure members follows:
*
+ * index MANDATORY
+ * An integer denoting the position of the specific option in opts array,
+ * counting from 0 up to MAX_OPTS.
+ *
* name MANDATORY
* Name is a single char, e.g., for '-d file', name is 'd'.
*
@@ -114,6 +119,7 @@ unsigned int sectorsize;
*
*/
struct opt_params {
+ int index;
const char name;
const char *subopts[MAX_SUBOPTS];
@@ -129,584 +135,592 @@ struct opt_params {
long long flagval;
const char *raw_input;
} subopt_params[MAX_SUBOPTS];
-};
-
-struct opt_params bopts = {
- .name = 'b',
- .subopts = {
+} opts[MAX_OPTS] = {
+#define OPT_B 0
+ {
+ .index = OPT_B,
+ .name = 'b',
+ .subopts = {
#define B_LOG 0
- "log",
+ "log",
#define B_SIZE 1
- "size",
- NULL
- },
- .subopt_params = {
- { .index = B_LOG,
- .conflicts = { B_SIZE,
- LAST_CONFLICT },
- .minval = XFS_MIN_BLOCKSIZE_LOG,
- .maxval = XFS_MAX_BLOCKSIZE_LOG,
- .flagval = SUBOPT_NEEDS_VAL,
+ "size",
+ NULL
},
- { .index = B_SIZE,
- .convert = true,
- .is_power_2 = true,
- .conflicts = { B_LOG,
- LAST_CONFLICT },
- .minval = XFS_MIN_BLOCKSIZE,
- .maxval = XFS_MAX_BLOCKSIZE,
- .flagval = SUBOPT_NEEDS_VAL,
+ .subopt_params = {
+ { .index = B_LOG,
+ .conflicts = { B_SIZE,
+ LAST_CONFLICT },
+ .minval = XFS_MIN_BLOCKSIZE_LOG,
+ .maxval = XFS_MAX_BLOCKSIZE_LOG,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = B_SIZE,
+ .convert = true,
+ .is_power_2 = true,
+ .conflicts = { B_LOG,
+ LAST_CONFLICT },
+ .minval = XFS_MIN_BLOCKSIZE,
+ .maxval = XFS_MAX_BLOCKSIZE,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
},
},
-};
-
-struct opt_params dopts = {
- .name = 'd',
- .subopts = {
-#define D_AGCOUNT 0
- "agcount",
-#define D_FILE 1
- "file",
-#define D_NAME 2
- "name",
-#define D_SIZE 3
- "size",
-#define D_SUNIT 4
- "sunit",
-#define D_SWIDTH 5
- "swidth",
-#define D_AGSIZE 6
- "agsize",
-#define D_SU 7
- "su",
-#define D_SW 8
- "sw",
-#define D_SECTLOG 9
- "sectlog",
-#define D_SECTSIZE 10
- "sectsize",
-#define D_NOALIGN 11
- "noalign",
-#define D_RTINHERIT 12
- "rtinherit",
-#define D_PROJINHERIT 13
- "projinherit",
-#define D_EXTSZINHERIT 14
- "extszinherit",
- NULL
- },
- .subopt_params = {
- { .index = D_AGCOUNT,
- .conflicts = { D_AGSIZE,
- LAST_CONFLICT },
- .minval = 1,
- .maxval = XFS_MAX_AGNUMBER,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = D_FILE,
- .conflicts = { LAST_CONFLICT },
- .minval = 0,
- .maxval = 1,
- .flagval = 1,
- },
- { .index = D_NAME,
- .conflicts = { LAST_CONFLICT },
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = D_SIZE,
- .conflicts = { LAST_CONFLICT },
- .convert = true,
- .minval = XFS_AG_MIN_BYTES,
- .maxval = LLONG_MAX,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = D_SUNIT,
- .conflicts = { D_NOALIGN,
- D_SU,
- D_SW,
- LAST_CONFLICT },
- .minval = 0,
- .maxval = UINT_MAX,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = D_SWIDTH,
- .conflicts = { D_NOALIGN,
- D_SU,
- D_SW,
- LAST_CONFLICT },
- .minval = 0,
- .maxval = UINT_MAX,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = D_AGSIZE,
- .conflicts = { D_AGCOUNT,
- LAST_CONFLICT },
- .convert = true,
- .minval = XFS_AG_MIN_BYTES,
- .maxval = XFS_AG_MAX_BYTES,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = D_SU,
- .conflicts = { D_NOALIGN,
- D_SUNIT,
- D_SWIDTH,
- LAST_CONFLICT },
- .convert = true,
- .minval = 0,
- .maxval = UINT_MAX,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = D_SW,
- .conflicts = { D_NOALIGN,
- D_SUNIT,
- D_SWIDTH,
- LAST_CONFLICT },
- .minval = 0,
- .maxval = UINT_MAX,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = D_SECTLOG,
- .conflicts = { D_SECTSIZE,
- LAST_CONFLICT },
- .minval = XFS_MIN_SECTORSIZE_LOG,
- .maxval = XFS_MAX_SECTORSIZE_LOG,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = D_SECTSIZE,
- .conflicts = { D_SECTLOG,
- LAST_CONFLICT },
- .convert = true,
- .is_power_2 = true,
- .minval = XFS_MIN_SECTORSIZE,
- .maxval = XFS_MAX_SECTORSIZE,
- .flagval = SUBOPT_NEEDS_VAL,
+#define OPT_D 1
+ {
+ .index = OPT_D,
+ .name = 'd',
+ .subopts = {
+ #define D_AGCOUNT 0
+ "agcount",
+ #define D_FILE 1
+ "file",
+ #define D_NAME 2
+ "name",
+ #define D_SIZE 3
+ "size",
+ #define D_SUNIT 4
+ "sunit",
+ #define D_SWIDTH 5
+ "swidth",
+ #define D_AGSIZE 6
+ "agsize",
+ #define D_SU 7
+ "su",
+ #define D_SW 8
+ "sw",
+ #define D_SECTLOG 9
+ "sectlog",
+ #define D_SECTSIZE 10
+ "sectsize",
+ #define D_NOALIGN 11
+ "noalign",
+ #define D_RTINHERIT 12
+ "rtinherit",
+ #define D_PROJINHERIT 13
+ "projinherit",
+ #define D_EXTSZINHERIT 14
+ "extszinherit",
+ NULL
},
- { .index = D_NOALIGN,
- .conflicts = { D_SU,
- D_SW,
- D_SUNIT,
- D_SWIDTH,
- LAST_CONFLICT },
- .minval = 0,
- .maxval = 1,
- .flagval = 1,
- },
- { .index = D_RTINHERIT,
- .conflicts = { LAST_CONFLICT },
- .minval = 1,
- .maxval = 1,
- .flagval = 1,
- },
- { .index = D_PROJINHERIT,
- .conflicts = { LAST_CONFLICT },
- .minval = 0,
- .maxval = UINT_MAX,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = D_EXTSZINHERIT,
- .conflicts = { LAST_CONFLICT },
- .minval = 0,
- .maxval = UINT_MAX,
- .flagval = SUBOPT_NEEDS_VAL,
+ .subopt_params = {
+ { .index = D_AGCOUNT,
+ .conflicts = { D_AGSIZE,
+ LAST_CONFLICT },
+ .minval = 1,
+ .maxval = XFS_MAX_AGNUMBER,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = D_FILE,
+ .conflicts = { LAST_CONFLICT },
+ .minval = 0,
+ .maxval = 1,
+ .flagval = 1,
+ },
+ { .index = D_NAME,
+ .conflicts = { LAST_CONFLICT },
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = D_SIZE,
+ .conflicts = { LAST_CONFLICT },
+ .convert = true,
+ .minval = XFS_AG_MIN_BYTES,
+ .maxval = LLONG_MAX,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = D_SUNIT,
+ .conflicts = { D_NOALIGN,
+ D_SU,
+ D_SW,
+ LAST_CONFLICT },
+ .minval = 0,
+ .maxval = UINT_MAX,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = D_SWIDTH,
+ .conflicts = { D_NOALIGN,
+ D_SU,
+ D_SW,
+ LAST_CONFLICT },
+ .minval = 0,
+ .maxval = UINT_MAX,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = D_AGSIZE,
+ .conflicts = { D_AGCOUNT,
+ LAST_CONFLICT },
+ .convert = true,
+ .minval = XFS_AG_MIN_BYTES,
+ .maxval = XFS_AG_MAX_BYTES,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = D_SU,
+ .conflicts = { D_NOALIGN,
+ D_SUNIT,
+ D_SWIDTH,
+ LAST_CONFLICT },
+ .convert = true,
+ .minval = 0,
+ .maxval = UINT_MAX,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = D_SW,
+ .conflicts = { D_NOALIGN,
+ D_SUNIT,
+ D_SWIDTH,
+ LAST_CONFLICT },
+ .minval = 0,
+ .maxval = UINT_MAX,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = D_SECTLOG,
+ .conflicts = { D_SECTSIZE,
+ LAST_CONFLICT },
+ .minval = XFS_MIN_SECTORSIZE_LOG,
+ .maxval = XFS_MAX_SECTORSIZE_LOG,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = D_SECTSIZE,
+ .conflicts = { D_SECTLOG,
+ LAST_CONFLICT },
+ .convert = true,
+ .is_power_2 = true,
+ .minval = XFS_MIN_SECTORSIZE,
+ .maxval = XFS_MAX_SECTORSIZE,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = D_NOALIGN,
+ .conflicts = { D_SU,
+ D_SW,
+ D_SUNIT,
+ D_SWIDTH,
+ LAST_CONFLICT },
+ .minval = 0,
+ .maxval = 1,
+ .flagval = 1,
+ },
+ { .index = D_RTINHERIT,
+ .conflicts = { LAST_CONFLICT },
+ .minval = 0,
+ .maxval = 1,
+ .flagval = 1,
+ },
+ { .index = D_PROJINHERIT,
+ .conflicts = { LAST_CONFLICT },
+ .minval = 0,
+ .maxval = UINT_MAX,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = D_EXTSZINHERIT,
+ .conflicts = { LAST_CONFLICT },
+ .minval = 0,
+ .maxval = UINT_MAX,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
},
},
-};
-
-
-struct opt_params iopts = {
- .name = 'i',
- .subopts = {
+#define OPT_I 2
+ {
+ .index = OPT_I,
+ .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",
+ "projid32bit",
#define I_SPINODES 7
- "sparse",
- NULL
- },
- .subopt_params = {
- { .index = I_ALIGN,
- .conflicts = { LAST_CONFLICT },
- .minval = 0,
- .maxval = 1,
- .flagval = 1,
- },
- { .index = I_LOG,
- .conflicts = { I_PERBLOCK,
- I_SIZE,
- LAST_CONFLICT },
- .minval = XFS_DINODE_MIN_LOG,
- .maxval = XFS_DINODE_MAX_LOG,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = I_MAXPCT,
- .conflicts = { LAST_CONFLICT },
- .minval = 0,
- .maxval = 100,
- .flagval = SUBOPT_NEEDS_VAL,
+ "sparse",
+ NULL
},
- { .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,
- .flagval = 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,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = I_ATTR,
- .conflicts = { LAST_CONFLICT },
- .minval = 0,
- .maxval = 2,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = I_PROJID32BIT,
- .conflicts = { LAST_CONFLICT },
- .minval = 0,
- .maxval = 1,
- .flagval = 1,
- },
- { .index = I_SPINODES,
- .conflicts = { LAST_CONFLICT },
- .minval = 0,
- .maxval = 1,
- .flagval = 1,
+ .subopt_params = {
+ { .index = I_ALIGN,
+ .conflicts = { LAST_CONFLICT },
+ .minval = 0,
+ .maxval = 1,
+ .flagval = 1,
+ },
+ { .index = I_LOG,
+ .conflicts = { I_PERBLOCK,
+ I_SIZE,
+ LAST_CONFLICT },
+ .minval = XFS_DINODE_MIN_LOG,
+ .maxval = XFS_DINODE_MAX_LOG,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = I_MAXPCT,
+ .conflicts = { LAST_CONFLICT },
+ .minval = 0,
+ .maxval = 100,
+ .flagval = 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,
+ .flagval = 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,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = I_ATTR,
+ .conflicts = { LAST_CONFLICT },
+ .minval = 0,
+ .maxval = 2,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = I_PROJID32BIT,
+ .conflicts = { LAST_CONFLICT },
+ .minval = 0,
+ .maxval = 1,
+ .flagval = 1,
+ },
+ { .index = I_SPINODES,
+ .conflicts = { LAST_CONFLICT },
+ .minval = 0,
+ .maxval = 1,
+ .flagval = 1,
+ },
},
},
-};
-
-struct opt_params lopts = {
- .name = 'l',
- .subopts = {
-#define L_AGNUM 0
- "agnum",
-#define L_INTERNAL 1
- "internal",
-#define L_SIZE 2
- "size",
-#define L_VERSION 3
- "version",
-#define L_SUNIT 4
- "sunit",
-#define L_SU 5
- "su",
-#define L_DEV 6
- "logdev",
-#define L_SECTLOG 7
- "sectlog",
-#define L_SECTSIZE 8
- "sectsize",
-#define L_FILE 9
- "file",
-#define L_NAME 10
- "name",
-#define L_LAZYSBCNTR 11
- "lazy-count",
- NULL
- },
- .subopt_params = {
- { .index = L_AGNUM,
- .conflicts = { L_DEV,
- LAST_CONFLICT },
- .minval = 0,
- .maxval = UINT_MAX,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = L_INTERNAL,
- .conflicts = { L_FILE,
- L_DEV,
- LAST_CONFLICT },
- .minval = 0,
- .maxval = 1,
- .flagval = 1,
+#define OPT_L 3
+ {
+ .index = OPT_L,
+ .name = 'l',
+ .subopts = {
+ #define L_AGNUM 0
+ "agnum",
+ #define L_INTERNAL 1
+ "internal",
+ #define L_SIZE 2
+ "size",
+ #define L_VERSION 3
+ "version",
+ #define L_SUNIT 4
+ "sunit",
+ #define L_SU 5
+ "su",
+ #define L_DEV 6
+ "logdev",
+ #define L_SECTLOG 7
+ "sectlog",
+ #define L_SECTSIZE 8
+ "sectsize",
+ #define L_FILE 9
+ "file",
+ #define L_NAME 10
+ "name",
+ #define L_LAZYSBCNTR 11
+ "lazy-count",
+ NULL
},
- { .index = L_SIZE,
- .conflicts = { LAST_CONFLICT },
- .convert = true,
- .minval = 2 * 1024 * 1024LL, /* XXX: XFS_MIN_LOG_BYTES */
- .maxval = XFS_MAX_LOG_BYTES,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = L_VERSION,
- .conflicts = { LAST_CONFLICT },
- .minval = 1,
- .maxval = 2,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = L_SUNIT,
- .conflicts = { L_SU,
- LAST_CONFLICT },
- .minval = 1,
- .maxval = BTOBB(XLOG_MAX_RECORD_BSIZE),
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = L_SU,
- .conflicts = { L_SUNIT,
- LAST_CONFLICT },
- .convert = true,
- .minval = BBTOB(1),
- .maxval = XLOG_MAX_RECORD_BSIZE,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = L_DEV,
- .conflicts = { L_AGNUM,
- L_INTERNAL,
- LAST_CONFLICT },
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = L_SECTLOG,
- .conflicts = { L_SECTSIZE,
- LAST_CONFLICT },
- .minval = XFS_MIN_SECTORSIZE_LOG,
- .maxval = XFS_MAX_SECTORSIZE_LOG,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = L_SECTSIZE,
- .conflicts = { L_SECTLOG,
- LAST_CONFLICT },
- .convert = true,
- .is_power_2 = true,
- .minval = XFS_MIN_SECTORSIZE,
- .maxval = XFS_MAX_SECTORSIZE,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = L_FILE,
- .conflicts = { L_INTERNAL,
- LAST_CONFLICT },
- .minval = 0,
- .maxval = 1,
- .flagval = 1,
- },
- { .index = L_NAME,
- .conflicts = { L_AGNUM,
- L_INTERNAL,
- LAST_CONFLICT },
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = L_LAZYSBCNTR,
- .conflicts = { LAST_CONFLICT },
- .minval = 0,
- .maxval = 1,
- .flagval = 1,
+ .subopt_params = {
+ { .index = L_AGNUM,
+ .conflicts = { L_DEV,
+ LAST_CONFLICT },
+ .minval = 0,
+ .maxval = UINT_MAX,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = L_INTERNAL,
+ .conflicts = { L_FILE,
+ L_DEV,
+ LAST_CONFLICT },
+ .minval = 0,
+ .maxval = 1,
+ .flagval = 1,
+ },
+ { .index = L_SIZE,
+ .conflicts = { LAST_CONFLICT },
+ .convert = true,
+ .minval = 2 * 1024 * 1024LL, /* XXX: XFS_MIN_LOG_BYTES */
+ .maxval = XFS_MAX_LOG_BYTES,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = L_VERSION,
+ .conflicts = { LAST_CONFLICT },
+ .minval = 1,
+ .maxval = 2,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = L_SUNIT,
+ .conflicts = { L_SU,
+ LAST_CONFLICT },
+ .minval = 1,
+ .maxval = BTOBB(XLOG_MAX_RECORD_BSIZE),
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = L_SU,
+ .conflicts = { L_SUNIT,
+ LAST_CONFLICT },
+ .convert = true,
+ .minval = BBTOB(1),
+ .maxval = XLOG_MAX_RECORD_BSIZE,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = L_DEV,
+ .conflicts = { L_AGNUM,
+ L_INTERNAL,
+ LAST_CONFLICT },
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = L_SECTLOG,
+ .conflicts = { L_SECTSIZE,
+ LAST_CONFLICT },
+ .minval = XFS_MIN_SECTORSIZE_LOG,
+ .maxval = XFS_MAX_SECTORSIZE_LOG,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = L_SECTSIZE,
+ .conflicts = { L_SECTLOG,
+ LAST_CONFLICT },
+ .convert = true,
+ .is_power_2 = true,
+ .minval = XFS_MIN_SECTORSIZE,
+ .maxval = XFS_MAX_SECTORSIZE,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = L_FILE,
+ .conflicts = { L_INTERNAL,
+ LAST_CONFLICT },
+ .minval = 0,
+ .maxval = 1,
+ .flagval = 1,
+ },
+ { .index = L_NAME,
+ .conflicts = { L_AGNUM,
+ L_INTERNAL,
+ LAST_CONFLICT },
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = L_LAZYSBCNTR,
+ .conflicts = { LAST_CONFLICT },
+ .minval = 0,
+ .maxval = 1,
+ .flagval = 1,
+ },
},
},
-};
-
-struct opt_params nopts = {
- .name = 'n',
- .subopts = {
-#define N_LOG 0
- "log",
-#define N_SIZE 1
- "size",
-#define N_VERSION 2
- "version",
-#define N_FTYPE 3
- "ftype",
- NULL,
- },
- .subopt_params = {
- { .index = N_LOG,
- .conflicts = { N_SIZE,
- LAST_CONFLICT },
- .minval = XFS_MIN_REC_DIRSIZE,
- .maxval = XFS_MAX_BLOCKSIZE_LOG,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = N_SIZE,
- .conflicts = { N_LOG,
- LAST_CONFLICT },
- .convert = true,
- .is_power_2 = true,
- .minval = 1 << XFS_MIN_REC_DIRSIZE,
- .maxval = XFS_MAX_BLOCKSIZE,
- .flagval = SUBOPT_NEEDS_VAL,
+#define OPT_N 4
+ {
+ .index = OPT_N,
+ .name = 'n',
+ .subopts = {
+ #define N_LOG 0
+ "log",
+ #define N_SIZE 1
+ "size",
+ #define N_VERSION 2
+ "version",
+ #define N_FTYPE 3
+ "ftype",
+ NULL,
},
- { .index = N_VERSION,
- .conflicts = { LAST_CONFLICT },
- .minval = 2,
- .maxval = 2,
- .flagval = SUBOPT_NEEDS_VAL,
+ .subopt_params = {
+ { .index = N_LOG,
+ .conflicts = { N_SIZE,
+ LAST_CONFLICT },
+ .minval = XFS_MIN_REC_DIRSIZE,
+ .maxval = XFS_MAX_BLOCKSIZE_LOG,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = N_SIZE,
+ .conflicts = { N_LOG,
+ LAST_CONFLICT },
+ .convert = true,
+ .is_power_2 = true,
+ .minval = 1 << XFS_MIN_REC_DIRSIZE,
+ .maxval = XFS_MAX_BLOCKSIZE,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = N_VERSION,
+ .conflicts = { LAST_CONFLICT },
+ .minval = 2,
+ .maxval = 2,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = N_FTYPE,
+ .conflicts = { LAST_CONFLICT },
+ .minval = 0,
+ .maxval = 1,
+ .flagval = 1,
+ },
},
- { .index = N_FTYPE,
- .conflicts = { LAST_CONFLICT },
- .minval = 0,
- .maxval = 1,
- .flagval = 1,
- },
- },
-};
-
-struct opt_params ropts = {
- .name = 'r',
- .subopts = {
-#define R_EXTSIZE 0
- "extsize",
-#define R_SIZE 1
- "size",
-#define R_DEV 2
- "rtdev",
-#define R_FILE 3
- "file",
-#define R_NAME 4
- "name",
-#define R_NOALIGN 5
- "noalign",
- NULL
},
- .subopt_params = {
- { .index = R_EXTSIZE,
- .conflicts = { LAST_CONFLICT },
- .convert = true,
- .minval = XFS_MIN_RTEXTSIZE,
- .maxval = XFS_MAX_RTEXTSIZE,
- .flagval = SUBOPT_NEEDS_VAL,
+#define OPT_R 5
+ {
+ .index = OPT_R,
+ .name = 'r',
+ .subopts = {
+ #define R_EXTSIZE 0
+ "extsize",
+ #define R_SIZE 1
+ "size",
+ #define R_DEV 2
+ "rtdev",
+ #define R_FILE 3
+ "file",
+ #define R_NAME 4
+ "name",
+ #define R_NOALIGN 5
+ "noalign",
+ NULL
},
- { .index = R_SIZE,
- .conflicts = { LAST_CONFLICT },
- .convert = true,
- .minval = 0,
- .maxval = LLONG_MAX,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = R_DEV,
- .conflicts = { LAST_CONFLICT },
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = R_FILE,
- .minval = 0,
- .maxval = 1,
- .flagval = 1,
- .conflicts = { LAST_CONFLICT },
- },
- { .index = R_NAME,
- .conflicts = { LAST_CONFLICT },
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = R_NOALIGN,
- .minval = 0,
- .maxval = 1,
- .flagval = 1,
- .conflicts = { LAST_CONFLICT },
+ .subopt_params = {
+ { .index = R_EXTSIZE,
+ .conflicts = { LAST_CONFLICT },
+ .convert = true,
+ .minval = XFS_MIN_RTEXTSIZE,
+ .maxval = XFS_MAX_RTEXTSIZE,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = R_SIZE,
+ .conflicts = { LAST_CONFLICT },
+ .convert = true,
+ .minval = 0,
+ .maxval = LLONG_MAX,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = R_DEV,
+ .conflicts = { LAST_CONFLICT },
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = R_FILE,
+ .minval = 0,
+ .maxval = 1,
+ .flagval = 1,
+ .conflicts = { LAST_CONFLICT },
+ },
+ { .index = R_NAME,
+ .conflicts = { LAST_CONFLICT },
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = R_NOALIGN,
+ .minval = 0,
+ .maxval = 1,
+ .flagval = 1,
+ .conflicts = { LAST_CONFLICT },
+ },
},
},
-};
-
-struct opt_params sopts = {
- .name = 's',
- .subopts = {
-#define S_LOG 0
- "log",
-#define S_SECTLOG 1
- "sectlog",
-#define S_SIZE 2
- "size",
-#define S_SECTSIZE 3
- "sectsize",
- NULL
- },
- .subopt_params = {
- { .index = S_LOG,
- .conflicts = { S_SIZE,
- S_SECTSIZE,
- LAST_CONFLICT },
- .minval = XFS_MIN_SECTORSIZE_LOG,
- .maxval = XFS_MAX_SECTORSIZE_LOG,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = S_SECTLOG,
- .conflicts = { S_SIZE,
- S_SECTSIZE,
- LAST_CONFLICT },
- .minval = XFS_MIN_SECTORSIZE_LOG,
- .maxval = XFS_MAX_SECTORSIZE_LOG,
- .flagval = SUBOPT_NEEDS_VAL,
+#define OPT_S 6
+ {
+ .index = OPT_S,
+ .name = 's',
+ .subopts = {
+ #define S_LOG 0
+ "log",
+ #define S_SECTLOG 1
+ "sectlog",
+ #define S_SIZE 2
+ "size",
+ #define S_SECTSIZE 3
+ "sectsize",
+ NULL
},
- { .index = S_SIZE,
- .conflicts = { S_LOG,
- S_SECTLOG,
- LAST_CONFLICT },
- .convert = true,
- .is_power_2 = true,
- .minval = XFS_MIN_SECTORSIZE,
- .maxval = XFS_MAX_SECTORSIZE,
- .flagval = SUBOPT_NEEDS_VAL,
+ .subopt_params = {
+ { .index = S_LOG,
+ .conflicts = { S_SIZE,
+ S_SECTSIZE,
+ LAST_CONFLICT },
+ .minval = XFS_MIN_SECTORSIZE_LOG,
+ .maxval = XFS_MAX_SECTORSIZE_LOG,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = S_SECTLOG,
+ .conflicts = { S_SIZE,
+ S_SECTSIZE,
+ LAST_CONFLICT },
+ .minval = XFS_MIN_SECTORSIZE_LOG,
+ .maxval = XFS_MAX_SECTORSIZE_LOG,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = S_SIZE,
+ .conflicts = { S_LOG,
+ S_SECTLOG,
+ LAST_CONFLICT },
+ .convert = true,
+ .is_power_2 = true,
+ .minval = XFS_MIN_SECTORSIZE,
+ .maxval = XFS_MAX_SECTORSIZE,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = S_SECTSIZE,
+ .conflicts = { S_LOG,
+ S_SECTLOG,
+ LAST_CONFLICT },
+ .convert = true,
+ .is_power_2 = true,
+ .minval = XFS_MIN_SECTORSIZE,
+ .maxval = XFS_MAX_SECTORSIZE,
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
},
- { .index = S_SECTSIZE,
- .conflicts = { S_LOG,
- S_SECTLOG,
- LAST_CONFLICT },
- .convert = true,
- .is_power_2 = true,
- .minval = XFS_MIN_SECTORSIZE,
- .maxval = XFS_MAX_SECTORSIZE,
- .flagval = SUBOPT_NEEDS_VAL,
- },
- },
-};
-
-struct opt_params mopts = {
- .name = 'm',
- .subopts = {
-#define M_CRC 0
- "crc",
-#define M_FINOBT 1
- "finobt",
-#define M_UUID 2
- "uuid",
-#define M_RMAPBT 3
- "rmapbt",
-#define M_REFLINK 4
- "reflink",
- NULL
},
- .subopt_params = {
- { .index = M_CRC,
- .conflicts = { LAST_CONFLICT },
- .minval = 0,
- .maxval = 1,
- .flagval = 1,
+#define OPT_M 7
+ {
+ .index = OPT_M,
+ .name = 'm',
+ .subopts = {
+ #define M_CRC 0
+ "crc",
+ #define M_FINOBT 1
+ "finobt",
+ #define M_UUID 2
+ "uuid",
+ #define M_RMAPBT 3
+ "rmapbt",
+ #define M_REFLINK 4
+ "reflink",
+ NULL
},
- { .index = M_FINOBT,
- .conflicts = { LAST_CONFLICT },
- .minval = 0,
- .maxval = 1,
- .flagval = 1,
- },
- { .index = M_UUID,
- .conflicts = { LAST_CONFLICT },
- .flagval = SUBOPT_NEEDS_VAL,
- },
- { .index = M_RMAPBT,
- .conflicts = { LAST_CONFLICT },
- .minval = 0,
- .maxval = 1,
- .flagval = 1,
- },
- { .index = M_REFLINK,
- .conflicts = { LAST_CONFLICT },
- .minval = 0,
- .maxval = 1,
- .flagval = 1,
+ .subopt_params = {
+ { .index = M_CRC,
+ .conflicts = { LAST_CONFLICT },
+ .minval = 0,
+ .maxval = 1,
+ .flagval = 1,
+ },
+ { .index = M_FINOBT,
+ .conflicts = { LAST_CONFLICT },
+ .minval = 0,
+ .maxval = 1,
+ .flagval = 1,
+ },
+ { .index = M_UUID,
+ .conflicts = { LAST_CONFLICT },
+ .flagval = SUBOPT_NEEDS_VAL,
+ },
+ { .index = M_RMAPBT,
+ .conflicts = { LAST_CONFLICT },
+ .minval = 0,
+ .maxval = 1,
+ .flagval = 1,
+ },
+ { .index = M_REFLINK,
+ .conflicts = { LAST_CONFLICT },
+ .minval = 0,
+ .maxval = 1,
+ .flagval = 1,
+ },
},
},
};
@@ -736,15 +750,15 @@ struct opt_params mopts = {
#define WHACK_SIZE (128 * 1024)
static inline void
-set_conf_raw(struct opt_params *opt, int subopt, const char *value)
+set_conf_raw(int opt, int subopt, const char *value)
{
- opt->subopt_params[subopt].raw_input = value;
+ opts[opt].subopt_params[subopt].raw_input = value;
}
static inline const char *
-get_conf_raw(const struct opt_params *opt, int subopt)
+get_conf_raw(int opt, int subopt)
{
- return opt->subopt_params[subopt].raw_input;
+ return opts[opt].subopt_params[subopt].raw_input;
}
/*
@@ -1345,7 +1359,7 @@ getnum(
long long c;
check_opt(opts, index, false);
- set_conf_raw(opts, index, str);
+ set_conf_raw(opts->index, index, str);
/* empty strings might just return a default value */
if (!str || *str == '\0') {
if (sp->flagval == SUBOPT_NEEDS_VAL)
@@ -1559,17 +1573,19 @@ main(
case 'b':
p = optarg;
while (*p != '\0') {
- char **subopts = (char **)bopts.subopts;
+ char **subopts =
+ (char **)opts[OPT_B].subopts;
char *value;
switch (getsubopt(&p, subopts, &value)) {
case B_LOG:
- blocklog = getnum(value, &bopts, B_LOG);
+ blocklog = getnum(value, &opts[OPT_B],
+ B_LOG);
blocksize = 1 << blocklog;
blflag = 1;
break;
case B_SIZE:
- blocksize = getnum(value, &bopts,
+ blocksize = getnum(value, &opts[OPT_B],
B_SIZE);
blocklog = libxfs_highbit32(blocksize);
bsflag = 1;
@@ -1582,74 +1598,82 @@ main(
case 'd':
p = optarg;
while (*p != '\0') {
- char **subopts = (char **)dopts.subopts;
+ char **subopts =
+ (char **)opts[OPT_D].subopts;
char *value;
switch (getsubopt(&p, subopts, &value)) {
case D_AGCOUNT:
- agcount = getnum(value, &dopts,
+ agcount = getnum(value, &opts[OPT_D],
D_AGCOUNT);
daflag = 1;
break;
case D_AGSIZE:
- agsize = getnum(value, &dopts, D_AGSIZE);
+ agsize = getnum(value, &opts[OPT_D],
+ D_AGSIZE);
dasize = 1;
break;
case D_FILE:
- xi.disfile = getnum(value, &dopts,
- D_FILE);
+ xi.disfile = getnum(value,
+ &opts[OPT_D], D_FILE);
break;
case D_NAME:
- xi.dname = getstr(value, &dopts, D_NAME);
+ xi.dname = getstr(value, &opts[OPT_D],
+ D_NAME);
break;
case D_SIZE:
- dbytes = getnum(value, &dopts, D_SIZE);
+ dbytes = getnum(value, &opts[OPT_D],
+ D_SIZE);
break;
case D_SUNIT:
- dsunit = getnum(value, &dopts, D_SUNIT);
+ dsunit = getnum(value, &opts[OPT_D],
+ D_SUNIT);
break;
case D_SWIDTH:
- dswidth = getnum(value, &dopts,
+ dswidth = getnum(value, &opts[OPT_D],
D_SWIDTH);
break;
case D_SU:
- dsu = getnum(value, &dopts, D_SU);
+ dsu = getnum(value, &opts[OPT_D],
+ D_SU);
break;
case D_SW:
- dsw = getnum(value, &dopts, D_SW);
+ dsw = getnum(value, &opts[OPT_D],
+ D_SW);
break;
case D_NOALIGN:
- nodsflag = getnum(value, &dopts,
+ nodsflag = getnum(value, &opts[OPT_D],
D_NOALIGN);
break;
case D_SECTLOG:
- sectorlog = getnum(value, &dopts,
+ sectorlog = getnum(value, &opts[OPT_D],
D_SECTLOG);
sectorsize = 1 << sectorlog;
slflag = 1;
break;
case D_SECTSIZE:
- sectorsize = getnum(value, &dopts,
- D_SECTSIZE);
+ sectorsize = getnum(value,
+ &opts[OPT_D], D_SECTSIZE);
sectorlog =
libxfs_highbit32(sectorsize);
ssflag = 1;
break;
case D_RTINHERIT:
- c = getnum(value, &dopts, D_RTINHERIT);
+ c = getnum(value, &opts[OPT_D],
+ D_RTINHERIT);
if (c)
fsx.fsx_xflags |=
XFS_DIFLAG_RTINHERIT;
break;
case D_PROJINHERIT:
- fsx.fsx_projid = getnum(value, &dopts,
- D_PROJINHERIT);
+ fsx.fsx_projid = getnum(value,
+ &opts[OPT_D], D_PROJINHERIT);
fsx.fsx_xflags |=
XFS_DIFLAG_PROJINHERIT;
break;
case D_EXTSZINHERIT:
- fsx.fsx_extsize = getnum(value, &dopts,
- D_EXTSZINHERIT);
+ fsx.fsx_extsize = getnum(value,
+ &opts[OPT_D], D_EXTSZINHERIT);
fsx.fsx_xflags |=
XFS_DIFLAG_EXTSZINHERIT;
break;
@@ -1661,46 +1685,51 @@ main(
case 'i':
p = optarg;
while (*p != '\0') {
- char **subopts = (char **)iopts.subopts;
+ char **subopts =
+ (char **)opts[OPT_I].subopts;
char *value;
switch (getsubopt(&p, subopts, &value)) {
case I_ALIGN:
sb_feat.inode_align = getnum(value,
- &iopts, I_ALIGN);
+ &opts[OPT_I], I_ALIGN);
break;
case I_LOG:
- inodelog = getnum(value, &iopts, I_LOG);
+ inodelog = getnum(value, &opts[OPT_I],
+ I_LOG);
isize = 1 << inodelog;
ilflag = 1;
break;
case I_MAXPCT:
- imaxpct = getnum(value, &iopts,
+ imaxpct = getnum(value, &opts[OPT_I],
I_MAXPCT);
imflag = 1;
break;
case I_PERBLOCK:
- inopblock = getnum(value, &iopts,
+ inopblock = getnum(value, &opts[OPT_I],
I_PERBLOCK);
ipflag = 1;
break;
case I_SIZE:
- isize = getnum(value, &iopts, I_SIZE);
+ isize = getnum(value, &opts[OPT_I],
+ I_SIZE);
inodelog = libxfs_highbit32(isize);
isflag = 1;
break;
case I_ATTR:
sb_feat.attr_version =
- getnum(value, &iopts, I_ATTR);
+ getnum(value, &opts[OPT_I],
+ I_ATTR);
break;
case I_PROJID32BIT:
sb_feat.projid16bit =
- !getnum(value, &iopts,
+ !getnum(value, &opts[OPT_I],
I_PROJID32BIT);
break;
case I_SPINODES:
sb_feat.spinodes = getnum(value,
- &iopts, I_SPINODES);
+ &opts[OPT_I],
+ I_SPINODES);
break;
default:
unknown('i', value);
@@ -1710,63 +1739,70 @@ main(
case 'l':
p = optarg;
while (*p != '\0') {
- char **subopts = (char **)lopts.subopts;
+ char **subopts =
+ (char **)opts[OPT_L].subopts;
char *value;
switch (getsubopt(&p, subopts, &value)) {
case L_AGNUM:
- logagno = getnum(value, &lopts, L_AGNUM);
+ logagno = getnum(value, &opts[OPT_L],
+ L_AGNUM);
laflag = 1;
break;
case L_FILE:
- xi.lisfile = getnum(value, &lopts,
- L_FILE);
+ xi.lisfile = getnum(value,
+ &opts[OPT_L], L_FILE);
break;
case L_INTERNAL:
- loginternal = getnum(value, &lopts,
- L_INTERNAL);
+ loginternal = getnum(value,
+ &opts[OPT_L], L_INTERNAL);
liflag = 1;
break;
case L_SU:
- lsu = getnum(value, &lopts, L_SU);
+ lsu = getnum(value, &opts[OPT_L],
+ L_SU);
lsuflag = 1;
break;
case L_SUNIT:
- lsunit = getnum(value, &lopts, L_SUNIT);
+ lsunit = getnum(value, &opts[OPT_L],
+ L_SUNIT);
lsunitflag = 1;
break;
case L_NAME:
case L_DEV:
- logfile = getstr(value, &lopts, L_NAME);
+ logfile = getstr(value, &opts[OPT_L],
+ L_NAME);
xi.logname = logfile;
ldflag = 1;
loginternal = 0;
break;
case L_VERSION:
sb_feat.log_version =
- getnum(value, &lopts, L_VERSION);
+ getnum(value, &opts[OPT_L],
+ L_VERSION);
lvflag = 1;
break;
case L_SIZE:
- logbytes = getnum(value, &lopts, L_SIZE);
+ logbytes = getnum(value,
+ &opts[OPT_L], L_SIZE);
break;
case L_SECTLOG:
- lsectorlog = getnum(value, &lopts,
- L_SECTLOG);
+ lsectorlog = getnum(value,
+ &opts[OPT_L], L_SECTLOG);
lsectorsize = 1 << lsectorlog;
lslflag = 1;
break;
case L_SECTSIZE:
- lsectorsize = getnum(value, &lopts,
- L_SECTSIZE);
+ lsectorsize = getnum(value,
+ &opts[OPT_L], L_SECTSIZE);
lsectorlog =
libxfs_highbit32(lsectorsize);
lssflag = 1;
break;
case L_LAZYSBCNTR:
sb_feat.lazy_sb_counters =
- getnum(value, &lopts,
- L_LAZYSBCNTR);
+ getnum(value, &opts[OPT_L],
+ L_LAZYSBCNTR);
break;
default:
unknown('l', value);
@@ -1781,19 +1817,21 @@ main(
case 'm':
p = optarg;
while (*p != '\0') {
- char **subopts = (char **)mopts.subopts;
+ char **subopts =
+ (char **)opts[OPT_M].subopts;
char *value;
switch (getsubopt(&p, subopts, &value)) {
case M_CRC:
sb_feat.crcs_enabled =
- getnum(value, &mopts, M_CRC);
+ getnum(value, &opts[OPT_M],
+ M_CRC);
if (sb_feat.crcs_enabled)
sb_feat.dirftype = true;
break;
case M_FINOBT:
sb_feat.finobt = getnum(
- value, &mopts, M_FINOBT);
+ value, &opts[OPT_M], M_FINOBT);
break;
case M_UUID:
if (!value || *value == '\0')
@@ -1803,11 +1841,12 @@ main(
break;
case M_RMAPBT:
sb_feat.rmapbt = getnum(
- value, &mopts, M_RMAPBT);
+ value, &opts[OPT_M], M_RMAPBT);
break;
case M_REFLINK:
- sb_feat.reflink = getnum(
- value, &mopts, M_REFLINK);
+ sb_feat.reflink =
+ getnum(value, &opts[OPT_M],
+ M_REFLINK);
break;
default:
unknown('m', value);
@@ -1817,38 +1856,41 @@ main(
case 'n':
p = optarg;
while (*p != '\0') {
- char **subopts = (char **)nopts.subopts;
+ char **subopts =
+ (char **)opts[OPT_N].subopts;
char *value;
switch (getsubopt(&p, subopts, &value)) {
case N_LOG:
- dirblocklog = getnum(value, &nopts,
- N_LOG);
+ dirblocklog = getnum(value,
+ &opts[OPT_N], N_LOG);
dirblocksize = 1 << dirblocklog;
nlflag = 1;
break;
case N_SIZE:
- dirblocksize = getnum(value, &nopts,
- N_SIZE);
+ dirblocksize = getnum(value,
+ &opts[OPT_N], N_SIZE);
dirblocklog =
libxfs_highbit32(dirblocksize);
nsflag = 1;
break;
case N_VERSION:
- value = getstr(value, &nopts, N_VERSION);
+ value = getstr(value, &opts[OPT_N],
+ N_VERSION);
if (!strcasecmp(value, "ci")) {
/* ASCII CI mode */
sb_feat.nci = true;
} else {
sb_feat.dir_version =
- getnum(value, &nopts,
+ getnum(value,
+ &opts[OPT_N],
N_VERSION);
}
nvflag = 1;
break;
case N_FTYPE:
- sb_feat.dirftype = getnum(value, &nopts,
- N_FTYPE);
+ sb_feat.dirftype = getnum(value,
+ &opts[OPT_N], N_FTYPE);
break;
default:
unknown('n', value);
@@ -1872,27 +1914,30 @@ main(
case 'r':
p = optarg;
while (*p != '\0') {
- char **subopts = (char **)ropts.subopts;
+ char **subopts =
+ (char **)opts[OPT_R].subopts;
char *value;
switch (getsubopt(&p, subopts, &value)) {
case R_EXTSIZE:
- rtextbytes = getnum(value, &ropts, R_EXTSIZE);
+ rtextbytes = getnum(value,
+ &opts[OPT_R], R_EXTSIZE);
break;
case R_FILE:
- xi.risfile = getnum(value, &ropts,
- R_FILE);
+ xi.risfile = getnum(value,
+ &opts[OPT_R], R_FILE);
break;
case R_NAME:
case R_DEV:
- xi.rtname = getstr(value, &ropts,
+ xi.rtname = getstr(value, &opts[OPT_R],
R_NAME);
break;
case R_SIZE:
- rtbytes = getnum(value, &ropts, R_SIZE);
+ rtbytes = getnum(value, &opts[OPT_R],
+ R_SIZE);
break;
case R_NOALIGN:
- norsflag = getnum(value, &ropts,
+ norsflag = getnum(value, &opts[OPT_R],
R_NOALIGN);
break;
default:
@@ -1903,7 +1948,8 @@ main(
case 's':
p = optarg;
while (*p != '\0') {
- char **subopts = (char **)sopts.subopts;
+ char **subopts =
+ (char **)opts[OPT_S].subopts;
char *value;
switch (getsubopt(&p, subopts, &value)) {
@@ -1911,8 +1957,9 @@ main(
case S_SECTLOG:
if (lssflag)
conflict('s', subopts,
- S_SECTSIZE, S_SECTLOG);
- sectorlog = getnum(value, &sopts,
+ S_SECTSIZE,
+ S_SECTLOG);
+ sectorlog = getnum(value, &opts[OPT_S],
S_SECTLOG);
lsectorlog = sectorlog;
sectorsize = 1 << sectorlog;
@@ -1922,10 +1969,11 @@ main(
case S_SIZE:
case S_SECTSIZE:
if (lslflag)
- conflict('s', subopts, S_SECTLOG,
+ conflict('s', subopts,
+ S_SECTLOG,
S_SECTSIZE);
- sectorsize = getnum(value, &sopts,
- S_SECTSIZE);
+ sectorsize = getnum(value,
+ &opts[OPT_S], S_SECTSIZE);
lsectorsize = sectorsize;
sectorlog =
libxfs_highbit32(sectorsize);
@@ -1948,7 +1996,8 @@ main(
fprintf(stderr, _("extra arguments\n"));
usage();
} else if (argc - optind == 1) {
- dfile = xi.volname = getstr(argv[optind], &dopts, D_NAME);
+ dfile = xi.volname = getstr(argv[optind],
+ &opts[OPT_D], D_NAME);
} else
dfile = xi.dname;
@@ -2127,7 +2176,8 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
* then issue an error.
* The same is also for sparse inodes.
*/
- if (sb_feat.finobt && mopts.subopt_params[M_FINOBT].seen) {
+ if (sb_feat.finobt &&
+ opts[OPT_M].subopt_params[M_FINOBT].seen) {
fprintf(stderr,
_("finobt not supported without CRC support\n"));
usage();
@@ -2372,7 +2422,7 @@ _("rmapbt not supported with realtime devices\n"));
fprintf(stderr,
_("size %s specified for data subvolume is too large, "
"maximum is %lld blocks\n"),
- get_conf_raw(&dopts, D_SIZE),
+ get_conf_raw(OPT_D, D_SIZE),
(long long)DTOBT(xi.dsize));
usage();
} else if (!dbytes && xi.dsize > 0)
@@ -2421,7 +2471,7 @@ reported by the device (%u).\n"),
fprintf(stderr,
_("size %s specified for rt subvolume is too large, "
"maximum is %lld blocks\n"),
- get_conf_raw(&ropts, R_SIZE),
+ get_conf_raw(OPT_R, R_SIZE),
(long long)DTOBT(xi.rtsize));
usage();
} else if (!rtbytes && xi.rtsize > 0)
@@ -2637,7 +2687,7 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
if (logbytes && xi.logBBsize > 0 && logblocks > DTOBT(xi.logBBsize)) {
fprintf(stderr,
_("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
- get_conf_raw(&lopts, L_SIZE),
+ get_conf_raw(OPT_L, L_SIZE),
(long long)DTOBT(xi.logBBsize));
usage();
} else if (!logbytes && xi.logBBsize > 0) {
--
2.11.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 5/7] mkfs: move getnum within the file
2017-07-20 9:29 [PATCH 0/7] mkfs: save user input into opts table Jan Tulak
` (3 preceding siblings ...)
2017-07-20 9:29 ` [PATCH 4/7] mkfs: merge tables for opts parsing into one table Jan Tulak
@ 2017-07-20 9:29 ` Jan Tulak
2017-07-26 23:27 ` Eric Sandeen
2017-07-20 9:29 ` [PATCH 6/7] mkfs: extend opt_params with a value field Jan Tulak
2017-07-20 9:29 ` [PATCH 7/7] mkfs: save user input values into opts Jan Tulak
6 siblings, 1 reply; 39+ messages in thread
From: Jan Tulak @ 2017-07-20 9:29 UTC (permalink / raw)
To: linux-xfs; +Cc: Jan Tulak
Move getnum, check_opt and illegal_option within the file. We have to do
this to avoid dependency issues with the following patch. There is no
other change in this patch, just cut&paste of these functions.
Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
mkfs/xfs_mkfs.c | 236 ++++++++++++++++++++++++++++----------------------------
1 file changed, 118 insertions(+), 118 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index f10cb15a..9d2db2a2 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -761,6 +761,124 @@ get_conf_raw(int opt, int subopt)
return opts[opt].subopt_params[subopt].raw_input;
}
+static __attribute__((noreturn)) void
+illegal_option(
+ const char *value,
+ struct opt_params *opts,
+ int index,
+ const char *reason)
+{
+ fprintf(stderr,
+ _("Illegal value %s for -%c %s option. %s\n"),
+ value, opts->name, opts->subopts[index],
+ reason ? reason : "");
+ usage();
+}
+
+/*
+ * Check for conflicts and option respecification.
+ */
+static void
+check_opt(
+ struct opt_params *opts,
+ int index,
+ bool str_seen)
+{
+ struct subopt_param *sp = &opts->subopt_params[index];
+ int i;
+
+ 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);
+ }
+
+ /*
+ * 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 (i = 0; i < MAX_CONFLICTS; i++) {
+ int conflict_opt = sp->conflicts[i];
+
+ if (conflict_opt == LAST_CONFLICT)
+ break;
+ if (opts->subopt_params[conflict_opt].seen ||
+ opts->subopt_params[conflict_opt].str_seen)
+ conflict(opts->name, (char **)opts->subopts,
+ conflict_opt, 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(opts, index, false);
+ set_conf_raw(opts->index, index, str);
+ /* empty strings might just return a default value */
+ if (!str || *str == '\0') {
+ if (sp->flagval == SUBOPT_NEEDS_VAL)
+ reqval(opts->name, (char **)opts->subopts, index);
+ return sp->flagval;
+ }
+
+ if (sp->minval == 0 && sp->maxval == 0) {
+ fprintf(stderr,
+ _("Option -%c %s has undefined minval/maxval."
+ "Can't verify value range. This is a bug.\n"),
+ opts->name, opts->subopts[index]);
+ exit(1);
+ }
+
+ /*
+ * 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 *str_end;
+
+ c = strtoll(str, &str_end, 0);
+ if (c == 0 && str_end == str)
+ illegal_option(str, opts, index, NULL);
+ if (*str_end != '\0')
+ illegal_option(str, opts, index, NULL);
+ }
+
+ /* Validity check the result. */
+ if (c < sp->minval)
+ illegal_option(str, opts, index, _("value is too small"));
+ else if (c > sp->maxval)
+ illegal_option(str, opts, index, _("value is too large"));
+ if (sp->is_power_2 && !ispow2(c))
+ illegal_option(str, opts, index, _("value must be a power of 2"));
+ return c;
+}
+
/*
* Convert lsu to lsunit for 512 bytes blocks and check validity of the values.
*/
@@ -1285,124 +1403,6 @@ sb_set_features(
}
-static __attribute__((noreturn)) void
-illegal_option(
- const char *value,
- struct opt_params *opts,
- int index,
- const char *reason)
-{
- fprintf(stderr,
- _("Illegal value %s for -%c %s option. %s\n"),
- value, opts->name, opts->subopts[index],
- reason ? reason : "");
- usage();
-}
-
-/*
- * Check for conflicts and option respecification.
- */
-static void
-check_opt(
- struct opt_params *opts,
- int index,
- bool str_seen)
-{
- struct subopt_param *sp = &opts->subopt_params[index];
- int i;
-
- 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);
- }
-
- /*
- * 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 (i = 0; i < MAX_CONFLICTS; i++) {
- int conflict_opt = sp->conflicts[i];
-
- if (conflict_opt == LAST_CONFLICT)
- break;
- if (opts->subopt_params[conflict_opt].seen ||
- opts->subopt_params[conflict_opt].str_seen)
- conflict(opts->name, (char **)opts->subopts,
- conflict_opt, 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(opts, index, false);
- set_conf_raw(opts->index, index, str);
- /* empty strings might just return a default value */
- if (!str || *str == '\0') {
- if (sp->flagval == SUBOPT_NEEDS_VAL)
- reqval(opts->name, (char **)opts->subopts, index);
- return sp->flagval;
- }
-
- if (sp->minval == 0 && sp->maxval == 0) {
- fprintf(stderr,
- _("Option -%c %s has undefined minval/maxval."
- "Can't verify value range. This is a bug.\n"),
- opts->name, opts->subopts[index]);
- exit(1);
- }
-
- /*
- * 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 *str_end;
-
- c = strtoll(str, &str_end, 0);
- if (c == 0 && str_end == str)
- illegal_option(str, opts, index, NULL);
- if (*str_end != '\0')
- illegal_option(str, opts, index, NULL);
- }
-
- /* Validity check the result. */
- if (c < sp->minval)
- illegal_option(str, opts, index, _("value is too small"));
- else if (c > sp->maxval)
- illegal_option(str, opts, index, _("value is too large"));
- if (sp->is_power_2 && !ispow2(c))
- illegal_option(str, opts, index, _("value must be a power of 2"));
- 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
--
2.11.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 6/7] mkfs: extend opt_params with a value field
2017-07-20 9:29 [PATCH 0/7] mkfs: save user input into opts table Jan Tulak
` (4 preceding siblings ...)
2017-07-20 9:29 ` [PATCH 5/7] mkfs: move getnum within the file Jan Tulak
@ 2017-07-20 9:29 ` Jan Tulak
2017-07-27 16:18 ` Luis R. Rodriguez
2017-07-20 9:29 ` [PATCH 7/7] mkfs: save user input values into opts Jan Tulak
6 siblings, 1 reply; 39+ messages in thread
From: Jan Tulak @ 2017-07-20 9:29 UTC (permalink / raw)
To: linux-xfs; +Cc: Jan Tulak
This patch adds infrastructure that will be used in subsequent patches.
The Value field is the actual value used in computations for creating
the filesystem. This is initialized with sensible default values. If
initialized to 0, the value is considered disabled. User input will
override default values. If the field is a string and not a number, the
value is set to a positive non-zero number when user input has been
supplied.
Add functions that can be used to get/set values to opts table.
Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
Change:
* update the description of commit/in code
* merge with another patch, which adds the get/set functions.
---
mkfs/xfs_mkfs.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 9d2db2a2..e008b5a4 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -117,6 +117,13 @@ unsigned int sectorsize;
* Filled raw string from the user, so we never lose that information e.g.
* to print it back in case of an issue.
*
+ * value OPTIONAL
+ * The actual value used in computations for creating the filesystem.
+ * This is initialized with sensible default values. If initialized to 0,
+ * the value is considered disabled. User input will override default
+ * values. If the field is a string and not a number, the value is set to
+ * a positive non-zero number when user input has been supplied.
+ *
*/
struct opt_params {
int index;
@@ -134,6 +141,7 @@ struct opt_params {
long long maxval;
long long flagval;
const char *raw_input;
+ uint64_t value;
} subopt_params[MAX_SUBOPTS];
} opts[MAX_OPTS] = {
#define OPT_B 0
@@ -749,6 +757,23 @@ struct opt_params {
*/
#define WHACK_SIZE (128 * 1024)
+/*
+ * Get and set values to the opts table.
+ */
+static inline uint64_t
+get_conf_val(int opt, int subopt)
+{
+ return opts[opt].subopt_params[subopt].value;
+}
+
+static void
+set_conf_val(int opt, int subopt, uint64_t val)
+{
+ struct subopt_param *sp = &opts[opt].subopt_params[subopt];
+
+ sp->value = val;
+}
+
static inline void
set_conf_raw(int opt, int subopt, const char *value)
{
@@ -880,6 +905,18 @@ getnum(
}
/*
+ * A wrapper for getnum and set_conf_val.
+ */
+static inline uint64_t
+parse_conf_val(int opt, int subopt, char *value)
+{
+ uint64_t num = getnum(value, &opts[opt], subopt);
+
+ set_conf_val(opt, subopt, num);
+ return num;
+}
+
+/*
* Convert lsu to lsunit for 512 bytes blocks and check validity of the values.
*/
static void
--
2.11.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 7/7] mkfs: save user input values into opts
2017-07-20 9:29 [PATCH 0/7] mkfs: save user input into opts table Jan Tulak
` (5 preceding siblings ...)
2017-07-20 9:29 ` [PATCH 6/7] mkfs: extend opt_params with a value field Jan Tulak
@ 2017-07-20 9:29 ` Jan Tulak
2017-07-26 23:53 ` Eric Sandeen
6 siblings, 1 reply; 39+ messages in thread
From: Jan Tulak @ 2017-07-20 9:29 UTC (permalink / raw)
To: linux-xfs; +Cc: Jan Tulak
Save the parsed values from users into the opts table. This will ensure that
the user values gets there and are validated, but we are not yet using them for
anything - the usage makes a lot of changes through the code, so it is better
if that is separated into smaller chunks.
Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
Edit:
* updated description
---
mkfs/xfs_mkfs.c | 260 +++++++++++++++++++++++++++++++++++---------------------
1 file changed, 165 insertions(+), 95 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index e008b5a4..bf480efe 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1616,16 +1616,19 @@ main(;
switch (getsubopt(&p, subopts, &value)) {
case B_LOG:
- blocklog = getnum(value, &opts[OPT_B],
- B_LOG);
+ blocklog = parse_conf_val(OPT_B, B_LOG,
+ value);
blocksize = 1 << blocklog;
blflag = 1;
+ set_conf_val(OPT_B, B_SIZE, blocksize);
break;
case B_SIZE:
- blocksize = getnum(value, &opts[OPT_B],
- B_SIZE);
+ blocksize = parse_conf_val(OPT_B,
+ B_SIZE,
+ value);
blocklog = libxfs_highbit32(blocksize);
bsflag = 1;
+ set_conf_val(OPT_B, B_LOG, blocklog);
break;
default:
unknown('b', value);
@@ -1641,76 +1644,92 @@ main(
switch (getsubopt(&p, subopts, &value)) {
case D_AGCOUNT:
- agcount = getnum(value, &opts[OPT_D],
- D_AGCOUNT);
+ agcount = parse_conf_val(OPT_D,
+ D_AGCOUNT,
+ value);
daflag = 1;
break;
case D_AGSIZE:
- agsize = getnum(value, &opts[OPT_D],
- D_AGSIZE);
+ agsize = parse_conf_val(OPT_D,
+ D_AGSIZE,
+ value);
dasize = 1;
break;
case D_FILE:
- xi.disfile = getnum(value,
- &opts[OPT_D], D_FILE);
+ xi.disfile = parse_conf_val(OPT_D,
+ D_FILE,
+ value);
break;
case D_NAME:
xi.dname = getstr(value, &opts[OPT_D],
D_NAME);
+ set_conf_val(OPT_D, D_NAME, 1);
break;
case D_SIZE:
- dbytes = getnum(value, &opts[OPT_D],
- D_SIZE);
+ dbytes = parse_conf_val(OPT_D, D_SIZE,
+ value);
break;
case D_SUNIT:
- dsunit = getnum(value, &opts[OPT_D],
- D_SUNIT);
+ dsunit = parse_conf_val(OPT_D, D_SUNIT,
+ value);
break;
case D_SWIDTH:
- dswidth = getnum(value, &opts[OPT_D],
- D_SWIDTH);
+ dswidth = parse_conf_val(OPT_D,
+ D_SWIDTH,
+ value);
break;
case D_SU:
- dsu = getnum(value, &opts[OPT_D],
- D_SU);
+ dsu = parse_conf_val(OPT_D, D_SU,
+ value);
break;
case D_SW:
- dsw = getnum(value, &opts[OPT_D],
- D_SW);
+ dsw = parse_conf_val(OPT_D, D_SW,
+ value);
break;
case D_NOALIGN:
- nodsflag = getnum(value, &opts[OPT_D],
- D_NOALIGN);
+ nodsflag = parse_conf_val(OPT_D,
+ D_NOALIGN,
+ value);
break;
case D_SECTLOG:
- sectorlog = getnum(value, &opts[OPT_D],
- D_SECTLOG);
+ sectorlog = parse_conf_val(OPT_D,
+ D_SECTLOG,
+ value);
sectorsize = 1 << sectorlog;
slflag = 1;
+ set_conf_val(OPT_D, D_SECTSIZE,
+ sectorsize);
break;
case D_SECTSIZE:
- sectorsize = getnum(value,
- &opts[OPT_D], D_SECTSIZE);
+ sectorsize = parse_conf_val(OPT_D,
+ D_SECTSIZE,
+ value);
sectorlog =
libxfs_highbit32(sectorsize);
ssflag = 1;
+ set_conf_val(OPT_D, D_SECTLOG,
+ sectorlog);
break;
case D_RTINHERIT:
- c = getnum(value, &opts[OPT_D],
- D_RTINHERIT);
+ c = parse_conf_val(OPT_D, D_RTINHERIT,
+ value);
if (c)
fsx.fsx_xflags |=
XFS_DIFLAG_RTINHERIT;
break;
case D_PROJINHERIT:
- fsx.fsx_projid = getnum(value,
- &opts[OPT_D], D_PROJINHERIT);
+ fsx.fsx_projid =
+ parse_conf_val(OPT_D,
+ D_PROJINHERIT,
+ value);
fsx.fsx_xflags |=
XFS_DIFLAG_PROJINHERIT;
break;
case D_EXTSZINHERIT:
- fsx.fsx_extsize = getnum(value,
- &opts[OPT_D], D_EXTSZINHERIT);
+ fsx.fsx_extsize =
+ parse_conf_val(OPT_D,
+ D_EXTSZINHERIT,
+ value);
fsx.fsx_xflags |=
XFS_DIFLAG_EXTSZINHERIT;
break;
@@ -1728,45 +1747,53 @@ main(
switch (getsubopt(&p, subopts, &value)) {
case I_ALIGN:
- sb_feat.inode_align = getnum(value,
- &opts[OPT_I], I_ALIGN);
+ sb_feat.inode_align =
+ parse_conf_val(OPT_I, I_ALIGN,
+ value);
break;
case I_LOG:
- inodelog = getnum(value, &opts[OPT_I],
- I_LOG);
+ inodelog = parse_conf_val(OPT_I, I_LOG,
+ value);
isize = 1 << inodelog;
ilflag = 1;
+ set_conf_val(OPT_I, I_SIZE, isize);
break;
case I_MAXPCT:
- imaxpct = getnum(value, &opts[OPT_I],
- I_MAXPCT);
+ imaxpct = parse_conf_val(OPT_I,
+ I_MAXPCT,
+ value);
imflag = 1;
break;
case I_PERBLOCK:
- inopblock = getnum(value, &opts[OPT_I],
- I_PERBLOCK);
+ inopblock = parse_conf_val(OPT_I,
+ I_PERBLOCK,
+ value);
ipflag = 1;
break;
case I_SIZE:
- isize = getnum(value, &opts[OPT_I],
- I_SIZE);
+ isize = parse_conf_val(OPT_I, I_SIZE,
+ value);
inodelog = libxfs_highbit32(isize);
isflag = 1;
+ set_conf_val(OPT_I, I_LOG, inodelog);
break;
case I_ATTR:
sb_feat.attr_version =
- getnum(value, &opts[OPT_I],
- I_ATTR);
+ parse_conf_val(OPT_I, I_ATTR,
+ value);
break;
case I_PROJID32BIT:
sb_feat.projid16bit =
!getnum(value, &opts[OPT_I],
I_PROJID32BIT);
+ set_conf_val(OPT_I, I_PROJID32BIT,
+ sb_feat.projid16bit);
break;
case I_SPINODES:
- sb_feat.spinodes = getnum(value,
- &opts[OPT_I],
- I_SPINODES);
+ sb_feat.spinodes =
+ parse_conf_val(OPT_I,
+ I_SPINODES,
+ value);
break;
default:
unknown('i', value);
@@ -1782,27 +1809,31 @@ main(
switch (getsubopt(&p, subopts, &value)) {
case L_AGNUM:
- logagno = getnum(value, &opts[OPT_L],
- L_AGNUM);
+ logagno = parse_conf_val(OPT_L,
+ L_AGNUM,
+ value);
laflag = 1;
break;
case L_FILE:
- xi.lisfile = getnum(value,
- &opts[OPT_L], L_FILE);
+ xi.lisfile = parse_conf_val(OPT_L,
+ L_FILE,
+ value);
break;
case L_INTERNAL:
- loginternal = getnum(value,
- &opts[OPT_L], L_INTERNAL);
+ loginternal =
+ parse_conf_val(OPT_L,
+ L_INTERNAL,
+ value);
liflag = 1;
break;
case L_SU:
- lsu = getnum(value, &opts[OPT_L],
- L_SU);
+ lsu = parse_conf_val(OPT_L, L_SU,
+ value);
lsuflag = 1;
break;
case L_SUNIT:
- lsunit = getnum(value, &opts[OPT_L],
- L_SUNIT);
+ lsunit = parse_conf_val(OPT_L, L_SUNIT,
+ value);
lsunitflag = 1;
break;
case L_NAME:
@@ -1812,34 +1843,48 @@ main(
xi.logname = logfile;
ldflag = 1;
loginternal = 0;
+ set_conf_val(OPT_L, L_NAME, 1);
+ set_conf_val(OPT_L, L_DEV, 1);
break;
case L_VERSION:
sb_feat.log_version =
- getnum(value, &opts[OPT_L],
- L_VERSION);
+ parse_conf_val(OPT_L,
+ L_VERSION,
+ value);
lvflag = 1;
+ set_conf_val(OPT_L, L_VERSION,
+ sb_feat.log_version);
break;
case L_SIZE:
- logbytes = getnum(value,
- &opts[OPT_L], L_SIZE);
+ logbytes = parse_conf_val(OPT_L,
+ L_SIZE,
+ value);
break;
case L_SECTLOG:
- lsectorlog = getnum(value,
- &opts[OPT_L], L_SECTLOG);
+ lsectorlog = parse_conf_val(OPT_L,
+ L_SECTLOG,
+ value);
lsectorsize = 1 << lsectorlog;
lslflag = 1;
+ set_conf_val(OPT_L, L_SECTSIZE,
+ lsectorsize);
break;
case L_SECTSIZE:
- lsectorsize = getnum(value,
- &opts[OPT_L], L_SECTSIZE);
+ lsectorsize =
+ parse_conf_val(OPT_L,
+ L_SECTSIZE,
+ value);
lsectorlog =
libxfs_highbit32(lsectorsize);
lssflag = 1;
+ set_conf_val(OPT_L, L_SECTLOG,
+ lsectorlog);
break;
case L_LAZYSBCNTR:
sb_feat.lazy_sb_counters =
- getnum(value, &opts[OPT_L],
- L_LAZYSBCNTR);
+ parse_conf_val(OPT_L,
+ L_LAZYSBCNTR,
+ value);
break;
default:
unknown('l', value);
@@ -1861,29 +1906,35 @@ main(
switch (getsubopt(&p, subopts, &value)) {
case M_CRC:
sb_feat.crcs_enabled =
- getnum(value, &opts[OPT_M],
- M_CRC);
+ parse_conf_val(OPT_M, M_CRC,
+ value);
if (sb_feat.crcs_enabled)
sb_feat.dirftype = true;
break;
case M_FINOBT:
- sb_feat.finobt = getnum(
- value, &opts[OPT_M], M_FINOBT);
+ sb_feat.finobt =
+ parse_conf_val(OPT_M, M_FINOBT,
+ value);
+ set_conf_val(OPT_M, M_FINOBT,
+ sb_feat.finobt);
break;
case M_UUID:
if (!value || *value == '\0')
reqval('m', subopts, M_UUID);
if (platform_uuid_parse(value, &uuid))
illegal(optarg, "m uuid");
+ set_conf_val(OPT_M, M_UUID, 1);
break;
case M_RMAPBT:
- sb_feat.rmapbt = getnum(
- value, &opts[OPT_M], M_RMAPBT);
+ sb_feat.rmapbt =
+ parse_conf_val(OPT_M, M_RMAPBT,
+ value);
break;
case M_REFLINK:
sb_feat.reflink =
- getnum(value, &opts[OPT_M],
- M_REFLINK);
+ parse_conf_val(OPT_M,
+ M_REFLINK,
+ value);
break;
default:
unknown('m', value);
@@ -1899,17 +1950,23 @@ main(
switch (getsubopt(&p, subopts, &value)) {
case N_LOG:
- dirblocklog = getnum(value,
- &opts[OPT_N], N_LOG);
+ dirblocklog = parse_conf_val(OPT_N,
+ N_LOG,
+ value);
dirblocksize = 1 << dirblocklog;
nlflag = 1;
+ set_conf_val(OPT_N, N_SIZE,
+ dirblocksize);
break;
case N_SIZE:
- dirblocksize = getnum(value,
- &opts[OPT_N], N_SIZE);
+ dirblocksize = parse_conf_val(OPT_N,
+ N_SIZE,
+ value);
dirblocklog =
libxfs_highbit32(dirblocksize);
nsflag = 1;
+ set_conf_val(OPT_N, N_LOG,
+ dirblocklog);
break;
case N_VERSION:
value = getstr(value, &opts[OPT_N],
@@ -1920,14 +1977,17 @@ main(
} else {
sb_feat.dir_version =
getnum(value,
- &opts[OPT_N],
- N_VERSION);
+ &opts[OPT_N],
+ N_VERSION);
}
nvflag = 1;
+ set_conf_val(OPT_N, N_VERSION,
+ sb_feat.dir_version);
break;
case N_FTYPE:
- sb_feat.dirftype = getnum(value,
- &opts[OPT_N], N_FTYPE);
+ sb_feat.dirftype =
+ parse_conf_val(OPT_N, N_FTYPE,
+ value);
break;
default:
unknown('n', value);
@@ -1957,25 +2017,30 @@ main(
switch (getsubopt(&p, subopts, &value)) {
case R_EXTSIZE:
- rtextbytes = getnum(value,
- &opts[OPT_R], R_EXTSIZE);
+ rtextbytes = parse_conf_val(OPT_R,
+ R_EXTSIZE,
+ value);
break;
case R_FILE:
- xi.risfile = getnum(value,
- &opts[OPT_R], R_FILE);
+ xi.risfile = parse_conf_val(OPT_R,
+ R_FILE,
+ value);
break;
case R_NAME:
case R_DEV:
xi.rtname = getstr(value, &opts[OPT_R],
R_NAME);
+ set_conf_val(OPT_R, R_NAME, 1);
+ set_conf_val(OPT_R, R_DEV, 1);
break;
case R_SIZE:
- rtbytes = getnum(value, &opts[OPT_R],
- R_SIZE);
+ rtbytes = parse_conf_val(OPT_R, R_SIZE,
+ value);
break;
case R_NOALIGN:
- norsflag = getnum(value, &opts[OPT_R],
- R_NOALIGN);
+ norsflag = parse_conf_val(OPT_R,
+ R_NOALIGN,
+ value);
break;
default:
unknown('r', value);
@@ -1996,12 +2061,14 @@ main(
conflict('s', subopts,
S_SECTSIZE,
S_SECTLOG);
- sectorlog = getnum(value, &opts[OPT_S],
- S_SECTLOG);
+ sectorlog = parse_conf_val(OPT_S,
+ S_SECTLOG,
+ value);
lsectorlog = sectorlog;
sectorsize = 1 << sectorlog;
lsectorsize = sectorsize;
lslflag = slflag = 1;
+ set_conf_val(OPT_S, S_LOG, sectorsize);
break;
case S_SIZE:
case S_SECTSIZE:
@@ -2009,13 +2076,16 @@ main(
conflict('s', subopts,
S_SECTLOG,
S_SECTSIZE);
- sectorsize = getnum(value,
- &opts[OPT_S], S_SECTSIZE);
+ sectorsize = parse_conf_val(OPT_S,
+ S_SECTSIZE,
+ value);
lsectorsize = sectorsize;
sectorlog =
libxfs_highbit32(sectorsize);
lsectorlog = sectorlog;
lssflag = ssflag = 1;
+ set_conf_val(OPT_S,
+ S_SIZE, sectorlog);
break;
default:
unknown('s', value);
--
2.11.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH 3/7] mkfs: remove intermediate getstr followed by getnum
2017-07-20 9:29 ` [PATCH 3/7] mkfs: remove intermediate getstr followed by getnum Jan Tulak
@ 2017-07-20 15:54 ` Darrick J. Wong
2017-07-21 8:56 ` Jan Tulak
2017-07-26 20:49 ` Eric Sandeen
2017-07-21 12:24 ` [PATCH 3/7 v2] " Jan Tulak
1 sibling, 2 replies; 39+ messages in thread
From: Darrick J. Wong @ 2017-07-20 15:54 UTC (permalink / raw)
To: Jan Tulak; +Cc: linux-xfs
On Thu, Jul 20, 2017 at 11:29:28AM +0200, Jan Tulak wrote:
> Some options loaded a number as a string with getstr and converted it to
> number with getnum later in the code, without any reason for this
> approach. (They were probably forgotten in some past cleaning.)
>
> This patch changes them to skip the string and use getnum directly in
> the main option-parsing loop, as do all the other numerical options.
> And as we now have two variables of the same type for the same value,
> merge them together. (e.g. former string dsize and number dbytes).
>
> Signed-off-by: Jan Tulak <jtulak@redhat.com>
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> Reviewed-by: Luis R. Rodriguez <mcgrof@kernel.org>
>
> ---
> In reply to Eric's comment, so it doesn't pop up again:
> This patch has to be applied after "mkfs: Save raw user input ...",
> because otherwise we would temporary lose the access to strings
> with user-given values and thus wouldn't be able to report them in
> case of any issue.
> ---
> mkfs/xfs_mkfs.c | 90 ++++++++++++++++++++++++++-------------------------------
> 1 file changed, 41 insertions(+), 49 deletions(-)
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index f2f6468e..127f14c3 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -1345,6 +1345,7 @@ getnum(
> long long c;
>
> check_opt(opts, index, false);
> + set_conf_raw(opts, index, str);
> /* empty strings might just return a default value */
> if (!str || *str == '\0') {
> if (sp->flagval == SUBOPT_NEEDS_VAL)
> @@ -1432,12 +1433,12 @@ main(
> char *dfile;
> int dirblocklog;
> int dirblocksize;
> - char *dsize;
> + int dbytes;
> int dsu;
> int dsw;
> int dsunit;
> int dswidth;
> - int force_overwrite;
> + bool force_overwrite;
> struct fsxattr fsx;
> int ilflag;
> int imaxpct;
> @@ -1456,7 +1457,7 @@ main(
> xfs_rfsblock_t logblocks;
> char *logfile;
> int loginternal;
> - char *logsize;
> + int logbytes;
> xfs_fsblock_t logstart;
> int lvflag;
> int lsflag;
> @@ -1485,11 +1486,11 @@ main(
> char *protostring;
> int qflag;
> xfs_rfsblock_t rtblocks;
> + int rtbytes;
> xfs_extlen_t rtextblocks;
> xfs_rtblock_t rtextents;
> - char *rtextsize;
> + int rtextbytes;
> char *rtfile;
> - char *rtsize;
> xfs_sb_t *sbp;
> int sectorlog;
> __uint64_t sector_mask;
> @@ -1537,7 +1538,8 @@ main(
> qflag = 0;
> imaxpct = inodelog = inopblock = isize = 0;
> dfile = logfile = rtfile = NULL;
> - dsize = logsize = rtsize = rtextsize = protofile = NULL;
> + protofile = NULL;
> + rtbytes = rtextbytes = logbytes = dbytes = 0;
> dsu = dsw = dsunit = dswidth = lalign = lsu = lsunit = 0;
> nodsflag = norsflag = 0;
> force_overwrite = 0;
> @@ -1601,7 +1603,7 @@ main(
> xi.dname = getstr(value, &dopts, D_NAME);
> break;
> case D_SIZE:
> - dsize = getstr(value, &dopts, D_SIZE);
> + dbytes = getnum(value, &dopts, D_SIZE);
> break;
> case D_SUNIT:
> dsunit = getnum(value, &dopts, D_SUNIT);
> @@ -1746,7 +1748,7 @@ main(
> lvflag = 1;
> break;
> case L_SIZE:
> - logsize = getstr(value, &lopts, L_SIZE);
> + logbytes = getnum(value, &lopts, L_SIZE);
> break;
> case L_SECTLOG:
> lsectorlog = getnum(value, &lopts,
> @@ -1875,8 +1877,7 @@ main(
>
> switch (getsubopt(&p, subopts, &value)) {
> case R_EXTSIZE:
> - rtextsize = getstr(value, &ropts,
> - R_EXTSIZE);
> + rtextbytes = getnum(value, &ropts, R_EXTSIZE);
> break;
> case R_FILE:
> xi.risfile = getnum(value, &ropts,
> @@ -1888,7 +1889,7 @@ main(
> R_NAME);
> break;
> case R_SIZE:
> - rtsize = getstr(value, &ropts, R_SIZE);
> + rtbytes = getnum(value, &ropts, R_SIZE);
> break;
> case R_NOALIGN:
> norsflag = getnum(value, &ropts,
> @@ -1991,14 +1992,14 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
> * sector size mismatches between the new filesystem and the underlying
> * host filesystem.
> */
> - check_device_type(dfile, &xi.disfile, !dsize, !dfile,
> + check_device_type(dfile, &xi.disfile, !dbytes, !dfile,
> Nflag ? NULL : &xi.dcreat, force_overwrite, "d");
> if (!loginternal)
> - check_device_type(xi.logname, &xi.lisfile, !logsize, !xi.logname,
> - Nflag ? NULL : &xi.lcreat,
> + check_device_type(xi.logname, &xi.lisfile, !logbytes,
> + !xi.logname, Nflag ? NULL : &xi.lcreat,
> force_overwrite, "l");
> if (xi.rtname)
> - check_device_type(xi.rtname, &xi.risfile, !rtsize, !xi.rtname,
> + check_device_type(xi.rtname, &xi.risfile, !rtbytes, !xi.rtname,
> Nflag ? NULL : &xi.rcreat,
> force_overwrite, "r");
> if (xi.disfile || xi.lisfile || xi.risfile)
> @@ -2179,10 +2180,7 @@ _("rmapbt not supported with realtime devices\n"));
> }
>
>
> - if (dsize) {
> - __uint64_t dbytes;
> -
> - dbytes = getnum(dsize, &dopts, D_SIZE);
> + if (dbytes) {
Why has dbytes been demoted from uint64_t to int? This eliminates the
ability to -d size=8G, right?
> if (dbytes % XFS_MIN_BLOCKSIZE) {
> fprintf(stderr,
> _("illegal data length %lld, not a multiple of %d\n"),
> @@ -2211,10 +2209,7 @@ _("rmapbt not supported with realtime devices\n"));
> usage();
> }
>
> - if (logsize) {
> - __uint64_t logbytes;
> -
> - logbytes = getnum(logsize, &lopts, L_SIZE);
> + if (logbytes) {
> if (logbytes % XFS_MIN_BLOCKSIZE) {
> fprintf(stderr,
> _("illegal log length %lld, not a multiple of %d\n"),
> @@ -2228,10 +2223,7 @@ _("rmapbt not supported with realtime devices\n"));
> (long long)logbytes, blocksize,
> (long long)(logblocks << blocklog));
> }
> - if (rtsize) {
> - __uint64_t rtbytes;
> -
> - rtbytes = getnum(rtsize, &ropts, R_SIZE);
> + if (rtbytes) {
Same complaint here.
--D
> if (rtbytes % XFS_MIN_BLOCKSIZE) {
> fprintf(stderr,
> _("illegal rt length %lld, not a multiple of %d\n"),
> @@ -2248,10 +2240,7 @@ _("rmapbt not supported with realtime devices\n"));
> /*
> * If specified, check rt extent size against its constraints.
> */
> - if (rtextsize) {
> - __uint64_t rtextbytes;
> -
> - rtextbytes = getnum(rtextsize, &ropts, R_EXTSIZE);
> + if (rtextbytes) {
> if (rtextbytes % blocksize) {
> fprintf(stderr,
> _("illegal rt extent size %lld, not a multiple of %d\n"),
> @@ -2268,7 +2257,7 @@ _("rmapbt not supported with realtime devices\n"));
> __uint64_t rswidth;
> __uint64_t rtextbytes;
>
> - if (!norsflag && !xi.risfile && !(!rtsize && xi.disfile))
> + if (!norsflag && !xi.risfile && !(!rtbytes && xi.disfile))
> rswidth = ft.rtswidth;
> else
> rswidth = 0;
> @@ -2379,15 +2368,16 @@ _("rmapbt not supported with realtime devices\n"));
> rtfile = _("volume rt");
> else if (!xi.rtdev)
> rtfile = _("none");
> - if (dsize && xi.dsize > 0 && dblocks > DTOBT(xi.dsize)) {
> + if (dbytes && xi.dsize > 0 && dblocks > DTOBT(xi.dsize)) {
> fprintf(stderr,
> _("size %s specified for data subvolume is too large, "
> "maximum is %lld blocks\n"),
> - dsize, (long long)DTOBT(xi.dsize));
> + get_conf_raw(&dopts, D_SIZE),
> + (long long)DTOBT(xi.dsize));
> usage();
> - } else if (!dsize && xi.dsize > 0)
> + } else if (!dbytes && xi.dsize > 0)
> dblocks = DTOBT(xi.dsize);
> - else if (!dsize) {
> + else if (!dbytes) {
> fprintf(stderr, _("can't get size of data subvolume\n"));
> usage();
> }
> @@ -2420,22 +2410,23 @@ reported by the device (%u).\n"),
> reported by the device (%u).\n"),
> lsectorsize, xi.lbsize);
> }
> - if (rtsize && xi.rtsize > 0 && xi.rtbsize > sectorsize) {
> + if (rtbytes && xi.rtsize > 0 && xi.rtbsize > sectorsize) {
> fprintf(stderr, _(
> "Warning: the realtime subvolume sector size %u is less than the sector size\n\
> reported by the device (%u).\n"),
> sectorsize, xi.rtbsize);
> }
>
> - if (rtsize && xi.rtsize > 0 && rtblocks > DTOBT(xi.rtsize)) {
> + if (rtbytes && xi.rtsize > 0 && rtblocks > DTOBT(xi.rtsize)) {
> fprintf(stderr,
> _("size %s specified for rt subvolume is too large, "
> "maximum is %lld blocks\n"),
> - rtsize, (long long)DTOBT(xi.rtsize));
> + get_conf_raw(&ropts, R_SIZE),
> + (long long)DTOBT(xi.rtsize));
> usage();
> - } else if (!rtsize && xi.rtsize > 0)
> + } else if (!rtbytes && xi.rtsize > 0)
> rtblocks = DTOBT(xi.rtsize);
> - else if (rtsize && !xi.rtdev) {
> + else if (rtbytes && !xi.rtdev) {
> fprintf(stderr,
> _("size specified for non-existent rt subvolume\n"));
> usage();
> @@ -2641,26 +2632,27 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
> sb_feat.inode_align);
> ASSERT(min_logblocks);
> min_logblocks = MAX(XFS_MIN_LOG_BLOCKS, min_logblocks);
> - if (!logsize && dblocks >= (1024*1024*1024) >> blocklog)
> + if (!logbytes && dblocks >= (1024*1024*1024) >> blocklog)
> min_logblocks = MAX(min_logblocks, XFS_MIN_LOG_BYTES>>blocklog);
> - if (logsize && xi.logBBsize > 0 && logblocks > DTOBT(xi.logBBsize)) {
> + if (logbytes && xi.logBBsize > 0 && logblocks > DTOBT(xi.logBBsize)) {
> fprintf(stderr,
> _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
> - logsize, (long long)DTOBT(xi.logBBsize));
> + get_conf_raw(&lopts, L_SIZE),
> + (long long)DTOBT(xi.logBBsize));
> usage();
> - } else if (!logsize && xi.logBBsize > 0) {
> + } else if (!logbytes && xi.logBBsize > 0) {
> logblocks = DTOBT(xi.logBBsize);
> - } else if (logsize && !xi.logdev && !loginternal) {
> + } else if (logbytes && !xi.logdev && !loginternal) {
> fprintf(stderr,
> _("size specified for non-existent log subvolume\n"));
> usage();
> - } else if (loginternal && logsize && logblocks >= dblocks) {
> + } else if (loginternal && logbytes && logblocks >= dblocks) {
> fprintf(stderr, _("size %lld too large for internal log\n"),
> (long long)logblocks);
> usage();
> } else if (!loginternal && !xi.logdev) {
> logblocks = 0;
> - } else if (loginternal && !logsize) {
> + } else if (loginternal && !logbytes) {
>
> if (dblocks < GIGABYTES(1, blocklog)) {
> /* tiny filesystems get minimum sized logs. */
> @@ -2724,7 +2716,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
> * Readjust the log size to fit within an AG if it was sized
> * automatically.
> */
> - if (!logsize) {
> + if (!logbytes) {
> logblocks = MIN(logblocks,
> libxfs_alloc_ag_max_usable(mp));
>
> --
> 2.11.0
>
> --
> 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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/7] mkfs: remove intermediate getstr followed by getnum
2017-07-20 15:54 ` Darrick J. Wong
@ 2017-07-21 8:56 ` Jan Tulak
2017-07-26 20:49 ` Eric Sandeen
1 sibling, 0 replies; 39+ messages in thread
From: Jan Tulak @ 2017-07-21 8:56 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Thu, Jul 20, 2017 at 5:54 PM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> On Thu, Jul 20, 2017 at 11:29:28AM +0200, Jan Tulak wrote:
>> Some options loaded a number as a string with getstr and converted it to
>> number with getnum later in the code, without any reason for this
>> approach. (They were probably forgotten in some past cleaning.)
>>
>> This patch changes them to skip the string and use getnum directly in
>> the main option-parsing loop, as do all the other numerical options.
>> And as we now have two variables of the same type for the same value,
>> merge them together. (e.g. former string dsize and number dbytes).
>>
>> Signed-off-by: Jan Tulak <jtulak@redhat.com>
>> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
>> Reviewed-by: Luis R. Rodriguez <mcgrof@kernel.org>
>>
>> ---
>> In reply to Eric's comment, so it doesn't pop up again:
>> This patch has to be applied after "mkfs: Save raw user input ...",
>> because otherwise we would temporary lose the access to strings
>> with user-given values and thus wouldn't be able to report them in
>> case of any issue.
>> ---
>> mkfs/xfs_mkfs.c | 90 ++++++++++++++++++++++++++-------------------------------
>> 1 file changed, 41 insertions(+), 49 deletions(-)
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index f2f6468e..127f14c3 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -1345,6 +1345,7 @@ getnum(
>> long long c;
>>
>> check_opt(opts, index, false);
>> + set_conf_raw(opts, index, str);
>> /* empty strings might just return a default value */
>> if (!str || *str == '\0') {
>> if (sp->flagval == SUBOPT_NEEDS_VAL)
>> @@ -1432,12 +1433,12 @@ main(
>> char *dfile;
>> int dirblocklog;
>> int dirblocksize;
>> - char *dsize;
>> + int dbytes;
>> int dsu;
>> int dsw;
>> int dsunit;
>> int dswidth;
>> - int force_overwrite;
>> + bool force_overwrite;
>> struct fsxattr fsx;
>> int ilflag;
>> int imaxpct;
>> @@ -1456,7 +1457,7 @@ main(
>> xfs_rfsblock_t logblocks;
>> char *logfile;
>> int loginternal;
>> - char *logsize;
>> + int logbytes;
>> xfs_fsblock_t logstart;
>> int lvflag;
>> int lsflag;
>> @@ -1485,11 +1486,11 @@ main(
>> char *protostring;
>> int qflag;
>> xfs_rfsblock_t rtblocks;
>> + int rtbytes;
>> xfs_extlen_t rtextblocks;
>> xfs_rtblock_t rtextents;
>> - char *rtextsize;
>> + int rtextbytes;
>> char *rtfile;
>> - char *rtsize;
>> xfs_sb_t *sbp;
>> int sectorlog;
>> __uint64_t sector_mask;
>> @@ -1537,7 +1538,8 @@ main(
>> qflag = 0;
>> imaxpct = inodelog = inopblock = isize = 0;
>> dfile = logfile = rtfile = NULL;
>> - dsize = logsize = rtsize = rtextsize = protofile = NULL;
>> + protofile = NULL;
>> + rtbytes = rtextbytes = logbytes = dbytes = 0;
>> dsu = dsw = dsunit = dswidth = lalign = lsu = lsunit = 0;
>> nodsflag = norsflag = 0;
>> force_overwrite = 0;
>> @@ -1601,7 +1603,7 @@ main(
>> xi.dname = getstr(value, &dopts, D_NAME);
>> break;
>> case D_SIZE:
>> - dsize = getstr(value, &dopts, D_SIZE);
>> + dbytes = getnum(value, &dopts, D_SIZE);
>> break;
>> case D_SUNIT:
>> dsunit = getnum(value, &dopts, D_SUNIT);
>> @@ -1746,7 +1748,7 @@ main(
>> lvflag = 1;
>> break;
>> case L_SIZE:
>> - logsize = getstr(value, &lopts, L_SIZE);
>> + logbytes = getnum(value, &lopts, L_SIZE);
>> break;
>> case L_SECTLOG:
>> lsectorlog = getnum(value, &lopts,
>> @@ -1875,8 +1877,7 @@ main(
>>
>> switch (getsubopt(&p, subopts, &value)) {
>> case R_EXTSIZE:
>> - rtextsize = getstr(value, &ropts,
>> - R_EXTSIZE);
>> + rtextbytes = getnum(value, &ropts, R_EXTSIZE);
>> break;
>> case R_FILE:
>> xi.risfile = getnum(value, &ropts,
>> @@ -1888,7 +1889,7 @@ main(
>> R_NAME);
>> break;
>> case R_SIZE:
>> - rtsize = getstr(value, &ropts, R_SIZE);
>> + rtbytes = getnum(value, &ropts, R_SIZE);
>> break;
>> case R_NOALIGN:
>> norsflag = getnum(value, &ropts,
>> @@ -1991,14 +1992,14 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
>> * sector size mismatches between the new filesystem and the underlying
>> * host filesystem.
>> */
>> - check_device_type(dfile, &xi.disfile, !dsize, !dfile,
>> + check_device_type(dfile, &xi.disfile, !dbytes, !dfile,
>> Nflag ? NULL : &xi.dcreat, force_overwrite, "d");
>> if (!loginternal)
>> - check_device_type(xi.logname, &xi.lisfile, !logsize, !xi.logname,
>> - Nflag ? NULL : &xi.lcreat,
>> + check_device_type(xi.logname, &xi.lisfile, !logbytes,
>> + !xi.logname, Nflag ? NULL : &xi.lcreat,
>> force_overwrite, "l");
>> if (xi.rtname)
>> - check_device_type(xi.rtname, &xi.risfile, !rtsize, !xi.rtname,
>> + check_device_type(xi.rtname, &xi.risfile, !rtbytes, !xi.rtname,
>> Nflag ? NULL : &xi.rcreat,
>> force_overwrite, "r");
>> if (xi.disfile || xi.lisfile || xi.risfile)
>> @@ -2179,10 +2180,7 @@ _("rmapbt not supported with realtime devices\n"));
>> }
>>
>>
>> - if (dsize) {
>> - __uint64_t dbytes;
>> -
>> - dbytes = getnum(dsize, &dopts, D_SIZE);
>> + if (dbytes) {
>
> Why has dbytes been demoted from uint64_t to int? This eliminates the
> ability to -d size=8G, right?
>
Ah, thanks for spotting it, I will check the other variables as well.
Jan
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 3/7 v2] mkfs: remove intermediate getstr followed by getnum
2017-07-20 9:29 ` [PATCH 3/7] mkfs: remove intermediate getstr followed by getnum Jan Tulak
2017-07-20 15:54 ` Darrick J. Wong
@ 2017-07-21 12:24 ` Jan Tulak
2017-07-26 23:23 ` Eric Sandeen
1 sibling, 1 reply; 39+ messages in thread
From: Jan Tulak @ 2017-07-21 12:24 UTC (permalink / raw)
To: linux-xfs; +Cc: Jan Tulak
Some options loaded a number as a string with getstr and converted it to
number with getnum later in the code, without any reason for this
approach. (They were probably forgotten in some past cleaning.)
This patch changes them to skip the string and use getnum directly in
the main option-parsing loop, as do all the other numerical options.
And as we now have two variables of the same type for the same value,
merge them together. (e.g. former string dsize and number dbytes).
Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
This patch has to be applied after "mkfs: Save raw user input ...",
because otherwise we would temporary lose the access to strings
with user-given values and thus wouldn't be able to report them in
case of an issue. (In reply to Eric's comment.)
UPDATE:
* fix dbytes, rtbytes, rtextbytes and logbytes - these variables should
be uint64, as the local old ones they are replacing, but were
unintentionally created as int
---
mkfs/xfs_mkfs.c | 90 ++++++++++++++++++++++++++-------------------------------
1 file changed, 41 insertions(+), 49 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index f2f6468e..7a240daf 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1345,6 +1345,7 @@ getnum(
long long c;
check_opt(opts, index, false);
+ set_conf_raw(opts, index, str);
/* empty strings might just return a default value */
if (!str || *str == '\0') {
if (sp->flagval == SUBOPT_NEEDS_VAL)
@@ -1432,12 +1433,12 @@ main(
char *dfile;
int dirblocklog;
int dirblocksize;
- char *dsize;
+ uint64_t dbytes;
int dsu;
int dsw;
int dsunit;
int dswidth;
- int force_overwrite;
+ bool force_overwrite;
struct fsxattr fsx;
int ilflag;
int imaxpct;
@@ -1456,7 +1457,7 @@ main(
xfs_rfsblock_t logblocks;
char *logfile;
int loginternal;
- char *logsize;
+ uint64_t logbytes;
xfs_fsblock_t logstart;
int lvflag;
int lsflag;
@@ -1485,11 +1486,11 @@ main(
char *protostring;
int qflag;
xfs_rfsblock_t rtblocks;
+ uint64_t rtbytes;
xfs_extlen_t rtextblocks;
xfs_rtblock_t rtextents;
- char *rtextsize;
+ uint64_t rtextbytes;
char *rtfile;
- char *rtsize;
xfs_sb_t *sbp;
int sectorlog;
__uint64_t sector_mask;
@@ -1537,7 +1538,8 @@ main(
qflag = 0;
imaxpct = inodelog = inopblock = isize = 0;
dfile = logfile = rtfile = NULL;
- dsize = logsize = rtsize = rtextsize = protofile = NULL;
+ protofile = NULL;
+ rtbytes = rtextbytes = logbytes = dbytes = 0;
dsu = dsw = dsunit = dswidth = lalign = lsu = lsunit = 0;
nodsflag = norsflag = 0;
force_overwrite = 0;
@@ -1601,7 +1603,7 @@ main(
xi.dname = getstr(value, &dopts, D_NAME);
break;
case D_SIZE:
- dsize = getstr(value, &dopts, D_SIZE);
+ dbytes = getnum(value, &dopts, D_SIZE);
break;
case D_SUNIT:
dsunit = getnum(value, &dopts, D_SUNIT);
@@ -1746,7 +1748,7 @@ main(
lvflag = 1;
break;
case L_SIZE:
- logsize = getstr(value, &lopts, L_SIZE);
+ logbytes = getnum(value, &lopts, L_SIZE);
break;
case L_SECTLOG:
lsectorlog = getnum(value, &lopts,
@@ -1875,8 +1877,7 @@ main(
switch (getsubopt(&p, subopts, &value)) {
case R_EXTSIZE:
- rtextsize = getstr(value, &ropts,
- R_EXTSIZE);
+ rtextbytes = getnum(value, &ropts, R_EXTSIZE);
break;
case R_FILE:
xi.risfile = getnum(value, &ropts,
@@ -1888,7 +1889,7 @@ main(
R_NAME);
break;
case R_SIZE:
- rtsize = getstr(value, &ropts, R_SIZE);
+ rtbytes = getnum(value, &ropts, R_SIZE);
break;
case R_NOALIGN:
norsflag = getnum(value, &ropts,
@@ -1991,14 +1992,14 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
* sector size mismatches between the new filesystem and the underlying
* host filesystem.
*/
- check_device_type(dfile, &xi.disfile, !dsize, !dfile,
+ check_device_type(dfile, &xi.disfile, !dbytes, !dfile,
Nflag ? NULL : &xi.dcreat, force_overwrite, "d");
if (!loginternal)
- check_device_type(xi.logname, &xi.lisfile, !logsize, !xi.logname,
- Nflag ? NULL : &xi.lcreat,
+ check_device_type(xi.logname, &xi.lisfile, !logbytes,
+ !xi.logname, Nflag ? NULL : &xi.lcreat,
force_overwrite, "l");
if (xi.rtname)
- check_device_type(xi.rtname, &xi.risfile, !rtsize, !xi.rtname,
+ check_device_type(xi.rtname, &xi.risfile, !rtbytes, !xi.rtname,
Nflag ? NULL : &xi.rcreat,
force_overwrite, "r");
if (xi.disfile || xi.lisfile || xi.risfile)
@@ -2179,10 +2180,7 @@ _("rmapbt not supported with realtime devices\n"));
}
- if (dsize) {
- __uint64_t dbytes;
-
- dbytes = getnum(dsize, &dopts, D_SIZE);
+ if (dbytes) {
if (dbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
_("illegal data length %lld, not a multiple of %d\n"),
@@ -2211,10 +2209,7 @@ _("rmapbt not supported with realtime devices\n"));
usage();
}
- if (logsize) {
- __uint64_t logbytes;
-
- logbytes = getnum(logsize, &lopts, L_SIZE);
+ if (logbytes) {
if (logbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
_("illegal log length %lld, not a multiple of %d\n"),
@@ -2228,10 +2223,7 @@ _("rmapbt not supported with realtime devices\n"));
(long long)logbytes, blocksize,
(long long)(logblocks << blocklog));
}
- if (rtsize) {
- __uint64_t rtbytes;
-
- rtbytes = getnum(rtsize, &ropts, R_SIZE);
+ if (rtbytes) {
if (rtbytes % XFS_MIN_BLOCKSIZE) {
fprintf(stderr,
_("illegal rt length %lld, not a multiple of %d\n"),
@@ -2248,10 +2240,7 @@ _("rmapbt not supported with realtime devices\n"));
/*
* If specified, check rt extent size against its constraints.
*/
- if (rtextsize) {
- __uint64_t rtextbytes;
-
- rtextbytes = getnum(rtextsize, &ropts, R_EXTSIZE);
+ if (rtextbytes) {
if (rtextbytes % blocksize) {
fprintf(stderr,
_("illegal rt extent size %lld, not a multiple of %d\n"),
@@ -2268,7 +2257,7 @@ _("rmapbt not supported with realtime devices\n"));
__uint64_t rswidth;
__uint64_t rtextbytes;
- if (!norsflag && !xi.risfile && !(!rtsize && xi.disfile))
+ if (!norsflag && !xi.risfile && !(!rtbytes && xi.disfile))
rswidth = ft.rtswidth;
else
rswidth = 0;
@@ -2379,15 +2368,16 @@ _("rmapbt not supported with realtime devices\n"));
rtfile = _("volume rt");
else if (!xi.rtdev)
rtfile = _("none");
- if (dsize && xi.dsize > 0 && dblocks > DTOBT(xi.dsize)) {
+ if (dbytes && xi.dsize > 0 && dblocks > DTOBT(xi.dsize)) {
fprintf(stderr,
_("size %s specified for data subvolume is too large, "
"maximum is %lld blocks\n"),
- dsize, (long long)DTOBT(xi.dsize));
+ get_conf_raw(&dopts, D_SIZE),
+ (long long)DTOBT(xi.dsize));
usage();
- } else if (!dsize && xi.dsize > 0)
+ } else if (!dbytes && xi.dsize > 0)
dblocks = DTOBT(xi.dsize);
- else if (!dsize) {
+ else if (!dbytes) {
fprintf(stderr, _("can't get size of data subvolume\n"));
usage();
}
@@ -2420,22 +2410,23 @@ reported by the device (%u).\n"),
reported by the device (%u).\n"),
lsectorsize, xi.lbsize);
}
- if (rtsize && xi.rtsize > 0 && xi.rtbsize > sectorsize) {
+ if (rtbytes && xi.rtsize > 0 && xi.rtbsize > sectorsize) {
fprintf(stderr, _(
"Warning: the realtime subvolume sector size %u is less than the sector size\n\
reported by the device (%u).\n"),
sectorsize, xi.rtbsize);
}
- if (rtsize && xi.rtsize > 0 && rtblocks > DTOBT(xi.rtsize)) {
+ if (rtbytes && xi.rtsize > 0 && rtblocks > DTOBT(xi.rtsize)) {
fprintf(stderr,
_("size %s specified for rt subvolume is too large, "
"maximum is %lld blocks\n"),
- rtsize, (long long)DTOBT(xi.rtsize));
+ get_conf_raw(&ropts, R_SIZE),
+ (long long)DTOBT(xi.rtsize));
usage();
- } else if (!rtsize && xi.rtsize > 0)
+ } else if (!rtbytes && xi.rtsize > 0)
rtblocks = DTOBT(xi.rtsize);
- else if (rtsize && !xi.rtdev) {
+ else if (rtbytes && !xi.rtdev) {
fprintf(stderr,
_("size specified for non-existent rt subvolume\n"));
usage();
@@ -2641,26 +2632,27 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
sb_feat.inode_align);
ASSERT(min_logblocks);
min_logblocks = MAX(XFS_MIN_LOG_BLOCKS, min_logblocks);
- if (!logsize && dblocks >= (1024*1024*1024) >> blocklog)
+ if (!logbytes && dblocks >= (1024*1024*1024) >> blocklog)
min_logblocks = MAX(min_logblocks, XFS_MIN_LOG_BYTES>>blocklog);
- if (logsize && xi.logBBsize > 0 && logblocks > DTOBT(xi.logBBsize)) {
+ if (logbytes && xi.logBBsize > 0 && logblocks > DTOBT(xi.logBBsize)) {
fprintf(stderr,
_("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
- logsize, (long long)DTOBT(xi.logBBsize));
+ get_conf_raw(&lopts, L_SIZE),
+ (long long)DTOBT(xi.logBBsize));
usage();
- } else if (!logsize && xi.logBBsize > 0) {
+ } else if (!logbytes && xi.logBBsize > 0) {
logblocks = DTOBT(xi.logBBsize);
- } else if (logsize && !xi.logdev && !loginternal) {
+ } else if (logbytes && !xi.logdev && !loginternal) {
fprintf(stderr,
_("size specified for non-existent log subvolume\n"));
usage();
- } else if (loginternal && logsize && logblocks >= dblocks) {
+ } else if (loginternal && logbytes && logblocks >= dblocks) {
fprintf(stderr, _("size %lld too large for internal log\n"),
(long long)logblocks);
usage();
} else if (!loginternal && !xi.logdev) {
logblocks = 0;
- } else if (loginternal && !logsize) {
+ } else if (loginternal && !logbytes) {
if (dblocks < GIGABYTES(1, blocklog)) {
/* tiny filesystems get minimum sized logs. */
@@ -2724,7 +2716,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
* Readjust the log size to fit within an AG if it was sized
* automatically.
*/
- if (!logsize) {
+ if (!logbytes) {
logblocks = MIN(logblocks,
libxfs_alloc_ag_max_usable(mp));
--
2.11.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH 3/7] mkfs: remove intermediate getstr followed by getnum
2017-07-20 15:54 ` Darrick J. Wong
2017-07-21 8:56 ` Jan Tulak
@ 2017-07-26 20:49 ` Eric Sandeen
2017-07-27 7:50 ` Jan Tulak
1 sibling, 1 reply; 39+ messages in thread
From: Eric Sandeen @ 2017-07-26 20:49 UTC (permalink / raw)
To: Darrick J. Wong, Jan Tulak; +Cc: linux-xfs
On 7/20/17 10:54 AM, Darrick J. Wong wrote:
> On Thu, Jul 20, 2017 at 11:29:28AM +0200, Jan Tulak wrote:
>> Some options loaded a number as a string with getstr and converted it to
>> number with getnum later in the code, without any reason for this
>> approach. (They were probably forgotten in some past cleaning.)
>>
>> This patch changes them to skip the string and use getnum directly in
>> the main option-parsing loop, as do all the other numerical options.
>> And as we now have two variables of the same type for the same value,
>> merge them together. (e.g. former string dsize and number dbytes).
>>
>> Signed-off-by: Jan Tulak <jtulak@redhat.com>
>> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
>> Reviewed-by: Luis R. Rodriguez <mcgrof@kernel.org>
>>
>> ---
>> In reply to Eric's comment, so it doesn't pop up again:
>> This patch has to be applied after "mkfs: Save raw user input ...",
>> because otherwise we would temporary lose the access to strings
>> with user-given values and thus wouldn't be able to report them in
>> case of any issue.
>> ---
>> mkfs/xfs_mkfs.c | 90 ++++++++++++++++++++++++++-------------------------------
>> 1 file changed, 41 insertions(+), 49 deletions(-)
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index f2f6468e..127f14c3 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -1345,6 +1345,7 @@ getnum(
>> long long c;
>>
>> check_opt(opts, index, false);
>> + set_conf_raw(opts, index, str);
>> /* empty strings might just return a default value */
>> if (!str || *str == '\0') {
>> if (sp->flagval == SUBOPT_NEEDS_VAL)
>> @@ -1432,12 +1433,12 @@ main(
>> char *dfile;
>> int dirblocklog;
>> int dirblocksize;
>> - char *dsize;
>> + int dbytes;
<snip>
>> @@ -2179,10 +2180,7 @@ _("rmapbt not supported with realtime devices\n"));
>> }
>>
>>
>> - if (dsize) {
>> - __uint64_t dbytes;
>> -
>> - dbytes = getnum(dsize, &dopts, D_SIZE);
>> + if (dbytes) {
>
> Why has dbytes been demoted from uint64_t to int? This eliminates the
> ability to -d size=8G, right?
That wasn't a problem in the version I reviewed.... pls don't keep reviewed-by's
on patches that change from the reviewed version. Further, best to indicate
explicitly what has changed since the last version, under the "---"
Thanks,
-Eric
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/7 v2] mkfs: remove intermediate getstr followed by getnum
2017-07-21 12:24 ` [PATCH 3/7 v2] " Jan Tulak
@ 2017-07-26 23:23 ` Eric Sandeen
0 siblings, 0 replies; 39+ messages in thread
From: Eric Sandeen @ 2017-07-26 23:23 UTC (permalink / raw)
To: Jan Tulak, linux-xfs
On 7/21/17 7:24 AM, Jan Tulak wrote:
> Some options loaded a number as a string with getstr and converted it to
> number with getnum later in the code, without any reason for this
> approach. (They were probably forgotten in some past cleaning.)
>
> This patch changes them to skip the string and use getnum directly in
> the main option-parsing loop, as do all the other numerical options.
> And as we now have two variables of the same type for the same value,
> merge them together. (e.g. former string dsize and number dbytes).
>
> Signed-off-by: Jan Tulak <jtulak@redhat.com>
The changes to the type of force_overwrite seems unrelated, but I'll let
that slide for now. ;)
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> This patch has to be applied after "mkfs: Save raw user input ...",
> because otherwise we would temporary lose the access to strings
> with user-given values and thus wouldn't be able to report them in
> case of an issue. (In reply to Eric's comment.)
>
> UPDATE:
> * fix dbytes, rtbytes, rtextbytes and logbytes - these variables should
> be uint64, as the local old ones they are replacing, but were
> unintentionally created as int
>
> ---
> mkfs/xfs_mkfs.c | 90 ++++++++++++++++++++++++++-------------------------------
> 1 file changed, 41 insertions(+), 49 deletions(-)
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index f2f6468e..7a240daf 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -1345,6 +1345,7 @@ getnum(
> long long c;
>
> check_opt(opts, index, false);
> + set_conf_raw(opts, index, str);
> /* empty strings might just return a default value */
> if (!str || *str == '\0') {
> if (sp->flagval == SUBOPT_NEEDS_VAL)
> @@ -1432,12 +1433,12 @@ main(
> char *dfile;
> int dirblocklog;
> int dirblocksize;
> - char *dsize;
> + uint64_t dbytes;
> int dsu;
> int dsw;
> int dsunit;
> int dswidth;
> - int force_overwrite;
> + bool force_overwrite;
> struct fsxattr fsx;
> int ilflag;
> int imaxpct;
> @@ -1456,7 +1457,7 @@ main(
> xfs_rfsblock_t logblocks;
> char *logfile;
> int loginternal;
> - char *logsize;
> + uint64_t logbytes;
> xfs_fsblock_t logstart;
> int lvflag;
> int lsflag;
> @@ -1485,11 +1486,11 @@ main(
> char *protostring;
> int qflag;
> xfs_rfsblock_t rtblocks;
> + uint64_t rtbytes;
> xfs_extlen_t rtextblocks;
> xfs_rtblock_t rtextents;
> - char *rtextsize;
> + uint64_t rtextbytes;
> char *rtfile;
> - char *rtsize;
> xfs_sb_t *sbp;
> int sectorlog;
> __uint64_t sector_mask;
> @@ -1537,7 +1538,8 @@ main(
> qflag = 0;
> imaxpct = inodelog = inopblock = isize = 0;
> dfile = logfile = rtfile = NULL;
> - dsize = logsize = rtsize = rtextsize = protofile = NULL;
> + protofile = NULL;
> + rtbytes = rtextbytes = logbytes = dbytes = 0;
> dsu = dsw = dsunit = dswidth = lalign = lsu = lsunit = 0;
> nodsflag = norsflag = 0;
> force_overwrite = 0;
> @@ -1601,7 +1603,7 @@ main(
> xi.dname = getstr(value, &dopts, D_NAME);
> break;
> case D_SIZE:
> - dsize = getstr(value, &dopts, D_SIZE);
> + dbytes = getnum(value, &dopts, D_SIZE);
> break;
> case D_SUNIT:
> dsunit = getnum(value, &dopts, D_SUNIT);
> @@ -1746,7 +1748,7 @@ main(
> lvflag = 1;
> break;
> case L_SIZE:
> - logsize = getstr(value, &lopts, L_SIZE);
> + logbytes = getnum(value, &lopts, L_SIZE);
> break;
> case L_SECTLOG:
> lsectorlog = getnum(value, &lopts,
> @@ -1875,8 +1877,7 @@ main(
>
> switch (getsubopt(&p, subopts, &value)) {
> case R_EXTSIZE:
> - rtextsize = getstr(value, &ropts,
> - R_EXTSIZE);
> + rtextbytes = getnum(value, &ropts, R_EXTSIZE);
> break;
> case R_FILE:
> xi.risfile = getnum(value, &ropts,
> @@ -1888,7 +1889,7 @@ main(
> R_NAME);
> break;
> case R_SIZE:
> - rtsize = getstr(value, &ropts, R_SIZE);
> + rtbytes = getnum(value, &ropts, R_SIZE);
> break;
> case R_NOALIGN:
> norsflag = getnum(value, &ropts,
> @@ -1991,14 +1992,14 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
> * sector size mismatches between the new filesystem and the underlying
> * host filesystem.
> */
> - check_device_type(dfile, &xi.disfile, !dsize, !dfile,
> + check_device_type(dfile, &xi.disfile, !dbytes, !dfile,
> Nflag ? NULL : &xi.dcreat, force_overwrite, "d");
> if (!loginternal)
> - check_device_type(xi.logname, &xi.lisfile, !logsize, !xi.logname,
> - Nflag ? NULL : &xi.lcreat,
> + check_device_type(xi.logname, &xi.lisfile, !logbytes,
> + !xi.logname, Nflag ? NULL : &xi.lcreat,
> force_overwrite, "l");
> if (xi.rtname)
> - check_device_type(xi.rtname, &xi.risfile, !rtsize, !xi.rtname,
> + check_device_type(xi.rtname, &xi.risfile, !rtbytes, !xi.rtname,
> Nflag ? NULL : &xi.rcreat,
> force_overwrite, "r");
> if (xi.disfile || xi.lisfile || xi.risfile)
> @@ -2179,10 +2180,7 @@ _("rmapbt not supported with realtime devices\n"));
> }
>
>
> - if (dsize) {
> - __uint64_t dbytes;
> -
> - dbytes = getnum(dsize, &dopts, D_SIZE);
> + if (dbytes) {
> if (dbytes % XFS_MIN_BLOCKSIZE) {
> fprintf(stderr,
> _("illegal data length %lld, not a multiple of %d\n"),
> @@ -2211,10 +2209,7 @@ _("rmapbt not supported with realtime devices\n"));
> usage();
> }
>
> - if (logsize) {
> - __uint64_t logbytes;
> -
> - logbytes = getnum(logsize, &lopts, L_SIZE);
> + if (logbytes) {
> if (logbytes % XFS_MIN_BLOCKSIZE) {
> fprintf(stderr,
> _("illegal log length %lld, not a multiple of %d\n"),
> @@ -2228,10 +2223,7 @@ _("rmapbt not supported with realtime devices\n"));
> (long long)logbytes, blocksize,
> (long long)(logblocks << blocklog));
> }
> - if (rtsize) {
> - __uint64_t rtbytes;
> -
> - rtbytes = getnum(rtsize, &ropts, R_SIZE);
> + if (rtbytes) {
> if (rtbytes % XFS_MIN_BLOCKSIZE) {
> fprintf(stderr,
> _("illegal rt length %lld, not a multiple of %d\n"),
> @@ -2248,10 +2240,7 @@ _("rmapbt not supported with realtime devices\n"));
> /*
> * If specified, check rt extent size against its constraints.
> */
> - if (rtextsize) {
> - __uint64_t rtextbytes;
> -
> - rtextbytes = getnum(rtextsize, &ropts, R_EXTSIZE);
> + if (rtextbytes) {
> if (rtextbytes % blocksize) {
> fprintf(stderr,
> _("illegal rt extent size %lld, not a multiple of %d\n"),
> @@ -2268,7 +2257,7 @@ _("rmapbt not supported with realtime devices\n"));
> __uint64_t rswidth;
> __uint64_t rtextbytes;
>
> - if (!norsflag && !xi.risfile && !(!rtsize && xi.disfile))
> + if (!norsflag && !xi.risfile && !(!rtbytes && xi.disfile))
> rswidth = ft.rtswidth;
> else
> rswidth = 0;
> @@ -2379,15 +2368,16 @@ _("rmapbt not supported with realtime devices\n"));
> rtfile = _("volume rt");
> else if (!xi.rtdev)
> rtfile = _("none");
> - if (dsize && xi.dsize > 0 && dblocks > DTOBT(xi.dsize)) {
> + if (dbytes && xi.dsize > 0 && dblocks > DTOBT(xi.dsize)) {
> fprintf(stderr,
> _("size %s specified for data subvolume is too large, "
> "maximum is %lld blocks\n"),
> - dsize, (long long)DTOBT(xi.dsize));
> + get_conf_raw(&dopts, D_SIZE),
> + (long long)DTOBT(xi.dsize));
> usage();
> - } else if (!dsize && xi.dsize > 0)
> + } else if (!dbytes && xi.dsize > 0)
> dblocks = DTOBT(xi.dsize);
> - else if (!dsize) {
> + else if (!dbytes) {
> fprintf(stderr, _("can't get size of data subvolume\n"));
> usage();
> }
> @@ -2420,22 +2410,23 @@ reported by the device (%u).\n"),
> reported by the device (%u).\n"),
> lsectorsize, xi.lbsize);
> }
> - if (rtsize && xi.rtsize > 0 && xi.rtbsize > sectorsize) {
> + if (rtbytes && xi.rtsize > 0 && xi.rtbsize > sectorsize) {
> fprintf(stderr, _(
> "Warning: the realtime subvolume sector size %u is less than the sector size\n\
> reported by the device (%u).\n"),
> sectorsize, xi.rtbsize);
> }
>
> - if (rtsize && xi.rtsize > 0 && rtblocks > DTOBT(xi.rtsize)) {
> + if (rtbytes && xi.rtsize > 0 && rtblocks > DTOBT(xi.rtsize)) {
> fprintf(stderr,
> _("size %s specified for rt subvolume is too large, "
> "maximum is %lld blocks\n"),
> - rtsize, (long long)DTOBT(xi.rtsize));
> + get_conf_raw(&ropts, R_SIZE),
> + (long long)DTOBT(xi.rtsize));
> usage();
> - } else if (!rtsize && xi.rtsize > 0)
> + } else if (!rtbytes && xi.rtsize > 0)
> rtblocks = DTOBT(xi.rtsize);
> - else if (rtsize && !xi.rtdev) {
> + else if (rtbytes && !xi.rtdev) {
> fprintf(stderr,
> _("size specified for non-existent rt subvolume\n"));
> usage();
> @@ -2641,26 +2632,27 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
> sb_feat.inode_align);
> ASSERT(min_logblocks);
> min_logblocks = MAX(XFS_MIN_LOG_BLOCKS, min_logblocks);
> - if (!logsize && dblocks >= (1024*1024*1024) >> blocklog)
> + if (!logbytes && dblocks >= (1024*1024*1024) >> blocklog)
> min_logblocks = MAX(min_logblocks, XFS_MIN_LOG_BYTES>>blocklog);
> - if (logsize && xi.logBBsize > 0 && logblocks > DTOBT(xi.logBBsize)) {
> + if (logbytes && xi.logBBsize > 0 && logblocks > DTOBT(xi.logBBsize)) {
> fprintf(stderr,
> _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
> - logsize, (long long)DTOBT(xi.logBBsize));
> + get_conf_raw(&lopts, L_SIZE),
> + (long long)DTOBT(xi.logBBsize));
> usage();
> - } else if (!logsize && xi.logBBsize > 0) {
> + } else if (!logbytes && xi.logBBsize > 0) {
> logblocks = DTOBT(xi.logBBsize);
> - } else if (logsize && !xi.logdev && !loginternal) {
> + } else if (logbytes && !xi.logdev && !loginternal) {
> fprintf(stderr,
> _("size specified for non-existent log subvolume\n"));
> usage();
> - } else if (loginternal && logsize && logblocks >= dblocks) {
> + } else if (loginternal && logbytes && logblocks >= dblocks) {
> fprintf(stderr, _("size %lld too large for internal log\n"),
> (long long)logblocks);
> usage();
> } else if (!loginternal && !xi.logdev) {
> logblocks = 0;
> - } else if (loginternal && !logsize) {
> + } else if (loginternal && !logbytes) {
>
> if (dblocks < GIGABYTES(1, blocklog)) {
> /* tiny filesystems get minimum sized logs. */
> @@ -2724,7 +2716,7 @@ _("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
> * Readjust the log size to fit within an AG if it was sized
> * automatically.
> */
> - if (!logsize) {
> + if (!logbytes) {
> logblocks = MIN(logblocks,
> libxfs_alloc_ag_max_usable(mp));
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/7] mkfs: move getnum within the file
2017-07-20 9:29 ` [PATCH 5/7] mkfs: move getnum within the file Jan Tulak
@ 2017-07-26 23:27 ` Eric Sandeen
0 siblings, 0 replies; 39+ messages in thread
From: Eric Sandeen @ 2017-07-26 23:27 UTC (permalink / raw)
To: Jan Tulak, linux-xfs
On 7/20/17 4:29 AM, Jan Tulak wrote:
> Move getnum, check_opt and illegal_option within the file. We have to do
> this to avoid dependency issues with the following patch. There is no
> other change in this patch, just cut&paste of these functions.
Yeah, nice to have these up at the top of the file anyway.
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Jan Tulak <jtulak@redhat.com>
> ---
> mkfs/xfs_mkfs.c | 236 ++++++++++++++++++++++++++++----------------------------
> 1 file changed, 118 insertions(+), 118 deletions(-)
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index f10cb15a..9d2db2a2 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -761,6 +761,124 @@ get_conf_raw(int opt, int subopt)
> return opts[opt].subopt_params[subopt].raw_input;
> }
>
> +static __attribute__((noreturn)) void
> +illegal_option(
> + const char *value,
> + struct opt_params *opts,
> + int index,
> + const char *reason)
> +{
> + fprintf(stderr,
> + _("Illegal value %s for -%c %s option. %s\n"),
> + value, opts->name, opts->subopts[index],
> + reason ? reason : "");
> + usage();
> +}
> +
> +/*
> + * Check for conflicts and option respecification.
> + */
> +static void
> +check_opt(
> + struct opt_params *opts,
> + int index,
> + bool str_seen)
> +{
> + struct subopt_param *sp = &opts->subopt_params[index];
> + int i;
> +
> + 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);
> + }
> +
> + /*
> + * 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 (i = 0; i < MAX_CONFLICTS; i++) {
> + int conflict_opt = sp->conflicts[i];
> +
> + if (conflict_opt == LAST_CONFLICT)
> + break;
> + if (opts->subopt_params[conflict_opt].seen ||
> + opts->subopt_params[conflict_opt].str_seen)
> + conflict(opts->name, (char **)opts->subopts,
> + conflict_opt, 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(opts, index, false);
> + set_conf_raw(opts->index, index, str);
> + /* empty strings might just return a default value */
> + if (!str || *str == '\0') {
> + if (sp->flagval == SUBOPT_NEEDS_VAL)
> + reqval(opts->name, (char **)opts->subopts, index);
> + return sp->flagval;
> + }
> +
> + if (sp->minval == 0 && sp->maxval == 0) {
> + fprintf(stderr,
> + _("Option -%c %s has undefined minval/maxval."
> + "Can't verify value range. This is a bug.\n"),
> + opts->name, opts->subopts[index]);
> + exit(1);
> + }
> +
> + /*
> + * 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 *str_end;
> +
> + c = strtoll(str, &str_end, 0);
> + if (c == 0 && str_end == str)
> + illegal_option(str, opts, index, NULL);
> + if (*str_end != '\0')
> + illegal_option(str, opts, index, NULL);
> + }
> +
> + /* Validity check the result. */
> + if (c < sp->minval)
> + illegal_option(str, opts, index, _("value is too small"));
> + else if (c > sp->maxval)
> + illegal_option(str, opts, index, _("value is too large"));
> + if (sp->is_power_2 && !ispow2(c))
> + illegal_option(str, opts, index, _("value must be a power of 2"));
> + return c;
> +}
> +
> /*
> * Convert lsu to lsunit for 512 bytes blocks and check validity of the values.
> */
> @@ -1285,124 +1403,6 @@ sb_set_features(
>
> }
>
> -static __attribute__((noreturn)) void
> -illegal_option(
> - const char *value,
> - struct opt_params *opts,
> - int index,
> - const char *reason)
> -{
> - fprintf(stderr,
> - _("Illegal value %s for -%c %s option. %s\n"),
> - value, opts->name, opts->subopts[index],
> - reason ? reason : "");
> - usage();
> -}
> -
> -/*
> - * Check for conflicts and option respecification.
> - */
> -static void
> -check_opt(
> - struct opt_params *opts,
> - int index,
> - bool str_seen)
> -{
> - struct subopt_param *sp = &opts->subopt_params[index];
> - int i;
> -
> - 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);
> - }
> -
> - /*
> - * 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 (i = 0; i < MAX_CONFLICTS; i++) {
> - int conflict_opt = sp->conflicts[i];
> -
> - if (conflict_opt == LAST_CONFLICT)
> - break;
> - if (opts->subopt_params[conflict_opt].seen ||
> - opts->subopt_params[conflict_opt].str_seen)
> - conflict(opts->name, (char **)opts->subopts,
> - conflict_opt, 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(opts, index, false);
> - set_conf_raw(opts->index, index, str);
> - /* empty strings might just return a default value */
> - if (!str || *str == '\0') {
> - if (sp->flagval == SUBOPT_NEEDS_VAL)
> - reqval(opts->name, (char **)opts->subopts, index);
> - return sp->flagval;
> - }
> -
> - if (sp->minval == 0 && sp->maxval == 0) {
> - fprintf(stderr,
> - _("Option -%c %s has undefined minval/maxval."
> - "Can't verify value range. This is a bug.\n"),
> - opts->name, opts->subopts[index]);
> - exit(1);
> - }
> -
> - /*
> - * 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 *str_end;
> -
> - c = strtoll(str, &str_end, 0);
> - if (c == 0 && str_end == str)
> - illegal_option(str, opts, index, NULL);
> - if (*str_end != '\0')
> - illegal_option(str, opts, index, NULL);
> - }
> -
> - /* Validity check the result. */
> - if (c < sp->minval)
> - illegal_option(str, opts, index, _("value is too small"));
> - else if (c > sp->maxval)
> - illegal_option(str, opts, index, _("value is too large"));
> - if (sp->is_power_2 && !ispow2(c))
> - illegal_option(str, opts, index, _("value must be a power of 2"));
> - 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
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 7/7] mkfs: save user input values into opts
2017-07-20 9:29 ` [PATCH 7/7] mkfs: save user input values into opts Jan Tulak
@ 2017-07-26 23:53 ` Eric Sandeen
2017-07-27 14:21 ` Jan Tulak
0 siblings, 1 reply; 39+ messages in thread
From: Eric Sandeen @ 2017-07-26 23:53 UTC (permalink / raw)
To: Jan Tulak, linux-xfs
On 7/20/17 4:29 AM, Jan Tulak wrote:
> Save the parsed values from users into the opts table. This will ensure that
> the user values gets there and are validated, but we are not yet using them for
> anything - the usage makes a lot of changes through the code, so it is better
> if that is separated into smaller chunks.
>
> Signed-off-by: Jan Tulak <jtulak@redhat.com>
This seems reasonable, but AFAICT nothing uses the set values yet,
right? As such I'll probably hold off on merging it until somethig
makes use of the result... i.e. merge it (and the prior patch)
along with later patches which make use of the stored values.
Also, questions below.
> case I_PROJID32BIT:
> sb_feat.projid16bit =
> !getnum(value, &opts[OPT_I],
> I_PROJID32BIT);
> + set_conf_val(OPT_I, I_PROJID32BIT,
> + sb_feat.projid16bit);
> break;
why isn't this just:
sb_feat.projid16bit = parse_conf_val(OPT_I, I_PROJID32BIT, value) ?
> @@ -1812,34 +1843,48 @@ main(
> xi.logname = logfile;
> ldflag = 1;
> loginternal = 0;
> + set_conf_val(OPT_L, L_NAME, 1);
> + set_conf_val(OPT_L, L_DEV, 1);
Hm, ok, maybe these subopts will collapse into one some day?
> break;
> case L_VERSION:
> sb_feat.log_version =
> - getnum(value, &opts[OPT_L],
> - L_VERSION);
> + parse_conf_val(OPT_L,
> + L_VERSION,
> + value);
> lvflag = 1;
> + set_conf_val(OPT_L, L_VERSION,
> + sb_feat.log_version);
> break;
wait, didn't parse_conf_val already set a value into OPT_L, L_VERSION?
> case M_FINOBT:
> - sb_feat.finobt = getnum(
> - value, &opts[OPT_M], M_FINOBT);
> + sb_feat.finobt =
> + parse_conf_val(OPT_M, M_FINOBT,
> + value);
> + set_conf_val(OPT_M, M_FINOBT,
> + sb_feat.finobt);
Same here, hasn't OPT_M, M_FINOBT's value already been set by the parse
call?
> @@ -1920,14 +1977,17 @@ main(
> } else {
> sb_feat.dir_version =
> getnum(value,
> - &opts[OPT_N],
> - N_VERSION);
> + &opts[OPT_N],
> + N_VERSION);
> }
> nvflag = 1;
> + set_conf_val(OPT_N, N_VERSION,
> + sb_feat.dir_version);
shouldn't this be in the else clause? We didn't necessarily set a
version number, right? Also, should the ci state be stored as well?
i.e.
case N_VERSION:
value = getstr(value, &nopts, N_VERSION);
if (!strcasecmp(value, "ci")) {
/* ASCII CI mode */
sb_feat.nci = true;
/* is this in the opts table anywhere? */
} else {
sb_feat.dir_version =
parse_conf_val(OPT_N,
N_VERSION,
value);
}
nvflag = 1;
break;
> break;
> case N_FTYPE:
> - sb_feat.dirftype = getnum(value,
> - &opts[OPT_N], N_FTYPE);
> + sb_feat.dirftype =
> + parse_conf_val(OPT_N, N_FTYPE,
> + value);
> break;
> default:
> unknown('n', value);
> @@ -1957,25 +2017,30 @@ main(
>
> switch (getsubopt(&p, subopts, &value)) {
> case R_EXTSIZE:
> - rtextbytes = getnum(value,
> - &opts[OPT_R], R_EXTSIZE);
> + rtextbytes = parse_conf_val(OPT_R,
> + R_EXTSIZE,
> + value);
> break;
> case R_FILE:
> - xi.risfile = getnum(value,
> - &opts[OPT_R], R_FILE);
> + xi.risfile = parse_conf_val(OPT_R,
> + R_FILE,
> + value);
> break;
> case R_NAME:
> case R_DEV:
> xi.rtname = getstr(value, &opts[OPT_R],
> R_NAME);
> + set_conf_val(OPT_R, R_NAME, 1);
> + set_conf_val(OPT_R, R_DEV, 1);
> break;
> case R_SIZE:
> - rtbytes = getnum(value, &opts[OPT_R],
> - R_SIZE);
> + rtbytes = parse_conf_val(OPT_R, R_SIZE,
> + value);
> break;
> case R_NOALIGN:
> - norsflag = getnum(value, &opts[OPT_R],
> - R_NOALIGN);
> + norsflag = parse_conf_val(OPT_R,
> + R_NOALIGN,
> + value);
> break;
> default:
> unknown('r', value);
> @@ -1996,12 +2061,14 @@ main(
> conflict('s', subopts,
> S_SECTSIZE,
> S_SECTLOG);
> - sectorlog = getnum(value, &opts[OPT_S],
> - S_SECTLOG);
> + sectorlog = parse_conf_val(OPT_S,
> + S_SECTLOG,
> + value);
> lsectorlog = sectorlog;
> sectorsize = 1 << sectorlog;
> lsectorsize = sectorsize;
> lslflag = slflag = 1;
> + set_conf_val(OPT_S, S_LOG, sectorsize);
Is this right? S_LOG is the log of the sectorsize, right, not the sector size
itself. Do S_SIZE & S_SECTSIZE need to be stored as well?
> break;
> case S_SIZE:
> case S_SECTSIZE:
> @@ -2009,13 +2076,16 @@ main(
> conflict('s', subopts,
> S_SECTLOG,
> S_SECTSIZE);
> - sectorsize = getnum(value,
> - &opts[OPT_S], S_SECTSIZE);
> + sectorsize = parse_conf_val(OPT_S,
> + S_SECTSIZE,
> + value);
> lsectorsize = sectorsize;
> sectorlog =
> libxfs_highbit32(sectorsize);
> lsectorlog = sectorlog;
> lssflag = ssflag = 1;
> + set_conf_val(OPT_S,
> + S_SIZE, sectorlog);
same questions here - this looks wrong, and other values not stored, do
they need to be?
> break;
> default:
> unknown('s', value);
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/7] mkfs: remove intermediate getstr followed by getnum
2017-07-26 20:49 ` Eric Sandeen
@ 2017-07-27 7:50 ` Jan Tulak
2017-07-27 13:35 ` Eric Sandeen
0 siblings, 1 reply; 39+ messages in thread
From: Jan Tulak @ 2017-07-27 7:50 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Darrick J. Wong, linux-xfs
On Wed, Jul 26, 2017 at 10:49 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
> On 7/20/17 10:54 AM, Darrick J. Wong wrote:
>> On Thu, Jul 20, 2017 at 11:29:28AM +0200, Jan Tulak wrote:
>>> Some options loaded a number as a string with getstr and converted it to
>>> number with getnum later in the code, without any reason for this
>>> approach. (They were probably forgotten in some past cleaning.)
>>>
>>> This patch changes them to skip the string and use getnum directly in
>>> the main option-parsing loop, as do all the other numerical options.
>>> And as we now have two variables of the same type for the same value,
>>> merge them together. (e.g. former string dsize and number dbytes).
>>>
>>> Signed-off-by: Jan Tulak <jtulak@redhat.com>
>>> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
>>> Reviewed-by: Luis R. Rodriguez <mcgrof@kernel.org>
>>>
>>> ---
>>> In reply to Eric's comment, so it doesn't pop up again:
>>> This patch has to be applied after "mkfs: Save raw user input ...",
>>> because otherwise we would temporary lose the access to strings
>>> with user-given values and thus wouldn't be able to report them in
>>> case of any issue.
>>> ---
>>> mkfs/xfs_mkfs.c | 90 ++++++++++++++++++++++++++-------------------------------
>>> 1 file changed, 41 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>>> index f2f6468e..127f14c3 100644
>>> --- a/mkfs/xfs_mkfs.c
>>> +++ b/mkfs/xfs_mkfs.c
>>> @@ -1345,6 +1345,7 @@ getnum(
>>> long long c;
>>>
>>> check_opt(opts, index, false);
>>> + set_conf_raw(opts, index, str);
>>> /* empty strings might just return a default value */
>>> if (!str || *str == '\0') {
>>> if (sp->flagval == SUBOPT_NEEDS_VAL)
>>> @@ -1432,12 +1433,12 @@ main(
>>> char *dfile;
>>> int dirblocklog;
>>> int dirblocksize;
>>> - char *dsize;
>>> + int dbytes;
>
> <snip>
>
>
>>> @@ -2179,10 +2180,7 @@ _("rmapbt not supported with realtime devices\n"));
>>> }
>>>
>>>
>>> - if (dsize) {
>>> - __uint64_t dbytes;
>>> -
>>> - dbytes = getnum(dsize, &dopts, D_SIZE);
>>> + if (dbytes) {
>>
>> Why has dbytes been demoted from uint64_t to int? This eliminates the
>> ability to -d size=8G, right?
>
> That wasn't a problem in the version I reviewed.... pls don't keep reviewed-by's
> on patches that change from the reviewed version. Further, best to indicate
> explicitly what has changed since the last version, under the "---"
>
Yes, I'm sorry about that. :( I forgot to remove the Reviewed-by after
rebasing it without the uint64 change. :(
Jan
> Thanks,
> -Eric
>
> --
> 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
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/7] mkfs: remove intermediate getstr followed by getnum
2017-07-27 7:50 ` Jan Tulak
@ 2017-07-27 13:35 ` Eric Sandeen
0 siblings, 0 replies; 39+ messages in thread
From: Eric Sandeen @ 2017-07-27 13:35 UTC (permalink / raw)
To: Jan Tulak; +Cc: Darrick J. Wong, linux-xfs
On 7/27/17 2:50 AM, Jan Tulak wrote:
> On Wed, Jul 26, 2017 at 10:49 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
>> On 7/20/17 10:54 AM, Darrick J. Wong wrote:
...
>>> Why has dbytes been demoted from uint64_t to int? This eliminates the
>>> ability to -d size=8G, right?
>>
>> That wasn't a problem in the version I reviewed.... pls don't keep reviewed-by's
>> on patches that change from the reviewed version. Further, best to indicate
>> explicitly what has changed since the last version, under the "---"
>>
>
> Yes, I'm sorry about that. :( I forgot to remove the Reviewed-by after
> rebasing it without the uint64 change. :(
No problem, just a friendly reminder. :)
-Eric
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 7/7] mkfs: save user input values into opts
2017-07-26 23:53 ` Eric Sandeen
@ 2017-07-27 14:21 ` Jan Tulak
0 siblings, 0 replies; 39+ messages in thread
From: Jan Tulak @ 2017-07-27 14:21 UTC (permalink / raw)
To: Eric Sandeen, linux-xfs
On 27/07/2017 01:53, Eric Sandeen wrote:
> On 7/20/17 4:29 AM, Jan Tulak wrote:
>> Save the parsed values from users into the opts table. This will ensure that
>> the user values gets there and are validated, but we are not yet using them for
>> anything - the usage makes a lot of changes through the code, so it is better
>> if that is separated into smaller chunks.
>>
>> Signed-off-by: Jan Tulak <jtulak@redhat.com>
> This seems reasonable, but AFAICT nothing uses the set values yet,
> right? As such I'll probably hold off on merging it until somethig
> makes use of the result... i.e. merge it (and the prior patch)
> along with later patches which make use of the stored values.
The second patchset I submitted just after this one uses it. I just
split it, so this set doesn't have to wait if there are any issues in
the second part. But if you would rather merge it together, I'm ok with
it and if I will need to respin a whole set once more in a final
version, I can put it together again.
>
> Also, questions below.
>
>> case I_PROJID32BIT:
>> sb_feat.projid16bit =
>> !getnum(value, &opts[OPT_I],
>> I_PROJID32BIT);
>> + set_conf_val(OPT_I, I_PROJID32BIT,
>> + sb_feat.projid16bit);
>> break;
> why isn't this just:
>
> sb_feat.projid16bit = parse_conf_val(OPT_I, I_PROJID32BIT, value) ?
Note the ! in the getnum assignment, we have two variables with opposite
values (16/32 bit) - that's the reason why I used getnum. But there
indeed is an issue with that - the set_conf_val shouldn't be the same
value as the sb_feat. So, I'm changing it to:
sb_feat.projid16bit = ! parse_conf_val(OPT_I, I_PROJID32BIT, value);
>
>
>> @@ -1812,34 +1843,48 @@ main(
>> xi.logname = logfile;
>> ldflag = 1;
>> loginternal = 0;
>> + set_conf_val(OPT_L, L_NAME, 1);
>> + set_conf_val(OPT_L, L_DEV, 1);
> Hm, ok, maybe these subopts will collapse into one some day?
It is on my (long) TODO. But it might be a change simple enough to do it
ASAP.
>
>> break;
>> case L_VERSION:
>> sb_feat.log_version =
>> - getnum(value, &opts[OPT_L],
>> - L_VERSION);
>> + parse_conf_val(OPT_L,
>> + L_VERSION,
>> + value);
>> lvflag = 1;
>> + set_conf_val(OPT_L, L_VERSION,
>> + sb_feat.log_version);
>> break;
> wait, didn't parse_conf_val already set a value into OPT_L, L_VERSION?
>
>
>> case M_FINOBT:
>> - sb_feat.finobt = getnum(
>> - value, &opts[OPT_M], M_FINOBT);
>> + sb_feat.finobt =
>> + parse_conf_val(OPT_M, M_FINOBT,
>> + value);
>> + set_conf_val(OPT_M, M_FINOBT,
>> + sb_feat.finobt);
> Same here, hasn't OPT_M, M_FINOBT's value already been set by the parse
> call?
Yes and yes. Removing.
>
>> @@ -1920,14 +1977,17 @@ main(
>> } else {
>> sb_feat.dir_version =
>> getnum(value,
>> - &opts[OPT_N],
>> - N_VERSION);
>> + &opts[OPT_N],
>> + N_VERSION);
>> }
>> nvflag = 1;
>> + set_conf_val(OPT_N, N_VERSION,
>> + sb_feat.dir_version);
> shouldn't this be in the else clause? We didn't necessarily set a
> version number, right? Also, should the ci state be stored as well?
> i.e.
>
> case N_VERSION:
> value = getstr(value, &nopts, N_VERSION);
> if (!strcasecmp(value, "ci")) {
> /* ASCII CI mode */
> sb_feat.nci = true;
> /* is this in the opts table anywhere? */
> } else {
> sb_feat.dir_version =
> parse_conf_val(OPT_N,
> N_VERSION,
> value);
> }
> nvflag = 1;
> break;
To be honest, I think this option is weird; it feels to me like two
options merged into one. It has only two valid values: 2 and 'ci'. But 2
is a version, which could be any number, and ci is a modifier for the
given version.
I mean, it looks like it should be accepting "2 or 2ci", rather than "2
or ci". Because the ci is just a modifier and the version remains as 2.
Or we can say that this option only decides ci/non-ci, and then the
option would better to be renamed and turned to a flag.
But either of those would be a visible change to the interface, so I
tried to avoid it. sb_feat.dir_version seems to be used later in the
code even when nci is set and the dir_version is currently always 2.
Thus I put the assignment to happen either way, to avoid hardcoding one
value at multiple places: we save whatever is the value of dir_version,
either from user or from default (XFS_DFL_DIR_VERSION).
At this moment, I'm not saving the ci flag anywhere. Once the opts can
hold strings and not only numbers, it becomes no issue, but now I would
have to add another option as a flag, or save something else than the
version into opt (e.g. 1 or 3 or anything else than 2...) but that would
bring its own issues. The way I did it (ignoring a value that we won't
read from opts anytime soon anyway) seems the safest to me.
>
>> break;
>> case N_FTYPE:
>> - sb_feat.dirftype = getnum(value,
>> - &opts[OPT_N], N_FTYPE);
>> + sb_feat.dirftype =
>> + parse_conf_val(OPT_N, N_FTYPE,
>> + value);
>> break;
>> default:
>> unknown('n', value);
>> @@ -1957,25 +2017,30 @@ main(
>>
>> switch (getsubopt(&p, subopts, &value)) {
>> case R_EXTSIZE:
>> - rtextbytes = getnum(value,
>> - &opts[OPT_R], R_EXTSIZE);
>> + rtextbytes = parse_conf_val(OPT_R,
>> + R_EXTSIZE,
>> + value);
>> break;
>> case R_FILE:
>> - xi.risfile = getnum(value,
>> - &opts[OPT_R], R_FILE);
>> + xi.risfile = parse_conf_val(OPT_R,
>> + R_FILE,
>> + value);
>> break;
>> case R_NAME:
>> case R_DEV:
>> xi.rtname = getstr(value, &opts[OPT_R],
>> R_NAME);
>> + set_conf_val(OPT_R, R_NAME, 1);
>> + set_conf_val(OPT_R, R_DEV, 1);
>> break;
>> case R_SIZE:
>> - rtbytes = getnum(value, &opts[OPT_R],
>> - R_SIZE);
>> + rtbytes = parse_conf_val(OPT_R, R_SIZE,
>> + value);
>> break;
>> case R_NOALIGN:
>> - norsflag = getnum(value, &opts[OPT_R],
>> - R_NOALIGN);
>> + norsflag = parse_conf_val(OPT_R,
>> + R_NOALIGN,
>> + value);
>> break;
>> default:
>> unknown('r', value);
>> @@ -1996,12 +2061,14 @@ main(
>> conflict('s', subopts,
>> S_SECTSIZE,
>> S_SECTLOG);
>> - sectorlog = getnum(value, &opts[OPT_S],
>> - S_SECTLOG);
>> + sectorlog = parse_conf_val(OPT_S,
>> + S_SECTLOG,
>> + value);
>> lsectorlog = sectorlog;
>> sectorsize = 1 << sectorlog;
>> lsectorsize = sectorsize;
>> lslflag = slflag = 1;
>> + set_conf_val(OPT_S, S_LOG, sectorsize);
> Is this right? S_LOG is the log of the sectorsize, right, not the sector size
> itself. Do S_SIZE & S_SECTSIZE need to be stored as well?
This appears to be an issue. I'll fix it.
>
>> break;
>> case S_SIZE:
>> case S_SECTSIZE:
>> @@ -2009,13 +2076,16 @@ main(
>> conflict('s', subopts,
>> S_SECTLOG,
>> S_SECTSIZE);
>> - sectorsize = getnum(value,
>> - &opts[OPT_S], S_SECTSIZE);
>> + sectorsize = parse_conf_val(OPT_S,
>> + S_SECTSIZE,
>> + value);
>> lsectorsize = sectorsize;
>> sectorlog =
>> libxfs_highbit32(sectorsize);
>> lsectorlog = sectorlog;
>> lssflag = ssflag = 1;
>> + set_conf_val(OPT_S,
>> + S_SIZE, sectorlog);
> same questions here - this looks wrong, and other values not stored, do
> they need to be?
>
>> break;
>> default:
>> unknown('s', value);
>>
Thanks,
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 6/7] mkfs: extend opt_params with a value field
2017-07-20 9:29 ` [PATCH 6/7] mkfs: extend opt_params with a value field Jan Tulak
@ 2017-07-27 16:18 ` Luis R. Rodriguez
2017-07-28 14:44 ` Jan Tulak
0 siblings, 1 reply; 39+ messages in thread
From: Luis R. Rodriguez @ 2017-07-27 16:18 UTC (permalink / raw)
To: Jan Tulak; +Cc: linux-xfs
On Thu, Jul 20, 2017 at 11:29:31AM +0200, Jan Tulak wrote:
> +/*
> + * Get and set values to the opts table.
> + */
> +static inline uint64_t
> +get_conf_val(int opt, int subopt)
> +{
> + return opts[opt].subopt_params[subopt].value;
> +}
> +
> +static void
> +set_conf_val(int opt, int subopt, uint64_t val)
> +{
> + struct subopt_param *sp = &opts[opt].subopt_params[subopt];
These are pass by value so its fine to do it this way.
> +
> + sp->value = val;
> +}
> +
> static inline void
> set_conf_raw(int opt, int subopt, const char *value)
> {
> @@ -880,6 +905,18 @@ getnum(
> }
>
> /*
> + * A wrapper for getnum and set_conf_val.
> + */
> +static inline uint64_t
> +parse_conf_val(int opt, int subopt, char *value)
> +{
> + uint64_t num = getnum(value, &opts[opt], subopt);
> +
> + set_conf_val(opt, subopt, num);
> + return num;
> +}
A good comment explaining getnum() will abort if parsing for some reasons fails
is worth it given the way you use parse_conf_val() is rather inconsistent -- in
some places you use the return value, in some other you do not, and someone
reading the code should hopefully get a sense of a relief that error checking
is *not* required given it is already built-in to the helpers used by
parse_conf_val().
This also limits the use of parse_conf_val() in that we won't be able to
customize error messages with better context, so its a good time to evaluate
if we want to continue with that tradition or let parse_conf_val() return
int and if it returns 0 it was successful, otherwise an error value.
Making this return an int and have the return value checked makes this code
IMHO cleaner and subject to growth. Right now this sort of hack IMHO limits
the code to growth, despite the fact that I know this would create more work
at the moment.
Thoughts?
Luis
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct
2017-07-20 9:29 ` [PATCH 1/7] mkfs: Save raw user input field to the opts struct Jan Tulak
@ 2017-07-27 16:27 ` Luis R. Rodriguez
2017-07-28 14:45 ` Jan Tulak
0 siblings, 1 reply; 39+ messages in thread
From: Luis R. Rodriguez @ 2017-07-27 16:27 UTC (permalink / raw)
To: Jan Tulak; +Cc: linux-xfs
On Thu, Jul 20, 2017 at 11:29:26AM +0200, Jan Tulak wrote:
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index a69190b9..4b030101 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -107,6 +107,11 @@ unsigned int sectorsize;
> * sets what is used with simple specifying the subopt (-d file).
> * A special SUBOPT_NEEDS_VAL can be used to require a user-given
> * value in any case.
> + *
> + * raw_input INTERNAL
> + * Filled raw string from the user, so we never lose that information e.g.
> + * to print it back in case of an issue.
> + *
> */
> struct opt_params {
> const char name;
> @@ -122,6 +127,7 @@ struct opt_params {
> long long minval;
> long long maxval;
> long long defaultval;
> + const char *raw_input;
> } subopt_params[MAX_SUBOPTS];
> };
>
> @@ -729,6 +735,18 @@ struct opt_params mopts = {
> */
> #define WHACK_SIZE (128 * 1024)
>
> +static inline void
> +set_conf_raw(struct opt_params *opt, int subopt, const char *value)
> +{
> + opt->subopt_params[subopt].raw_input = value;
> +}
There are no bounds check on the array here, I think set_conf_raw()
should return int and we would check the return value. It could
return -EINVAL if the subopt is invalid for instance.
> +
> +static inline const char *
> +get_conf_raw(const struct opt_params *opt, int subopt)
> +{
> + return opt->subopt_params[subopt].raw_input;
> +}
> +
> /*
> * Convert lsu to lsunit for 512 bytes blocks and check validity of the values.
These are not pass by value.
The usage of set_conf_raw() and get_conf_raw() therefore have strict
constraints and can be only used within certain contexts:
o Since they are pointers the lifetime usage of these functions
are limited to the lifetime of the pointers
o Since they are *currently* used on main() this is fine but this would
limit its use. In the future if we want to defer access to these
pointers outside of main() or if main() uses a library which would
parse some string and free it we'd have to make another change
yet again.
Even if its *OK* today, if some helpers are used later which for instance call
set_conf_raw() and then free the passed pointer right away we are screwed,
leading to potentially using random values. An alternative to limiting the use
of these routines would be to instead have set_conf_raw() to use strdup() and
have it return an int in case of -ENOMEM.
Luis
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 6/7] mkfs: extend opt_params with a value field
2017-07-27 16:18 ` Luis R. Rodriguez
@ 2017-07-28 14:44 ` Jan Tulak
2017-07-29 17:02 ` Luis R. Rodriguez
0 siblings, 1 reply; 39+ messages in thread
From: Jan Tulak @ 2017-07-28 14:44 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: linux-xfs
On 27/07/2017 18:18, Luis R. Rodriguez wrote:
> On Thu, Jul 20, 2017 at 11:29:31AM +0200, Jan Tulak wrote:
>> +/*
>> + * Get and set values to the opts table.
>> + */
>> +static inline uint64_t
>> +get_conf_val(int opt, int subopt)
>> +{
>> + return opts[opt].subopt_params[subopt].value;
>> +}
>> +
>> +static void
>> +set_conf_val(int opt, int subopt, uint64_t val)
>> +{
>> + struct subopt_param *sp = &opts[opt].subopt_params[subopt];
> These are pass by value so its fine to do it this way.
>
>> +
>> + sp->value = val;
>> +}
>> +
>> static inline void
>> set_conf_raw(int opt, int subopt, const char *value)
>> {
>> @@ -880,6 +905,18 @@ getnum(
>> }
>>
>> /*
>> + * A wrapper for getnum and set_conf_val.
>> + */
>> +static inline uint64_t
>> +parse_conf_val(int opt, int subopt, char *value)
>> +{
>> + uint64_t num = getnum(value, &opts[opt], subopt);
>> +
>> + set_conf_val(opt, subopt, num);
>> + return num;
>> +}
> A good comment explaining getnum() will abort if parsing for some reasons fails
> is worth it given the way you use parse_conf_val() is rather inconsistent -- in
> some places you use the return value, in some other you do not, and someone
> reading the code should hopefully get a sense of a relief that error checking
> is *not* required given it is already built-in to the helpers used by
> parse_conf_val().
Good idea.
>
> This also limits the use of parse_conf_val() in that we won't be able to
> customize error messages with better context, so its a good time to evaluate
> if we want to continue with that tradition or let parse_conf_val() return
> int and if it returns 0 it was successful, otherwise an error value.
Quoting also the other email (Re: [PATCH 1/5 v2] mkfs: replace variables
with opts table: -b,d,s options):
> Here parse_conf_val() is used only to update D_AGCOUNT based on the parsed
> value. But note that the return value is ignored. In some other places the
> return value is used. This is inconsistent, and I realize that the reason
> we ignore errors is that getnum() is used, however now is a good time to
> consider if we should just allow the caller to check the error value and
> return a proper error message and bail on their own. This would allow for
> better contextual error messages as the code grows. We can discuss this in
> the patch that adds parse_conf_val().
And do we need better messages? If some value is out of range, or it is
a conflict, there is not much context needed, and better to not have to
care about these errors... Do you have an example when it would be
helpful? If it just spits out a return code, you have to add a check to
every use and you will have many times the same code like what is in
getnum() at this moment:
/* Validity check the result. */
if (c == E_SMALL)
illegal_option(str, opts, index, _("value is too small"));
else if (c == E_BIG)
illegal_option(str, opts, index, _("value is too large"));
else if (c == E_PO2)
illegal_option(str, opts, index, _("value must be a power of 2"));
... and other checks for conflicts, etc...
I think this goes exactly against what I'm trying to do...
Jan
>
> Making this return an int and have the return value checked makes this code
> IMHO cleaner and subject to growth. Right now this sort of hack IMHO limits
> the code to growth, despite the fact that I know this would create more work
> at the moment.
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct
2017-07-27 16:27 ` Luis R. Rodriguez
@ 2017-07-28 14:45 ` Jan Tulak
2017-07-29 17:12 ` Luis R. Rodriguez
0 siblings, 1 reply; 39+ messages in thread
From: Jan Tulak @ 2017-07-28 14:45 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: linux-xfs
On 27/07/2017 18:27, Luis R. Rodriguez wrote:
> On Thu, Jul 20, 2017 at 11:29:26AM +0200, Jan Tulak wrote:
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index a69190b9..4b030101 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -107,6 +107,11 @@ unsigned int sectorsize;
>> * sets what is used with simple specifying the subopt (-d file).
>> * A special SUBOPT_NEEDS_VAL can be used to require a user-given
>> * value in any case.
>> + *
>> + * raw_input INTERNAL
>> + * Filled raw string from the user, so we never lose that information e.g.
>> + * to print it back in case of an issue.
>> + *
>> */
>> struct opt_params {
>> const char name;
>> @@ -122,6 +127,7 @@ struct opt_params {
>> long long minval;
>> long long maxval;
>> long long defaultval;
>> + const char *raw_input;
>> } subopt_params[MAX_SUBOPTS];
>> };
>>
>> @@ -729,6 +735,18 @@ struct opt_params mopts = {
>> */
>> #define WHACK_SIZE (128 * 1024)
>>
>> +static inline void
>> +set_conf_raw(struct opt_params *opt, int subopt, const char *value)
>> +{
>> + opt->subopt_params[subopt].raw_input = value;
>> +}
> There are no bounds check on the array here, I think set_conf_raw()
> should return int and we would check the return value. It could
> return -EINVAL if the subopt is invalid for instance.
Good idea. The only issue is with the return code, that causes some
issues when we are also returning values - I wanted the values to be
turned into uint64. But do we need to return an error? I don't see what
usecase there would be for it, other than detecting a bug. So an assert
might be a better solution - then it can't happen that a wrong index is
used and result not tested.
>> +
>> +static inline const char *
>> +get_conf_raw(const struct opt_params *opt, int subopt)
>> +{
>> + return opt->subopt_params[subopt].raw_input;
>> +}
>> +
>> /*
>> * Convert lsu to lsunit for 512 bytes blocks and check validity of the values.
> These are not pass by value.
>
> The usage of set_conf_raw() and get_conf_raw() therefore have strict
> constraints and can be only used within certain contexts:
>
> o Since they are pointers the lifetime usage of these functions
> are limited to the lifetime of the pointers
> o Since they are *currently* used on main() this is fine but this would
> limit its use. In the future if we want to defer access to these
> pointers outside of main() or if main() uses a library which would
> parse some string and free it we'd have to make another change
> yet again.
>
> Even if its *OK* today, if some helpers are used later which for instance call
> set_conf_raw() and then free the passed pointer right away we are screwed,
> leading to potentially using random values. An alternative to limiting the use
> of these routines would be to instead have set_conf_raw() to use strdup() and
> have it return an int in case of -ENOMEM.
>
> Luis
Sounds reasonable.
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 6/7] mkfs: extend opt_params with a value field
2017-07-28 14:44 ` Jan Tulak
@ 2017-07-29 17:02 ` Luis R. Rodriguez
2017-08-02 14:43 ` Jan Tulak
0 siblings, 1 reply; 39+ messages in thread
From: Luis R. Rodriguez @ 2017-07-29 17:02 UTC (permalink / raw)
To: Jan Tulak; +Cc: Luis R. Rodriguez, linux-xfs
On Fri, Jul 28, 2017 at 04:44:50PM +0200, Jan Tulak wrote:
> On 27/07/2017 18:18, Luis R. Rodriguez wrote:
> > On Thu, Jul 20, 2017 at 11:29:31AM +0200, Jan Tulak wrote:
> > This also limits the use of parse_conf_val() in that we won't be able to
> > customize error messages with better context, so its a good time to evaluate
> > if we want to continue with that tradition or let parse_conf_val() return
> > int and if it returns 0 it was successful, otherwise an error value.
> Quoting also the other email (Re: [PATCH 1/5 v2] mkfs: replace variables
> with opts table: -b,d,s options):
> > Here parse_conf_val() is used only to update D_AGCOUNT based on the parsed
> > value. But note that the return value is ignored. In some other places the
> > return value is used. This is inconsistent, and I realize that the reason
> > we ignore errors is that getnum() is used, however now is a good time to
> > consider if we should just allow the caller to check the error value and
> > return a proper error message and bail on their own. This would allow for
> > better contextual error messages as the code grows. We can discuss this in
> > the patch that adds parse_conf_val().
>
> And do we need better messages?
We could later when parsing the configuration file, yes. An abort is fine too but
seems rather grotesque. But also more importantly this is a matter of style and I
realize the old style is to just abort/assert, so I figured it would be a good time
now to ask ourselves if we want proper error messages dealt with / passed slowly
moving forward.
But yes, I do suspect we may want better error messages when parsing these. This
may mean for instance we want a string built into the struct that defines the sobopt
so we can use it to inform userspace using a pointer to the description rather than
doing a switch statement on each one and interpreting back to plain english.
> If some value is out of range, or it is a
> conflict, there is not much context needed, and better to not have to care
> about these errors... Do you have an example when it would be helpful? If it
> just spits out a return code, you have to add a check to every use and you
> will have many times the same code like what is in getnum() at this moment
Not really, if we are parsing say D_AGCOUNT we could have a member as part of the
struct, say "description" then we can use say subopt[D_AGCOUNT].description on
the error message, perhaps the only thing that would change for instance would be
the context on which the error was run into, command line option passed or config
file read, say with the filename and line number.
How would we be able to detect an error happened and pass exactly where the
error happened otherwise on a config file for example?
Luis
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct
2017-07-28 14:45 ` Jan Tulak
@ 2017-07-29 17:12 ` Luis R. Rodriguez
2017-08-02 14:30 ` Jan Tulak
0 siblings, 1 reply; 39+ messages in thread
From: Luis R. Rodriguez @ 2017-07-29 17:12 UTC (permalink / raw)
To: Jan Tulak; +Cc: Luis R. Rodriguez, linux-xfs
On Fri, Jul 28, 2017 at 04:45:58PM +0200, Jan Tulak wrote:
>
>
> On 27/07/2017 18:27, Luis R. Rodriguez wrote:
> > On Thu, Jul 20, 2017 at 11:29:26AM +0200, Jan Tulak wrote:
> > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > > index a69190b9..4b030101 100644
> > > --- a/mkfs/xfs_mkfs.c
> > > +++ b/mkfs/xfs_mkfs.c
> > > @@ -107,6 +107,11 @@ unsigned int sectorsize;
> > > * sets what is used with simple specifying the subopt (-d file).
> > > * A special SUBOPT_NEEDS_VAL can be used to require a user-given
> > > * value in any case.
> > > + *
> > > + * raw_input INTERNAL
> > > + * Filled raw string from the user, so we never lose that information e.g.
> > > + * to print it back in case of an issue.
> > > + *
> > > */
> > > struct opt_params {
> > > const char name;
> > > @@ -122,6 +127,7 @@ struct opt_params {
> > > long long minval;
> > > long long maxval;
> > > long long defaultval;
> > > + const char *raw_input;
> > > } subopt_params[MAX_SUBOPTS];
> > > };
> > > @@ -729,6 +735,18 @@ struct opt_params mopts = {
> > > */
> > > #define WHACK_SIZE (128 * 1024)
> > > +static inline void
> > > +set_conf_raw(struct opt_params *opt, int subopt, const char *value)
> > > +{
> > > + opt->subopt_params[subopt].raw_input = value;
> > > +}
> > There are no bounds check on the array here, I think set_conf_raw()
> > should return int and we would check the return value. It could
> > return -EINVAL if the subopt is invalid for instance.
> Good idea. The only issue is with the return code, that causes some issues
> when we are also returning values - I wanted the values to be turned into
> uint64. But do we need to return an error? I don't see what usecase there
> would be for it, other than detecting a bug. So an assert might be a better
> solution - then it can't happen that a wrong index is used and result not
> tested.
The setting of the value can be done by using an extra argument pointer. Then
if its set it be assigned. Otherwise it would be left alone. The return value
would return 0 on success, otherwise a standard return value indicating the
cause of the error.
I don't think we need the too small or too big, a simple range issue should
suffice and we have -ERANGE.
Luis
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct
2017-07-29 17:12 ` Luis R. Rodriguez
@ 2017-08-02 14:30 ` Jan Tulak
2017-08-02 15:51 ` Jan Tulak
2017-08-02 19:19 ` Luis R. Rodriguez
0 siblings, 2 replies; 39+ messages in thread
From: Jan Tulak @ 2017-08-02 14:30 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: linux-xfs
On 29/07/2017 19:12, Luis R. Rodriguez wrote:
> On Fri, Jul 28, 2017 at 04:45:58PM +0200, Jan Tulak wrote:
>>
>> On 27/07/2017 18:27, Luis R. Rodriguez wrote:
>>> On Thu, Jul 20, 2017 at 11:29:26AM +0200, Jan Tulak wrote:
>>>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>>>> index a69190b9..4b030101 100644
>>>> --- a/mkfs/xfs_mkfs.c
>>>> +++ b/mkfs/xfs_mkfs.c
>>>> @@ -107,6 +107,11 @@ unsigned int sectorsize;
>>>> * sets what is used with simple specifying the subopt (-d file).
>>>> * A special SUBOPT_NEEDS_VAL can be used to require a user-given
>>>> * value in any case.
>>>> + *
>>>> + * raw_input INTERNAL
>>>> + * Filled raw string from the user, so we never lose that information e.g.
>>>> + * to print it back in case of an issue.
>>>> + *
>>>> */
>>>> struct opt_params {
>>>> const char name;
>>>> @@ -122,6 +127,7 @@ struct opt_params {
>>>> long long minval;
>>>> long long maxval;
>>>> long long defaultval;
>>>> + const char *raw_input;
>>>> } subopt_params[MAX_SUBOPTS];
>>>> };
>>>> @@ -729,6 +735,18 @@ struct opt_params mopts = {
>>>> */
>>>> #define WHACK_SIZE (128 * 1024)
>>>> +static inline void
>>>> +set_conf_raw(struct opt_params *opt, int subopt, const char *value)
>>>> +{
>>>> + opt->subopt_params[subopt].raw_input = value;
>>>> +}
>>> There are no bounds check on the array here, I think set_conf_raw()
>>> should return int and we would check the return value. It could
>>> return -EINVAL if the subopt is invalid for instance.
>> Good idea. The only issue is with the return code, that causes some issues
>> when we are also returning values - I wanted the values to be turned into
>> uint64. But do we need to return an error? I don't see what usecase there
>> would be for it, other than detecting a bug. So an assert might be a better
>> solution - then it can't happen that a wrong index is used and result not
>> tested.
> The setting of the value can be done by using an extra argument pointer. Then
> if its set it be assigned. Otherwise it would be left alone. The return value
> would return 0 on success, otherwise a standard return value indicating the
> cause of the error.
I strongly prefer to return the value, not an error code. We can do the
other way around, put the error code into an argument to get roughly the
same result, while constructions like set_conf_raw(FOO, BAR, baz *
get_conf_raw(FOO, BAR)) will continue to work without the need for
intermediate variables.
The *_raw functions are used on few places only, so it would be only a
small issue there, but for consistency, (get|set)_conf_val should have
the same behavior and an intermediate variable for every use of those
would be really annoying. So, how about this?
static inline void
set_conf_raw(struct opt_params *opt, int subopt, const char *value, int
*err)
{
if (subopt < 0 || subopt >= MAX_SUBOPTS) {
if (err != NULL) *err = EINVAL;
return;
}
opt->subopt_params[subopt].raw_input = value;
}
> I don't think we need the too small or too big, a simple range issue should
> suffice and we have -ERANGE.
>
At this moment, we are telling if it is too small or too big, but when
there is no standard error code for that, ERANGE has to suffice.
Cheers,
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 6/7] mkfs: extend opt_params with a value field
2017-07-29 17:02 ` Luis R. Rodriguez
@ 2017-08-02 14:43 ` Jan Tulak
2017-08-02 16:57 ` Luis R. Rodriguez
0 siblings, 1 reply; 39+ messages in thread
From: Jan Tulak @ 2017-08-02 14:43 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: linux-xfs
On 29/07/2017 19:02, Luis R. Rodriguez wrote:
> On Fri, Jul 28, 2017 at 04:44:50PM +0200, Jan Tulak wrote:
>> And do we need better messages?
> We could later when parsing the configuration file, yes. An abort is fine too but
> seems rather grotesque. But also more importantly this is a matter of style and I
> realize the old style is to just abort/assert, so I figured it would be a good time
> now to ask ourselves if we want proper error messages dealt with / passed slowly
> moving forward.
>
> But yes, I do suspect we may want better error messages when parsing these. This
> may mean for instance we want a string built into the struct that defines the sobopt
> so we can use it to inform userspace using a pointer to the description rather than
> doing a switch statement on each one and interpreting back to plain english.
>
The reason for better messages is reasonable. About the style... well,
it makes sense. But I would certainly not do this in this set. So, I
think about a way how to keep the current behavior, but slowly build up
the ground for something like what you suggest.
Using the same style of error-returning logic from the other email
([PATCH 1/7] mkfs: Save raw ...), the error argument pointer would be
optional. So, when you do parse_conf_val(OPT_D, D_AGCOUNT, str, NULL);
then you get the old behavior and any error causes a termination inside
of this function. But if you instead pass some pointer:
parse_conf_val(OPT_D, D_AGCOUNT, str, &err); then right now, we would
print the message but do not terminate. And later on, some other message
handling can be added.
>> If some value is out of range, or it is a
>> conflict, there is not much context needed, and better to not have to care
>> about these errors... Do you have an example when it would be helpful? If it
>> just spits out a return code, you have to add a check to every use and you
>> will have many times the same code like what is in getnum() at this moment
> Not really, if we are parsing say D_AGCOUNT we could have a member as part of the
> struct, say "description" then we can use say subopt[D_AGCOUNT].description on
> the error message, perhaps the only thing that would change for instance would be
> the context on which the error was run into, command line option passed or config
> file read, say with the filename and line number.
>
> How would we be able to detect an error happened and pass exactly where the
> error happened otherwise on a config file for example?
Yes, I see the issue - getnum finds an error, but it doesn't know the
line in the config file to report it. But with what I write above about
the error handling, this could work.
if (c < sp->minval) {
if (config_file) illegal_config(str, line, opts, index, _("value is
too small"), err);
else illegal_option(str, opts, index, _("value is too small"), err);
}
... and later in the code, if you are in the config file, you could do
something like:
parse_conf_val(opt, subopt, str, &err);
if (err) report_invalid_line(current_line);
Thoughts?
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct
2017-08-02 14:30 ` Jan Tulak
@ 2017-08-02 15:51 ` Jan Tulak
2017-08-02 19:41 ` Luis R. Rodriguez
2017-08-02 19:19 ` Luis R. Rodriguez
1 sibling, 1 reply; 39+ messages in thread
From: Jan Tulak @ 2017-08-02 15:51 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: linux-xfs
An addendum to the previous email.
On 02/08/2017 16:30, Jan Tulak wrote:
> On 29/07/2017 19:12, Luis R. Rodriguez wrote:
>> On Fri, Jul 28, 2017 at 04:45:58PM +0200, Jan Tulak wrote:
>>>
>>> On 27/07/2017 18:27, Luis R. Rodriguez wrote:
>>>> On Thu, Jul 20, 2017 at 11:29:26AM +0200, Jan Tulak wrote:
>>>>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>>>>> index a69190b9..4b030101 100644
>>>>> --- a/mkfs/xfs_mkfs.c
>>>>> +++ b/mkfs/xfs_mkfs.c
>>>>> @@ -107,6 +107,11 @@ unsigned int sectorsize;
>>>>> * sets what is used with simple specifying the subopt (-d
>>>>> file).
>>>>> * A special SUBOPT_NEEDS_VAL can be used to require a
>>>>> user-given
>>>>> * value in any case.
>>>>> + *
>>>>> + * raw_input INTERNAL
>>>>> + * Filled raw string from the user, so we never lose that
>>>>> information e.g.
>>>>> + * to print it back in case of an issue.
>>>>> + *
>>>>> */
>>>>> struct opt_params {
>>>>> const char name;
>>>>> @@ -122,6 +127,7 @@ struct opt_params {
>>>>> long long minval;
>>>>> long long maxval;
>>>>> long long defaultval;
>>>>> + const char *raw_input;
>>>>> } subopt_params[MAX_SUBOPTS];
>>>>> };
>>>>> @@ -729,6 +735,18 @@ struct opt_params mopts = {
>>>>> */
>>>>> #define WHACK_SIZE (128 * 1024)
>>>>> +static inline void
>>>>> +set_conf_raw(struct opt_params *opt, int subopt, const char *value)
>>>>> +{
>>>>> + opt->subopt_params[subopt].raw_input = value;
>>>>> +}
>>>> There are no bounds check on the array here, I think set_conf_raw()
>>>> should return int and we would check the return value. It could
>>>> return -EINVAL if the subopt is invalid for instance.
>>> Good idea. The only issue is with the return code, that causes some
>>> issues
>>> when we are also returning values - I wanted the values to be turned
>>> into
>>> uint64. But do we need to return an error? I don't see what usecase
>>> there
>>> would be for it, other than detecting a bug. So an assert might be a
>>> better
>>> solution - then it can't happen that a wrong index is used and
>>> result not
>>> tested.
>> The setting of the value can be done by using an extra argument
>> pointer. Then
>> if its set it be assigned. Otherwise it would be left alone. The
>> return value
>> would return 0 on success, otherwise a standard return value
>> indicating the
>> cause of the error.
> I strongly prefer to return the value, not an error code. We can do
> the other way around, put the error code into an argument to get
> roughly the same result, while constructions like set_conf_raw(FOO,
> BAR, baz * get_conf_raw(FOO, BAR)) will continue to work without the
> need for intermediate variables.
>
> The *_raw functions are used on few places only, so it would be only a
> small issue there, but for consistency, (get|set)_conf_val should have
> the same behavior and an intermediate variable for every use of those
> would be really annoying. So, how about this?
>
> static inline void
> set_conf_raw(struct opt_params *opt, int subopt, const char *value,
> int *err)
> {
> if (subopt < 0 || subopt >= MAX_SUBOPTS) {
> if (err != NULL) *err = EINVAL;
> return;
> }
> opt->subopt_params[subopt].raw_input = value;
> }
>
I just realized that there is probably no reason for set_conf_raw to
expect invalid subopt - that's clearly a bug and we should just print a
message and die, because who knows what happened... But for errors that
can arose from user input, the style presented above is still valid.
Jan
>> I don't think we need the too small or too big, a simple range issue
>> should
>> suffice and we have -ERANGE.
>>
> At this moment, we are telling if it is too small or too big, but when
> there is no standard error code for that, ERANGE has to suffice.
>
> Cheers,
> Jan
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 6/7] mkfs: extend opt_params with a value field
2017-08-02 14:43 ` Jan Tulak
@ 2017-08-02 16:57 ` Luis R. Rodriguez
2017-08-02 18:11 ` Jan Tulak
0 siblings, 1 reply; 39+ messages in thread
From: Luis R. Rodriguez @ 2017-08-02 16:57 UTC (permalink / raw)
To: Jan Tulak; +Cc: Luis R. Rodriguez, linux-xfs
On Wed, Aug 02, 2017 at 04:43:09PM +0200, Jan Tulak wrote:
> On 29/07/2017 19:02, Luis R. Rodriguez wrote:
> > On Fri, Jul 28, 2017 at 04:44:50PM +0200, Jan Tulak wrote:
> > > And do we need better messages?
> > We could later when parsing the configuration file, yes. An abort is fine too but
> > seems rather grotesque. But also more importantly this is a matter of style and I
> > realize the old style is to just abort/assert, so I figured it would be a good time
> > now to ask ourselves if we want proper error messages dealt with / passed slowly
> > moving forward.
> >
> > But yes, I do suspect we may want better error messages when parsing these. This
> > may mean for instance we want a string built into the struct that defines the sobopt
> > so we can use it to inform userspace using a pointer to the description rather than
> > doing a switch statement on each one and interpreting back to plain english.
> >
> The reason for better messages is reasonable. About the style... well, it
> makes sense. But I would certainly not do this in this set. So, I think
> about a way how to keep the current behavior, but slowly build up the ground
> for something like what you suggest.
>
> Using the same style of error-returning logic from the other email ([PATCH
> 1/7] mkfs: Save raw ...), the error argument pointer would be optional. So,
> when you do parse_conf_val(OPT_D, D_AGCOUNT, str, NULL); then you get the
> old behavior and any error causes a termination inside of this function. But
> if you instead pass some pointer: parse_conf_val(OPT_D, D_AGCOUNT, str,
> &err); then right now, we would print the message but do not terminate. And
> later on, some other message handling can be added.
I think this is grotesque. Also, how would we know an error did happen then?
> > > If some value is out of range, or it is a
> > > conflict, there is not much context needed, and better to not have to care
> > > about these errors... Do you have an example when it would be helpful? If it
> > > just spits out a return code, you have to add a check to every use and you
> > > will have many times the same code like what is in getnum() at this moment
> > Not really, if we are parsing say D_AGCOUNT we could have a member as part of the
> > struct, say "description" then we can use say subopt[D_AGCOUNT].description on
> > the error message, perhaps the only thing that would change for instance would be
> > the context on which the error was run into, command line option passed or config
> > file read, say with the filename and line number.
> >
> > How would we be able to detect an error happened and pass exactly where the
> > error happened otherwise on a config file for example?
> Yes, I see the issue - getnum finds an error, but it doesn't know the line
> in the config file to report it. But with what I write above about the error
> handling, this could work.
>
> if (c < sp->minval) {
> if (config_file) illegal_config(str, line, opts, index, _("value is too
> small"), err);
How would it know what str and line are?
> else illegal_option(str, opts, index, _("value is too small"), err);
> }
This seems convoluted and I don't really like it one bit.
> ... and later in the code, if you are in the config file, you could do
> something like:
>
> parse_conf_val(opt, subopt, str, &err);
> if (err) report_invalid_line(current_line);
>
> Thoughts?
Just my take: I prefer we do the right thing from the start. Even
if it takes us ages to move forward, baby steps, and clean patches
and evolutions, moving slowly away from the old crazy habits.
Luis
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 6/7] mkfs: extend opt_params with a value field
2017-08-02 16:57 ` Luis R. Rodriguez
@ 2017-08-02 18:11 ` Jan Tulak
2017-08-02 19:48 ` Luis R. Rodriguez
0 siblings, 1 reply; 39+ messages in thread
From: Jan Tulak @ 2017-08-02 18:11 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: linux-xfs
On 02/08/2017 18:57, Luis R. Rodriguez wrote:
> On Wed, Aug 02, 2017 at 04:43:09PM +0200, Jan Tulak wrote:
>> On 29/07/2017 19:02, Luis R. Rodriguez wrote:
>>> On Fri, Jul 28, 2017 at 04:44:50PM +0200, Jan Tulak wrote:
>>>> And do we need better messages?
>>> We could later when parsing the configuration file, yes. An abort is fine too but
>>> seems rather grotesque. But also more importantly this is a matter of style and I
>>> realize the old style is to just abort/assert, so I figured it would be a good time
>>> now to ask ourselves if we want proper error messages dealt with / passed slowly
>>> moving forward.
>>>
>>> But yes, I do suspect we may want better error messages when parsing these. This
>>> may mean for instance we want a string built into the struct that defines the sobopt
>>> so we can use it to inform userspace using a pointer to the description rather than
>>> doing a switch statement on each one and interpreting back to plain english.
>>>
>> The reason for better messages is reasonable. About the style... well, it
>> makes sense. But I would certainly not do this in this set. So, I think
>> about a way how to keep the current behavior, but slowly build up the ground
>> for something like what you suggest.
>>
>> Using the same style of error-returning logic from the other email ([PATCH
>> 1/7] mkfs: Save raw ...), the error argument pointer would be optional. So,
>> when you do parse_conf_val(OPT_D, D_AGCOUNT, str, NULL); then you get the
>> old behavior and any error causes a termination inside of this function. But
>> if you instead pass some pointer: parse_conf_val(OPT_D, D_AGCOUNT, str,
>> &err); then right now, we would print the message but do not terminate. And
>> later on, some other message handling can be added.
> I think this is grotesque. Also, how would we know an error did happen then?
Just test if err is 0 or not, same as with errno.h. And the dual
behavior would be only a temporary measure. Once all uses are converted
to the new behavior, the terminating part can be dropped and the error
argument will become mandatory.
>
>>>> If some value is out of range, or it is a
>>>> conflict, there is not much context needed, and better to not have to care
>>>> about these errors... Do you have an example when it would be helpful? If it
>>>> just spits out a return code, you have to add a check to every use and you
>>>> will have many times the same code like what is in getnum() at this moment
>>> Not really, if we are parsing say D_AGCOUNT we could have a member as part of the
>>> struct, say "description" then we can use say subopt[D_AGCOUNT].description on
>>> the error message, perhaps the only thing that would change for instance would be
>>> the context on which the error was run into, command line option passed or config
>>> file read, say with the filename and line number.
>>>
>>> How would we be able to detect an error happened and pass exactly where the
>>> error happened otherwise on a config file for example?
>> Yes, I see the issue - getnum finds an error, but it doesn't know the line
>> in the config file to report it. But with what I write above about the error
>> handling, this could work.
>>
>> if (c < sp->minval) {
>> if (config_file) illegal_config(str, line, opts, index, _("value is too
>> small"), err);
> How would it know what str and line are?
The "line" argument is a mistake, it shouldn't be here - it is solved by
the snipped below. The "str" is already there, it is what getnum()
parses - only the "err" at the end would be added.
>
>> else illegal_option(str, opts, index, _("value is too small"), err);
>> }
> This seems convoluted and I don't really like it one bit.
>
>> ... and later in the code, if you are in the config file, you could do
>> something like:
>>
>> parse_conf_val(opt, subopt, str, &err);
>> if (err) report_invalid_line(current_line);
>>
>> Thoughts?
> Just my take: I prefer we do the right thing from the start. Even
> if it takes us ages to move forward, baby steps, and clean patches
> and evolutions, moving slowly away from the old crazy habits.
In which case, I would just continue as we do now (terminating on an
error), and then change it in the whole mkfs at once in some other patch
set. There always will be something old and ugly we have to use
temporarily, or we end up stashing one patchset after another, always
trying to fix some other thing first, and never really fixing anything.
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct
2017-08-02 14:30 ` Jan Tulak
2017-08-02 15:51 ` Jan Tulak
@ 2017-08-02 19:19 ` Luis R. Rodriguez
2017-08-03 13:07 ` Jan Tulak
1 sibling, 1 reply; 39+ messages in thread
From: Luis R. Rodriguez @ 2017-08-02 19:19 UTC (permalink / raw)
To: Jan Tulak; +Cc: Luis R. Rodriguez, linux-xfs
On Wed, Aug 02, 2017 at 04:30:09PM +0200, Jan Tulak wrote:
> On 29/07/2017 19:12, Luis R. Rodriguez wrote:
> > On Fri, Jul 28, 2017 at 04:45:58PM +0200, Jan Tulak wrote:
> > >
> > > On 27/07/2017 18:27, Luis R. Rodriguez wrote:
> > > > On Thu, Jul 20, 2017 at 11:29:26AM +0200, Jan Tulak wrote:
> > > > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > > > > index a69190b9..4b030101 100644
> > > > > --- a/mkfs/xfs_mkfs.c
> > > > > +++ b/mkfs/xfs_mkfs.c
> > > > > @@ -107,6 +107,11 @@ unsigned int sectorsize;
> > > > > * sets what is used with simple specifying the subopt (-d file).
> > > > > * A special SUBOPT_NEEDS_VAL can be used to require a user-given
> > > > > * value in any case.
> > > > > + *
> > > > > + * raw_input INTERNAL
> > > > > + * Filled raw string from the user, so we never lose that information e.g.
> > > > > + * to print it back in case of an issue.
> > > > > + *
> > > > > */
> > > > > struct opt_params {
> > > > > const char name;
> > > > > @@ -122,6 +127,7 @@ struct opt_params {
> > > > > long long minval;
> > > > > long long maxval;
> > > > > long long defaultval;
> > > > > + const char *raw_input;
> > > > > } subopt_params[MAX_SUBOPTS];
> > > > > };
> > > > > @@ -729,6 +735,18 @@ struct opt_params mopts = {
> > > > > */
> > > > > #define WHACK_SIZE (128 * 1024)
> > > > > +static inline void
> > > > > +set_conf_raw(struct opt_params *opt, int subopt, const char *value)
> > > > > +{
> > > > > + opt->subopt_params[subopt].raw_input = value;
> > > > > +}
> > > > There are no bounds check on the array here, I think set_conf_raw()
> > > > should return int and we would check the return value. It could
> > > > return -EINVAL if the subopt is invalid for instance.
> > > Good idea. The only issue is with the return code, that causes some issues
> > > when we are also returning values - I wanted the values to be turned into
> > > uint64. But do we need to return an error? I don't see what usecase there
> > > would be for it, other than detecting a bug. So an assert might be a better
> > > solution - then it can't happen that a wrong index is used and result not
> > > tested.
> > The setting of the value can be done by using an extra argument pointer. Then
> > if its set it be assigned. Otherwise it would be left alone. The return value
> > would return 0 on success, otherwise a standard return value indicating the
> > cause of the error.
> I strongly prefer to return the value, not an error code. We can do the
> other way around, put the error code into an argument to get roughly the
> same result, while constructions like set_conf_raw(FOO, BAR, baz *
> get_conf_raw(FOO, BAR)) will continue to work without the need for
> intermediate variables.
>
> The *_raw functions are used on few places only, so it would be only a small
> issue there, but for consistency, (get|set)_conf_val should have the same
> behavior and an intermediate variable for every use of those would be really
> annoying. So, how about this?
It would not be intermediate, the main error variable from the start of
each function could be used, as is typical in many properly written C
programs.
> static inline void
> set_conf_raw(struct opt_params *opt, int subopt, const char *value, int
> *err)
> {
> if (subopt < 0 || subopt >= MAX_SUBOPTS) {
> if (err != NULL) *err = EINVAL;
> return;
> }
> opt->subopt_params[subopt].raw_input = value;
> }
If you go with the strdup thing to avoid limiting the context of the use of
the pointer then you'll still have to return an error or abort, and I think
returning an error is best.
> > I don't think we need the too small or too big, a simple range issue should
> > suffice and we have -ERANGE.
> >
> At this moment, we are telling if it is too small or too big, but when there
> is no standard error code for that, ERANGE has to suffice.
Sure, my point was that we have special values for too big or too small, and
I consider that hacky, we could just *say* if it was too big or too small
but just use ERANGE as its standard and non-hacky.
Luis
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct
2017-08-02 15:51 ` Jan Tulak
@ 2017-08-02 19:41 ` Luis R. Rodriguez
0 siblings, 0 replies; 39+ messages in thread
From: Luis R. Rodriguez @ 2017-08-02 19:41 UTC (permalink / raw)
To: Jan Tulak, g; +Cc: Luis R. Rodriguez, linux-xfs
On Wed, Aug 02, 2017 at 05:51:47PM +0200, Jan Tulak wrote:
> An addendum to the previous email.
>
> On 02/08/2017 16:30, Jan Tulak wrote:
> > On 29/07/2017 19:12, Luis R. Rodriguez wrote:
> > > On Fri, Jul 28, 2017 at 04:45:58PM +0200, Jan Tulak wrote:
> > > >
> > > > On 27/07/2017 18:27, Luis R. Rodriguez wrote:
> > > > > On Thu, Jul 20, 2017 at 11:29:26AM +0200, Jan Tulak wrote:
> > > > > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > > > > > index a69190b9..4b030101 100644
> > > > > > --- a/mkfs/xfs_mkfs.c
> > > > > > +++ b/mkfs/xfs_mkfs.c
> > > > > > @@ -107,6 +107,11 @@ unsigned int sectorsize;
> > > > > > * sets what is used with simple specifying the
> > > > > > subopt (-d file).
> > > > > > * A special SUBOPT_NEEDS_VAL can be used to
> > > > > > require a user-given
> > > > > > * value in any case.
> > > > > > + *
> > > > > > + * raw_input INTERNAL
> > > > > > + * Filled raw string from the user, so we never
> > > > > > lose that information e.g.
> > > > > > + * to print it back in case of an issue.
> > > > > > + *
> > > > > > */
> > > > > > struct opt_params {
> > > > > > const char name;
> > > > > > @@ -122,6 +127,7 @@ struct opt_params {
> > > > > > long long minval;
> > > > > > long long maxval;
> > > > > > long long defaultval;
> > > > > > + const char *raw_input;
> > > > > > } subopt_params[MAX_SUBOPTS];
> > > > > > };
> > > > > > @@ -729,6 +735,18 @@ struct opt_params mopts = {
> > > > > > */
> > > > > > #define WHACK_SIZE (128 * 1024)
> > > > > > +static inline void
> > > > > > +set_conf_raw(struct opt_params *opt, int subopt, const char *value)
> > > > > > +{
> > > > > > + opt->subopt_params[subopt].raw_input = value;
> > > > > > +}
> > > > > There are no bounds check on the array here, I think set_conf_raw()
> > > > > should return int and we would check the return value. It could
> > > > > return -EINVAL if the subopt is invalid for instance.
> > > > Good idea. The only issue is with the return code, that causes
> > > > some issues
> > > > when we are also returning values - I wanted the values to be
> > > > turned into
> > > > uint64. But do we need to return an error? I don't see what
> > > > usecase there
> > > > would be for it, other than detecting a bug. So an assert might
> > > > be a better
> > > > solution - then it can't happen that a wrong index is used and
> > > > result not
> > > > tested.
> > > The setting of the value can be done by using an extra argument
> > > pointer. Then
> > > if its set it be assigned. Otherwise it would be left alone. The
> > > return value
> > > would return 0 on success, otherwise a standard return value
> > > indicating the
> > > cause of the error.
> > I strongly prefer to return the value, not an error code. We can do the
> > other way around, put the error code into an argument to get roughly the
> > same result, while constructions like set_conf_raw(FOO, BAR, baz *
> > get_conf_raw(FOO, BAR)) will continue to work without the need for
> > intermediate variables.
> >
> > The *_raw functions are used on few places only, so it would be only a
> > small issue there, but for consistency, (get|set)_conf_val should have
> > the same behavior and an intermediate variable for every use of those
> > would be really annoying. So, how about this?
> >
> > static inline void
> > set_conf_raw(struct opt_params *opt, int subopt, const char *value, int
> > *err)
> > {
> > if (subopt < 0 || subopt >= MAX_SUBOPTS) {
> > if (err != NULL) *err = EINVAL;
> > return;
> > }
> > opt->subopt_params[subopt].raw_input = value;
> > }
> >
> I just realized that there is probably no reason for set_conf_raw to expect
> invalid subopt - that's clearly a bug and we should just print a message and
> die, because who knows what happened... But for errors that can arose from
> user input, the style presented above is still valid.
True however the issue of limiting the context of the use of the pointer
is still present and if you strdup you have to check for ENOMEM. If this
is done in a helper then its done only once, specially if a description
for the subopt is placed into the subopt structure.
Luis
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 6/7] mkfs: extend opt_params with a value field
2017-08-02 18:11 ` Jan Tulak
@ 2017-08-02 19:48 ` Luis R. Rodriguez
2017-08-03 13:23 ` Jan Tulak
0 siblings, 1 reply; 39+ messages in thread
From: Luis R. Rodriguez @ 2017-08-02 19:48 UTC (permalink / raw)
To: Jan Tulak; +Cc: Luis R. Rodriguez, linux-xfs
On Wed, Aug 02, 2017 at 08:11:38PM +0200, Jan Tulak wrote:
> On 02/08/2017 18:57, Luis R. Rodriguez wrote:
> > On Wed, Aug 02, 2017 at 04:43:09PM +0200, Jan Tulak wrote:
> > > The reason for better messages is reasonable. About the style... well, it
> > > makes sense. But I would certainly not do this in this set. So, I think
> > > about a way how to keep the current behavior, but slowly build up the ground
> > > for something like what you suggest.
> > >
> > > Using the same style of error-returning logic from the other email ([PATCH
> > > 1/7] mkfs: Save raw ...), the error argument pointer would be optional. So,
> > > when you do parse_conf_val(OPT_D, D_AGCOUNT, str, NULL); then you get the
> > > old behavior and any error causes a termination inside of this function. But
> > > if you instead pass some pointer: parse_conf_val(OPT_D, D_AGCOUNT, str,
> > > &err); then right now, we would print the message but do not terminate. And
> > > later on, some other message handling can be added.
> > I think this is grotesque. Also, how would we know an error did happen then?
>
> Just test if err is 0 or not, same as with errno.h. And the dual behavior
> would be only a temporary measure. Once all uses are converted to the new
> behavior, the terminating part can be dropped and the error argument will
> become mandatory.
Yes, I think this is not nice if I understood what you say below correctly.
> > > > > If some value is out of range, or it is a
> > > > > conflict, there is not much context needed, and better to not have to care
> > > > > about these errors... Do you have an example when it would be helpful? If it
> > > > > just spits out a return code, you have to add a check to every use and you
> > > > > will have many times the same code like what is in getnum() at this moment
> > > > Not really, if we are parsing say D_AGCOUNT we could have a member as part of the
> > > > struct, say "description" then we can use say subopt[D_AGCOUNT].description on
> > > > the error message, perhaps the only thing that would change for instance would be
> > > > the context on which the error was run into, command line option passed or config
> > > > file read, say with the filename and line number.
> > > >
> > > > How would we be able to detect an error happened and pass exactly where the
> > > > error happened otherwise on a config file for example?
> > > Yes, I see the issue - getnum finds an error, but it doesn't know the line
> > > in the config file to report it. But with what I write above about the error
> > > handling, this could work.
> > >
> > > if (c < sp->minval) {
> > > if (config_file) illegal_config(str, line, opts, index, _("value is too
> > > small"), err);
> > How would it know what str and line are?
>
> The "line" argument is a mistake, it shouldn't be here - it is solved by the
> snipped below. The "str" is already there, it is what getnum() parses - only
> the "err" at the end would be added.
So you mean the error code would only be visible on the print out, but not
passed to the caller as a starting step. A secondary step would go and change
that to then return the error code and have each caller check it?
> >
> > > else illegal_option(str, opts, index, _("value is too small"), err);
> > > }
> > This seems convoluted and I don't really like it one bit.
> >
> > > ... and later in the code, if you are in the config file, you could do
> > > something like:
> > >
> > > parse_conf_val(opt, subopt, str, &err);
> > > if (err) report_invalid_line(current_line);
> > >
> > > Thoughts?
> > Just my take: I prefer we do the right thing from the start. Even
> > if it takes us ages to move forward, baby steps, and clean patches
> > and evolutions, moving slowly away from the old crazy habits.
>
> In which case, I would just continue as we do now (terminating on an error),
> and then change it in the whole mkfs at once in some other patch set. There
> always will be something old and ugly we have to use temporarily, or we end
> up stashing one patchset after another, always trying to fix some other
> thing first, and never really fixing anything.
It seems like a change which could be fit into your series, it is more work,
but then again most of the work here was expected to be significant and doing
away with a lot of crap.
Up to you, I just am providing my own feedback and making clear the requirements
for the config stuff. I think it will be silly to add config support without
having the ability to catch errors and then indicate on its own however it
chooses exactly where the error occurred.
Will you take on doing the error code changes after ?
Luis
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct
2017-08-02 19:19 ` Luis R. Rodriguez
@ 2017-08-03 13:07 ` Jan Tulak
2017-08-03 22:25 ` Luis R. Rodriguez
0 siblings, 1 reply; 39+ messages in thread
From: Jan Tulak @ 2017-08-03 13:07 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: linux-xfs
On 02/08/2017 21:19, Luis R. Rodriguez wrote:
> On Wed, Aug 02, 2017 at 04:30:09PM +0200, Jan Tulak wrote:
>> On 29/07/2017 19:12, Luis R. Rodriguez wrote:
>>> On Fri, Jul 28, 2017 at 04:45:58PM +0200, Jan Tulak wrote:
>>>> On 27/07/2017 18:27, Luis R. Rodriguez wrote:
>>>>> On Thu, Jul 20, 2017 at 11:29:26AM +0200, Jan Tulak wrote:
>>>>>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>>>>>> index a69190b9..4b030101 100644
>>>>>> --- a/mkfs/xfs_mkfs.c
>>>>>> +++ b/mkfs/xfs_mkfs.c
>>>>>> @@ -107,6 +107,11 @@ unsigned int sectorsize;
>>>>>> * sets what is used with simple specifying the subopt (-d file).
>>>>>> * A special SUBOPT_NEEDS_VAL can be used to require a user-given
>>>>>> * value in any case.
>>>>>> + *
>>>>>> + * raw_input INTERNAL
>>>>>> + * Filled raw string from the user, so we never lose that information e.g.
>>>>>> + * to print it back in case of an issue.
>>>>>> + *
>>>>>> */
>>>>>> struct opt_params {
>>>>>> const char name;
>>>>>> @@ -122,6 +127,7 @@ struct opt_params {
>>>>>> long long minval;
>>>>>> long long maxval;
>>>>>> long long defaultval;
>>>>>> + const char *raw_input;
>>>>>> } subopt_params[MAX_SUBOPTS];
>>>>>> };
>>>>>> @@ -729,6 +735,18 @@ struct opt_params mopts = {
>>>>>> */
>>>>>> #define WHACK_SIZE (128 * 1024)
>>>>>> +static inline void
>>>>>> +set_conf_raw(struct opt_params *opt, int subopt, const char *value)
>>>>>> +{
>>>>>> + opt->subopt_params[subopt].raw_input = value;
>>>>>> +}
>>>>> There are no bounds check on the array here, I think set_conf_raw()
>>>>> should return int and we would check the return value. It could
>>>>> return -EINVAL if the subopt is invalid for instance.
>>>> Good idea. The only issue is with the return code, that causes some issues
>>>> when we are also returning values - I wanted the values to be turned into
>>>> uint64. But do we need to return an error? I don't see what usecase there
>>>> would be for it, other than detecting a bug. So an assert might be a better
>>>> solution - then it can't happen that a wrong index is used and result not
>>>> tested.
>>> The setting of the value can be done by using an extra argument pointer. Then
>>> if its set it be assigned. Otherwise it would be left alone. The return value
>>> would return 0 on success, otherwise a standard return value indicating the
>>> cause of the error.
>> I strongly prefer to return the value, not an error code. We can do the
>> other way around, put the error code into an argument to get roughly the
>> same result, while constructions like set_conf_raw(FOO, BAR, baz *
>> get_conf_raw(FOO, BAR)) will continue to work without the need for
>> intermediate variables.
>>
>> The *_raw functions are used on few places only, so it would be only a small
>> issue there, but for consistency, (get|set)_conf_val should have the same
>> behavior and an intermediate variable for every use of those would be really
>> annoying. So, how about this?
> It would not be intermediate, the main error variable from the start of
> each function could be used, as is typical in many properly written C
> programs.
I meant value-carrying variables, not the error one:
int temp; // a variable useful only on the next two lines
err = foo(&temp);
bar(temp);
versus:
bar(foo(&err));
The composition of functions would not be usable all the time, it
depends on what would be the return value in case of an error and how
would the outer function deal with it. But when I checked the code, I
think that it could work in a lot of places.
>> static inline void
>> set_conf_raw(struct opt_params *opt, int subopt, const char *value, int
>> *err)
>> {
>> if (subopt < 0 || subopt >= MAX_SUBOPTS) {
>> if (err != NULL) *err = EINVAL;
>> return;
>> }
>> opt->subopt_params[subopt].raw_input = value;
>> }
> If you go with the strdup thing to avoid limiting the context of the use of
> the pointer then you'll still have to return an error or abort, and I think
> returning an error is best.
OK, I'm willing to return errors for the _raw functions. These are used
only on few places, so it is not a big issue. Especially if I add a
wrapper for the get_conf_raw function - right now, these are used only
as fprintf() arguments to print an error. So the wrapper makes it easy
to use in this case (with the old die-on-error behavior), but if you
want to use it for something else, you can use it directly and get an
error as a return code. Does this looks good?
+/*
+ * Return 0 on success, -ENOMEM if it could not allocate enough memory for
+ * the string to be saved into the out pointer.
+ */
+static int
+get_conf_raw(const struct opt_params *opt, const int subopt, char **out)
+{
+ if (subopt < 0 || subopt >= MAX_SUBOPTS) {
+ fprintf(stderr,
+ "This is a bug: get_conf_raw called with invalid
opt/subopt: %c/%d\n",
+ opt->name, subopt);
+ exit(1);
+ }
+ *out = strdup(opt->subopt_params[subopt].raw_input);
+ if (*out == NULL)
+ return -ENOMEM;
+ return 0;
+
+}
+
+/*
+ * Same as get_conf_raw(), except it returns the string through return
+ * and dies on any error.
+ */
+static char *
+get_conf_raw_safe(const struct opt_params *opt, const int subopt)
+{
+ char *str;
+ if (get_conf_raw(opt, subopt, &str) == -ENOMEM) {
+ fprintf(stderr, "Out of memory!");
+ exit(1);
+ }
+ return str;
+}
>
>>> I don't think we need the too small or too big, a simple range issue should
>>> suffice and we have -ERANGE.
>>>
>> At this moment, we are telling if it is too small or too big, but when there
>> is no standard error code for that, ERANGE has to suffice.
> Sure, my point was that we have special values for too big or too small, and
> I consider that hacky, we could just *say* if it was too big or too small
> but just use ERANGE as its standard and non-hacky.
We don't have special values, we just print it out and die. But yes, if
we will pass the information anywhere, then it is better to use ERANGE
rather than some custom error number.
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 6/7] mkfs: extend opt_params with a value field
2017-08-02 19:48 ` Luis R. Rodriguez
@ 2017-08-03 13:23 ` Jan Tulak
2017-08-03 20:47 ` Luis R. Rodriguez
0 siblings, 1 reply; 39+ messages in thread
From: Jan Tulak @ 2017-08-03 13:23 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: linux-xfs
On 02/08/2017 21:48, Luis R. Rodriguez wrote:
> On Wed, Aug 02, 2017 at 08:11:38PM +0200, Jan Tulak wrote:
>> On 02/08/2017 18:57, Luis R. Rodriguez wrote:
>>> On Wed, Aug 02, 2017 at 04:43:09PM +0200, Jan Tulak wrote:
>>>> The reason for better messages is reasonable. About the style... well, it
>>>> makes sense. But I would certainly not do this in this set. So, I think
>>>> about a way how to keep the current behavior, but slowly build up the ground
>>>> for something like what you suggest.
>>>>
>>>> Using the same style of error-returning logic from the other email ([PATCH
>>>> 1/7] mkfs: Save raw ...), the error argument pointer would be optional. So,
>>>> when you do parse_conf_val(OPT_D, D_AGCOUNT, str, NULL); then you get the
>>>> old behavior and any error causes a termination inside of this function. But
>>>> if you instead pass some pointer: parse_conf_val(OPT_D, D_AGCOUNT, str,
>>>> &err); then right now, we would print the message but do not terminate. And
>>>> later on, some other message handling can be added.
>>> I think this is grotesque. Also, how would we know an error did happen then?
>> Just test if err is 0 or not, same as with errno.h. And the dual behavior
>> would be only a temporary measure. Once all uses are converted to the new
>> behavior, the terminating part can be dropped and the error argument will
>> become mandatory.
> Yes, I think this is not nice if I understood what you say below correctly.
>
>>>>>> If some value is out of range, or it is a
>>>>>> conflict, there is not much context needed, and better to not have to care
>>>>>> about these errors... Do you have an example when it would be helpful? If it
>>>>>> just spits out a return code, you have to add a check to every use and you
>>>>>> will have many times the same code like what is in getnum() at this moment
>>>>> Not really, if we are parsing say D_AGCOUNT we could have a member as part of the
>>>>> struct, say "description" then we can use say subopt[D_AGCOUNT].description on
>>>>> the error message, perhaps the only thing that would change for instance would be
>>>>> the context on which the error was run into, command line option passed or config
>>>>> file read, say with the filename and line number.
>>>>>
>>>>> How would we be able to detect an error happened and pass exactly where the
>>>>> error happened otherwise on a config file for example?
>>>> Yes, I see the issue - getnum finds an error, but it doesn't know the line
>>>> in the config file to report it. But with what I write above about the error
>>>> handling, this could work.
>>>>
>>>> if (c < sp->minval) {
>>>> if (config_file) illegal_config(str, line, opts, index, _("value is too
>>>> small"), err);
>>> How would it know what str and line are?
>> The "line" argument is a mistake, it shouldn't be here - it is solved by the
>> snipped below. The "str" is already there, it is what getnum() parses - only
>> the "err" at the end would be added.
> So you mean the error code would only be visible on the print out, but not
> passed to the caller as a starting step. A secondary step would go and change
> that to then return the error code and have each caller check it?
Actually, it would go both ways. Passing it down to the printout may not
be necessary, depending on what you want to do with it in the config
case, but it would be send up as well.
>
>>>> else illegal_option(str, opts, index, _("value is too small"), err);
>>>> }
>>> This seems convoluted and I don't really like it one bit.
>>>
>>>> ... and later in the code, if you are in the config file, you could do
>>>> something like:
>>>>
>>>> parse_conf_val(opt, subopt, str, &err);
>>>> if (err) report_invalid_line(current_line);
>>>>
>>>> Thoughts?
>>> Just my take: I prefer we do the right thing from the start. Even
>>> if it takes us ages to move forward, baby steps, and clean patches
>>> and evolutions, moving slowly away from the old crazy habits.
>> In which case, I would just continue as we do now (terminating on an error),
>> and then change it in the whole mkfs at once in some other patch set. There
>> always will be something old and ugly we have to use temporarily, or we end
>> up stashing one patchset after another, always trying to fix some other
>> thing first, and never really fixing anything.
> It seems like a change which could be fit into your series, it is more work,
> but then again most of the work here was expected to be significant and doing
> away with a lot of crap.
Yes, I just don't think it match the focus of this specific set.
>
> Up to you, I just am providing my own feedback and making clear the requirements
Thanks :-)
> for the config stuff. I think it will be silly to add config support without
> having the ability to catch errors and then indicate on its own however it
> chooses exactly where the error occurred.
I fully agree. We differ only in the opinion if it is important to do it
right now, or if it would be better as a standalone change.
> Will you take on doing the error code changes after ?
>
I put it into my todo list. Maybe it is not going to be the very next
patch set, but it is there.
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 6/7] mkfs: extend opt_params with a value field
2017-08-03 13:23 ` Jan Tulak
@ 2017-08-03 20:47 ` Luis R. Rodriguez
0 siblings, 0 replies; 39+ messages in thread
From: Luis R. Rodriguez @ 2017-08-03 20:47 UTC (permalink / raw)
To: Jan Tulak; +Cc: Luis R. Rodriguez, linux-xfs
On Thu, Aug 03, 2017 at 03:23:24PM +0200, Jan Tulak wrote:
> On 02/08/2017 21:48, Luis R. Rodriguez wrote:
> > I think it will be silly to add config support without
> > having the ability to catch errors and then indicate on its own however it
> > chooses exactly where the error occurred.
> I fully agree. We differ only in the opinion if it is important to do it
> right now, or if it would be better as a standalone change.
Not really, I don't care when it goes in so long as there is commitment
to follow up to do that missing piece of work. The config file parsing
thing can wait until this is all done, but it depends on it.
> > Will you take on doing the error code changes after ?
> >
> I put it into my todo list. Maybe it is not going to be the very next patch
> set, but it is there.
OK cool! Then sure!
Luis
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct
2017-08-03 13:07 ` Jan Tulak
@ 2017-08-03 22:25 ` Luis R. Rodriguez
2017-08-04 13:50 ` Jan Tulak
0 siblings, 1 reply; 39+ messages in thread
From: Luis R. Rodriguez @ 2017-08-03 22:25 UTC (permalink / raw)
To: Jan Tulak; +Cc: Luis R. Rodriguez, linux-xfs
On Thu, Aug 03, 2017 at 03:07:20PM +0200, Jan Tulak wrote:
> OK, I'm willing to return errors for the _raw functions. These are used only
> on few places, so it is not a big issue. Especially if I add a wrapper for
> the get_conf_raw function - right now, these are used only as fprintf()
> arguments to print an error. So the wrapper makes it easy to use in this
> case (with the old die-on-error behavior), but if you want to use it for
> something else, you can use it directly and get an error as a return code.
> Does this looks good?
>
> +/*
> + * Return 0 on success, -ENOMEM if it could not allocate enough memory for
> + * the string to be saved into the out pointer.
> + */
> +static int
> +get_conf_raw(const struct opt_params *opt, const int subopt, char **out)
> +{
> + if (subopt < 0 || subopt >= MAX_SUBOPTS) {
> + fprintf(stderr,
> + "This is a bug: get_conf_raw called with invalid opt/subopt:
> %c/%d\n",
> + opt->name, subopt);
> + exit(1);
Why not return -EINVAL?
> + }
> + *out = strdup(opt->subopt_params[subopt].raw_input);
> + if (*out == NULL)
> + return -ENOMEM;
> + return 0;
> +
> +}
> +
> +/*
> + * Same as get_conf_raw(), except it returns the string through return
> + * and dies on any error.
> + */
> +static char *
> +get_conf_raw_safe(const struct opt_params *opt, const int subopt)
> +{
> + char *str;
> + if (get_conf_raw(opt, subopt, &str) == -ENOMEM) {
> + fprintf(stderr, "Out of memory!");
> + exit(1);
I'd say no, just return NULL; these aborts drive me personally nuts.
> + }
> + return str;
> +}
>
>
> >
> > > > I don't think we need the too small or too big, a simple range issue should
> > > > suffice and we have -ERANGE.
> > > >
> > > At this moment, we are telling if it is too small or too big, but when there
> > > is no standard error code for that, ERANGE has to suffice.
> > Sure, my point was that we have special values for too big or too small, and
> > I consider that hacky, we could just *say* if it was too big or too small
> > but just use ERANGE as its standard and non-hacky.
> We don't have special values, we just print it out and die. But yes, if we
> will pass the information anywhere, then it is better to use ERANGE rather
> than some custom error number.
Great.
Luis
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct
2017-08-03 22:25 ` Luis R. Rodriguez
@ 2017-08-04 13:50 ` Jan Tulak
2017-08-07 17:26 ` Luis R. Rodriguez
0 siblings, 1 reply; 39+ messages in thread
From: Jan Tulak @ 2017-08-04 13:50 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: linux-xfs
On 04/08/2017 00:25, Luis R. Rodriguez wrote:
> On Thu, Aug 03, 2017 at 03:07:20PM +0200, Jan Tulak wrote:
>> OK, I'm willing to return errors for the _raw functions. These are used only
>> on few places, so it is not a big issue. Especially if I add a wrapper for
>> the get_conf_raw function - right now, these are used only as fprintf()
>> arguments to print an error. So the wrapper makes it easy to use in this
>> case (with the old die-on-error behavior), but if you want to use it for
>> something else, you can use it directly and get an error as a return code.
>> Does this looks good?
>>
>> +/*
>> + * Return 0 on success, -ENOMEM if it could not allocate enough memory for
>> + * the string to be saved into the out pointer.
>> + */
>> +static int
>> +get_conf_raw(const struct opt_params *opt, const int subopt, char **out)
>> +{
>> + if (subopt < 0 || subopt >= MAX_SUBOPTS) {
>> + fprintf(stderr,
>> + "This is a bug: get_conf_raw called with invalid opt/subopt:
>> %c/%d\n",
>> + opt->name, subopt);
>> + exit(1);
> Why not return -EINVAL?
If we know we hit a bug, we should terminate as soon as possible. We are
in an indeterminable state and we shouldn't risk that we will write
anything. C does not have exceptions, so I think that here we really
should just exit. The memory issue can have a solution, but a bug? Time
to end ASAP.
And set/get_conf_val is yet another issue. I really don't want to return
errors there, because then we can't do things like:
if (get_conf_val(OPT_D, D_AGCOUNT) > XFS_MAX_AGNUMBER + 1)
There is over 350 uses of get_conf_val similar to this and if every
usage should be changed to something like:
test_error(get_conf_val(OPT_D, D_AGCOUNT, &tmp_x));
if(tmp_x > XFS_MAX_AGNUMBER + 1)
Then this whole thing with temporary variables would make the situation
worse than it is now.
We are not speaking about handling of issues that can arose from user
input - that *should* be handled with returns - but about bugs and
severe situations "the system can't allocate even few more bytes."
I really don't see how to avoid the aborts at all time here, while at
the same time:
1) being able to detect that something happened and abort immediately
2) and having a simple usage that does not inflate every access to a
multi-line mess.
>
>> + }
>> + *out = strdup(opt->subopt_params[subopt].raw_input);
>> + if (*out == NULL)
>> + return -ENOMEM;
>> + return 0;
>> +
>> +}
>> +
>> +/*
>> + * Same as get_conf_raw(), except it returns the string through return
>> + * and dies on any error.
>> + */
>> +static char *
>> +get_conf_raw_safe(const struct opt_params *opt, const int subopt)
>> +{
>> + char *str;
>> + if (get_conf_raw(opt, subopt, &str) == -ENOMEM) {
>> + fprintf(stderr, "Out of memory!");
>> + exit(1);
> I'd say no, just return NULL; these aborts drive me personally nuts.
OK, NULL works here.
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct
2017-08-04 13:50 ` Jan Tulak
@ 2017-08-07 17:26 ` Luis R. Rodriguez
2017-08-07 17:36 ` Jan Tulak
0 siblings, 1 reply; 39+ messages in thread
From: Luis R. Rodriguez @ 2017-08-07 17:26 UTC (permalink / raw)
To: Jan Tulak; +Cc: Luis R. Rodriguez, linux-xfs
On Fri, Aug 04, 2017 at 03:50:19PM +0200, Jan Tulak wrote:
>
>
> On 04/08/2017 00:25, Luis R. Rodriguez wrote:
> > On Thu, Aug 03, 2017 at 03:07:20PM +0200, Jan Tulak wrote:
> > > OK, I'm willing to return errors for the _raw functions. These are used only
> > > on few places, so it is not a big issue. Especially if I add a wrapper for
> > > the get_conf_raw function - right now, these are used only as fprintf()
> > > arguments to print an error. So the wrapper makes it easy to use in this
> > > case (with the old die-on-error behavior), but if you want to use it for
> > > something else, you can use it directly and get an error as a return code.
> > > Does this looks good?
> > >
> > > +/*
> > > + * Return 0 on success, -ENOMEM if it could not allocate enough memory for
> > > + * the string to be saved into the out pointer.
> > > + */
> > > +static int
> > > +get_conf_raw(const struct opt_params *opt, const int subopt, char **out)
> > > +{
> > > + if (subopt < 0 || subopt >= MAX_SUBOPTS) {
> > > + fprintf(stderr,
> > > + "This is a bug: get_conf_raw called with invalid opt/subopt:
> > > %c/%d\n",
> > > + opt->name, subopt);
> > > + exit(1);
> > Why not return -EINVAL?
>
> If we know we hit a bug, we should terminate as soon as possible. We are in
> an indeterminable state and we shouldn't risk that we will write anything. C
> does not have exceptions, so I think that here we really should just exit.
> The memory issue can have a solution, but a bug? Time to end ASAP.
>
> And set/get_conf_val is yet another issue. I really don't want to return
> errors there, because then we can't do things like:
>
> if (get_conf_val(OPT_D, D_AGCOUNT) > XFS_MAX_AGNUMBER + 1)
>
> There is over 350 uses of get_conf_val similar to this and if every usage
> should be changed to something like:
>
> test_error(get_conf_val(OPT_D, D_AGCOUNT, &tmp_x));
> if(tmp_x > XFS_MAX_AGNUMBER + 1)
>
> Then this whole thing with temporary variables would make the situation
> worse than it is now.
Then one can keep the behaviour for get_conf_val() and it would use __get_conf_val()
which in turn *does* do the return. This way if I need to capture and handle the return
differently later this can be done and the code for existing callers does not need
to change, and the same paranoid behaviour can be kept?
Luis
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct
2017-08-07 17:26 ` Luis R. Rodriguez
@ 2017-08-07 17:36 ` Jan Tulak
0 siblings, 0 replies; 39+ messages in thread
From: Jan Tulak @ 2017-08-07 17:36 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: linux-xfs
On Mon, Aug 7, 2017 at 7:26 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Fri, Aug 04, 2017 at 03:50:19PM +0200, Jan Tulak wrote:
>>
>>
>> On 04/08/2017 00:25, Luis R. Rodriguez wrote:
>> > On Thu, Aug 03, 2017 at 03:07:20PM +0200, Jan Tulak wrote:
>> > > OK, I'm willing to return errors for the _raw functions. These are used only
>> > > on few places, so it is not a big issue. Especially if I add a wrapper for
>> > > the get_conf_raw function - right now, these are used only as fprintf()
>> > > arguments to print an error. So the wrapper makes it easy to use in this
>> > > case (with the old die-on-error behavior), but if you want to use it for
>> > > something else, you can use it directly and get an error as a return code.
>> > > Does this looks good?
>> > >
>> > > +/*
>> > > + * Return 0 on success, -ENOMEM if it could not allocate enough memory for
>> > > + * the string to be saved into the out pointer.
>> > > + */
>> > > +static int
>> > > +get_conf_raw(const struct opt_params *opt, const int subopt, char **out)
>> > > +{
>> > > + if (subopt < 0 || subopt >= MAX_SUBOPTS) {
>> > > + fprintf(stderr,
>> > > + "This is a bug: get_conf_raw called with invalid opt/subopt:
>> > > %c/%d\n",
>> > > + opt->name, subopt);
>> > > + exit(1);
>> > Why not return -EINVAL?
>>
>> If we know we hit a bug, we should terminate as soon as possible. We are in
>> an indeterminable state and we shouldn't risk that we will write anything. C
>> does not have exceptions, so I think that here we really should just exit.
>> The memory issue can have a solution, but a bug? Time to end ASAP.
>>
>> And set/get_conf_val is yet another issue. I really don't want to return
>> errors there, because then we can't do things like:
>>
>> if (get_conf_val(OPT_D, D_AGCOUNT) > XFS_MAX_AGNUMBER + 1)
>>
>> There is over 350 uses of get_conf_val similar to this and if every usage
>> should be changed to something like:
>>
>> test_error(get_conf_val(OPT_D, D_AGCOUNT, &tmp_x));
>> if(tmp_x > XFS_MAX_AGNUMBER + 1)
>>
>> Then this whole thing with temporary variables would make the situation
>> worse than it is now.
>
> Then one can keep the behaviour for get_conf_val() and it would use __get_conf_val()
> which in turn *does* do the return. This way if I need to capture and handle the return
> differently later this can be done and the code for existing callers does not need
> to change, and the same paranoid behaviour can be kept?
>
Yes, I'm ok with two versions, one safe and one
unsafe-you-has-to-test-for-errors, if you have a use for it.
Jan
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2017-08-07 17:36 UTC | newest]
Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-20 9:29 [PATCH 0/7] mkfs: save user input into opts table Jan Tulak
2017-07-20 9:29 ` [PATCH 1/7] mkfs: Save raw user input field to the opts struct Jan Tulak
2017-07-27 16:27 ` Luis R. Rodriguez
2017-07-28 14:45 ` Jan Tulak
2017-07-29 17:12 ` Luis R. Rodriguez
2017-08-02 14:30 ` Jan Tulak
2017-08-02 15:51 ` Jan Tulak
2017-08-02 19:41 ` Luis R. Rodriguez
2017-08-02 19:19 ` Luis R. Rodriguez
2017-08-03 13:07 ` Jan Tulak
2017-08-03 22:25 ` Luis R. Rodriguez
2017-08-04 13:50 ` Jan Tulak
2017-08-07 17:26 ` Luis R. Rodriguez
2017-08-07 17:36 ` Jan Tulak
2017-07-20 9:29 ` [PATCH 2/7] mkfs: rename defaultval to flagval in opts Jan Tulak
2017-07-20 9:29 ` [PATCH 3/7] mkfs: remove intermediate getstr followed by getnum Jan Tulak
2017-07-20 15:54 ` Darrick J. Wong
2017-07-21 8:56 ` Jan Tulak
2017-07-26 20:49 ` Eric Sandeen
2017-07-27 7:50 ` Jan Tulak
2017-07-27 13:35 ` Eric Sandeen
2017-07-21 12:24 ` [PATCH 3/7 v2] " Jan Tulak
2017-07-26 23:23 ` Eric Sandeen
2017-07-20 9:29 ` [PATCH 4/7] mkfs: merge tables for opts parsing into one table Jan Tulak
2017-07-20 9:29 ` [PATCH 5/7] mkfs: move getnum within the file Jan Tulak
2017-07-26 23:27 ` Eric Sandeen
2017-07-20 9:29 ` [PATCH 6/7] mkfs: extend opt_params with a value field Jan Tulak
2017-07-27 16:18 ` Luis R. Rodriguez
2017-07-28 14:44 ` Jan Tulak
2017-07-29 17:02 ` Luis R. Rodriguez
2017-08-02 14:43 ` Jan Tulak
2017-08-02 16:57 ` Luis R. Rodriguez
2017-08-02 18:11 ` Jan Tulak
2017-08-02 19:48 ` Luis R. Rodriguez
2017-08-03 13:23 ` Jan Tulak
2017-08-03 20:47 ` Luis R. Rodriguez
2017-07-20 9:29 ` [PATCH 7/7] mkfs: save user input values into opts Jan Tulak
2017-07-26 23:53 ` Eric Sandeen
2017-07-27 14:21 ` Jan Tulak
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.