code review 770041: syscall: implementing some mingw syscalls required by o...

syscall: implementing some mingw syscalls required by os package

[brainman, 2010-03-26 03:22:03]

Hello golang-dev@googlegroups.com,

I'd like you to review this change.

[rsc1, 2010-04-01 00:45:53]

Looks pretty good.

http://codereview.appspot.com/770041/diff/2001/3001
File src/pkg/os/dir_mingw.go (right):

http://codereview.appspot.com/770041/diff/2001/3001#newcode43
src/pkg/os/dir_mingw.go:43: d.nbuf, errno = syscall.Getdents(file.fd, d.buf)
Seems very unlikely that this code is going to be
correct on Windows.  If this is just a placeholder, I'd
rather see something like

func (file *File) Readdirnames(count int) (names []string, err Error) {
    panic("windows Readdirnames not implemented")
}

http://codereview.appspot.com/770041/diff/2001/3002
File src/pkg/os/file.go (right):

http://codereview.appspot.com/770041/diff/2001/3002#newcode48
src/pkg/os/file.go:48: Stdin  = NewFile(syscall.StdHandleToFD(0), "/dev/stdin")
I don't like needing syscall.StdHandleToFD.  It's just ugly.
Instead, I'd be happy to define

const (
    Stdin = 0
    Stdout = 1
    Stderr = 2
)

in package syscall (on Unix) and then use syscall.Stdin, etc. here.

http://codereview.appspot.com/770041/diff/2001/3003
File src/pkg/os/stat_mingw.go (right):

http://codereview.appspot.com/770041/diff/2001/3003#newcode9
src/pkg/os/stat_mingw.go:9: func isSymlink(stat *syscall.Stat_t) bool {
Another file that seems exceedingly unlikely to be correct.

http://codereview.appspot.com/770041/diff/2001/3004
File src/pkg/os/sys_mingw.go (right):

http://codereview.appspot.com/770041/diff/2001/3004#newcode7
src/pkg/os/sys_mingw.go:7: func Hostname() (name string, err Error) { return
"nacl", nil }
s/"nacl"/"windows"/?

http://codereview.appspot.com/770041/diff/2001/3006
File src/pkg/syscall/syscall_mingw.go (right):

http://codereview.appspot.com/770041/diff/2001/3006#newcode102
src/pkg/syscall/syscall_mingw.go:102: //sys	CreateFile(name *uint16, access
uint32, mode uint32, sa *byte, createmode uint32, attrs uint32, templatefile
int32) (handle int32, errno int) = CreateFileW,-1
the ,-1 here is a bit cryptic.
Maybe use [error=-1] instead?
That will take some tweaking of the script
but then something like GetStdHandle can
be 

//sys GetStdHandle(stdhandle int32) (handle int32, errno int) [error=-1]

Maybe setting the value is overkill anyway: [negerror]  could mean
error signified by "-1" return, and the compare could always use r0 = 0xfffffff
and not need the caller to spell it out.

http://codereview.appspot.com/770041/diff/2001/3006#newcode105
src/pkg/syscall/syscall_mingw.go:105: //sys	SetFilePointer(handle int32,
lowoffset int32, highoffsetptr *int32, whence uint32) (newlowoffset uint32,
errno int) = ,0xffffffff
lowoffset should be uint32 to avoid confusion

http://codereview.appspot.com/770041/diff/2001/3006#newcode106
src/pkg/syscall/syscall_mingw.go:106: //sys	CloseHandle(handle int32) (ok bool,
errno int)
Why return the ok bool if you have errno?

//sys CloseHandle(handle int32) (errno int)

seems like it would work just as well.  Same applies to
ReadFile, WriteFile, FreeLibrary.

http://codereview.appspot.com/770041/diff/2001/3006#newcode107
src/pkg/syscall/syscall_mingw.go:107: //sys	GetStdHandle(stdhandle int32)
(handle int32, errno int) = ,-1
Don't the standard handles have standard values?
Would it be easier to just remember what they are?

http://codereview.appspot.com/770041/diff/2001/3006#newcode171
src/pkg/syscall/syscall_mingw.go:171: // not sure if I should do that
Are you sure the Seek back is necessary?
Reading the docs it looks like ReadFile with a non-nil overlapped
might not change the file pointer.

http://codereview.appspot.com/770041/diff/2001/3008
File src/pkg/syscall/syscall_unix.go (right):

