Re: [Bug-gnulib] proposed patch for getpass and unlocked-io

[Bruno Haible]

Paul Eggert wrote:
>       Merge getpass from libc, plus a few fixes.

Thanks for the work!

>       * lib/unlocked-io.h: Include <stdio.h>, so that the caller
>       doesn't have to include <stdio.h> before us.
>       (clearerr_unlocked, feof_unlocked, ferror_unlocked,
>       fflush_unlocked, fgets_unlocked, fputc_unlocked, fputs_unlocked,
>       fread_unlocked, fwrite_unlocked, getc_unlocked, getchar_unlocked,
>       putc_unlocked, putchar_unlocked): Define to the unlocked counterpart
>       if not declared, so that we can use getpass.c code from libc without
>       rewriting it.
>       (flockfile, ftrylockfile, funlockfile): New macros.

Agreed.

>       [HAVE_WCHAR_H]: Include <wchar.h>.
>       (fputws, fputws_unlocked, putwc, putwc_unlocked) [HAVE_WCHAR_H]:
>       New macros, defined similarly to the above.

>       * lib/getpass.c
>       (USE_IN_LIBIO, __IO_fwide, __fwprintf) [!_LIBC && HAVE_DECL_FWIDE &&
>       HAVE_DECL_FWPRINTF]: New macros.

I don't think supporting wide-character I/O in gnulib is worth the
complexities, because:
  - Hardly anyone can use fwprintf & friends, because it doesn't mix
    with fprintf & friends. (Once a FILE* stream is opened, you have
    no portable way to switch it from narrow to wide or vice versa.)
  - It introduces additional porting hassles. For example, here you
    have only checked whether fwprintf() is declared, not whether it
    really exists. If you had done the same with putwc() or fputws(),
    then on early Solaris systems, all executables that use the 'getpass'
    module would need to link with -lw. (You have been lucky this time
    that Solaris defines fwprintf() in libc, whereas putwc() and fputws()
    are in libw!)

>       * lib/getpass.c (_LIBC): Define to 0 if not defined.
> +           if (!_LIBC)

Why this? The customary idiom throughout glibc is to use

    #  if !_LIBC

>       [!_LIBC]: Unconditionally do the fseek, since we don't know what
>       stream might go where.

I don't understand this. Larry Jones explained that the problem is inside
the stdio, not inside the kernel or tty driver, therefore the test
(out == in) is the right one.

> +static void
> +call_fclose (void *arg)
> +{
> +  if (arg != NULL)
> +    fclose (arg);
> +}

How about a more descriptive argument name, like 'void *fp' or 'void *tty' ?

>  #  if HAVE_DECL_PUTC_UNLOCKED
>  #   undef putc
>  #   define putc(x,y) putc_unlocked (x,y)
> +#  else
> +#   define putc_unlocked(x, y) putc (x, y)
>  #  endif

The distribution of spaces here is inconsistent with the rest of the file.

Bruno