Re: [PATCH 07/17] malloc: Add specialized dynarray for C strings

[Florian Weimer <fweimer at redhat dot com>]

Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> The provided function are not extensive and meant mainly to be use in
> subsequent glob implementation cleanup.  For internal object
> consistency only the function provided by char_array.c should be used,
> including internal object manipulation.

It's surprising that glob needs these many functions.

I'm not sure if the automatic NUL termination makes sense if we ever
want to generalize this interface.  How ingrained is that to the glob
use case?

> +++ b/malloc/char_array.c

Should this be malloc/char_array-skeleton.c, to indicate that this file
is parameterized and intended for inclusion into another .c file?

> +#define DYNARRAY_ELEMENT_INIT(__e) (*__e = '\0')

Why is this needed?  I don't think the code cares for the contents of
freshly added bytes.  For the trailing NUL byte, it seems better to add
that separately from the initialization operations.

> +static bool __attribute_used__
> +char_array_replace_str_pos (struct char_array *array, size_t pos,
> +                            const char *str, size_t len)
> +{
> +  if (pos > array->dynarray_header.used)
> +    return false;
> +
> +  size_t newsize;
> +  if (check_add_wrapv_size_t (pos, len, &newsize)
> +      || check_add_wrapv_size_t (newsize, 1, &newsize)
> +      || !char_array_resize (array, newsize))
> +    return false;

I don't think it is a good idea to mix the reporting of usage errors
(index out of bounds) with environmental conditions (memory allocation
failure).  Have you considered calling __libc_fatal if pos is out of
range, similar to the existing *_at function?

> +++ b/malloc/malloc-internal.h

> +/* Set *R = A + B.  Return true if the answer is mathematically incorrect due
> +   to overflow; in this case, *R is the low order bits of the correct
> +   answer.  */
> +static inline bool
> +check_add_wrapv_size_t (size_t a, size_t b, size_t *r)

Why not use check_add_overflow_size_t, for consistency with
check_mul_overflow_size_t?