code review 6972047: os: provide access to file LastAccessTime and CreationT...

os: provide access to file LastAccessTime and CreationTime on windows

Fixes issue 4569.

[brainman, 2012-12-20 05:19:43]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2012-12-20 19:32:57]

https://codereview.appspot.com/6972047/diff/5001/src/pkg/syscall/syscall_wind...
File src/pkg/syscall/syscall_windows.go (right):

https://codereview.appspot.com/6972047/diff/5001/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:873: // Stat_t is an invented structure on
Windows, but here for
Any reason it needs the _t suffix?  If we're inventing things, could they be
prettier?

https://codereview.appspot.com/6972047/diff/5001/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:880: sync.Mutex
// guards fi?

https://codereview.appspot.com/6972047/diff/5001/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:882: path string // used to retrieve fi when
not supplied by constructor
not mutable; put above the Mutex?

[brainman, 2012-12-21 04:44:16]

https://codereview.appspot.com/6972047/diff/5001/src/pkg/syscall/syscall_wind...
File src/pkg/syscall/syscall_windows.go (right):

https://codereview.appspot.com/6972047/diff/5001/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:873: // Stat_t is an invented structure on
Windows, but here for
Here is my thinking. Russ said "Plenty of systems don't support access time"
(http://code.google.com/p/go/issues/detail?id=3952#c4), but all I know of (linux
and windows) do. So second best to having it right in os.FileInfo is be as much
consistent as we can. Also being os specific, it should go to syscall. I would
like to return something "real", like Stat_t. But unfortunately we are given
syscall.Win32finddata when we enumerate files in a directory;
syscall.ByHandleFileInformation when we have opened file (we have file handle);
and syscall.Win32FileAttributeData when we just have file path. All these are
very similar, but cannot replace each other. So it must be "invented" structure
in syscall, and syscall.Stat_t makes it consistent with other os.

I am even happy to move it into os package, then there will be less exported
moving parts. But that goes against all my logic above.

I am opened to suggestions.

https://codereview.appspot.com/6972047/diff/5001/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:880: sync.Mutex
On 2012/12/20 19:32:57, bradfitz wrote:
> // guards fi?

Done.

https://codereview.appspot.com/6972047/diff/5001/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:882: path string // used to retrieve fi when
not supplied by constructor
On 2012/12/20 19:32:57, bradfitz wrote:
> not mutable; put above the Mutex?

Done.

[brainman, 2012-12-21 04:44:33]

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

Please take another look.

[bradfitz, 2012-12-21 05:13:43]

Leaving for Russ.

One more nitpick comment, though:

https://codereview.appspot.com/6972047/diff/11001/src/pkg/syscall/syscall_win...
File src/pkg/syscall/syscall_windows.go (right):

https://codereview.appspot.com/6972047/diff/11001/src/pkg/syscall/syscall_win...
src/pkg/syscall/syscall_windows.go:881: sync.Mutex            // guards fi
give this a name ("mu") so it's not exported.  it would be an ugly wart in the
public API.

[brainman, 2012-12-21 05:39:34]

https://codereview.appspot.com/6972047/diff/11001/src/pkg/syscall/syscall_win...
File src/pkg/syscall/syscall_windows.go (right):

https://codereview.appspot.com/6972047/diff/11001/src/pkg/syscall/syscall_win...
src/pkg/syscall/syscall_windows.go:881: sync.Mutex            // guards fi
Good catch. Fixed.

[brainman, 2012-12-21 05:39:52]

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

Please take another look.

[brainman, 2013-01-07 02:05:32]

ping

[rsc, 2013-01-07 04:42:34]

I am concerned about the delayed fetch of metadata when using one of these
syscall.Stat_t. In particular, traditionally Stat returns a snapshot of the file
as it exists now. This API returns some information about now and the ability to
fetch other information as it exists when you ask for it later, which is a bit
of an odd mix between the two. 

It looks like the difference between ByHandleFileInformation and
Win32FileAttributeData/Win32finddata is that the latter pair omits
VolumeSerialNumber, NumberOfLinks, FileIndexHigh, and FileIndexLow. 

