Re: maint: update the mbsalign module

[Jim Meyering]

Pádraig Brady wrote:
> I'm updating this module with a view to fixing
> alignment issues in df etc. soon.

Great!
That has been a sore point for a long time.

> This fixes an unlikely over truncation bug
> and cleans up the interface so that one doesn't
> have to check for errors in all cases.
>
> I've also synced this with the util-linux-ng project
> for alignment in cal and fdisk et. al. there.
...
> Subject: [PATCH] maint: update the mbsalign module
>
> * gl/lib/mbsalign.c (mbsalign):  Support the MBA_UNIBYTE_FALLBACK
> flag which reverts to unibyte mode if one can't allocate memory
> or if there are invalid multibyte characters present.
> Note memory is no longer dynamically allocated in unibyte mode so
> one can assume that mbsalign() will not return an error if this
> flag is present.  Fix an error where we would truncate too many
> characters in the presence of single byte non printable chars.
> Suppress a signed/unsigned comparison warning.
> (ambsalign): A new wrapper function to dynamically allocate
> the minimum memory required to hold the aligned string.
> * gl/lib/mbsalign.h: Add the MBA_UNIBYTE_FALLBACK flag and
> also document others that may be implemented in future.
> (ambsalign): A prototype for the new wrapper.

These look like fine improvements.
Have you considered dividing it into two or more change sets?
Mixing "fix" and "clean-up" changes in one change-set is
best avoided, if only to ease review and bisection.
In addition, it would ease long-term maintenance if you would
at least outline how to exercise the bug(s?) you're fixing.
Then I'll try to make time to write a test for it.

> ---
>  gl/lib/mbsalign.c |  101 +++++++++++++++++++++++++++++++++++++++++-----------
>  gl/lib/mbsalign.h |   23 ++++++++++++
>  2 files changed, 102 insertions(+), 22 deletions(-)
>
> diff --git a/gl/lib/mbsalign.c b/gl/lib/mbsalign.c
...
>  size_t
>  mbsalign (const char *src, char *dest, size_t dest_size,
...
> -  if (conversion || (n_cols > *width))
> +  if (wc_enabled && (conversion || (n_cols > *width)))
>      {
> -      newstr = malloc (src_size);
> -      if (newstr == NULL)
> -        goto mbsalign_cleanup;
> -      str_to_print = newstr;
> -      if (wc_enabled)
> -        {
> -          n_cols = wc_truncate (str_wc, *width);
> -          n_used_bytes = wcstombs (newstr, str_wc, src_size);
> -        }
> -      else
> +        if (conversion)
> +          {
> +             /* May have increased the size by converting
> +                \t to \uFFFD for example.  */
> +            src_size = wcstombs(NULL, str_wc, 0) + 1;

missing space-before open-paren

...
> +/* A wrapper around mbsalign() to dynamically allocate the
> +   minimum amount of memory to store the result.
> +   NULL is returned on failure.  */

Use the active voice:

  "Return NULL upon failure."

> +
> +char *
> +ambsalign (const char *src, size_t *width, mbs_align_t align, int flags)
> +{
...