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

[Jan Tulak <jtulak@redhat.com>]

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