code review 1715046: Add Pipe windows version.

Add Pipe windows version.

[vcc, 2010-06-30 09:11:01]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com, vcc),

I'd like you to review this change.

[brainman, 2010-07-09 06:21:30]

Thank you for your help.

http://codereview.appspot.com/1715046/diff/2001/5
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/1715046/diff/2001/5#newcode134
src/pkg/syscall/syscall_windows.go:134: //sys	CreatePipe(readhandle *int32,
writehandle *int32, lpsa *SECURITY_ATTRIBUTES, size int32) (ok bool, errno int)
Sorry, I'm picky here, they are all uint32:

//sys CreatePipe(readhandle *uint32, writehandle *uint32, lpsa
*SECURITY_ATTRIBUTES, size uint32) (ok bool, errno int)

http://codereview.appspot.com/1715046/diff/2001/5#newcode135
src/pkg/syscall/syscall_windows.go:135: //sys	SetHandleInformation(object int32,
mask uint32, flag uint32) (ok bool, errno int)
Same:

//sys SetHandleInformation(object uint32, mask uint32, flag uint32) (ok bool, e
rrno int)

http://codereview.appspot.com/1715046/diff/2001/5#newcode136
src/pkg/syscall/syscall_windows.go:136: //sys	GetFileType(filehandle int32) (n
uint32, errno int)
Same:

//sys GetFileType(filehandle uint32) (n uint32, errno int)

http://codereview.appspot.com/1715046/diff/2001/5#newcode258
src/pkg/syscall/syscall_windows.go:258: if ft == FILE_TYPE_PIPE {
Do not read here, do what unix lseek does, just fail:
return 0, ESPIPE

http://codereview.appspot.com/1715046/diff/2001/5#newcode618
src/pkg/syscall/syscall_windows.go:618: _, errno = CreatePipe(&r,&w,sd,0)
I suggest:

_, errno = CreatePipe(&r,&w,nil,0)

for now. We have quite a few functions that use nil for SECURITY_ATTRIBUTES. It
is simpler for now. We'll change it when we need to.

http://codereview.appspot.com/1715046/diff/2001/5#newcode619
src/pkg/syscall/syscall_windows.go:619: p[0] = int(r)
Don't need to copy results if CreatePipe failed. So if failed, just return.

http://codereview.appspot.com/1715046/diff/2001/7
File src/pkg/syscall/ztypes_windows_386.go (right):

http://codereview.appspot.com/1715046/diff/2001/7#newcode271
src/pkg/syscall/ztypes_windows_386.go:271: InheritHandle uint8
It is:

InheritHandle uint32.

[vcc, 2010-07-10 03:40:29]

http://codereview.appspot.com/1715046/diff/2001/5
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/1715046/diff/2001/5#newcode134
src/pkg/syscall/syscall_windows.go:134: //sys	CreatePipe(readhandle *int32,
writehandle *int32, lpsa *SECURITY_ATTRIBUTES, size int32) (ok bool, errno int)
On 2010/07/09 06:21:30, brainman wrote:
> Sorry, I'm picky here, they are all uint32:
> 
> //sys CreatePipe(readhandle *uint32, writehandle *uint32, lpsa
> *SECURITY_ATTRIBUTES, size uint32) (ok bool, errno int)

Done.

http://codereview.appspot.com/1715046/diff/2001/5#newcode135
src/pkg/syscall/syscall_windows.go:135: //sys	SetHandleInformation(object int32,
mask uint32, flag uint32) (ok bool, errno int)
On 2010/07/09 06:21:30, brainman wrote:
> Same:
> 
> //sys SetHandleInformation(object uint32, mask uint32, flag uint32) (ok bool,
e
> rrno int)

Done.

http://codereview.appspot.com/1715046/diff/2001/5#newcode136
src/pkg/syscall/syscall_windows.go:136: //sys	GetFileType(filehandle int32) (n
uint32, errno int)
On 2010/07/09 06:21:30, brainman wrote:
> Same:
> 
> //sys GetFileType(filehandle uint32) (n uint32, errno int)

Done.

