|
|
Created:
10 years, 3 months ago by mattn Modified:
9 years, 3 months ago Reviewers:
CC:
golang-codereviews, gobot, brainman, minux1, rsc, peterGo Visibility:
Public. |
Descriptionos,syscall: use ReadFile/MultiByteToWideChar to read from console
Patch Set 1 #Patch Set 2 : diff -r 7e1a4e190b02 http://go.googlecode.com/hg/ #Patch Set 3 : diff -r 7e1a4e190b02 http://go.googlecode.com/hg/ #Patch Set 4 : diff -r f96d3354edb6 http://go.googlecode.com/hg/ #
MessagesTotal messages: 24
Hello golang-dev@googlegroups.com, I'd like you to review this change to http://go.googlecode.com/hg/
Sign in to reply to this message.
Replacing golang-dev with golang-codereviews. To the author of this CL: If you are using 'hg mail -r golang-dev' to mail the CL, use simply 'hg mail' instead. If you did not name golang-dev explicitly and it was still added to the CL, it means your working copy of the repo has a stale codereview.cfg (or lib/codereview/codereview.cfg). Please run 'hg sync' to update your client to the most recent codereview.cfg. If the most recent codereview.cfg has defaultcc set to golang-dev instead of golang-codereviews, please send a CL correcting it. Thanks very much.
Sign in to reply to this message.
R=alex.brainman@gmail.com (assigned by mattn.jp@gmail.com)
Sign in to reply to this message.
On Dec 23, 2013 7:30 PM, <mattn.jp@gmail.com> wrote: > Description: > os,syscall: use ReadFile/MultiByteToWideChar to read from console any rationales behind this change? is this fixing a bug? if so, which is it?
Sign in to reply to this message.
On 2013/12/24 00:57:55, minux wrote: > On Dec 23, 2013 7:30 PM, <mailto:mattn.jp@gmail.com> wrote: > > Description: > > os,syscall: use ReadFile/MultiByteToWideChar to read from console > any rationales behind this change? > is this fixing a bug? if so, which is it? Fixes issue #6303 Discussion thread is https://groups.google.com/forum/#!searchin/golang-dev/ctrl-z/golang-dev/xnjP0...
Sign in to reply to this message.
Yes. It has been discussed before. And I am against this change, because I don't think issue 6303 is valid. Perhaps this change improves current situation in general, but maybe it makes it worse. I prefer to leave things as they are now. Alex
Sign in to reply to this message.
> think issue 6303 is valid What is your reason? Why you had broken compatibility and functionally of interrupting console input-stream ?
Sign in to reply to this message.
On 2014/01/15 02:14:44, mattn wrote: > > think issue 6303 is valid > > What is your reason? We use ReadConsole for stdin input, and it does what it does. No one (but you) complained about it yet. You say "Ctrl-Z should interrupt stdin". Perhaps you right, perhaps you wrong. But you failed to convince me one way or other. I don't want to change something that works. > Why you had broken compatibility and functionally of interrupting console > input-stream ? I don't think we promise that. It is not documented anywhere. Alex
Sign in to reply to this message.
On 2014/01/15 02:25:33, brainman wrote: > On 2014/01/15 02:14:44, mattn wrote: > > > think issue 6303 is valid > > > > What is your reason? > > We use ReadConsole for stdin input, and it does what it does. No one (but you) > complained about it yet. > > You say "Ctrl-Z should interrupt stdin". Perhaps you right, perhaps you wrong. > But you failed to convince me one way or other. I don't want to change something > that works. > > > Why you had broken compatibility and functionally of interrupting console > > input-stream ? > > I don't think we promise that. It is not documented anywhere. > > Alex For posix (or posix like) system, as you know, using stdin (write/interrupting stdin) is generally way to get text from user input. fmt.Println("Type your comment and finish with typing ^D") bytes, err := ioutil.ReadAll(os.Stdin) And we could imitate that on windows also. Currently, we don't have a way to interrupt stdin with ^D, ^Z, or else on windows. We must stop the process via process manager. I can see some web contents about golang that contains using stdin/ctrl-d. I know we can imitate it with workarounds on windows. But, probably, I believe there are many users who want to interrupt stdin on windows also.
Sign in to reply to this message.
ping
Sign in to reply to this message.
I think this code looks good. It makes stdin reading more like a file and less like a tty, which is what it should be. If, as mattn says, it makes ^Z actually mean EOF, that seems like progress. Alex, what's your objection? Thanks. Russ
Sign in to reply to this message.
On 2014/03/05 20:06:11, rsc wrote: > ... It makes stdin reading more like a file and less > like a tty, ... How is that? Is that because new code uses ReadConsole instead of ReadFile? But then ReadFile can read from anything - files, sockets, console, communication devices. So its name is a misnomer. > ... it makes ^Z actually > mean EOF, that seems like progress. I am not sure about that. Who will be the users of this new functionality, if we proceed with CL? What are other windows programs that handle ^Z this way? > Alex, what's your objection? There are too many unknown, if we change. Here are some I can think of: - New and different semantics (interpreting keystrokes differently - line ends, ctls+z, ... what else). Who will be affected? How? - We will use different API to read data from console. It might behave differently when handling errors and Ctrl+C. Who will be affected? How? - We know how ReadConsole works in terms of interpreting non-english input. AFAIK, we don't have any outstanding complains about that. How can we be sure that new code works in a similar fashion? I don't see any documentation that says ReadFile returns data that can be processed with MultiByteToWideChar. I am also not convinced that using GetACP and MultiByteToWideChar combination will work to produce utf16 every time. When will it fail? - We don't have any tests for console io. Any changes we make will be silently accepted by our all.bat and builders. So this code is easy to break. I don't think the benefits you've mentioned are worth the risk. Alex
Sign in to reply to this message.
On 2014/03/06 00:07:55, brainman wrote: > > ... Here are some I can think of: > > - ... > > - There is currently nice symmetry between ReadConsole / WriteConsole. Both input and output behave the same way. Even if they broken. Alex
Sign in to reply to this message.
On Wed, Mar 5, 2014 at 7:07 PM, <alex.brainman@gmail.com> wrote: > On 2014/03/05 20:06:11, rsc wrote: > >> ... It makes stdin reading more like a file and less >> like a tty, ... >> > > How is that? Is that because new code uses ReadConsole instead of > ReadFile? But then ReadFile can read from anything - files, sockets, > console, communication devices. So its name is a misnomer. > You have it backward. The current code uses ReadConsole. This CL changes to use ReadFile instead. That seems like a step forward based on the name alone. > ... it makes ^Z actually >> >> mean EOF, that seems like progress. >> > > I am not sure about that. Who will be the users of this new > functionality, if we proceed with CL? What are other windows programs > that handle ^Z this way? Everything I know about that reads from a console behaves this way. The simplest example is this: copy con x.txt hello world ^Z It is the Windows equivalent of cat >x.txt hello world ^D This CL makes that work for Go programs. Alex, what's your objection? >> > > There are too many unknown, if we change. Here are some I can think of: > > - New and different semantics (interpreting keystrokes differently - > line ends, ctls+z, ... what else). Who will be affected? How? > > - We will use different API to read data from console. It might behave > differently when handling errors and Ctrl+C. Who will be affected? How? > > - We know how ReadConsole works in terms of interpreting non-english > input. AFAIK, we don't have any outstanding complains about that. How > can we be sure that new code works in a similar fashion? I don't see any > documentation that says ReadFile returns data that can be processed with > MultiByteToWideChar. I am also not convinced that using GetACP and > MultiByteToWideChar combination will work to produce utf16 every time. > When will it fail? > The question is whether GetACP describes the result of ReadFile correctly. That I can't find the answer to. MultiByteToWideChar is guaranteed to produce UTF-16. > - We don't have any tests for console io. Any changes we make will be > silently accepted by our all.bat and builders. So this code is easy to > break. > > I don't think the benefits you've mentioned are worth the risk. > We have three months of testing ahead of us to figure that out. mattn clearly uses console input in his Go programs or he wouldn't have run into this. And he uses non-English character sets. So I am willing to defer to him on this. The code looks worth trying at least. On Wed, Mar 5, 2014 at 7:27 PM, <alex.brainman@gmail.com> wrote: > - There is currently nice symmetry between ReadConsole / WriteConsole. > Both input and output behave the same way. Even if they broken. > You know this is a silly thing to say. If they're broken, it doesn't matter how pretty they are. Here's a different summary. ^Z is broken right now. I think that's worth fixing. How? mattn has suggested using ReadFile + GetACP + MultiByteToWideChar. Can we tweak the arguments to ReadConsole instead? It looks like maybe we can - the final argument is some kind of struct that you can set up to describe the desired EOF marker. Maybe changing that one argument would be better than switching which function we use? Does anyone want to try that? Thanks. Russ
Sign in to reply to this message.
On 2014/03/06 03:47:46, rsc wrote: > > We have three months of testing ahead of us to figure that out. mattn > clearly uses console input in his Go programs or he wouldn't have run into > this. And he uses non-English character sets. So I am willing to defer to > him on this. The code looks worth trying at least. Sure. It is your call. > > - There is currently nice symmetry between ReadConsole / WriteConsole. > > Both input and output behave the same way. Even if they broken. > > > > You know this is a silly thing to say. If they're broken, it doesn't matter > how pretty they are. Perhaps I didn't explain myself very well. Both ReadConsole and WriteConsole will translate between one single console encoding and utf16 (in both way). If we replace ReadConsole with MultiByteToWideChar/GetACP, will my previous statement remain true? I am not sure. So, you might end up in situations where you input and output speak different languages. > Here's a different summary. ^Z is broken right now. I think that's worth > fixing. How? mattn has suggested using ReadFile + GetACP + > MultiByteToWideChar. Can we tweak the arguments to ReadConsole instead? Maybe it is possible. I don't know. I think I was looking into it long time ago (maybe look into SetConsoleMode). Unfortunately I don't remember. > ... Maybe changing that one > argument would be better than switching which function we use? ... I agree. > ... Does anyone > want to try that? Sorry, but no. I am short of time now. Alex
Sign in to reply to this message.
Russ, On 2014/03/06 03:47:46, rsc wrote: > > Here's a different summary. ^Z is broken right now. I think that's worth > fixing. How? mattn has suggested using ReadFile + GetACP + > MultiByteToWideChar. Can we tweak the arguments to ReadConsole instead? It > looks like maybe we can - the final argument is some kind of struct that > you can set up to describe the desired EOF marker. Maybe changing that one > argument would be better than switching which function we use? Does anyone > want to try that? > > Russ Yes. The ReadConsole pInputControl parameter is a pointer to a CONSOLE_READCONSOLE_CONTROL structure. ReadConsole: http://msdn.microsoft.com/en-us/library/windows/desktop/ms684958.aspx CONSOLE_READCONSOLE_CONTROL: http://msdn.microsoft.com/en-us/library/windows/desktop/aa363485.aspx The dwCtrlWakeupMask parameter of the CONSOLE_READCONSOLE_CONTROL structure looks interesting: A user-defined control character used to signal that the read is complete. I'm not sure what they mean by a mask. The only example I've found uses the value 0x200 for the Tab key, which seems to work. I don't know what the value should be for Control-Z. Any suggestions? Unfortunately the minimum requirements for CONSOLE_READCONSOLE_CONTROL are Windows Vista and Windows Server 2008. Go needs to provide support for Windows XP, at least for the next two years. Given how long Go clung to Windows 2000 after it became obsolete, five or more years may be more realistic. I've looked at and tested the original ReadFile, the current ReadConsole, and the proposed ReadFile + GetACP + MultiByteToWideChar implementions of readConsole. None of them work properly. I also ran the test cases against my ReadConsole implementation of readConsole; they all seemed to work. My implementation of readConsole addresses the failure of the Windows port to fulfill the package os portability promise: "Package os provides a platform-independent interface to operating system functionality. The design is Unix-like, although the error handling is Go-like; failing calls return values of type error rather than error numbers. Often, more information is available within the error. For example, if a call that takes a file name fails, such as Open or Stat, the error will include the failing file name when printed and will be of type *PathError, which may be unpacked for more information." "The os interface is intended to be uniform across all operating systems. Features not generally available appear in the system-specific package syscall." http://golang.org/pkg/os/ To test my implementation of Windows readConsole, I compare the count, data and error (including io.EOF) values read by the program from the terminal/console to ensure that they are exactly the same on both Linux and Windows. Simplistically, if you replace all occurrences of ^D for terminal input on Linux by ^Z<Enter> for console input on Windows then the results will be exactly the same. Actually, it's smarter than that. If you are willing to accept an issue that the Windows port readConsole function should fulfill the package os portability promise, including returning io.EOF for zero data, I'll submit a CL. We should ask mattn to test it to make sure that it resolves his issues. Peter
Sign in to reply to this message.
On Mon, Apr 21, 2014 at 7:50 AM, <go.peter.90@gmail.com> wrote: > Unfortunately the minimum requirements for CONSOLE_READCONSOLE_CONTROL > are Windows Vista and Windows Server 2008. Go needs to provide support > for Windows XP, at least for the next two years. Given how long Go clung > to Windows 2000 after it became obsolete, five or more years may be more > realistic. > I think at this stage, any solution that drops XP support is unacceptable. > > I've looked at and tested the original ReadFile, the current > ReadConsole, and the proposed ReadFile + GetACP + MultiByteToWideChar > implementions of readConsole. None of them work properly. I also ran the > test cases against my ReadConsole implementation of readConsole; they > all seemed to work. > > My implementation of readConsole addresses the failure of the Windows > port to fulfill the package os portability promise: > > "Package os provides a platform-independent interface to operating > system functionality. The design is Unix-like, although the error > handling is Go-like; failing calls return values of type error rather > than error numbers. Often, more information is available within the > error. For example, if a call that takes a file name fails, such as Open > or Stat, the error will include the failing file name when printed and > will be of type *PathError, which may be unpacked for more information." > > "The os interface is intended to be uniform across all operating > systems. Features not generally available appear in the system-specific > package syscall." > > http://golang.org/pkg/os/ > > To test my implementation of Windows readConsole, I compare the count, > data and error (including io.EOF) values read by the program from the > terminal/console to ensure that they are exactly the same on both Linux > and Windows. Simplistically, if you replace all occurrences of ^D for > terminal input on Linux by ^Z<Enter> for console input on Windows then > the results will be exactly the same. Actually, it's smarter than that. > > If you are willing to accept an issue that the Windows port readConsole > function should fulfill the package os portability promise, including > returning io.EOF for zero data, I'll submit a CL. We should ask mattn to > test it to make sure that it resolves his issues. > It all depends how much effort it needs to be uniform across platforms. And IMO we've already invested way too much energy on this issue.
Sign in to reply to this message.
Russ, On 2014/04/21 11:50:45, peterGo wrote: > Russ, > > The dwCtrlWakeupMask parameter of the CONSOLE_READCONSOLE_CONTROL structure > looks interesting: A user-defined control character used to signal that the read > is complete. I'm not sure what they mean by a mask. The only example I've found > uses the value 0x200 for the Tab key, which seems to work. I don't know what the > value should be for Control-Z. Any suggestions? > > Peter The dwCtrlWakeupMask value for the Tab key is 0x200 (1 << '\t' or 1 << 0x09). Therefore, the dwCtrlWakeupMask value for Control-Z is 0x4000000 (1 << Ctrl-Z or 1 << 0x1A). Peter
Sign in to reply to this message.
Russ, On 2014/03/06 03:47:46, rsc wrote: > > Here's a different summary. ^Z is broken right now. I think that's worth > fixing. How? mattn has suggested using ReadFile + GetACP + > MultiByteToWideChar. Can we tweak the arguments to ReadConsole instead? It > looks like maybe we can - the final argument is some kind of struct that > you can set up to describe the desired EOF marker. Maybe changing that one > argument would be better than switching which function we use? Does anyone > want to try that? > > Thanks. > Russ Yes. I've published my solution as: os,syscall: On Windows, recognize Control-Z for console input https://codereview.appspot.com/95780046 Peter
Sign in to reply to this message.
mattn, On 2013/12/24 02:02:12, mattn wrote: > Fixes issue #6303 After patching the Go tip with your CL, issue45150045_40001.diff, go version devel +2e591e82a8c8 Fri May 02 13:17:55 2014 -0700 + windows/amd64 I ran i6303.go, a mimimal test program for issue 6303, https://code.google.com/p/go/issues/detail?id=6303, package main import ( "fmt" "io" "os" ) func main() { n, err := io.Copy(os.Stdout, os.Stdin) if err != nil { fmt.Println(n, err) } } The test program failed. C:\gopath\src\mattn>go run i6303.go 0 read /dev/stdin: Not enough storage is available to process this command. C:\gopath\src\mattn> Peter
Sign in to reply to this message.
2e591e82a8c8 seems not related on this issue? right? On 2014/05/04 18:31:33, peterGo wrote: > mattn, > > On 2013/12/24 02:02:12, mattn wrote: > > Fixes issue #6303 > > After patching the Go tip with your CL, issue45150045_40001.diff, > > go version devel +2e591e82a8c8 Fri May 02 13:17:55 2014 -0700 + windows/amd64 > > I ran i6303.go, a mimimal test program for issue 6303, > https://code.google.com/p/go/issues/detail?id=6303, > > package main > > import ( > "fmt" > "io" > "os" > ) > > func main() { > n, err := io.Copy(os.Stdout, os.Stdin) > if err != nil { > fmt.Println(n, err) > } > } > > The test program failed. > > C:\gopath\src\mattn>go run i6303.go > 0 read /dev/stdin: Not enough storage is available to process this command. > C:\gopath\src\mattn> > > Peter
Sign in to reply to this message.
I updated patch. Windows can't read bytes over max of int16.
Sign in to reply to this message.
ping
Sign in to reply to this message.
R=close To the author of this CL: The Go project has moved to Gerrit Code Review. If this CL should be continued, please see the latest version of https://golang.org/doc/contribute.html for instructions on how to set up Git and the Go project's Gerrit codereview plugin, and then create a new change with your current code. If there has been discussion on this CL, please give a link to it (golang.org/cl/45150045 is best) in the description in your new CL. Thanks very much.
Sign in to reply to this message.
|