Re: [PATCH] xsetmode: new module

[Bruno Haible]

Hi Paul,

> -/* set_binary_mode (fd, mode)
> -   sets the binary/text I/O mode of file descriptor fd to the given mode
> -   (must be O_BINARY or O_TEXT) and returns the previous mode.  */
> +/* Set FD's mode to MODE.  Return the old mode if successful, -1
> +   (setting errno) on failure.  */

The new comment does not state whether the 'mode' argument is expected to be
O_TEXT / O_BINARY or 0 / 1.

Also, the new comment is positioned so that it appears to apply to the
'__gl_setmode_check' function. It should be placed in front of the
'set_binary_mode' function, I guess.

> +__gl_setmode_check (int fd)
> +{
> +  if (isatty (fd))
> +    {
> +      errno = ENOTTY;

This is highly confusing. ENOTTY is the common errno for an operation that
expects a tty and is applied to a regular file. For the opposite case,
an operation that expects a regular file and is applied to a tty, the common
errno is EINVAL. - Just imagine some program calls perror after 
set_binary_mode...

>  Date        Modules         Changes
>  
> +2017-02-15  binary-io       On MS-DOS and OS/2, set_binary_mode now fails
> +                            on ttys, and sets errno == ENOTTY.  SET_BINARY
> +                            has been removed; use set_binary_mode instead.

When SET_BINARY is removed, the callers (gettext, libiconv, among others)
need to change.
If you recommend to use set_binary_mode always, instead of SET_BINARY, then
what is the benefit of removing SET_BINARY?
In the majority of the cases, I would use xsetmode instead. Then the removal
makes sense - but then the NEWS entry should say "use set_binary_mode or
xsetmode instead, whichever is more appropriate".

> +XSETMODE_INLINE void
> +xsetmode (int fd, int mode)

This function lacks documentation. In particular, the comment should state
whether the 'mode' argument is expected to be O_TEXT / O_BINARY or 0 / 1.

> +xsetmode (int fd, int mode)
> +{
> +  if (set_binary_mode (fd, mode) < 0)

I'm missing some systematic naming of the functions. I can easily remember
a rule "x means checking, x<func> is like <func> with checks". but I can't
easily remember the association between 'xsetmode' and 'set_binary_mode'.
How about renaming 'xsetmode' to 'xset_binary_mode'?

Bruno