http://codereview.appspot.com/1715046/diff/2001/5#newcode258
src/pkg/syscall/syscall_windows.go:258: if ft == FILE_TYPE_PIPE {
On 2010/07/09 06:21:30, brainman wrote:
> Do not read here, do what unix lseek does, just fail:
> return 0, ESPIPE
> 

This can make archive/tar reader_test PASS on windows. If just fail, we need
modify archive/tar reader_test (use file instead of pipe).

http://codereview.appspot.com/1715046/diff/2001/5#newcode618
src/pkg/syscall/syscall_windows.go:618: _, errno = CreatePipe(&r,&w,sd,0)
On 2010/07/09 06:21:30, brainman wrote:
> I suggest:
> 
> _, errno = CreatePipe(&r,&w,nil,0)
> 
> for now. We have quite a few functions that use nil for SECURITY_ATTRIBUTES.
It
> is simpler for now. We'll change it when we need to.

Done.

http://codereview.appspot.com/1715046/diff/2001/7
File src/pkg/syscall/ztypes_windows_386.go (right):

http://codereview.appspot.com/1715046/diff/2001/7#newcode271
src/pkg/syscall/ztypes_windows_386.go:271: InheritHandle uint8
On 2010/07/09 06:21:30, brainman wrote:
> It is:
> 
> InheritHandle uint32.

Done.

[vcc, 2010-07-10 03:49:11]

Hello golang-dev@googlegroups.com, brainman (cc: golang-dev@googlegroups.com,
vcc),

Please take another look.

[brainman, 2010-07-12 02:08:40]

Sorry, some more changes.

And, please, update your repo (hg pull -u), your changes do not apply against
tip cleanly.

Thank you.

http://codereview.appspot.com/1715046/diff/9001/10001
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/1715046/diff/9001/10001#newcode134
src/pkg/syscall/syscall_windows.go:134: //sys	CreatePipe(readhandle *uint32,
writehandle *uint32, lpsa *SECURITY_ATTRIBUTES, size uint32) (ok bool, errno
int)
Please, replace:

lpsa *SECURITY_ATTRIBUTES

with

lpsa *byte

for now.

http://codereview.appspot.com/1715046/diff/9001/10001#newcode135
src/pkg/syscall/syscall_windows.go:135: //sys	SetHandleInformation(object
uint32, mask uint32, flag uint32) (ok bool, errno int)
Do not need this function yet, please remove it for now.

