code review 4984050: os: implemented os.Getpid, os.Link and os.Symlink for W...

os: implemented os.Getpid, os.Link, os.Symlink, os.Readlink for Windows

[jp, 2011-09-06 08:35:30]

Hello golang-dev@googlegroups.com (cc: alex.brainman@gmail.com,
golang-dev@googlegroups.com, rsc@golang.org),

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

[jp, 2011-09-06 11:18:31]

added os.Readlink and the difference of os.Stat / os.Lstat

[mattn, 2011-09-06 11:28:40]

AFAIK, minimum spec of go-windows is windows2000.

Some APIs you add does not work on windows2000.

On 2011/09/06 11:18:31, jp wrote:
> added os.Readlink and the difference of os.Stat / os.Lstat

[jp, 2011-09-06 12:24:43]

On 2011/09/06 11:28:40, mattn wrote:

Recently approved CL which uses API (GetFileAttributesEx) which is not available
on Windows 2000: http://codereview.appspot.com/4934049/

> AFAIK, minimum spec of go-windows is windows2000.
> 
> Some APIs you add does not work on windows2000.
> 
> On 2011/09/06 11:18:31, jp wrote:
> > added os.Readlink and the difference of os.Stat / os.Lstat

[mattn, 2011-09-06 12:46:24]

Ah, it seems that msdn dropped windows2k from supported clients of windows 
family.
windows2k have(had?) GetFileAttributesEx(). :)

http://goo.gl/pN4MC

It seems windows2k had CreateHardLink() also.

Sorry that.

[jp, 2011-09-06 13:01:45]

On 2011/09/06 12:46:24, mattn wrote:
> Ah, it seems that msdn dropped windows2k from supported clients of windows 
> family.
> windows2k have(had?) GetFileAttributesEx(). :)
> 
> http://goo.gl/pN4MC
> 
> It seems windows2k had CreateHardLink() also.

Hm. seems there are big differences between Japanese and English MSDN :)

GetFileAttributesEx Function
Minimum supported client: Windows XP
http://msdn.microsoft.com/en-us/library/aa364946(v=vs.85).aspx

CreateHardLink Function
Minimum supported client: Windows XP
http://msdn.microsoft.com/en-us/library/aa363860(v=vs.85).aspx

CreateSymbolicLink Function
Minimum supported client: Windows Vista
http://msdn.microsoft.com/en-us/library/aa363866(v=VS.85).aspx

The could be a problem with the latest.
I will install Windows 2000 and XP and make os.SymLink() to return error code in
case if API is not available.

[jp, 2011-09-06 22:07:52]

Added recover() to catch the panic of GetProcAddress(..., "CreateSymbolicLinkW")

Also, TestEvalSymlinks (src\pkg\path\filepath\path_test.go) passes on Windows 7.
But I kept the test disabled.
I do not know how to enable it only for Windows Vista and Windows 7.
Calling syscall.GetVersion() will make the code not compilable for Linux and
other OSes.
Is there any nice way to check for the Windows version (or, better, for NTFS
version) in the test code ?

[rsc, 2011-09-07 18:10:20]

looks good.  leaving for alex.

http://codereview.appspot.com/4984050/diff/3035/src/pkg/os/file_plan9.go
File src/pkg/os/file_plan9.go (right):

http://codereview.appspot.com/4984050/diff/3035/src/pkg/os/file_plan9.go#newc...
src/pkg/os/file_plan9.go:333: // Readlink reads the contents of a symbolic link:
the destination of
Revert.  There's a Readlink just a few lines above.

http://codereview.appspot.com/4984050/diff/3035/src/pkg/path/filepath/path_te...
File src/pkg/path/filepath/path_test.go (right):

http://codereview.appspot.com/4984050/diff/3035/src/pkg/path/filepath/path_te...
src/pkg/path/filepath/path_test.go:492: symlinksNotSupported := e != nil &&
e.(*os.LinkError).Error.String() == "not supported by windows"
Please make this

symlinkSupported := ...

to avoid the double-negative below.

[jp, 2011-09-08 07:28:45]

