Re: [PATCH] binary-io: do not treat set_binary_mode() on stdin/out/err as an error on OS/2

[KO Myung-Hun]

Hi/2.

Bruno Haible wrote:
> KO Myung-Hun wrote:
>> * lib/binary-io.c (__gl_setmode_check) [__EMX__]: Remove __EMX__ guard.
>> * lib/binary-io.h (__gl_setmode_check) [__EMX__]: Remove __EMX__ guard.
>> (set_binary_mode) [__EMX__]: Override mode with O_TEXT if tty.
> 
> According to the EMX documentation of the setmode function
> ------------------------------------------------------------------------------
> #include <io.h>                                                           [PC]
> #include <fcntl.h>
> 
> int setmode (int handle, int mode);
> 
>     Change the text/binary mode of a file handle.  MODE must be either
>     O_BINARY or O_TEXT.
> 
>     Note: Use _fsetmode() to change the mode of a stream.
> 
>   Return value:
> 
>     If there's an error, setmode() returns -1 and sets errno to EBADF
>     or EINVAL otherwise setmode() returns the previous mode, that is,
>     O_BINARY or O_TEXT.
> 
>   See also: _fsetmode(), open()
> ------------------------------------------------------------------------------
> 
> your proposed patch has the following effect:
> 
> 
> BEFORE:
>                                              sets mode to   return value
> set_binary_mode on tty                        ---           -1, errno=EINVAL
> set_binary_mode on regular file fails         ---           -1, errno=EBADF 
> or EINVAL
> set_binary_mode on regular file succeeds      mode          previous mode
> 
> AFTER:
> 
>                                              sets mode to   return value
> set_binary_mode on tty                        O_TEXT        previous mode
> set_binary_mode on regular file fails         ---           -1, errno=EBADF 
> or EINVAL
> set_binary_mode on regular file succeeds      mode          previous mode
> 
> 
> Changing a function to *silently* do a different thing than what it was
> requested to do? This is not good. The previous behaviour, to set an error
> indicator, is better.
> 

I agree. But previous set_binary_mode() also alter the behavior of
setmode() on OS/2, because setmode(O_BINARY) on tty works well instead
of returning -1 as previous set_binary_mode() does.

In addition, set_binary_mode() breaks the compatibility with
SET_BINARY() before set_binary_mode(). Because SET_BINARY() does nothing
on tty. It does not cause any error at all. My patch is compatible with
SET_BINARY().

>> Setting stdin/out/err to binary mode is allowed on OS/2. But it's not
>> useful, because it generates stair-output hard to read.
> 
> On a tty, the existing code will refrain from invoking setmode. So no
> stair-output in this case.
> 

Instead, it return an error. If some codes checks the error code, it
will determine that an error occurs.

Especially, programs using xset_binary_mode() will abort due to that
error. tee of coreutils is a good example.

> On a regular file, it depends on your text viewer. Emacs surely doesn't
> produce stair-cased rendering of a file.
> 
>> Instead, let's set them to text mode all the time.
>>
>> This fixes that tee always crashes at startup if stdin or stdout
>> are not piped.
> 
> 1) I don't see why the proposed patch would fix a crash. It only
>    changes the way output is rendered outside of the program.

Sorry, 'crash' is not proper word. Maybe 'abort' is more proper. Anyway,
the 'abort' is the result of xset_binary_mode() using the result of
set_binary_mode() which affected by __gl_setmode_check().

> 2) The proper place to do such changes is the 'tee' program.
> 

No.many programs of coreutils including tee are using
xset_binary_mode(). Maybe more programs using xset_binary_mode() of
gnulib. And they will abort as soon as trying to use xset_binary_mode()
at startup.


Therefore, the proper place is gnulib.

-- 
KO Myung-Hun

Using Mozilla SeaMonkey 2.7.2
Under OS/2 Warp 4 for Korean with FixPak #15
In VirtualBox v6.0.8 on Intel Core i7-3615QM 2.30GHz with 8GB RAM

Korean OS/2 User Community : http://www.os2.kr/