Re: [PATCH] Enable HAVE_DEV_TTY for Solaris
On Tue, Aug 7, 2012 at 6:10 AM, Jeff King p...@peff.net wrote: Subject: [PATCH] terminal: seek when switching between reading and writing When a stdio stream is opened in update mode (e.g., w+), the C standard forbids switching between reading or writing without an intervening positioning function. Many implementations are lenient about this, but Solaris libc will flush the recently-read contents to the output buffer. In this instance, that meant writing the non-echoed password that the user just typed to the terminal. Fix it by inserting a no-op fseek between the read and write. My Windows-patches for git_terminal_prompt would probably also solve this problem. Instead of opening a read-write handle to /dev/tty, they open two handles to the terminal instead; one for reading and one for writing. This is because the terminal cannot be opened in read-write mode on Windows (we need to open CONIN$ and CONOUT$ separately). You can have a look at the series here if you're interested: https://github.com/kusma/git/tree/work/terminal-cleanup That last patch is the reason why I haven't submitted the series yet, but perhaps some of the preparatory patches could be worth-while for other platforms in the mean time? -- 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] Enable HAVE_DEV_TTY for Solaris
On Wed, Aug 08, 2012 at 04:13:03PM +0200, Erik Faye-Lund wrote: On Tue, Aug 7, 2012 at 6:10 AM, Jeff King p...@peff.net wrote: Subject: [PATCH] terminal: seek when switching between reading and writing When a stdio stream is opened in update mode (e.g., w+), the C standard forbids switching between reading or writing without an intervening positioning function. Many implementations are lenient about this, but Solaris libc will flush the recently-read contents to the output buffer. In this instance, that meant writing the non-echoed password that the user just typed to the terminal. Fix it by inserting a no-op fseek between the read and write. My Windows-patches for git_terminal_prompt would probably also solve this problem. Instead of opening a read-write handle to /dev/tty, they open two handles to the terminal instead; one for reading and one for writing. This is because the terminal cannot be opened in read-write mode on Windows (we need to open CONIN$ and CONOUT$ separately). Yeah, it would solve it, although it means opening /dev/tty twice (which is probably not a big deal, though). I'm fine if we go that way in the long run to share implementations, but let's treat it as a separate topic. This fix is an obvious one-liner, and is just fixing me being stupid about actually following the C standard. So it's a no-brainer for as a maintenance fix. You can have a look at the series here if you're interested: https://github.com/kusma/git/tree/work/terminal-cleanup That last patch is the reason why I haven't submitted the series yet, but perhaps some of the preparatory patches could be worth-while for other platforms in the mean time? Yeah, that last patch is really gross. There's no explanation of the race issue, so I'll refrain from thinking about it until you are ready to post a series. :) -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] Enable HAVE_DEV_TTY for Solaris
Excerpts from Jeff King's message of Tue Aug 07 00:10:26 -0400 2012: Signed-off-by: Jeff King p...@peff.net Acked-by: Ben Walton bwal...@artsci.utoronto.ca I agree with your assesment that this is the right way to go (vs migrating out of stdio) for now. If the changes Tay needs to make require the migration then it can become part of that series. Otherwise those changes would just be change for change's sake at this time. If my HAVE_DEV_TTY enabling patch for Solaris is added on top of this, I'm a happy camper. Thanks -Ben -- Ben Walton Systems Programmer - CHASS University of Toronto C:416.407.5610 | W:416.978.4302 -- 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] Enable HAVE_DEV_TTY for Solaris
Ben Walton bwal...@artsci.utoronto.ca writes: Now that git_terminal_prompt can cleanly interact with /dev/tty on Solaris, enable HAVE_DEV_TTY so that this code path is used for credential reading instead of relying on the crippled getpass(). Signed-off-by: Ben Walton bwal...@artsci.utoronto.ca --- This is a follow up to Jeff's patch that fixes git_terminal_prompt on Solaris. I don't have 5.6 or 5.7 for testing but I believe this should be valid for both of those releases as well. So which direction do you guys want to go? Use the bidirectional stdio with fseek() for now, with the expectation that Tay's other series will rewrite it to fd based one? Makefile |1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index 15d1319..6b0c961 100644 --- a/Makefile +++ b/Makefile @@ -1014,6 +1014,7 @@ ifeq ($(uname_S),SunOS) NO_REGEX = YesPlease NO_FNMATCH_CASEFOLD = YesPlease NO_MSGFMT_EXTENDED_OPTIONS = YesPlease + HAVE_DEV_TTY = YesPlease ifeq ($(uname_R),5.6) SOCKLEN_T = int NO_HSTRERROR = YesPlease -- 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] Enable HAVE_DEV_TTY for Solaris
On Mon, Aug 06, 2012 at 08:43:02PM -0700, Junio C Hamano wrote: Ben Walton bwal...@artsci.utoronto.ca writes: Now that git_terminal_prompt can cleanly interact with /dev/tty on Solaris, enable HAVE_DEV_TTY so that this code path is used for credential reading instead of relying on the crippled getpass(). Signed-off-by: Ben Walton bwal...@artsci.utoronto.ca --- This is a follow up to Jeff's patch that fixes git_terminal_prompt on Solaris. I don't have 5.6 or 5.7 for testing but I believe this should be valid for both of those releases as well. So which direction do you guys want to go? Use the bidirectional stdio with fseek() for now, with the expectation that Tay's other series will rewrite it to fd based one? I think so. The stdio fix is short and obviously correct, and then Tay can either refactor or not as he sees fit for his topic (although if we do switch to just a terminal_can_prompt() interface and get rid of the term_t ugliness, then there is not even any need to do the rewrite). -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] Enable HAVE_DEV_TTY for Solaris
On Tue, Aug 07, 2012 at 12:03:26AM -0400, Jeff King wrote:
So which direction do you guys want to go? Use the bidirectional
stdio with fseek() for now, with the expectation that Tay's other
series will rewrite it to fd based one?
I think so. The stdio fix is short and obviously correct, and then Tay
can either refactor or not as he sees fit for his topic (although if we
do switch to just a terminal_can_prompt() interface and get rid of the
term_t ugliness, then there is not even any need to do the rewrite).
And here it is again, this time with a signed-off-by (I fixed my script
after our last discussion, but accidentally copied an old version to the
Solaris VM I just installed. ;) ).
-- 8 --
Subject: [PATCH] terminal: seek when switching between reading and writing
When a stdio stream is opened in update mode (e.g., w+),
the C standard forbids switching between reading or writing
without an intervening positioning function. Many
implementations are lenient about this, but Solaris libc
will flush the recently-read contents to the output buffer.
In this instance, that meant writing the non-echoed password
that the user just typed to the terminal.
Fix it by inserting a no-op fseek between the read and
write.
The opposite direction (writing followed by reading) is also
disallowed, but our intervening fflush is an acceptable
positioning function for that alternative.
Signed-off-by: Jeff King p...@peff.net
---
compat/terminal.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/compat/terminal.c b/compat/terminal.c
index 6d16c8f..bbb038d 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -59,6 +59,7 @@ char *git_terminal_prompt(const char *prompt, int echo)
r = strbuf_getline(buf, fh, '\n');
if (!echo) {
+ fseek(fh, SEEK_CUR, 0);
putc('\n', fh);
fflush(fh);
}
--
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