PTAL

[mattn, 2011-09-08 07:40:02]

LGTM

On 2011/09/08 07:28:45, jp wrote:
> PTAL

[brainman, 2011-09-13 06:00:12]

Sorry for long delay. But I can't apply your changes anymore:

codereview issue 4984050 is out of date: patch and recent changes conflict
(2a748f320ba5->2302c9faa3ff)

Alex

http://codereview.appspot.com/4984050/diff/2019/src/pkg/syscall/syscall_windo...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4984050/diff/2019/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:429: // GetProcAddress(..,
"CreateSymbolicLinkW") panics on Win2000 and WinXP
I think we need to change our interface to get rid of panic inside of
CreateSymbolicLink. Perhaps we could check if such function exists in the DLL.

http://codereview.appspot.com/4984050/diff/2019/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:566: func Getpid() (pid int) {
I am OK to submit Getpid implementation, if you send it in a separate CL.

[jp, 2011-09-13 07:35:01]

On 2011/09/13 06:00:12, brainman wrote:
> Sorry for long delay. But I can't apply your changes anymore:
> 
> codereview issue 4984050 is out of date: patch and recent changes conflict
> (2a748f320ba5->2302c9faa3ff)
> 

fixed

> Alex
> 
>
http://codereview.appspot.com/4984050/diff/2019/src/pkg/syscall/syscall_windo...
> File src/pkg/syscall/syscall_windows.go (right):
> 
>
http://codereview.appspot.com/4984050/diff/2019/src/pkg/syscall/syscall_windo...
> src/pkg/syscall/syscall_windows.go:429: // GetProcAddress(..,
> "CreateSymbolicLinkW") panics on Win2000 and WinXP
> I think we need to change our interface to get rid of panic inside of
> CreateSymbolicLink. Perhaps we could check if such function exists in the DLL.
> 
PTAL
>
http://codereview.appspot.com/4984050/diff/2019/src/pkg/syscall/syscall_windo...
> src/pkg/syscall/syscall_windows.go:566: func Getpid() (pid int) {
> I am OK to submit Getpid implementation, if you send it in a separate CL.

[brainman, 2011-09-14 04:32:06]

I think you have a few files missing now:

INSTALL FAIL syscall
make[1]: Entering directory `/root/hg/go/src/pkg/syscall'
8g  -p syscall -o _go_.8 str.go syscall.go syscall_386.go syscall_windows.go
syscall_windo
ws_386.go zerrors_windows_386.go zsyscall_windows_386.go zsysnum_windows_386.go
ztypes_win
dows_386.go  exec_windows.go zerrors_windows.go ztypes_windows.go
syscall_windows.go:450: undefined: FILE_FLAG_OPEN_REPARSE_POINT
syscall_windows.go:450: undefined: FILE_FLAG_BACKUP_SEMANTICS
make[1]: *** [_go_.8] Error 1
make[1]: Leaving directory `/root/hg/go/src/pkg/syscall'
make: *** [syscall.install] Error 1
#

http://codereview.appspot.com/4984050/diff/37001/src/pkg/syscall/syscall_wind...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4984050/diff/37001/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:395: func Link(oldpath, newpath string)
(errno int) {
Please, take all these into os package. There is no Link, Symlink or Readlink
Windows API. Now that we have os specific code in os, you might as well put it
there.

http://codereview.appspot.com/4984050/diff/37001/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:436: type ReparseDataBuffer struct {
All types and consts as defined by Microsoft should go into ztypes_windows.go,
so other people can use them if they want to.

[jp, 2011-09-14 07:01:01]

On 2011/09/14 04:32:06, brainman wrote:
> I think you have a few files missing now:
> 
> INSTALL FAIL syscall
> make[1]: Entering directory `/root/hg/go/src/pkg/syscall'
> 8g  -p syscall -o _go_.8 str.go syscall.go syscall_386.go syscall_windows.go
> syscall_windo
> ws_386.go zerrors_windows_386.go zsyscall_windows_386.go
zsysnum_windows_386.go
> ztypes_win
> dows_386.go  exec_windows.go zerrors_windows.go ztypes_windows.go
> syscall_windows.go:450: undefined: FILE_FLAG_OPEN_REPARSE_POINT
> syscall_windows.go:450: undefined: FILE_FLAG_BACKUP_SEMANTICS
> make[1]: *** [_go_.8] Error 1
> make[1]: Leaving directory `/root/hg/go/src/pkg/syscall'
> make: *** [syscall.install] Error 1
> #
> 
fixed

>
http://codereview.appspot.com/4984050/diff/37001/src/pkg/syscall/syscall_wind...
> File src/pkg/syscall/syscall_windows.go (right):
> 
>
http://codereview.appspot.com/4984050/diff/37001/src/pkg/syscall/syscall_wind...
> src/pkg/syscall/syscall_windows.go:395: func Link(oldpath, newpath string)
> (errno int) {
> Please, take all these into os package. There is no Link, Symlink or Readlink
> Windows API. Now that we have os specific code in os, you might as well put it
> there.

Sounds logical. Moved.
Seems that syscall.Ftruncate and syscall.Chmod should me moved to os as well.
and maybe syscall.Open.
 
>
http://codereview.appspot.com/4984050/diff/37001/src/pkg/syscall/syscall_wind...
> src/pkg/syscall/syscall_windows.go:436: type ReparseDataBuffer struct {
> All types and consts as defined by Microsoft should go into ztypes_windows.go,
> so other people can use them if they want to.

ok, PTAL

[brainman, 2011-09-15 23:15:26]

Everything builds now, but some tests fail.

In path/filepath:

--- FAIL: filepath_test.TestEvalSymlinks (0.03 seconds)
        symlink ..\..\ test/dir/link3: A required privilege is not held by the
client.
--- FAIL: filepath_test.TestAbs (0.02 seconds)
        ../AUTHORS: GetFileAttributesEx ../AUTHORS: The system cannot find the
file specified.
        Abs("../AUTHORS")="G:\\src\\pkg\\path\\AUTHORS", not the same file
        pkg/../../AUTHORS: GetFileAttributesEx pkg/../../AUTHORS: The system
cannot find the file specified.
        Abs("pkg/../../AUTHORS")="G:\\src\\pkg\\path\\AUTHORS", not the same
file
        Make.pkg: GetFileAttributesEx Make.pkg: The system cannot find the file
specified.
        Abs("Make.pkg")="G:\\src\\pkg\\path\\filepath\\Make.pkg", not the same
file
        pkg/Makefile: GetFileAttributesEx pkg/Makefile: The system cannot find
the path specified.
        Abs("pkg/Makefile")="G:\\src\\pkg\\path\\filepath\\pkg\\Makefile", not
the same file
        Abs("/src/Make.pkg")="G:\\src\\pkg\\path\\filepath\\src\\Make.pkg", not
the same file
       
Abs("/src/../src/Make.pkg")="G:\\src\\pkg\\path\\filepath\\src\\Make.pkg", not
the same file

I also enabled some tests in os:

--- FAIL: os_test.TestSymLink (0.05 seconds)
        symlink "symlinktestto", "symlinktestfrom" failed: symlink symlinktestto
symlinktestfrom: A required privilege is not held by the client.
--- FAIL: os_test.TestMkdirAllWithSymlink (0.03 seconds)
        Symlink "dir", "_test/link": symlink dir _test/link: A required
privilege is not held by the client.
--- FAIL: os_test.TestLongSymlink (0.00 seconds)
        symlink "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcde
f0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcde
f0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcde
f0123456789abcdef", "longsymlinktestfrom" failed: symlink 0123456789abcdef012345
6789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef012345
6789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef012345
6789abcdef0123456789abcdef0123456789abcdef0123456789abcdef longsymlinktestfrom:
A required privilege is not held by the client.

I run these tests on Windows 7, so I expected them to work. My go tree resides
on network drive, so perhaps this is a problem.

But I am trying to understand why we should make this change. The change is far
from trivial. If you use this code, that is understandable. Do you? Another
reason would be to be compatible with other OSes, but this change is not going
to work for most Windows users we have here (only Windows Vista +). Also, I do
not know who and why would use links on Windows. Why should we include this
change?

Alex

[jp, 2011-09-15 23:59:31]

On 2011/09/15 23:15:26, brainman wrote:

Failed TestAbs seems not relevant to this change.

Creating symlinks requires privileges which are by default enabled only for
Administrators. Hardlinks can be created by any user.

> Everything builds now, but some tests fail.
> 
> In path/filepath:
> 
> --- FAIL: filepath_test.TestEvalSymlinks (0.03 seconds)
>         symlink ..\..\ test/dir/link3: A required privilege is not held by the
> client.
> --- FAIL: filepath_test.TestAbs (0.02 seconds)
>         ../AUTHORS: GetFileAttributesEx ../AUTHORS: The system cannot find the
> file specified.
>         Abs("../AUTHORS")="G:\\src\\pkg\\path\\AUTHORS", not the same file
>         pkg/../../AUTHORS: GetFileAttributesEx pkg/../../AUTHORS: The system
> cannot find the file specified.
>         Abs("pkg/../../AUTHORS")="G:\\src\\pkg\\path\\AUTHORS", not the same
> file
>         Make.pkg: GetFileAttributesEx Make.pkg: The system cannot find the
file
> specified.
>         Abs("Make.pkg")="G:\\src\\pkg\\path\\filepath\\Make.pkg", not the same
> file
>         pkg/Makefile: GetFileAttributesEx pkg/Makefile: The system cannot find
> the path specified.
>         Abs("pkg/Makefile")="G:\\src\\pkg\\path\\filepath\\pkg\\Makefile", not
> the same file
>         Abs("/src/Make.pkg")="G:\\src\\pkg\\path\\filepath\\src\\Make.pkg",
not
> the same file
>        
> Abs("/src/../src/Make.pkg")="G:\\src\\pkg\\path\\filepath\\src\\Make.pkg", not
> the same file
> 
> I also enabled some tests in os:
> 
> --- FAIL: os_test.TestSymLink (0.05 seconds)
>         symlink "symlinktestto", "symlinktestfrom" failed: symlink
symlinktestto
> symlinktestfrom: A required privilege is not held by the client.
> --- FAIL: os_test.TestMkdirAllWithSymlink (0.03 seconds)
>         Symlink "dir", "_test/link": symlink dir _test/link: A required
> privilege is not held by the client.
> --- FAIL: os_test.TestLongSymlink (0.00 seconds)
>         symlink
"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcde
>
f0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcde
>
f0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcde
> f0123456789abcdef", "longsymlinktestfrom" failed: symlink
0123456789abcdef012345
>
6789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef012345
>
6789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef012345
> 6789abcdef0123456789abcdef0123456789abcdef0123456789abcdef
longsymlinktestfrom:
> A required privilege is not held by the client.
> 
> I run these tests on Windows 7, so I expected them to work. My go tree resides
> on network drive, so perhaps this is a problem.
> 
> But I am trying to understand why we should make this change. The change is
far
> from trivial. If you use this code, that is understandable. Do you? Another
> reason would be to be compatible with other OSes, but this change is not going
> to work for most Windows users we have here (only Windows Vista +). Also, I do
> not know who and why would use links on Windows. Why should we include this
> change?

I needed to run a program which was written for Linux.
It was easier to add those functions and share the result.
Reject the change if nobody else needs it.

[brainman, 2011-09-16 00:21:41]

I do not like these changes. There are too many buts, for no real benefit that I
can see. Sorry. Leaving for others to decide, I don't like my opinion influence
other people.

As I have mentioned earlier, happy to accept Getpid implementation. If you
submit it as a separate change. Small changes are always easier to look at and
test.

Alex

http://codereview.appspot.com/4984050/diff/43001/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

http://codereview.appspot.com/4984050/diff/43001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:115: // avoid "C:."
Are you saying that Clean(`c:.`) is `c:\`?

[jp, 2011-09-16 00:24:38]

On 2011/09/16 00:21:41, brainman wrote:
> I do not like these changes. There are too many buts, for no real benefit that
I
> can see. Sorry. Leaving for others to decide, I don't like my opinion
influence
> other people.
> 
> As I have mentioned earlier, happy to accept Getpid implementation. If you
> submit it as a separate change. Small changes are always easier to look at and
> test.
> 
> Alex
> 
> http://codereview.appspot.com/4984050/diff/43001/src/pkg/path/filepath/path.go
> File src/pkg/path/filepath/path.go (right):
> 
>
http://codereview.appspot.com/4984050/diff/43001/src/pkg/path/filepath/path.g...
> src/pkg/path/filepath/path.go:115: // avoid "C:."
> Are you saying that Clean(`c:.`) is `c:\`?

No, "c:." appeared as a result of Clean()

[jp, 2011-09-16 00:27:52]

On 2011/09/16 00:21:41, brainman wrote:
> I do not like these changes. There are too many buts, for no real benefit that
I
> can see. Sorry. Leaving for others to decide, I don't like my opinion
influence
> other people.
> 
> As I have mentioned earlier, happy to accept Getpid implementation. If you
> submit it as a separate change. Small changes are always easier to look at and
> test.
> 
> Alex
> 
> http://codereview.appspot.com/4984050/diff/43001/src/pkg/path/filepath/path.go
> File src/pkg/path/filepath/path.go (right):
> 
>
http://codereview.appspot.com/4984050/diff/43001/src/pkg/path/filepath/path.g...
> src/pkg/path/filepath/path.go:115: // avoid "C:."
> Are you saying that Clean(`c:.`) is `c:\`?

Clean("C:") is "C:."

  println(filepath.Clean("\\\\q\\c$"))
  println(filepath.Clean("\\\\q\\c$\\"))
  println(filepath.Clean("C:"))
  println(filepath.Clean("C:\\"))

\\q\c$
\\q\c$\
C:.
C:\

[jp, 2011-09-16 00:34:13]

On 2011/09/16 00:27:52, jp wrote:
> On 2011/09/16 00:21:41, brainman wrote:
> > I do not like these changes. There are too many buts, for no real benefit
that
> I
> > can see. Sorry. Leaving for others to decide, I don't like my opinion
> influence
> > other people.
> > 
> > As I have mentioned earlier, happy to accept Getpid implementation. If you
> > submit it as a separate change. Small changes are always easier to look at
and
> > test.
> > 
> > Alex
> > 
> >
http://codereview.appspot.com/4984050/diff/43001/src/pkg/path/filepath/path.go
> > File src/pkg/path/filepath/path.go (right):
> > 
> >
>
http://codereview.appspot.com/4984050/diff/43001/src/pkg/path/filepath/path.g...
> > src/pkg/path/filepath/path.go:115: // avoid "C:."
> > Are you saying that Clean(`c:.`) is `c:\`?
> 
> Clean("C:") is "C:."
> 
>   println(filepath.Clean("\\\\q\\c$"))
>   println(filepath.Clean("\\\\q\\c$\\"))
>   println(filepath.Clean("C:"))
>   println(filepath.Clean("C:\\"))
> 
> \\q\c$
> \\q\c$\
> C:.
> C:\

And yes, println(filepath.Clean("C:.")) results in "C:\" which is not correct,
should be C:
My patch does not fix that.
C:. and C: point to the current directory on C, while C:\ is root directory on
C.

[brainman, 2011-09-16 00:35:54]

On 2011/09/16 00:34:13, jp wrote:
> > 
> >   println(filepath.Clean("\\\\q\\c$"))
> >   println(filepath.Clean("\\\\q\\c$\\"))
> >   println(filepath.Clean("C:"))
> >   println(filepath.Clean("C:\\"))
> > 
> > \\q\c$
> > \\q\c$\
> > C:.
> > C:\

All these match with your change and without.

> 
> And yes, println(filepath.Clean("C:.")) results in "C:\" which is not correct,

Yes.

Alex