This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 2/7] tunables: Add support for tunables of uint64_t type
On 11/05/2017 11:51, Siddhesh Poyarekar wrote:
> Recognize the uint64_t type in addition to the current int32_t and
> size_t. This allows addition of tunables of uint64_t types. In
> addition to adding the uint64_t type, this patch also consolidates
> validation and reading of integer types in tunables.
>
> One notable change is that of overflow computation in
> tunables_strtoul. The function was lifted from __internal_strtoul,
> but it does not need the boundary condition check (i.e. result ==
> ULONG_MAX) since it does not need to set errno. As a result the check
> can be simplified, which I have now done.
>
> * elf/dl-tunable-types.h (tunable_type_code_t): New type
> TUNABLE_TYPE_UINT_64.
> * elf/dl-tunables.c (tunables_strtoul): Return uint64_t.
> Simplify computation of overflow.
> (tunable_set_val_if_valid_range_signed,
> tunable_set_val_if_valid_range_unsigned): Remove and replace
> with this...
> (TUNABLE_SET_VAL_IF_VALID_RANGE): ... New macro.
> (tunable_initialize): Adjust. Add uint64_t support.
> (__tunable_set_val): Add uint64_t support.
LGTM with some remarks.
> ---
> elf/dl-tunable-types.h | 1 +
> elf/dl-tunables.c | 94 +++++++++++++++++++++-----------------------------
> 2 files changed, 41 insertions(+), 54 deletions(-)
>
> diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h
> index 37a4e80..1d516df 100644
> --- a/elf/dl-tunable-types.h
> +++ b/elf/dl-tunable-types.h
> @@ -24,6 +24,7 @@
> typedef enum
> {
> TUNABLE_TYPE_INT_32,
> + TUNABLE_TYPE_UINT_64,
> TUNABLE_TYPE_SIZE_T,
> TUNABLE_TYPE_STRING
As for previous patch we should update README.tunables with this new allowed
type. Also, I think we should add that both hexadecimal and octal are also
supported.
> } tunable_type_code_t;
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index ebf48bb..8d72e26 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -105,10 +105,10 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val,
> /* A stripped down strtoul-like implementation for very early use. It does not
> set errno if the result is outside bounds because it gets called before
> errno may have been set up. */
> -static unsigned long int
> +static uint64_t
> tunables_strtoul (const char *nptr)
> {
> - unsigned long int result = 0;
> + uint64_t result = 0;
> long int sign = 1;
> unsigned max_digit;
>
> @@ -144,7 +144,7 @@ tunables_strtoul (const char *nptr)
>
> while (1)
> {
> - unsigned long int digval;
> + int digval;
> if (*nptr >= '0' && *nptr <= '0' + max_digit)
> digval = *nptr - '0';
> else if (base == 16)
> @@ -159,9 +159,8 @@ tunables_strtoul (const char *nptr)
> else
> break;
>
> - if (result > ULONG_MAX / base
> - || (result == ULONG_MAX / base && digval > ULONG_MAX % base))
> - return ULONG_MAX;
> + if (result >= (UINT64_MAX - digval) / base)
> + return UINT64_MAX;
> result *= base;
> result += digval;
I think this does not really handle overflows correctly and I would suggest
to actually use the new check_mul_overflow_size_t macro from reallocarray
patch to actually check it and result UINT64_MAX for the case.
Also, do we have any generic value range input test for tunable interface
(to check for validation, overflow, underflow, etc.)?
> ++nptr;
> @@ -170,68 +169,50 @@ tunables_strtoul (const char *nptr)
> return result * sign;
> }
>
> -/* Initialize the internal type if the value validates either using the
> - explicit constraints of the tunable or with the implicit constraints of its
> - type. */
> -static void
> -tunable_set_val_if_valid_range_signed (tunable_t *cur, const char *strval,
> - int64_t default_min, int64_t default_max)
> -{
> - int64_t val = (int64_t) tunables_strtoul (strval);
> -
> - int64_t min = cur->type.min;
> - int64_t max = cur->type.max;
> -
> - if (min == max)
> - {
> - min = default_min;
> - max = default_max;
> - }
> -
> - if (val >= min && val <= max)
> - {
> - cur->val.numval = val;
> - cur->strval = strval;
> - }
> -}
> -
> -static void
> -tunable_set_val_if_valid_range_unsigned (tunable_t *cur, const char *strval,
> - uint64_t default_min, uint64_t default_max)
> -{
> - uint64_t val = (uint64_t) tunables_strtoul (strval);
> -
> - uint64_t min = cur->type.min;
> - uint64_t max = cur->type.max;
> -
> - if (min == max)
> - {
> - min = default_min;
> - max = default_max;
> - }
> -
> - if (val >= min && val <= max)
> - {
> - cur->val.numval = val;
> - cur->strval = strval;
> - }
> -}
> +#define TUNABLE_SET_VAL_IF_VALID_RANGE(__cur, __val, __type, __default_min, \
> + __default_max) \
> +({ \
> + __type min = (__cur)->type.min; \
> + __type max = (__cur)->type.max; \
> + \
> + if (min == max) \
> + { \
> + min = __default_min; \
> + max = __default_max; \
> + } \
> + \
> + if ((__type) (__val) >= min && (__type) (val) <= max) \
> + { \
> + (__cur)->val.numval = val; \
> + (__cur)->strval = strval; \
> + } \
> +})
>
> /* Validate range of the input value and initialize the tunable CUR if it looks
> good. */
> static void
> tunable_initialize (tunable_t *cur, const char *strval)
> {
> + uint64_t val;
> +
> + if (cur->type.type_code != TUNABLE_TYPE_STRING)
> + val = tunables_strtoul (strval);
> +
> switch (cur->type.type_code)
> {
> case TUNABLE_TYPE_INT_32:
> {
> - tunable_set_val_if_valid_range_signed (cur, strval, INT32_MIN, INT32_MAX);
> + TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, int64_t, INT32_MIN, INT32_MAX);
> + break;
> + }
> + case TUNABLE_TYPE_UINT_64:
> + {
> + TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t, 0, UINT64_MAX);
> break;
> }
> case TUNABLE_TYPE_SIZE_T:
> {
> - tunable_set_val_if_valid_range_unsigned (cur, strval, 0, SIZE_MAX);
> + TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t, 0, SIZE_MAX);
> break;
> }
> case TUNABLE_TYPE_STRING:
> @@ -461,6 +442,11 @@ __tunable_set_val (tunable_id_t id, void *valp, tunable_callback_t callback)
>
> switch (cur->type.type_code)
> {
> + case TUNABLE_TYPE_UINT_64:
> + {
> + *((uint64_t *) valp) = (uint64_t) cur->val.numval;
> + break;
> + }
> case TUNABLE_TYPE_INT_32:
> {
> *((int32_t *) valp) = (int32_t) cur->val.numval;
>