Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(3494)

Issue 6488044: code review 6488044: os: detect and handle console in File.Write on windows (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 7 months ago by brainman
Modified:
11 years, 6 months ago
Reviewers:
CC:
golang-dev, bsiegert, minux1, rsc
Visibility:
Public.

Description

os: detect and handle console in File.Write on windows Fixes issue 3376.

Patch Set 1 #

Patch Set 2 : diff -r 6836919e6ff1 https://go.googlecode.com/hg/ #

Total comments: 4

Patch Set 3 : diff -r 3b78b41a4b50 https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 4 : diff -r cdee8bf43694 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+99 lines, -0 lines) Patch
M src/pkg/os/file_windows.go View 1 2 4 chunks +45 lines, -0 lines 0 comments Download
M src/pkg/syscall/syscall_windows.go View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M src/pkg/syscall/zsyscall_windows_386.go View 1 2 chunks +26 lines, -0 lines 0 comments Download
M src/pkg/syscall/zsyscall_windows_amd64.go View 1 2 chunks +26 lines, -0 lines 0 comments Download

Messages

Total messages: 12
brainman
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
11 years, 7 months ago (2012-08-28 05:53:24 UTC) #1
bsiegert
I am wondering if the code in consoleWrite can be simplified by casting the byte ...
11 years, 7 months ago (2012-08-28 08:24:40 UTC) #2
minux1
can we test it? this is a important issue, and i think it's worthwhile even ...
11 years, 7 months ago (2012-08-28 08:27:31 UTC) #3
minux1
I think you also need to do the same for the runtime package (possibly in ...
11 years, 7 months ago (2012-08-28 17:04:59 UTC) #4
brainman
Hello golang-dev@googlegroups.com, bsiegert@gmail.com, minux.ma@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 7 months ago (2012-08-29 02:33:09 UTC) #5
brainman
On 2012/08/28 08:24:40, bsiegert wrote: > I am wondering if the code in consoleWrite can ...
11 years, 7 months ago (2012-08-29 02:33:16 UTC) #6
minux1
On Wed, Aug 29, 2012 at 10:33 AM, <alex.brainman@gmail.com> wrote: > On 2012/08/28 17:04:59, minux ...
11 years, 7 months ago (2012-08-29 03:46:13 UTC) #7
brainman
On 2012/08/29 03:46:13, minux wrote: > (http://code.google.com/p/go/issues/detail?id=3376#c2) How can you tell it is Go stack ...
11 years, 7 months ago (2012-08-29 04:44:18 UTC) #8
minux1
On Wed, Aug 29, 2012 at 12:44 PM, <alex.brainman@gmail.com> wrote: > On 2012/08/29 03:46:13, minux ...
11 years, 7 months ago (2012-08-29 04:56:37 UTC) #9
brainman
On 2012/08/29 04:56:37, minux wrote: > > > that comment doesn't mention stack trace, but ...
11 years, 7 months ago (2012-08-29 05:09:58 UTC) #10
rsc
LGTM Very happy to see this fixed. http://codereview.appspot.com/6488044/diff/8001/src/pkg/os/file_windows.go File src/pkg/os/file_windows.go (right): http://codereview.appspot.com/6488044/diff/8001/src/pkg/os/file_windows.go#newcode261 src/pkg/os/file_windows.go:261: if len(runes) ...
11 years, 6 months ago (2012-09-11 01:18:22 UTC) #11
brainman
11 years, 6 months ago (2012-09-12 02:04:58 UTC) #12
*** Submitted as http://code.google.com/p/go/source/detail?r=ffe134f40269 ***

os: detect and handle console in File.Write on windows

Fixes issue 3376.

R=golang-dev, bsiegert, minux.ma, rsc
CC=golang-dev
http://codereview.appspot.com/6488044

http://codereview.appspot.com/6488044/diff/8001/src/pkg/os/file_windows.go
File src/pkg/os/file_windows.go (right):

http://codereview.appspot.com/6488044/diff/8001/src/pkg/os/file_windows.go#ne...
src/pkg/os/file_windows.go:261: if len(runes) > 0 {
On 2012/09/11 01:18:22, rsc wrote:
> Do we know that WriteConsole will gracefully handle too much data and do a
short
> write instead of giving an error? ...
> 

I do not know yet. I will submit this one as is and deal with big writes in a
separate CL. I might even come up with a test for it.

I am not very keen on fiddling with io buffers "under the covers", but I see no
better way to fix our problem. Having to do it for "console" io only makes it
easier for me. Oh well.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b