code review 70480044: os: try openFile before openDir in windows os.OpenFile

os: try openFile before openDir in windows os.OpenFile

Logging calls when running "go install -a std" turns:

  547  openDir succeeded
  3593 openDir failed and fell back to openFile
  3592 openFile succeeded
  1    both failed

into:

  3592 openFile succeeded
  548  openFile failed and fell back
  547  openDir succeeded
  1    both failed

Here the change trades 3593 failed openDir for 548 failed openFile.

Fix issue 7426.

[patrick2, 2014-03-02 14:23:21]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[brainman, 2014-03-04 01:31:22]

LGTM

But, please, remove this paragraph:

According to CreateFile documentation, it operates on directories only if
FILE_FLAG_BACKUP_SEMANTICS flag is used, which never happens in openFile.
openFile will not succeed where openDir previously succeeded.

from CL description. I don't believe it makes any sense - we don't call
CreateFile for directories, only for files. We also don't use
FILE_FLAG_BACKUP_SEMANTICS anywhere. Why it needs to be mentioned in this CL
description?

I will leave this CL for a week to allow others to comment. Then submit.

Thank you.

Alex

[patrick2, 2014-03-04 06:32:31]

On 2014/03/04 01:31:22, brainman wrote:
> LGTM
> 
> But, please, remove this paragraph:
> 
> According to CreateFile documentation, it operates on directories only if
> FILE_FLAG_BACKUP_SEMANTICS flag is used, which never happens in openFile.
> openFile will not succeed where openDir previously succeeded.
> 
> from CL description. I don't believe it makes any sense - we don't call
> CreateFile for directories, only for files. We also don't use
> FILE_FLAG_BACKUP_SEMANTICS anywhere. Why it needs to be mentioned in this CL
> description?

My main concern with this CL was having openFile succeeding where openDir would
have, but returning a handle unsuitable for directory calls. The
FILE_FLAG_BACKUP_SEMANTICS ensures it cannot happen. Now, this was a false issue
as (at least on Win7) FindFirstFile just wraps CreateFile( ...,
FILE_FLAG_BACKUP_SEMANTICS ).

I will remove the paragraph.

[patrick2, 2014-03-04 06:39:10]

Fixed, PTAL.

[brainman, 2014-03-04 06:40:50]

On 2014/03/04 06:39:10, patrick2 wrote:
> Fixed, PTAL.

Still looks good. I will submit it in a week. And thank you for your
explanation.

Alex

[bradfitz, 2014-03-04 17:02:49]

On Mon, Mar 3, 2014 at 10:40 PM, <alex.brainman@gmail.com> wrote:

> On 2014/03/04 06:39:10, patrick2 wrote:
>
>> Fixed, PTAL.
>>
>
> Still looks good. I will submit it in a week. And thank you for your
> explanation.


What's in a week? Could we submit earlier so it has more soak time? (But I
think the fix looks good and the change makes sense....)

[brainman, 2014-03-05 01:20:38]

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

os: try openFile before openDir in windows os.OpenFile

Logging calls when running "go install -a std" turns:

  547  openDir succeeded
  3593 openDir failed and fell back to openFile
  3592 openFile succeeded
  1    both failed

into:

  3592 openFile succeeded
  548  openFile failed and fell back
  547  openDir succeeded
  1    both failed

Here the change trades 3593 failed openDir for 548 failed openFile.

Fix issue 7426.

LGTM=alex.brainman
R=golang-codereviews, alex.brainman, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/70480044

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