This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 07/17] malloc: Add specialized dynarray for C strings
On 13/06/2017 06:46, Florian Weimer wrote:
> 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?
glob internally prepends the search directory by replacing the tilde with
a external home directory (GLOB_TILDE, either by environment variable or
through getXXX functions) or append new data by calling new patterns
recursively (GLOB_BRACE).
It is not an specific usage to glob, but rather a generic one which with
more higher level languages all the memory buffer bikeshedding will be
avoided by a string object. The idea is to keep a consistent C string buffer
stack-allocated (for general use) or heap based (to not bound due the
internal expansion) which can be used as constant argument for other
function call (fnmatch or recursively in case of GLOB_BRACE).
>
>> +++ 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?
I am not sure, the idea of char_array is to be self-contained, there is
no need to define anything in the file to include it. Also, if we see
other usage of dynamic C string inside glibc it would be a good idea to
add internal symbols for common symbols.
>
>> +#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.
It does not, it is an artefact I added and it should be removed.
>
>> +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?
Indeed, I will change to call __libc_fatal in such cases.
>
>> +++ 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?
>
I will change it.