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

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

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