http://codereview.appspot.com/1715046/diff/9001/10001#newcode258
src/pkg/syscall/syscall_windows.go:258: if ft == FILE_TYPE_PIPE {
> ... If just fail, we need
> modify archive/tar reader_test (use file instead of pipe).
> 

This code:

...
ft, _ := GetFileType(uint32(fd))
if ft == FILE_TYPE_PIPE {
	return 0, ESPIPE
}
...

PASSes archive/tar reader_test on windows for me. Am I missing something?

http://codereview.appspot.com/1715046/diff/9001/10001#newcode610
src/pkg/syscall/syscall_windows.go:610: func Pipe(p []int) (errno int) {
func Pipe(p []int) (errno int) {
	if len(p) != 2 {
		return EINVAL
	}
	var r, w uint32
	if ok, errno := CreatePipe(&r, &w, nil, 0); !ok {
		return errno
	}
	p[0] = int(r)
	p[1] = int(w)
	return 0
}

http://codereview.appspot.com/1715046/diff/9001/10001#newcode610
src/pkg/syscall/syscall_windows.go:610: func Pipe(p []int) (errno int) {
Also, please move Pipe() up next to other "implemented" functions. For example,
put it after Sleep() or something.

http://codereview.appspot.com/1715046/diff/9001/10003
File src/pkg/syscall/ztypes_windows_386.go (right):

http://codereview.appspot.com/1715046/diff/9001/10003#newcode268
src/pkg/syscall/ztypes_windows_386.go:268: type SECURITY_ATTRIBUTES struct {
Do not need SECURITY_ATTRIBUTES for now. Please, delete it.

[vcc, 2010-07-12 14:34:07]

Please take another look.

http://codereview.appspot.com/1715046/diff/9001/10001
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/1715046/diff/9001/10001#newcode134
src/pkg/syscall/syscall_windows.go:134: //sys	CreatePipe(readhandle *uint32,
writehandle *uint32, lpsa *SECURITY_ATTRIBUTES, size uint32) (ok bool, errno
int)
On 2010/07/12 02:08:40, brainman wrote:
> Please, replace:
> 
> lpsa *SECURITY_ATTRIBUTES
> 
> with
> 
> lpsa *byte
> 
> for now.
> 

Done.

http://codereview.appspot.com/1715046/diff/9001/10001#newcode135
src/pkg/syscall/syscall_windows.go:135: //sys	SetHandleInformation(object
uint32, mask uint32, flag uint32) (ok bool, errno int)
On 2010/07/12 02:08:40, brainman wrote:
> Do not need this function yet, please remove it for now.

Done.

http://codereview.appspot.com/1715046/diff/9001/10001#newcode258
src/pkg/syscall/syscall_windows.go:258: if ft == FILE_TYPE_PIPE {
On 2010/07/12 02:08:40, brainman wrote:
> > ... If just fail, we need
> > modify archive/tar reader_test (use file instead of pipe).
> > 
> 
> This code:
> 
> ...
> ft, _ := GetFileType(uint32(fd))
> if ft == FILE_TYPE_PIPE {
> 	return 0, ESPIPE
> }
> ...
> 
> PASSes archive/tar reader_test on windows for me. Am I missing something?
> 

Done.

http://codereview.appspot.com/1715046/diff/9001/10001#newcode610
src/pkg/syscall/syscall_windows.go:610: func Pipe(p []int) (errno int) {
On 2010/07/12 02:08:40, brainman wrote:
> func Pipe(p []int) (errno int) {
> 	if len(p) != 2 {
> 		return EINVAL
> 	}
> 	var r, w uint32
> 	if ok, errno := CreatePipe(&r, &w, nil, 0); !ok {
> 		return errno
> 	}
> 	p[0] = int(r)
> 	p[1] = int(w)
> 	return 0
> }
> 

Done.

http://codereview.appspot.com/1715046/diff/9001/10003
File src/pkg/syscall/ztypes_windows_386.go (right):

http://codereview.appspot.com/1715046/diff/9001/10003#newcode268
src/pkg/syscall/ztypes_windows_386.go:268: type SECURITY_ATTRIBUTES struct {
On 2010/07/12 02:08:40, brainman wrote:
> Do not need SECURITY_ATTRIBUTES for now. Please, delete it.

Done.

[rsc1, 2010-07-12 21:42:58]

http://codereview.appspot.com/1715046/diff/2001/5
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/1715046/diff/2001/5#newcode258
src/pkg/syscall/syscall_windows.go:258: if ft == FILE_TYPE_PIPE {
On 2010/07/10 03:40:30, vcc wrote:
> On 2010/07/09 06:21:30, brainman wrote:
> > Do not read here, do what unix lseek does, just fail:
> > return 0, ESPIPE
> > 
> 
> This can make archive/tar reader_test PASS on windows. If just fail, we need
> modify archive/tar reader_test (use file instead of pipe).

Then the test needs fixing.
Doing a Read inside Seek is definitely wrong.

http://codereview.appspot.com/1715046/diff/2001/5#newcode618
src/pkg/syscall/syscall_windows.go:618: _, errno = CreatePipe(&r,&w,sd,0)
This hasn't been run through gofmt

[brainman, 2010-07-13 00:02:10]

LGTM

[vcc, 2010-07-15 04:26:50]

On 2010/07/12 21:42:58, rsc1 wrote:
> http://codereview.appspot.com/1715046/diff/2001/5
> File src/pkg/syscall/syscall_windows.go (right):
> 
> http://codereview.appspot.com/1715046/diff/2001/5#newcode258
> src/pkg/syscall/syscall_windows.go:258: if ft == FILE_TYPE_PIPE {
> On 2010/07/10 03:40:30, vcc wrote:
> > On 2010/07/09 06:21:30, brainman wrote:
> > > Do not read here, do what unix lseek does, just fail:
> > > return 0, ESPIPE
> > > 
> > 
> > This can make archive/tar reader_test PASS on windows. If just fail, we need
> > modify archive/tar reader_test (use file instead of pipe).
> 
> Then the test needs fixing.
> Doing a Read inside Seek is definitely wrong.
> 

Yes, I should not work around it.

> http://codereview.appspot.com/1715046/diff/2001/5#newcode618
> src/pkg/syscall/syscall_windows.go:618: _, errno = CreatePipe(&r,&w,sd,0)
> This hasn't been run through gofmt

Done.

[rsc, 2010-07-26 01:08:09]

LGTM

[brainman, 2010-07-26 01:27:13]

Could you please complete the CLA as described at
http://golang.org/doc/contribute.html#copyright ?

[vcc, 2010-07-26 02:50:43]

I completed online use sign electronically.

Wei guangjing

2010/7/26 <alex.brainman@gmail.com>

> Could you please complete the CLA as described at
> http://golang.org/doc/contribute.html#copyright ?
>
>
> http://codereview.appspot.com/1715046/show
>

[brainman, 2010-07-26 03:48:10]

> I completed online use sign electronically.

Thank you. Could you, please, refresh this patch against latest tree.


Alex

[vcc, 2010-07-26 05:25:18]

Please take another look.

[brainman, 2010-07-26 05:55:16]

*** Submitted as http://code.google.com/p/go/source/detail?r=6916b48fbc0d ***

syscall: add windows version of Pipe()

R=brainman, rsc
CC=golang-dev
http://codereview.appspot.com/1715046

Committer: Alex Brainman <alex.brainman@gmail.com>