code review 6307065: syscall: revert API changes in Windows Win32finddata fix.

syscall: revert API changes in Windows Win32finddata fix.

Preserve old API by using correct struct in system call
and then copying the results, as we did for SetsockoptLinger.

[rsc, 2012-06-08 17:44:04]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2012-06-08 17:46:22]

LGTM

On Fri, Jun 8, 2012 at 10:44 AM, <rsc@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> syscall: revert API changes in Windows Win32finddata fix.
>
> Preserve old API by using correct struct in system call
> and then copying the results, as we did for SetsockoptLinger.
>
> Please review this at
http://codereview.appspot.com/**6307065/<http://codereview.appspot.com/6307065/>
>
> Affected files:
>  M api/next.txt
>  M src/pkg/syscall/syscall_**windows.go
>  M src/pkg/syscall/zsyscall_**windows_386.go
>  M src/pkg/syscall/zsyscall_**windows_amd64.go
>  M src/pkg/syscall/ztypes_**windows.go
>
>
> Index: api/next.txt
> ==============================**==============================**=======
> --- a/api/next.txt
> +++ b/api/next.txt
> @@ -422,38 +422,12 @@
>  pkg syscall (windows-386), const CREATE_NEW_PROCESS_GROUP ideal-int
>  pkg syscall (windows-386), const CTRL_BREAK_EVENT ideal-int
>  pkg syscall (windows-386), const CTRL_C_EVENT ideal-int
> -pkg syscall (windows-386), func FindFirstFile1(*uint16, *Win32finddata1)
> (Handle, error)
> -pkg syscall (windows-386), func FindNextFile1(Handle, *Win32finddata1)
> error
>  pkg syscall (windows-386), func GetCurrentProcessId() uint32
>  pkg syscall (windows-386), func Getsockopt(Handle, int32, int32, *byte,
> *int32) error
>  pkg syscall (windows-386), type SysProcAttr struct, CreationFlags uint32
> -pkg syscall (windows-386), type Win32finddata1 struct
> -pkg syscall (windows-386), type Win32finddata1 struct, AlternateFileName
> [14]uint16
> -pkg syscall (windows-386), type Win32finddata1 struct, CreationTime
> Filetime
> -pkg syscall (windows-386), type Win32finddata1 struct, FileAttributes
> uint32
> -pkg syscall (windows-386), type Win32finddata1 struct, FileName
> [MAX_PATH]uint16
> -pkg syscall (windows-386), type Win32finddata1 struct, FileSizeHigh uint32
> -pkg syscall (windows-386), type Win32finddata1 struct, FileSizeLow uint32
> -pkg syscall (windows-386), type Win32finddata1 struct, LastAccessTime
> Filetime
> -pkg syscall (windows-386), type Win32finddata1 struct, LastWriteTime
> Filetime
> -pkg syscall (windows-386), type Win32finddata1 struct, Reserved0 uint32
> -pkg syscall (windows-386), type Win32finddata1 struct, Reserved1 uint32
>  pkg syscall (windows-amd64), const CREATE_NEW_PROCESS_GROUP ideal-int
>  pkg syscall (windows-amd64), const CTRL_BREAK_EVENT ideal-int
>  pkg syscall (windows-amd64), const CTRL_C_EVENT ideal-int
> -pkg syscall (windows-amd64), func FindFirstFile1(*uint16,
> *Win32finddata1) (Handle, error)
> -pkg syscall (windows-amd64), func FindNextFile1(Handle, *Win32finddata1)
> error
>  pkg syscall (windows-amd64), func GetCurrentProcessId() uint32
>  pkg syscall (windows-amd64), func Getsockopt(Handle, int32, int32, *byte,
> *int32) error
>  pkg syscall (windows-amd64), type SysProcAttr struct, CreationFlags uint32
> -pkg syscall (windows-amd64), type Win32finddata1 struct
> -pkg syscall (windows-amd64), type Win32finddata1 struct,
> AlternateFileName [14]uint16
> -pkg syscall (windows-amd64), type Win32finddata1 struct, CreationTime
> Filetime
> -pkg syscall (windows-amd64), type Win32finddata1 struct, FileAttributes
> uint32
> -pkg syscall (windows-amd64), type Win32finddata1 struct, FileName
> [MAX_PATH]uint16
> -pkg syscall (windows-amd64), type Win32finddata1 struct, FileSizeHigh
> uint32
> -pkg syscall (windows-amd64), type Win32finddata1 struct, FileSizeLow
> uint32
> -pkg syscall (windows-amd64), type Win32finddata1 struct, LastAccessTime
> Filetime
> -pkg syscall (windows-amd64), type Win32finddata1 struct, LastWriteTime
> Filetime
> -pkg syscall (windows-amd64), type Win32finddata1 struct, Reserved0 uint32
> -pkg syscall (windows-amd64), type Win32finddata1 struct, Reserved1 uint32
> Index: src/pkg/syscall/syscall_**windows.go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/syscall_**windows.go
> +++ b/src/pkg/syscall/syscall_**windows.go
> @@ -130,8 +130,8 @@
>  //sys  SetFilePointer(handle Handle, lowoffset int32, highoffsetptr
> *int32, whence uint32) (newlowoffset uint32, err error)
> [failretval==0xffffffff]
>  //sys  CloseHandle(handle Handle) (err error)
>  //sys  GetStdHandle(stdhandle int) (handle Handle, err error)
> [failretval==InvalidHandle]
> -//sys  FindFirstFile1(name *uint16, data *Win32finddata1) (handle Handle,
> err error) [failretval==InvalidHandle] = FindFirstFileW
> -//sys  FindNextFile1(handle Handle, data *Win32finddata1) (err error) =
> FindNextFileW
> +//sys  findFirstFile1(name *uint16, data *Win32finddata1) (handle Handle,
> err error) [failretval==InvalidHandle] = FindFirstFileW
> +//sys  findNextFile1(handle Handle, data *Win32finddata1) (err error) =
> FindNextFileW
>  //sys  FindClose(handle Handle) (err error)
>  //sys  GetFileInformationByHandle(**handle Handle, data
> *ByHandleFileInformation) (err error)
>  //sys  GetCurrentDirectory(buflen uint32, buf *uint16) (n uint32, err
> error) = GetCurrentDirectoryW
> @@ -706,11 +706,29 @@
>  func Getpid() (pid int) { return int(GetCurrentProcessId()) }
>
>  func FindFirstFile(name *uint16, data *Win32finddata) (handle Handle, err
> error) {
> -       return InvalidHandle, errorspkg.New("FindFirstFile is broken, use
> FindFirstFile1 instead")
> +       // NOTE(rsc): The Win32finddata struct is wrong for the system
> call:
> +       // the two paths are each one uint16 short. Use the correct struct,
> +       // a win32finddata1, and then copy the results out.
> +       // There is no loss of expressivity here, because the final
> +       // uint16, if it is used, is supposed to be a NUL, and Go doesn't
> need that.
> +       // For Go 1.1, we might avoid the allocation of win32finddata1 here
> +       // by adding a final Bug [2]uint16 field to the struct and then
> +       // adjusting the fields in the result directly.
> +       var data1 win32finddata1
> +       handle, err = findFirstFile1(name, &data1)
> +       if err == nil {
> +               copyFindData(data, &data1)
> +       }
> +       return
>  }
>
>  func FindNextFile(handle Handle, data *Win32finddata) (err error) {
> -       return errorspkg.New("FindNextFile is broken, use FindNextFile1
> instead")
> +       var data1 win32finddata1
> +       err = findFirstFile1(name, &data1)
> +       if err == nil {
> +               copyFindData(data, &data1)
> +       }
> +       return
>  }
>
>  // TODO(brainman): fix all needed for os
> Index: src/pkg/syscall/zsyscall_**windows_386.go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/zsyscall_**windows_386.go
> +++ b/src/pkg/syscall/zsyscall_**windows_386.go
> @@ -310,7 +310,7 @@
>        return
>  }
>
> -func FindFirstFile1(name *uint16, data *Win32finddata1) (handle Handle,
> err error) {
> +func findFirstFile1(name *uint16, data *Win32finddata1) (handle Handle,
> err error) {
>        r0, _, e1 := Syscall(procFindFirstFileW.**Addr(), 2,
> uintptr(unsafe.Pointer(name)), uintptr(unsafe.Pointer(data)), 0)
>        handle = Handle(r0)
>        if handle == InvalidHandle {
> @@ -323,7 +323,7 @@
>        return
>  }
>
> -func FindNextFile1(handle Handle, data *Win32finddata1) (err error) {
> +func findNextFile1(handle Handle, data *Win32finddata1) (err error) {
>        r1, _, e1 := Syscall(procFindNextFileW.**Addr(), 2,
> uintptr(handle), uintptr(unsafe.Pointer(data)), 0)
>        if int(r1) == 0 {
>                if e1 != 0 {
> Index: src/pkg/syscall/zsyscall_**windows_amd64.go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/zsyscall_**windows_amd64.go
> +++ b/src/pkg/syscall/zsyscall_**windows_amd64.go
> @@ -310,7 +310,7 @@
>        return
>  }
>
> -func FindFirstFile1(name *uint16, data *Win32finddata1) (handle Handle,
> err error) {
> +func findFirstFile1(name *uint16, data *Win32finddata1) (handle Handle,
> err error) {
>        r0, _, e1 := Syscall(procFindFirstFileW.**Addr(), 2,
> uintptr(unsafe.Pointer(name)), uintptr(unsafe.Pointer(data)), 0)
>        handle = Handle(r0)
>        if handle == InvalidHandle {
> @@ -323,7 +323,7 @@
>        return
>  }
>
> -func FindNextFile1(handle Handle, data *Win32finddata1) (err error) {
> +func findNextFile1(handle Handle, data *Win32finddata1) (err error) {
>        r1, _, e1 := Syscall(procFindNextFileW.**Addr(), 2,
> uintptr(handle), uintptr(unsafe.Pointer(data)), 0)
>        if int(r1) == 0 {
>                if e1 != 0 {
> Index: src/pkg/syscall/ztypes_**windows.go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/ztypes_**windows.go
> +++ b/src/pkg/syscall/ztypes_**windows.go
> @@ -338,9 +338,6 @@
>        return ft
>  }
>
> -// Win32finddata is an incorrect struct definition, preserved for
> -// backwards compatibility. Use Win32finddata1 and the
> -// FindFirstFile1 and FindNextFile1 functions instead.
>  type Win32finddata struct {
>        FileAttributes    uint32
>        CreationTime      Filetime
> @@ -354,7 +351,9 @@
>        AlternateFileName [13]uint16
>  }
>
> -type Win32finddata1 struct {
> +// This is the actual system call structure.
> +// Win32finddata is what we committed to in Go 1.
> +type win32finddata1 struct {
>        FileAttributes    uint32
>        CreationTime      Filetime
>        LastAccessTime    Filetime
> @@ -367,6 +366,23 @@
>        AlternateFileName [14]uint16
>  }
>
> +func copyFindData(dst *Win32finddata, src *Win32finddata1) {
> +       dst.FileAttributes = src.FileAttributes
> +       dst.CreationTime = src.CreationTime
> +       dst.LastAccessTime = src.LastAccessTime
> +       dst.LastWriteTime = src.LastWriteTime
> +       dst.FileSizeHigh = src.FileSizeHigh
> +       dst.FileSizeLow = src.FileSizeLow
> +       dst.Reserved0 = src.Reserved0
> +       dst.Reserved1 = src.Reserved1
> +
> +       // The src is 1 element shorter than dst. Zero that last one.
> +       copy(dst.FileName[:], src.FileName[:])
> +       dst.FileName[len(dst.FileName)**-1] = 0
> +       copy(dst.AlternateFileName[:], src.AlternateFileName[:])
> +       src.AlternateFileName[len(src.**AlternateFileName)-1] = 0
> +}
> +
>  type ByHandleFileInformation struct {
>        FileAttributes     uint32
>        CreationTime       Filetime
>
>
>

[rsc, 2012-06-08 17:54:56]

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

syscall: revert API changes in Windows Win32finddata fix.

Preserve old API by using correct struct in system call
and then copying the results, as we did for SetsockoptLinger.

R=golang-dev, bradfitz
CC=golang-dev
http://codereview.appspot.com/6307065

[brainman, 2012-06-09 12:15:31]

Sorry, I am late to the party.

Alex

http://codereview.appspot.com/6307065/diff/7001/src/pkg/syscall/ztypes_window...
File src/pkg/syscall/ztypes_windows.go (right):

http://codereview.appspot.com/6307065/diff/7001/src/pkg/syscall/ztypes_window...
src/pkg/syscall/ztypes_windows.go:381: dst.FileName[len(dst.FileName)-1] = 0
You should not override last char of FileName with 0. There could be real char
of the name there. As per your own comment:

// There is no loss of expressivity here, because the final
// uint16, if it is used, is supposed to be a NUL, and Go doesn't need that.

So, you should leave FileName as is without terminating 0. We use
syscall.UTF16ToString to convert it into string, it does not care if there is no
terminating 0.

http://codereview.appspot.com/6307065/diff/7001/src/pkg/syscall/ztypes_window...
src/pkg/syscall/ztypes_windows.go:383:
src.AlternateFileName[len(dst.AlternateFileName)-1] = 0
This line must be wrong. Why are you changing src? I think you should remove
that line as well.

[rsc, 2012-06-13 19:22:05]

On Sat, Jun 9, 2012 at 8:15 AM,  <alex.brainman@gmail.com> wrote:
> Sorry, I am late to the party.

Not at all; thank you for pointing these out. I will send out a CL,
and it will be included in the release. I might even resurrect my
Windows VM and try to test them.

Russ