On Tue, Apr 25, 2017 at 6:52 PM, Eric Sandeen <sandeen@xxxxxxxxxxx> wrote:
> On 4/23/17 1:54 PM, Jan Tulak wrote:
>> 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.
>
> The value should only ever be 0 or 1, correct?
Not necessarily. I think we are not using anything else, but
technically, it is possible to have any number here. I admit it is
hard to came up with any useful case for what we have in mkfs, though.
>
> Ok, so I see something interesting here:
>
> # grep "flagval = " mkfs/xfs_mkfs.c | sort | uniq -c
> 1 .flagval = 0,
> 1 .flagval = 0,
> 14 .flagval = 1,
> 40 .flagval = SUBOPT_NEEDS_VAL,
>
> of the 56 subopts, only 16 are treated as a flag, and only 2 default
> to 0, while the rest default to 1.
>
> Those 2 seem like outliers: -m rmapbt and -m reflink. What's
> going on there?
>
> In every other case where there is a subopt which is actually a
> boolean flag, a bare specification without a value means "enable
> the feature."
>
> It seems very unintuitive to have:
>
> -X THING1 -> THING1 is enabled 14 times, but
> -X THING12 - THING2 is still /disabled/ in only 2 cases
>
> I'd argue that "-m rmapbt" and "-m reflink" leaving those features
> /disabled/ is a bug, due to misunderstanding of what "defaultval"
> means, and that they should be fixed to behave like the other 14
> boolean flags.
I agree. It looks this way...
>
> i.e. I'd argue that a bare boolean flag specification should /always/
> enable the feature. At that point, you can say within your code
> "if min = 0 and max = 1, this is a boolean, and the flag may be specified
> without a value, in which case the value is set to 1, and the feature
> is enabled."
>
> At that point you don't need this extra structure member, and you won't
> have to re-state it 56 times, thus simplifying the code.
>
> What do you think? I'd happily review and merge 2 patches, which:
>
> 1) Makes rmapbt and reflink behave like every other boolean flag, and
>
This sounds reasonable.
> 2) Removes "defaultval" altogether, and uses min=0/max=1 as a sign that
> the option is a boolean, with a bare flag name allowed to enable it.
>
I wonder if a .is_flag = true (and no other min/max/defaultval...)
wouldn't be better solution. the min=0/max=1 detection is a bit
implicit and may not be apparent in the future. If we limit flags to
0/1 values, then defaultval can be removed either way, but it is clear
on the first glance what the option does.
Jan
> -Eric
>
>> Signed-off-by: Jan Tulak <jtulak@xxxxxxxxxx>
>> ---
>> 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 84674d5..0a795a6 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -112,7 +112,7 @@ uint64_t 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).
>> @@ -144,7 +144,7 @@ struct opt_params {
>> int conflicts[MAX_CONFLICTS];
>> uint64_t minval;
>> uint64_t maxval;
>> - uint64_t defaultval;
>> + uint64_t flagval;
>> const char *raw_input;
>> } subopt_params[MAX_SUBOPTS];
>> };
>> @@ -164,7 +164,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,
>> @@ -173,7 +173,7 @@ struct opt_params bopts = {
>> LAST_CONFLICT },
>> .minval = XFS_MIN_BLOCKSIZE,
>> .maxval = XFS_MAX_BLOCKSIZE,
>> - .defaultval = SUBOPT_NEEDS_VAL,
>> + .flagval = SUBOPT_NEEDS_VAL,
>> },
>> },
>> };
>> @@ -219,24 +219,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,
>> @@ -245,7 +245,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,
>> @@ -254,7 +254,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,
>> @@ -262,7 +262,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,
>> @@ -272,7 +272,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,
>> @@ -281,14 +281,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,
>> @@ -297,7 +297,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,
>> @@ -307,25 +307,25 @@ struct opt_params dopts = {
>> LAST_CONFLICT },
>> .minval = 0,
>> .maxval = 1,
>> - .defaultval = 1,
>> + .flagval = 1,
>> },
>> { .index = D_RTINHERIT,
>> .conflicts = { LAST_CONFLICT },
>> .minval = 0,
>> .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,
>> },
>> },
>> };
>> @@ -357,7 +357,7 @@ struct opt_params iopts = {
>> .conflicts = { LAST_CONFLICT },
>> .minval = 0,
>> .maxval = 1,
>> - .defaultval = 1,
>> + .flagval = 1,
>> },
>> { .index = I_LOG,
>> .conflicts = { I_PERBLOCK,
>> @@ -365,13 +365,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,
>> @@ -380,7 +380,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,
>> @@ -389,25 +389,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,
>> },
>> },
>> };
>> @@ -447,7 +447,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,
>> @@ -455,27 +455,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,
>> @@ -483,20 +483,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,
>> @@ -505,26 +505,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,
>> },
>> },
>> };
>> @@ -548,7 +548,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,
>> @@ -557,19 +557,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,
>> },
>> },
>> };
>> @@ -597,33 +597,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 },
>> },
>> },
>> @@ -649,7 +649,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,
>> @@ -657,7 +657,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,
>> @@ -667,7 +667,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,
>> @@ -677,7 +677,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,
>> },
>> },
>> };
>> @@ -702,29 +702,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 = 0,
>> + .flagval = 0,
>> },
>> { .index = M_REFLINK,
>> .conflicts = { LAST_CONFLICT },
>> .minval = 0,
>> .maxval = 1,
>> - .defaultval = 0,
>> + .flagval = 0,
>> },
>> },
>> };
>> @@ -1365,9 +1365,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) {
>>
--
Jan Tulak
jtulak@xxxxxxxxxx / jan@xxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