Since all three get used as FileInfo, my suggestion would be to give up on the
extra fields in ByHandleFileInformation and the simulated Stat_t and use a
Win32FileAttributeData as the official underlying sys type. It is possible to
construct one of those from any of the other forms, and that seems like a better
name for an API than Win32finddata.

The alternative would be to change the code that currently gets a
Win32FileAttributeData or a Win32finddata to do an extra
Open+GetFileInformationByHandle+Close for each FileInfo. That seems too
expensive.

[brainman, 2013-01-09 03:58:10]

On 2013/01/07 04:42:34, rsc wrote:
> I am concerned about the delayed fetch of metadata when using one of these
> syscall.Stat_t. In particular, traditionally Stat returns a snapshot of the
file
> as it exists now. This API returns some information about now and the ability
to
> fetch other information as it exists when you ask for it later, which is a bit
> of an odd mix between the two. 

I am concerned about "delayed fetch" too, but
- I do not see efficient way of avoiding it;
- Our current implementation behave in exactly same way;
- The only bits that are "delayed fetched" are VolumeSerialNumber, 
FileIndexHigh and FileIndexLow, all used to implement "SameFile" method.
I am, probably, wrong, but I do not consider SameFile is that useful on
windows, so I am OK with it to be wrong sometimes. Mind you, this CL exposes
whole "delayed" ByHandleFileInformation struct now, but I am prepared to change
that. Perhaps, instead of exposing "delayed" ByHandleFileInformation, I could
have a function that allows to compare 2 syscall.Stat_t for "SameFile".

> It looks like the difference between ByHandleFileInformation and
> Win32FileAttributeData/Win32finddata is that the latter pair omits
> VolumeSerialNumber, NumberOfLinks, FileIndexHigh, and FileIndexLow. 
> 
> Since all three get used as FileInfo, my suggestion would be to give up on the
> extra fields in ByHandleFileInformation and the simulated Stat_t and use a
> Win32FileAttributeData as the official underlying sys type. It is possible to
> construct one of those from any of the other forms, and that seems like a
better
> name for an API than Win32finddata.

Sure we could use syscall.Win32FileAttributeData instead of syscall.Stat_t, but
- then we assume that it is ok to convert appropriate fields of
ByHandleFileInformation
and Win32finddata into Win32FileAttributeData (which it, probably, is ok);
- how do you propose to use Win32FileAttributeData to implement "SameFile"?

> The alternative would be to change the code that currently gets a
> Win32FileAttributeData or a Win32finddata to do an extra
> Open+GetFileInformationByHandle+Close for each FileInfo. That seems too
> expensive.

I agree about expensive. I would like to avoid it at all cost.

Thank you for input.

Alex

[rsc, 2013-01-09 19:47:30]

My suggestion is to use Win32FileAttributeData in the sys field, and
then to change os.SameFile to pass the file names to the sameFile
helper. On Windows, the implementation of sameFile can use the
ByHandleInformation calls and ignore the sys.

Russ

[brainman, 2013-01-14 05:38:12]

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

Please take another look.

[bradfitz, 2013-01-15 17:45:20]

https://codereview.appspot.com/6972047/diff/21001/src/pkg/os/types_notwin.go
File src/pkg/os/types_notwin.go (right):

