On Mon, Aug 7, 2017 at 7:26 PM, Luis R. Rodriguez <mcgrof@xxxxxxxxxx> wrote:
> 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?
>
Yes, I'm ok with two versions, one safe and one
unsafe-you-has-to-test-for-errors, if you have a use for it.
Jan
--
Jan Tulak
jtulak@xxxxxxxxxx / jan@xxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
