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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html