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

Jan Tulak
2017-08-04T13:50:22Z

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