[PATCH 0/7] mkfs: save user input into opts table

[PATCH 0/7] mkfs: save user input into opts table

[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

[PATCH 1/7] mkfs: Save raw user input field to the opts struct

[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

Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct

[Luis R. Rodriguez]

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

Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct

[Jan Tulak]

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

Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct

[Luis R. Rodriguez]

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

Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct

[Jan Tulak]

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

Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct

[Jan Tulak]

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

Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct

[Luis R. Rodriguez]

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

Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct

[Luis R. Rodriguez]

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

Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct

[Jan Tulak]

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

Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct

[Luis R. Rodriguez]

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

Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct

[Jan Tulak]

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

Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct

[Luis R. Rodriguez]

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

Re: [PATCH 1/7] mkfs: Save raw user input field to the opts struct

[Jan Tulak]

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

[PATCH 2/7] mkfs: rename defaultval to flagval in opts

[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

[PATCH 3/7] mkfs: remove intermediate getstr followed by getnum

[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

Re: [PATCH 3/7] mkfs: remove intermediate getstr followed by getnum

[Darrick J. Wong]

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

Re: [PATCH 3/7] mkfs: remove intermediate getstr followed by getnum

[Jan Tulak]

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

Re: [PATCH 3/7] mkfs: remove intermediate getstr followed by getnum

[Eric Sandeen]

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

Re: [PATCH 3/7] mkfs: remove intermediate getstr followed by getnum

[Jan Tulak]

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

Re: [PATCH 3/7] mkfs: remove intermediate getstr followed by getnum

[Eric Sandeen]

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

[PATCH 3/7 v2] mkfs: remove intermediate getstr followed by getnum

[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

Re: [PATCH 3/7 v2] mkfs: remove intermediate getstr followed by getnum

[Eric Sandeen]

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));
>  
> 

[PATCH 4/7] mkfs: merge tables for opts parsing into one table

[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

[PATCH 5/7] mkfs: move getnum within the file

[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

Re: [PATCH 5/7] mkfs: move getnum within the file

[Eric Sandeen]

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
> 

[PATCH 6/7] mkfs: extend opt_params with a value field

[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

Re: [PATCH 6/7] mkfs: extend opt_params with a value field

[Luis R. Rodriguez]

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

Re: [PATCH 6/7] mkfs: extend opt_params with a value field

[Jan Tulak]

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.
>

Re: [PATCH 6/7] mkfs: extend opt_params with a value field

[Luis R. Rodriguez]

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

Re: [PATCH 6/7] mkfs: extend opt_params with a value field

[Jan Tulak]

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

Re: [PATCH 6/7] mkfs: extend opt_params with a value field

[Luis R. Rodriguez]

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

Re: [PATCH 6/7] mkfs: extend opt_params with a value field

[Jan Tulak]

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

Re: [PATCH 6/7] mkfs: extend opt_params with a value field

[Luis R. Rodriguez]

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

Re: [PATCH 6/7] mkfs: extend opt_params with a value field

[Jan Tulak]

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

Re: [PATCH 6/7] mkfs: extend opt_params with a value field

[Luis R. Rodriguez]

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

[PATCH 7/7] mkfs: save user input values into opts

[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

Re: [PATCH 7/7] mkfs: save user input values into opts

[Eric Sandeen]

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);
> 

Re: [PATCH 7/7] mkfs: save user input values into opts

[Jan Tulak]

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