[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] xsetmode: new module
From: |
Bruno Haible |
Subject: |
Re: [PATCH] xsetmode: new module |
Date: |
Thu, 16 Feb 2017 03:40:11 +0100 |
User-agent: |
KMail/5.1.3 (Linux/4.4.0-62-generic; KDE/5.18.0; x86_64; ; ) |
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