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

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

quoted

This is not your fault but seeing term_t made me go "eek, yuck".

Agreed.

quoted

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