Re: [msysGit] [PATCH/RFC 2/5] compat/terminal: factor out echo-disabling
Hi kusma, On Sat, 1 Dec 2012, Erik Faye-Lund wrote: > On Fri, Nov 30, 2012 at 6:59 PM, Johannes Schindelin > wrote: > > Hi, > > > > On Tue, 13 Nov 2012, Erik Faye-Lund wrote: > > > >> By moving the echo-disabling code to a separate function, we can > >> implement OS-specific versions of it for non-POSIX platforms. > >> > >> Signed-off-by: Erik Faye-Lund > >> --- > >> compat/terminal.c | 43 +-- > >> 1 file changed, 25 insertions(+), 18 deletions(-) > >> > >> diff --git a/compat/terminal.c b/compat/terminal.c > >> index bbb038d..3217838 100644 > >> --- a/compat/terminal.c > >> +++ b/compat/terminal.c > >> @@ -14,6 +14,7 @@ static void restore_term(void) > >> return; > >> > >> tcsetattr(term_fd, TCSAFLUSH, &old_term); > >> + close(term_fd); > >> term_fd = -1; > >> } > > > > That looks like an independent resource leak fix... correct? > > It might look like it, but it's not; term_fd used to be returned by > "fileno(fh)", and fh did get properly closed. > > With my refactoring, disable_echo/restore_term takes opens /dev/tty a > second time, like Jeff points out. And that second file descriptor > needs to be closed. Thanks for clarifying, Dscho -- 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: [msysGit] [PATCH/RFC 2/5] compat/terminal: factor out echo-disabling
On Fri, Nov 30, 2012 at 6:59 PM, Johannes Schindelin wrote: > Hi, > > On Tue, 13 Nov 2012, Erik Faye-Lund wrote: > >> By moving the echo-disabling code to a separate function, we can >> implement OS-specific versions of it for non-POSIX platforms. >> >> Signed-off-by: Erik Faye-Lund >> --- >> compat/terminal.c | 43 +-- >> 1 file changed, 25 insertions(+), 18 deletions(-) >> >> diff --git a/compat/terminal.c b/compat/terminal.c >> index bbb038d..3217838 100644 >> --- a/compat/terminal.c >> +++ b/compat/terminal.c >> @@ -14,6 +14,7 @@ static void restore_term(void) >> return; >> >> tcsetattr(term_fd, TCSAFLUSH, &old_term); >> + close(term_fd); >> term_fd = -1; >> } > > That looks like an independent resource leak fix... correct? It might look like it, but it's not; term_fd used to be returned by "fileno(fh)", and fh did get properly closed. With my refactoring, disable_echo/restore_term takes opens /dev/tty a second time, like Jeff points out. And that second file descriptor needs to be closed. -- 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: [msysGit] [PATCH/RFC 2/5] compat/terminal: factor out echo-disabling
On Fri, Nov 30, 2012 at 06:59:30PM +0100, Johannes Schindelin wrote: > > diff --git a/compat/terminal.c b/compat/terminal.c > > index bbb038d..3217838 100644 > > --- a/compat/terminal.c > > +++ b/compat/terminal.c > > @@ -14,6 +14,7 @@ static void restore_term(void) > > return; > > > > tcsetattr(term_fd, TCSAFLUSH, &old_term); > > + close(term_fd); > > term_fd = -1; > > } > > That looks like an independent resource leak fix... correct? I don't think so. In the current code, term_fd does not take ownership of the fd. It is fundamentally part of the FILE* in git_terminal_prompt, and is closed when we fclose() that. But in Erik's refactor, we actually open a _second_ descriptor to /dev/tty, which needs to be closed. I don't think there is any reason that should not work (the terminal characteristics should be per-tty, not per-descriptor), though it does feel a little hacky to have to open /dev/tty twice. -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: [msysGit] [PATCH/RFC 2/5] compat/terminal: factor out echo-disabling
Hi, On Tue, 13 Nov 2012, Erik Faye-Lund wrote: > By moving the echo-disabling code to a separate function, we can > implement OS-specific versions of it for non-POSIX platforms. > > Signed-off-by: Erik Faye-Lund > --- > compat/terminal.c | 43 +-- > 1 file changed, 25 insertions(+), 18 deletions(-) > > diff --git a/compat/terminal.c b/compat/terminal.c > index bbb038d..3217838 100644 > --- a/compat/terminal.c > +++ b/compat/terminal.c > @@ -14,6 +14,7 @@ static void restore_term(void) > return; > > tcsetattr(term_fd, TCSAFLUSH, &old_term); > + close(term_fd); > term_fd = -1; > } That looks like an independent resource leak fix... correct? Rest looks awsomely fine. Dscho -- 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
[PATCH/RFC 2/5] compat/terminal: factor out echo-disabling
By moving the echo-disabling code to a separate function, we can
implement OS-specific versions of it for non-POSIX platforms.
Signed-off-by: Erik Faye-Lund
---
compat/terminal.c | 43 +--
1 file changed, 25 insertions(+), 18 deletions(-)
diff --git a/compat/terminal.c b/compat/terminal.c
index bbb038d..3217838 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -14,6 +14,7 @@ static void restore_term(void)
return;
tcsetattr(term_fd, TCSAFLUSH, &old_term);
+ close(term_fd);
term_fd = -1;
}
@@ -24,6 +25,27 @@ static void restore_term_on_signal(int sig)
raise(sig);
}
+static int disable_echo()
+{
+ struct termios t;
+
+ term_fd = open("/dev/tty", O_RDWR);
+ if (tcgetattr(term_fd, &t) < 0)
+ goto error;
+
+ old_term = t;
+ sigchain_push_common(restore_term_on_signal);
+
+ t.c_lflag &= ~ECHO;
+ if (!tcsetattr(term_fd, TCSAFLUSH, &t))
+ return 0;
+
+error:
+ close(term_fd);
+ term_fd = -1;
+ return -1;
+}
+
char *git_terminal_prompt(const char *prompt, int echo)
{
static struct strbuf buf = STRBUF_INIT;
@@ -34,24 +56,9 @@ char *git_terminal_prompt(const char *prompt, int echo)
if (!fh)
return NULL;
- if (!echo) {
- struct termios t;
-
- if (tcgetattr(fileno(fh), &t) < 0) {
- fclose(fh);
- return NULL;
- }
-
- old_term = t;
- term_fd = fileno(fh);
- sigchain_push_common(restore_term_on_signal);
-
- t.c_lflag &= ~ECHO;
- if (tcsetattr(fileno(fh), TCSAFLUSH, &t) < 0) {
- term_fd = -1;
- fclose(fh);
- return NULL;
- }
+ if (!echo && disable_echo()) {
+ fclose(fh);
+ return NULL;
}
fputs(prompt, fh);
--
1.8.0.7.gbeffeda
--
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
