Re: [PATCHv2 2/2] [long] improvements to limit/ulimit API and doc (the rest)

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Stephane Chazelas wrote on Thu, Nov 26, 2020 at 20:58:19 +0000:
> +++ b/Src/Builtins/rlimits.c
> @@ -255,38 +267,204 @@ printrlim(rlim_t val, const char *unit)
>  # endif /* RLIM_T_IS_QUAD_T */
>  }
>  
> +/*
> + * Parse a string into the corresponding value based on the limit type
> + *
> + *   ZLIMTYPE_UNKNOWN / ZLIMTYPE_NUMBER:
> + *     decimal integer without sign only: raw value
> + *
> + *   ZLIMTYPE_TIME / ZLIMTYPE_MICROSECONDS:
> + *     <decimal> only gives seconds or microseconds depending on type
> + *     or:
> + *     [[hour:]min:]sec[.usec]
> + *     or:
> + *     <decimal> with h (hour), m (min), s (sec), ms, us suffix.
> + *
> + *   ZLIMTYPE_MEMORY:
> + *     <decimal> without suffix interpreted as KiB by "limit" (except for
> + *     RLIMIT_MSGQUEUE, see below) and based on resinfo.unit by "ulimit".
> + *
> + *     K/M/G/T/P/E suffix and same with iB suffix use 1024 factor
> + *     KB/MB/GB... use 1000 factor.
> + *
> + *     B for bytes (avoids that mess about default units).
> + *
> + * All suffixes are case insensitive.
> + *
> + */
> +
>  /**/
>  static rlim_t
> -zstrtorlimt(const char *s, char **t, int base)
> +zstrtorlimt(const char *s, int lim, int ulimit, char **err)

Please specify in the docstring the types, values, and meanings of the
formal parameters.

>  {
>      rlim_t ret = 0;
> +    const char *orig = s;
> +    enum zlimtype type = resinfo[lim]->type;
> +    *err = NULL;
⋮
> +    else if (type == ZLIMTYPE_TIME ||
> +	     type == ZLIMTYPE_MICROSECONDS) {
⋮
> +	    default:
> +		*err = "invalid time specification";

Could you arrange for the invalid value to be included in the error
message, as in «zwarnnam(nam, "invalid time specification: %s", foo)»?
That tends to be more user-friendly, particularly when the call stack is
deep.

> +		return 0;
> +	    }
> +
> +	    if (*s) {
> +		*err = "invalid time specification";
> +		return 0;
> +	    }
> +
> +	    ret *= factor;
> +	    if (type == ZLIMTYPE_MICROSECONDS)
> +		ret *= 1000000 / divisor;
> +	    else
> +		ret /= divisor;
> +	}
> +    }
> @@ -543,61 +763,12 @@ bin_limit(char *nam, char **argv, Options ops, UNUSED(int func))
> +	val = zstrtorlimt(s, lim, 0, &err);
> +	if (err) {
> +	    zwarnnam(nam, err);

This should be zwarnnam(nam, "%s", err), otherwise any percent signs in
«err» would cause breakage.

> +	    return 1;
>  	}
>  	if (do_limit(nam, lim, val, hard, !hard, OPT_ISSET(ops, 's')))
>  	    ret++;

Cheers,

Daniel
(haven't done a full review; just ran into thils in skimming)