[Mesa-dev] [PATCH 1/5] util: Add a string buffer implementation

[Nicolai Hähnle]

Thank you for doing all the cleanup work!

Two small issues that I could still find:

On 11.09.2017 22:21, Thomas Helland wrote:
> Based on Vladislav Egorovs work on the preprocessor, but split
> out to a util functionality that should be universal. Setup, teardown,
> memory handling and general layout is modeled around the hash_table
> and the set, to make it familiar for everyone.
> 
> A notable change is that this implementation is always null terminated.
> The rationale is that it will be less error-prone, as one might
> access the buffer directly, thereby reading a non-terminated string.
> Also, vsnprintf and friends prints the null-terminator.
> 
> V2: Address review feedback from Timothy and Grazvydas
>     - Fix MINGW preprocessor check
>     - Changed len from uint to int
>     - Make string argument const in append function
>     - Move to header and inline append function
>     - Add crimp_to_fit function for resizing buffer
> 
> V3: Move include of ralloc to string_buffer.h
> 
> V4: Use u_string.h for a cross-platform working vsnprintf
> 
> V5: Remember to cast to char * in crimp function
> 
> V6: Address review feedback from Nicolai
>     - Handle !str->buf in buffer_create
>     - Ensure va_end is always called in buffer_append_all
>     - Add overflow check in buffer_append_len
>     - Do not expose buffer_space_left, just remove it
>     - Clarify why a loop is used in vprintf, change to for-loop
>     - Add a va_copy to buffer_vprintf to fix failure to append arguments
>       when having to resize the buffer for vsnprintf.
[snip]
> +bool
> +_mesa_string_buffer_vprintf(struct _mesa_string_buffer *str,
> +                            const char *format, va_list args)
> +{
> +   /* We're looping two times to avoid duplicating code */
> +   for (uint32_t i = 0; i < 2; i++) {
> +      va_list arg_copy;
> +      va_copy(arg_copy, args);
> +      uint32_t space_left = str->capacity - str->length;
> +
> +      int32_t len = util_vsnprintf(str->buf + str->length,
> +                                   space_left, format, arg_copy);

va_copy also needs to be matched with a va_end here.


> +
> +      /* Error in vsnprintf() or measured len overflows size_t */
> +      if (unlikely(len < 0 || str->length + len + 1 < str->length))
> +         return false;
> +
> +      /* There was enough space for the string; we're done */
> +      if (len < space_left) {
> +         str->length += len;
> +         return true;
> +      }
> +
> +      /* Not enough space, resize and retry */
> +      ensure_capacity(str, str->length + len + 1);
> +   }
> +
> +   return false;
> +}
[snip]
> +static inline void
> +_mesa_string_buffer_crimp_to_fit(struct _mesa_string_buffer *str)
> +{
> +   str->capacity = str->length + 1;
> +   str->buf = (char *) reralloc_array_size(str, str->buf,
> +                                           sizeof(char), str->capacity);

What if the re-realloc fails?

I'd suggest:

    char *crimped = (char *) reralloc_array_size(...);
    if (!crimped)
       return;
    str->capacity = str->length + 1;
    str->buf = crimped;

With those two issues addressed, patches 1&2 are also:

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>

No need to re-send the series as far as I'm concerned.


> +}
> +
> +bool
> +_mesa_string_buffer_vprintf(struct _mesa_string_buffer *str,
> +                            const char *format, va_list args);
> +
> +bool
> +_mesa_string_buffer_printf(struct _mesa_string_buffer *str,
> +                            const char *format, ...);
> +
> +#ifdef __cplusplus
> +} /* extern "C" */
> +#endif
> +
> +#endif /* _STRING_BUFFER_H */
> 


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.