http://codereview.appspot.com/770041/diff/2001/3008#newcode15
src/pkg/syscall/syscall_unix.go:15: return stdhandle
Is this the correct implementation of StdHandleToFD?
If so, maybe just leave the NewFile calls in os alone,
using 0, 1, 2.

http://codereview.appspot.com/770041/diff/2001/3011
File src/pkg/syscall/ztypes_mingw_386.go (right):

http://codereview.appspot.com/770041/diff/2001/3011#newcode29
src/pkg/syscall/ztypes_mingw_386.go:29: O_RDONLY   = 0x00000
// Invented values to support what package os expects.

[brainman, 2010-04-01 05:03:40]

http://codereview.appspot.com/770041/diff/2001/3001
File src/pkg/os/dir_mingw.go (right):

http://codereview.appspot.com/770041/diff/2001/3001#newcode43
src/pkg/os/dir_mingw.go:43: d.nbuf, errno = syscall.Getdents(file.fd, d.buf)
> ...  If this is just a placeholder, ...

It is a placeholder, I just want it to compile.

> ... I'd
> rather see something like
> 
> func (file *File) Readdirnames(count int) (names []string, err Error) {
>     panic("windows Readdirnames not implemented")
> }
> 

Done.

http://codereview.appspot.com/770041/diff/2001/3002
File src/pkg/os/file.go (right):

http://codereview.appspot.com/770041/diff/2001/3002#newcode48
src/pkg/os/file.go:48: Stdin  = NewFile(syscall.StdHandleToFD(0), "/dev/stdin")
On 2010/04/01 00:45:53, rsc1 wrote:
> I don't like needing syscall.StdHandleToFD.  It's just ugly.
> Instead, I'd be happy to define
> 
> const (
>     Stdin = 0
>     Stdout = 1
>     Stderr = 2
> )
> 
> in package syscall (on Unix) and then use syscall.Stdin, etc. here.

I like the way you're thinking <g>.
Done.

http://codereview.appspot.com/770041/diff/2001/3003
File src/pkg/os/stat_mingw.go (right):

http://codereview.appspot.com/770041/diff/2001/3003#newcode9
src/pkg/os/stat_mingw.go:9: func isSymlink(stat *syscall.Stat_t) bool {
On 2010/04/01 00:45:53, rsc1 wrote:
> Another file that seems exceedingly unlikely to be correct.
> 

Done.

http://codereview.appspot.com/770041/diff/2001/3004
File src/pkg/os/sys_mingw.go (right):

http://codereview.appspot.com/770041/diff/2001/3004#newcode7
src/pkg/os/sys_mingw.go:7: func Hostname() (name string, err Error) { return
"nacl", nil }
Done.

http://codereview.appspot.com/770041/diff/2001/3006
File src/pkg/syscall/syscall_mingw.go (right):

http://codereview.appspot.com/770041/diff/2001/3006#newcode102
src/pkg/syscall/syscall_mingw.go:102: //sys	CreateFile(name *uint16, access
uint32, mode uint32, sa *byte, createmode uint32, attrs uint32, templatefile
int32) (handle int32, errno int) = CreateFileW,-1
On 2010/04/01 00:45:53, rsc1 wrote:
> the ,-1 here is a bit cryptic.
> Maybe use [error=-1] instead?

Sounds good, but [failretval=-1]. It is return value of api that gets checked
for failure.

http://codereview.appspot.com/770041/diff/2001/3006#newcode105
src/pkg/syscall/syscall_mingw.go:105: //sys	SetFilePointer(handle int32,
lowoffset int32, highoffsetptr *int32, whence uint32) (newlowoffset uint32,
errno int) = ,0xffffffff
On 2010/04/01 00:45:53, rsc1 wrote:
> lowoffset should be uint32 to avoid confusion
> 

