code review 7033046: os: fix Open for empty root directories on windows

os: fix Open for empty root directories on windows

Fixes issue 4601.

[brainman, 2013-01-02 23:48:16]

Hello golang-dev@googlegroups.com,

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

[rsc, 2013-01-04 15:29:44]

LGTM

Yay Windows.

[bradfitz, 2013-01-04 16:05:11]

No new test?
 On Jan 2, 2013 3:48 PM, <alex.brainman@gmail.com> 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:
> os: fix Open for empty root directories on windows
>
> Fixes issue 4601.
>
> Please review this at
https://codereview.appspot.**com/7033046/<https://codereview.appspot.com/7033...
>
> Affected files:
>   M src/pkg/os/file_windows.go
>
>
> Index: src/pkg/os/file_windows.go
> ==============================**==============================**=======
> --- a/src/pkg/os/file_windows.go
> +++ b/src/pkg/os/file_windows.go
> @@ -11,6 +11,7 @@
>         "syscall"
>         "unicode/utf16"
>         "unicode/utf8"
> +       "unsafe"
>  )
>
>  // File represents an open file descriptor.
> @@ -41,12 +42,9 @@
>         return uintptr(file.fd)
>  }
>
> -// NewFile returns a new File with the given file descriptor and name.
> -func NewFile(fd uintptr, name string) *File {
> -       h := syscall.Handle(fd)
> -       if h == syscall.InvalidHandle {
> -               return nil
> -       }
> +// newFile returns a new File with the given file handle and name.
> +// Unlike NewFile, it does not check that h is syscall.InvalidHandle.
> +func newFile(h syscall.Handle, name string) *File {
>         f := &File{&file{fd: h, name: name}}
>         var m uint32
>         if syscall.GetConsoleMode(f.fd, &m) == nil {
> @@ -56,11 +54,21 @@
>         return f
>  }
>
> +// NewFile returns a new File with the given file descriptor and name.
> +func NewFile(fd uintptr, name string) *File {
> +       h := syscall.Handle(fd)
> +       if h == syscall.InvalidHandle {
> +               return nil
> +       }
> +       return newFile(h, name)
> +}
> +
>  // Auxiliary information if the File describes a directory
>  type dirInfo struct {
>         data     syscall.Win32finddata
>         needdata bool
>         path     string
> +       isempty  bool // set if FindFirstFile returns ERROR_FILE_NOT_FOUND
>  }
>
>  func epipecheck(file *File, e error) {
> @@ -73,7 +81,7 @@
>  func openFile(name string, flag int, perm FileMode) (file *File, err
> error) {
>         r, e := syscall.Open(name, flag|syscall.O_CLOEXEC,
> syscallMode(perm))
>         if e != nil {
> -               return nil, &PathError{"open", name, e}
> +               return nil, e
>         }
>         return NewFile(uintptr(r), name), nil
>  }
> @@ -81,19 +89,37 @@
>  func openDir(name string) (file *File, err error) {
>         maskp, e := syscall.UTF16PtrFromString(**name + `\*`)
>         if e != nil {
> -               return nil, &PathError{"open", name, e}
> +               return nil, e
>         }
>         d := new(dirInfo)
>         r, e := syscall.FindFirstFile(maskp, &d.data)
>         if e != nil {
> -               return nil, &PathError{"open", name, e}
> +               // FindFirstFile returns ERROR_FILE_NOT_FOUND when
> +               // no matching files can be found. Then, if directory
> +               // exists, we should proceed.
> +               if e != syscall.ERROR_FILE_NOT_FOUND {
> +                       return nil, e
> +               }
> +               var fa syscall.Win32FileAttributeData
> +               namep, e := syscall.UTF16PtrFromString(**name)
> +               if e != nil {
> +                       return nil, e
> +               }
> +               e = syscall.GetFileAttributesEx(**namep,
> syscall.GetFileExInfoStandard, (*byte)(unsafe.Pointer(&fa)))
> +               if e != nil {
> +                       return nil, e
> +               }
> +               if fa.FileAttributes&syscall.**FILE_ATTRIBUTE_DIRECTORY
> == 0 {
> +                       return nil, e
> +               }
> +               d.isempty = true
>         }
>         d.path = name
>         if !isAbs(d.path) {
>                 cwd, _ := Getwd()
>                 d.path = cwd + `\` + d.path
>         }
> -       f := NewFile(uintptr(r), name)
> +       f := newFile(r, name)
>         f.dirinfo = d
>         return f, nil
>  }
> @@ -120,7 +146,7 @@
>         if e == nil {
>                 return r, nil
>         }
> -       return nil, e
> +       return nil, &PathError{"open", name, e}
>  }
>
>  // Close closes the File, rendering it unusable for I/O.
> @@ -130,7 +156,14 @@
>  }
>
>  func (file *file) close() error {
> -       if file == nil || file.fd == syscall.InvalidHandle {
> +       if file == nil {
> +               return syscall.EINVAL
> +       }
> +       if file.isdir() && file.dirinfo.isempty {
> +               // "special" empty directories
> +               return nil
> +       }
> +       if file.fd == syscall.InvalidHandle {
>                 return syscall.EINVAL
>         }
>         var e error
> @@ -151,12 +184,15 @@
>  }
>
>  func (file *File) readdir(n int) (fi []FileInfo, err error) {
> -       if file == nil || file.fd == syscall.InvalidHandle {
> +       if file == nil {
>                 return nil, syscall.EINVAL
>         }
>         if !file.isdir() {
>                 return nil, &PathError{"Readdir", file.name,
> syscall.ENOTDIR}
>         }
> +       if !file.dirinfo.isempty && file.fd == syscall.InvalidHandle {
> +               return nil, syscall.EINVAL
> +       }
>         wantAll := n <= 0
>         size := n
>         if wantAll {
> @@ -165,7 +201,7 @@
>         }
>         fi = make([]FileInfo, 0, size) // Empty with room to grow.
>         d := &file.dirinfo.data
> -       for n != 0 {
> +       for n != 0 && !file.dirinfo.isempty {
>                 if file.dirinfo.needdata {
>                         e := syscall.FindNextFile(syscall.**Handle(file.fd),
> d)
>                         if e != nil {
>
>
>

[rsc, 2013-01-04 16:06:29]

It's not like one can just create an empty root directory during a
test, unfortunately.

[kardia, 2013-01-04 17:05:08]

Would this help?

C:\Users\danielt>subst /?
Associates a path with a drive letter.

SUBST [drive1: [drive2:]path]
SUBST drive1: /D

  drive1:        Specifies a virtual drive to which you want to assign a 
path.
  [drive2:]path  Specifies a physical drive and path you want to assign to
                 a virtual drive.
  /D             Deletes a substituted (virtual) drive.

Type SUBST with no parameters to display a list of current virtual drives.


On Friday, January 4, 2013 8:06:27 AM UTC-8, rsc wrote:
>
> It's not like one can just create an empty root directory during a 
> test, unfortunately. 
>

[brainman, 2013-01-04 23:10:58]

On 2013/01/04 17:05:08, kardia wrote:
> Would this help?
> 

I will try that. Thank you.

Alex

[brainman, 2013-01-07 01:20:46]

On 2013/01/04 23:10:58, brainman wrote:
> On 2013/01/04 17:05:08, kardia wrote:
> > Would this help?
> > 
> 
> I will try that. Thank you.
> 

Unfortunately, no cigar. Subst-ed directories behave just like any empty
directory - they are not really "empty", they have "." and ".." entries. So, I
will submit tomorrow as is with no test, unless I hear more objections.

Alex

[rsc, 2013-01-07 01:22:56]

submit away

[brainman, 2013-01-07 01:48:41]

*** Submitted as https://code.google.com/p/go/source/detail?r=f088b33ca75f ***

os: fix Open for empty root directories on windows

Fixes issue 4601.

R=golang-dev, rsc, bradfitz, kardianos
CC=golang-dev
https://codereview.appspot.com/7033046