https://codereview.appspot.com/6972047/diff/21001/src/pkg/os/types_notwin.go#...
src/pkg/os/types_notwin.go:27: func samefile(fs1, fs2 *fileStat) bool {
samefile calls sameFile?  gross.

https://codereview.appspot.com/6972047/diff/21001/src/pkg/os/types_windows.go
File src/pkg/os/types_windows.go (right):

https://codereview.appspot.com/6972047/diff/21001/src/pkg/os/types_windows.go...
src/pkg/os/types_windows.go:90: e := s1.loadFileId()
looks like a lot of syscalls for a SameFile operation.  locks + alloc +
CreateFile + GetFileInformationByHandle + CloseHandle, all twice.

If that's the only way, I guess.

[brainman, 2013-01-16 03:09:44]

https://codereview.appspot.com/6972047/diff/21001/src/pkg/os/types_notwin.go
File src/pkg/os/types_notwin.go (right):

https://codereview.appspot.com/6972047/diff/21001/src/pkg/os/types_notwin.go#...
src/pkg/os/types_notwin.go:27: func samefile(fs1, fs2 *fileStat) bool {
I know, I am very dull person. Make me an offer.

https://codereview.appspot.com/6972047/diff/21001/src/pkg/os/types_windows.go
File src/pkg/os/types_windows.go (right):

https://codereview.appspot.com/6972047/diff/21001/src/pkg/os/types_windows.go...
src/pkg/os/types_windows.go:90: e := s1.loadFileId()
It is expensive. But I do not see any better ways. This code is not changing -
it is how it was before this CL. Again, suggestions are welcome.

[rsc, 2013-01-18 22:42:45]

I'm a little lost. Why do we have SameFile calling samefile calling sameFile
instead of just SameFile calling sameFile?

[brainman, 2013-01-18 23:26:30]

unix sameFile needs access to (*fileStat).sys while windows one needs access to
fileStat. samefile does the translation. If not samefile, I would have to make
SameFile os specific.

I am not happy with the name myself. Please, suggest an alternative approach.

Alex

[rsc, 2013-01-18 23:29:00]

How about having just the current SameFile and the existing sameFile but
with different arguments:

func sameFile(f1, f2 *fileStat) bool

The Unix ones can use f1.sys and f2.sys.

Russ

[brainman, 2013-01-18 23:33:43]

On 2013/01/18 23:29:00, rsc wrote:
> How about having just the current SameFile and the existing sameFile but
> with different arguments:
> 
> func sameFile(f1, f2 *fileStat) bool
> 
> The Unix ones can use f1.sys and f2.sys.
> 

That should be fine with me. But current contents of SameFile (about 5 lines)
will have to move into multiple sameFile implementations. Are you PK with that?

Alex

[rsc, 2013-01-22 18:29:05]

I think that we should have (bold is changed):

func SameFile(fi1, fi2 FileInfo) bool {
fs1, ok1 := fi1.(*fileStat)
fs2, ok2 := fi2.(*fileStat)
if !ok1 || !ok2 {
return false
}
return sameFile(*fs1, fs2*)
}

The change is only to pass fs1, fs2 instead of fs1.sys, fs2.sys.

Then a typical non-Windows sameFile (this is the Linux one) would change to:

func sameFile(*fi1, fi2 *fileStat*) bool {
stat1 := *fi1.sys*.(*syscall.Stat_t)
stat2 := *fi2.sys*.(*syscall.Stat_t)
return stat1.Dev == stat2.Dev && stat1.Ino == stat2.Ino
}

On Windows, having access to the file fileStat instead of just the winSys
should suffice to do the necessary stat.

Russ

[brainman, 2013-01-22 21:08:25]

I realized what to do after my last email (brain freezes are common with me). I
will do the changes next week, when I am back home.

Alex

[brainman, 2013-01-30 06:33:02]

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

Please take another look.

[rsc, 2013-01-30 16:03:10]

LGTM

https://codereview.appspot.com/6972047/diff/35001/src/pkg/os/types_windows.go
File src/pkg/os/types_windows.go (right):

https://codereview.appspot.com/6972047/diff/35001/src/pkg/os/types_windows.go...
src/pkg/os/types_windows.go:92: panic(e)
Library routines do not panic under reasonable circumstances. Doing a Stat +
Remove + SameFile will trigger this. Returning false is fine.

[brainman, 2013-01-31 06:18:08]

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

os: provide access to file LastAccessTime and CreationTime on windows

Fixes issue 4569.

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

https://codereview.appspot.com/6972047/diff/35001/src/pkg/os/types_windows.go
File src/pkg/os/types_windows.go (right):

https://codereview.appspot.com/6972047/diff/35001/src/pkg/os/types_windows.go...
src/pkg/os/types_windows.go:92: panic(e)
On 2013/01/30 16:03:10, rsc wrote:
> Library routines do not panic under reasonable circumstances. Doing a Stat +
> Remove + SameFile will trigger this. Returning false is fine.

Done.