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

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

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