I was, probably still am, confused myself. The spec says
(http://msdn.microsoft.com/en-us/library/ms891933.aspx) it is signed. This api
could be used with small offsets only, you specify highoffsetptr = nil and put
your small offset into lowoffset. Since offsets sometimes are negative,
lowoffset has to be signed.

I tested some edge test cases, and current code seems to work all right:

test_seek(fname, 0, int64(0))                 
test_seek(fname, 0, int64(1))                 
test_seek(fname, 0, int64(0xffffffff / 2 - 1))
test_seek(fname, 0, int64(0xffffffff / 2 + 0))
test_seek(fname, 0, int64(0xffffffff / 2 + 1))
test_seek(fname, 0, int64(0xffffffff - 1))    
test_seek(fname, 0, int64(0xffffffff + 0))    
test_seek(fname, 0, int64(0xffffffff + 1))    
test_seek(fname, 0, int64(0xffffffff + 2))    
test_seek(fname, 2, int64(-2))                
test_seek(fname, 2, int64(-1))                
test_seek(fname, 2, int64(0))

http://codereview.appspot.com/770041/diff/2001/3006#newcode106
src/pkg/syscall/syscall_mingw.go:106: //sys	CloseHandle(handle int32) (ok bool,
errno int)
On 2010/04/01 00:45:53, rsc1 wrote:
> Why return the ok bool if you have errno?
> 
> //sys CloseHandle(handle int32) (errno int)
> 
> seems like it would work just as well.  Same applies to
> ReadFile, WriteFile, FreeLibrary.

All windows api return one value, that needs to be examined to determine if the
call was a success, then if you know that it has failed you call GetLastError()
to see why. Remember the dance we do in zsyscall_mingw_386.go. For these
particular apis return value has no other meaning, but to indicate failure = 0
or success (anything else). If you insist we drop "ok" return value, we would
have to write function body by hand (as opposed to mksyscall_mingw.sh script).

Also, I prefer to have all windows api calls look as per manual. It would
confuse people less.

http://codereview.appspot.com/770041/diff/2001/3006#newcode107
src/pkg/syscall/syscall_mingw.go:107: //sys	GetStdHandle(stdhandle int32)
(handle int32, errno int) = ,-1
On 2010/04/01 00:45:53, rsc1 wrote:
> Don't the standard handles have standard values?
> Would it be easier to just remember what they are?
> 

fmt.Printf("os.Stdin: %d\n", os.Stdin.Fd())  
fmt.Printf("os.Stdout: %d\n", os.Stdout.Fd())
fmt.Printf("os.Stderr: %d\n", os.Stderr.Fd())

prints

os.Stdin: 3
os.Stdout: 7
os.Stderr: 11

I'm sure, they are not constant.

I'm not an expert, but, I suspect, the story could be even more complicated (see
CreateFile windows api, part about consoles). But, I suggest, we leave it the
way it is for now.

http://codereview.appspot.com/770041/diff/2001/3006#newcode171
src/pkg/syscall/syscall_mingw.go:171: // not sure if I should do that
> Are you sure the Seek back is necessary?

Yep. Without Seek(s) magic ReadFile does change file position. I tested it.

> Reading the docs it looks like ReadFile with a non-nil overlapped
> might not change the file pointer.

It is convoluted (and that is the light word), but the way I read it, you have
to use "asynchronous I/O". It means, you have to open file with
FILE_FLAG_OVERLAPPED, you must set HEvent field of overlapped parameter, and
your read would return immediately with ERROR_IO_PENDING and you would have to
wait for io completion and then get result by calling GetOverlappedResult.

I don't do any of that (we will one day, when we get to do socket io). So I
decided to use couple of Seek instead.

I did it because unix pread does preserve file position. I didn't find that
mentioned anywhere in "os" package docs/comments. Is that a requirement? If not,
I'll just drop Seek(s) altogether.

http://codereview.appspot.com/770041/diff/2001/3008
File src/pkg/syscall/syscall_unix.go (right):

http://codereview.appspot.com/770041/diff/2001/3008#newcode15
src/pkg/syscall/syscall_unix.go:15: return stdhandle
On 2010/04/01 00:45:53, rsc1 wrote:
> Is this the correct implementation of StdHandleToFD?
> If so, maybe just leave the NewFile calls in os alone,
> using 0, 1, 2.
> 

Done.

http://codereview.appspot.com/770041/diff/2001/3011
File src/pkg/syscall/ztypes_mingw_386.go (right):

http://codereview.appspot.com/770041/diff/2001/3011#newcode29
src/pkg/syscall/ztypes_mingw_386.go:29: O_RDONLY   = 0x00000
On 2010/04/01 00:45:53, rsc1 wrote:
> // Invented values to support what package os expects.
> 

If you mean to insert this comment, then done.

[brainman, 2010-04-01 05:04:02]

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

Please take another look.

[rsc1, 2010-04-02 07:17:10]

LGTM

thanks

[rsc, 2010-04-02 08:11:22]

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

syscall: implementing some mingw syscalls required by os package

R=rsc
CC=golang-dev
http://codereview.appspot.com/770041

Committer: Russ Cox <rsc@golang.org>