Re: [PATCH v3 1/2] add interface for /dev/tty interaction

[Jeff King <peff@peff.net>]

On Mon, Aug 06, 2012 at 03:45:11PM -0400, Jeff King wrote:

> > This is not your fault but seeing term_t made me go "eek, yuck".
> 
> Agreed.
> 
> > As far as I can see, use of "FILE *" in existing compat/terminal.c
> > is not buying us anything useful.  The stdio calls made on FILE *fh
> > are only fopen(), fputs(), fflush() and fclose(), and everything
> > else goes through fileno(fh).
> > 
> > So perhaps it is a saner approach to fix that function first before
> > this patch so that it works on file descriptors.
> 
> Yeah, I think that is a good path. I think my original use of stdio
> was mostly because I started by paring down glibc's implementation of
> getpass.  Since we have niceties like write_in_full, I don't think
> there's any reason not to just skip stdio.

I forgot to mention: even if we changed the HAVE_DEV_TTY code path to
use an integer descriptor (which I think is a sane thing to do
regardless), that may not be sufficient to solve this problem.

Erik has looked into doing a Windows alternative in compat/terminal.c,
and I think an integer would be insufficient there. In particular, I
think Windows needs two descriptors to accomplish the same thing (one
for CONIN$ and one for CONOUT$).  So you'd need to turn term_t into a
struct (and you'd probably not want to return it by value then).

Maybe it would be better to keep the abstraction as non-leaky as
possible, and just provide "terminal_can_prompt()" or similar?

Returning the open descriptor (as Tay's patch does) avoids a race
condition where /dev/tty can be opened when terminal_can_prompt runs,
but not when we try to actually read from it. But we can either:

  1. Not care. Even if the tty is opened, if a user has closed the
     terminal we are going to get a read error anyway. So you can never
     avoid that race condition in some form.

  2. Open the terminal descriptor when either function is called, and
     never close it. I don't think there is any reason we can't just
     leak the descriptor.

-Peff