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

["Luis R. Rodriguez" <mcgrof@kernel.org>]

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