Error on negative number when non-negative required

[zevweiss]

Instead of silently pretending the user passed zero, let's issue an error message and bail out.

[guijan]

I don't like that it doesn't notify the user about which option needs a non negative number but it's better than the current situation.

[N-R-K]

I don't like that it doesn't notify the user about which option needs a non negative number

Can (and IMO should) be done by removing the function entirely and checking for it at call site. For example:

         case 'd':
-            opt.delay = nonNegativeNumber(optionsParseRequiredNumber(optarg));
+            if ((opt.delay = optionsParseRequiredNumber(optarg)) < 0)
+                errx(EXIT_FAILURE, "delay needs to be non-negative");
             break;

Or maybe the function could accept a name:

```diff
         case 'd':
-            opt.delay = nonNegativeNumber(optionsParseRequiredNumber(optarg));
+            opt.delay = nonNegativeNumber("delay", optionsParseRequiredNumber(optarg));

And inside the function you'd do errx(1, "%s needs to be non-negative (got %d)", name, num);.

[zevweiss]

Can (and IMO should) be done by removing the function entirely and checking for it at call site.

Yeah, after @guijan's comment I came to the same conclusion -- I'll admit that the main reason I haven't already changed it to do that instead is that it would introduce a conflict with another (slightly larger) PR that I've currently got open awaiting review (#199). If that one gets merged first I'll change this one to also replace the new callsite that patch adds.

[guijan]

optionsParseRequiredNumber(), optionsParseRequireRange(), and nonNegativeNumber() should all be replaced by strtonum(). That would make it really easy to print much more informative error messages and we would shed some lines of code. The one catch is that I already know upfront Cygwin is missing this function, this would introduce a dependency on my libbsd-compatible library on that system which is something @daltomi was against in a previous PR.

[daltomi]

The nonNegativeNumber function was introduced in #115 (The delay option should not be a negative number). The use of strtonum() was partially discussed.

@N-R-K say:
Can (and IMO should) be done by removing the function entirely and checking for it at call site. For example:

            if ((opt.delay = optionsParseRequiredNumber(optarg)) < 0)
                errx(EXIT_FAILURE, "delay needs to be non-negative");
             break;```

Bad idea :-) , please note how you come to that conclusion (opt.delay must be >= 0) thanks to the fact that the name of the function (nonNegativeNumber) indicates its intention.
That's why it's important to create simple functions for modularization and reuse.

@N-R-K say:
Or maybe the function could accept a name:

```diff
         case 'd':
-            opt.delay = nonNegativeNumber(optionsParseRequiredNumber(optarg));
+            opt.delay = nonNegativeNumber("delay", optionsParseRequiredNumber(optarg));

And inside the function you'd do errx(1, "%s needs to be non-negative (got %d)", name, num);.

I agree with this 👍

[N-R-K]

please note how you come to that conclusion thanks to the fact that the name of the function (nonNegativeNumber) indicates its intention.

As someone reading this snippet for the first time, the function actually obfuscated the intent compared to an immediate check such as < 0.

	opt.delay = nonNegativeNumber(optionsParseRequiredNumber(optarg));

Here I can assume what nonNegativeNumber does if the number isn't negative, but I have no clue what happens if it is negative.

0. Does it exit the process?
1. Does it throw an exception (via longjmp) ?
2. Does it set the number to 0?
3. Does it set the number to absolute value of the argument?

All of these are valid action that a function named 'nonNegativeNumber' might take. And in this case it does 2, but no where in the function name does it say that.

So in order to actually figure out what's happening I need to go jump to the function definition. Compare that with the following; the intent is very clear and doesn't require having to look up a function definition somewhere else.

	if ((opt.delay = optionsParseRequiredNumber(optarg)) < 0)
		errx(EXIT_FAILURE, "delay must be non-negative");

It's immediately obvious that the intention is that the number cannot be negative and if it is negative then exit the process.

For things that are so simple as a check against 0, I don't think a function adds any value. If anything shallow functions harms readability more than it helps.

[N-R-K]

this would introduce a dependency on my libbsd-compatible library on that system which is something @daltomi was against in a previous PR.

I agree with @daltomi's decision. Something as simple as parsing a number and checking weather it's in range shouldn't depend on some 3rd party library :)

[daltomi]

As someone reading this snippet for the first time, the function actually obfuscated the intent compared to an immediate check such as < 0.

In other function names I used the word required, I think nonNegativeNumber was clear but I see that it is not.

[daltomi]

Can (and IMO should) be done by removing the function entirely and checking for it at call site.

Yeah, after @guijan's comment I came to the same conclusion -- I'll admit that the main reason I haven't already changed it to do that instead is that it would introduce a conflict with another (slightly larger) PR that I've currently got open awaiting review (#199). If that one gets merged first I'll change this one to also replace the new callsite that patch adds.

Hi @zevweiss

We are in the process of launching a new release, I would like your PR to be included.
The conclusion of the discussion is: remove the function (nonNegativeNumber) entirely and do the check on the call site.
If you can do it in this same PR it would be ideal.

[guijan]

I'm working on this.

I wrote my own strtonum() alternative, this way we don't have to add a dependency for a single function. Is this fine?

/* options_parsenum: "string to number" function.
 *
 * Parses the string representation of an integer in str, and simultaneously
 * ensures that it is >= min and <= max.
 *
 * Returns the integer and sets *errmsg to NULL on success.
 * Returns 0 and sets *errmsg to a pointer to a string containing the
 * reason why the number can't be parsed on error.
 *
 * usage:
 * char *errmsg;
 * unsigned int nonnegative;
 * if ((nonnegative = options_parsenum(optarg, 0, UINT_MAX, &errmsg)) == NULL)
 * 	errx(1, "-n: '%s' is %s", optarg, errmsg);
 */
long long options_parsenum(const char *str, long long min, long long max,
    const char *errmsg[static 1])
{
    char *end = NULL;
    long long rval;
    int saved_errno = errno;
    *errmsg = NULL;
    
    errno = 0;
    rval = strtoll(str, &end, 10);
    if (errno == ERANGE)
        *errmsg = "not representable";
    else if (rval < min)
        *errmsg = min == 0 ? "negative" : "too small";
    else if (rval > max)
        *errmsg = "too large";
    else if (*str == '\0' || *end != '\0')
        *errmsg = "not a number";
    errno = saved_errno;

    return (*errmsg ? 0 : rval);
}

[N-R-K]

Seems unnecessarily complicated just to test for negative number, also optionsParseRequiredNumber already does a lot of those checks so there's some duplicate code as well.

Unless you're talking about replacing optionsParseRequiredNumber with that version - in which case I don't have any strong preference one way or another. Both version seems OK to me.

[guijan]

Yes, it's for replacing optionsParseRequiredNumber(), optionsParseRequireRange(), and nonNegativeNumber() all at once. The usage is definitely simpler, I'm working through the source right now.