Re: [PATCH v3 1/2] add interface for /dev/tty interaction
Jeff King writes: > 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? OK. As we won't be giving separate instances of terminals to different callers anyway, compat/terminal.c can keep a static variable of whatever type that is necessary for the implementation around. That sounds like a reasonable way to go. > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] add interface for /dev/tty interaction
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] add interface for /dev/tty interaction
On Sun, Aug 05, 2012 at 01:11:47PM -0700, Junio C Hamano wrote: > Tay Ray Chuan writes: > > > Factor out the opening and closing of /dev/tty from > > git_terminal_prompt(), so that callers may first test if a controlling > > terminal is available before proceeding with prompting proper. > > > > When HAVE_DEV_TTY is not defined, terminal_open() falls back to checking > > tty-ness of stdin and stderr, as getpass() uses them both. > > > > Signed-off-by: Tay Ray Chuan > > --- > > 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. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] add interface for /dev/tty interaction
Tay Ray Chuan writes:
> Factor out the opening and closing of /dev/tty from
> git_terminal_prompt(), so that callers may first test if a controlling
> terminal is available before proceeding with prompting proper.
>
> When HAVE_DEV_TTY is not defined, terminal_open() falls back to checking
> tty-ness of stdin and stderr, as getpass() uses them both.
>
> Signed-off-by: Tay Ray Chuan
> ---
This is not your fault but seeing term_t made me go "eek, yuck".
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.
> compat/terminal.c | 52
> compat/terminal.h | 10 ++
> 2 files changed, 54 insertions(+), 8 deletions(-)
>
> diff --git a/compat/terminal.c b/compat/terminal.c
> index 6d16c8f..c85d5c7 100644
> --- a/compat/terminal.c
> +++ b/compat/terminal.c
> @@ -24,15 +24,21 @@ static void restore_term_on_signal(int sig)
> raise(sig);
> }
>
> -char *git_terminal_prompt(const char *prompt, int echo)
> +term_t terminal_open(void)
> +{
> + return fopen("/dev/tty", "w+");
> +}
> +
> +int terminal_close(term_t term)
> +{
> + return fclose(term);
> +}
> +
> +char *terminal_prompt(term_t term, const char *prompt, int echo)
> {
> static struct strbuf buf = STRBUF_INIT;
> int r;
> - FILE *fh;
> -
> - fh = fopen("/dev/tty", "w+");
> - if (!fh)
> - return NULL;
> + FILE *fh = term;
>
> if (!echo) {
> struct termios t;
> @@ -64,18 +70,48 @@ char *git_terminal_prompt(const char *prompt, int echo)
> }
>
> restore_term();
> - fclose(fh);
>
> if (r == EOF)
> return NULL;
> return buf.buf;
> }
>
> +char *git_terminal_prompt(const char *prompt, int echo)
> +{
> + char *ret;
> + term_t term;
> +
> + term = terminal_open();
> + if (!term)
> + return NULL;
> +
> + ret = terminal_prompt(term, prompt, echo);
> +
> + terminal_close(term);
> +
> + return ret;
> +}
> +
> #else
>
> -char *git_terminal_prompt(const char *prompt, int echo)
> +term_t terminal_open()
> +{
> + return isatty(0) && isatty(2);
> +}
> +
> +int terminal_close(term_t term)
> +{
> + return 0;
> +}
> +
> +char *terminal_prompt(term_t term, const char *prompt, int echo)
> {
> return getpass(prompt);
> }
>
> +char *git_terminal_prompt(const char *prompt, int echo)
> +{
> + return terminal_prompt(prompt, echo);
> +}
> +
> #endif
> diff --git a/compat/terminal.h b/compat/terminal.h
> index 97db7cd..cf2aa10 100644
> --- a/compat/terminal.h
> +++ b/compat/terminal.h
> @@ -1,6 +1,16 @@
> #ifndef COMPAT_TERMINAL_H
> #define COMPAT_TERMINAL_H
>
> +#ifdef HAVE_DEV_TTY
> +typedef FILE *term_t;
> +#else
> +typedef int term_t;
> +#endif
> +
> +term_t terminal_open();
> +int terminal_close(term_t term);
> +char *terminal_prompt(term_t term, const char *prompt, int echo);
> +
> char *git_terminal_prompt(const char *prompt, int echo);
>
> #endif /* COMPAT_TERMINAL_H */
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
