code review 86160044: os: Implement symlink support for Windows

os: Implement symlink support for Windows

Fixes Issue 5750.

https://code.google.com/p/go/issues/detail?id=5750

os: Separate windows from posix. Implement windows support.
path/filepath: Use the same implementation as other platforms
syscall: Add/rework new APIs for Windows

[mlf, 2014-04-09 19:49:57]

Hello golang-codereviews@googlegroups.com,

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

[bradfitz, 2014-04-10 15:49:05]

This is too late for Go 1.3 (the new feature code freeze was March 1st...
we're only in bug fixing mode now).

But I've updated https://code.google.com/p/go/issues/detail?id=5750 with a
reference to this and flagged that issue with Go1.4 so we look at this
again.  Feel free to ping this thread again once Go 1.3 comes out.  Also,
be sure you've submitted a CLA as described at
http://golang.org/doc/contribute.html#copyright

Thanks!



On Wed, Apr 9, 2014 at 12:49 PM, <michael.fraenkel@gmail.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> os: Implement symlink support for Windows
>
> os: Separate windows from posix. Implement windows support.
> path/filepath: Use the same implementation as other platforms
> syscall: Add/rework new APIs for Windows
>
> Please review this at https://codereview.appspot.com/86160044/
>
> Affected files (+337, -179 lines):
>   M src/pkg/os/file_posix.go
>   M src/pkg/os/file_unix.go
>   M src/pkg/os/file_windows.go
>   M src/pkg/os/os_test.go
>   M src/pkg/os/path_test.go
>   M src/pkg/os/stat_windows.go
>   M src/pkg/os/types_windows.go
>   M src/pkg/path/filepath/match_test.go
>   M src/pkg/path/filepath/path_test.go
>   M src/pkg/path/filepath/symlink.go
>   R src/pkg/path/filepath/symlink_windows.go
>   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
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[gobot, 2014-04-14 23:48:39]

R=alex.brainman@gmail.com (assigned by bradfitz@golang.org)

[rsc, 2014-04-17 02:52:29]

R=close

Please run 'hg mail' once 1.3 is out to reopen this CL for 1.4.

[brainman, 2014-06-16 04:06:53]

I tried to apply this to current tip, but no luck.

Alex

[mlf, 2014-06-19 05:36:16]

I have managed to fix the changes, however per the rules of testing, I caused an
API compatibility issue.  I did it on purpose.
The previous definitions for Symlink and ReadLink do not work for what is
needed.  I can maintain compatibility by introducing two new version and leave
the old ones which return an ERROR.

What is the best approach to fix this?

[mlf, 2014-06-19 05:36:22]

I have managed to fix the changes, however per the rules of testing, I caused an
API compatibility issue.  I did it on purpose.
The previous definitions for Symlink and ReadLink do not work for what is
needed.  I can maintain compatibility by introducing two new version and leave
the old ones which return an ERROR.

What is the best approach to fix this?

[brainman, 2014-06-20 00:15:37]

I still cannot review your changes, because I cannot apply them to the current
tip:

# hg par
changeset:   20221:5df696558635
tag:         tip
user:        Rui Ueyama <ruiu@google.com>
date:        Thu Jun 19 12:04:59 2014 -0700
summary:     encoding/base64, encoding/base32: speed up Encode

# hg st
# hg clpatch 86160044
abort: codereview issue 86160044 is out of date: patch and recent changes
conflict (1eaf29392348->5df696558635)

Alex

[mlf, 2014-06-20 12:59:10]

Just updated the patch, however my delete of
src/pkg/path/filepath/symlink_windows.go won't go through.
I end up with the following error:

$ hg file 86160044 src/pkg/path/filepath/symlink_windows.go
warning: src/pkg/path/filepath/symlink_windows.go did not match any modified
files
Traceback (most recent call last):
  File "/usr/local/bin/hg", line 38, in <module>
    mercurial.dispatch.run()
  File
"/usr/local/Cellar/mercurial/3.0.1/lib/python2.7/site-packages/mercurial/dispatch.py",
line 28, in run
    sys.exit((dispatch(request(sys.argv[1:])) or 0) & 255)
TypeError: unsupported operand type(s) for &: 'str' and 'int'

[mlf, 2014-06-20 13:03:33]

Figured it out... This should be something that can be applied now.

[brainman, 2014-06-23 07:03:51]

I can use you patch now. But some tests are failing:

...
ok      net/smtp        0.156s
ok      net/textproto   0.062s
ok      net/url 0.062s
--- FAIL: TestSymlink (0.00s)
        os_test.go:516: symlink "symlinktestto", "symlinktestfrom" failed:
symlink symlinktestto symlinktestfrom: not supported by windows
--- FAIL: TestLongSymlink (0.00s)
        os_test.go:573: symlink
"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef",
"longsymlinktestfrom" failed: symlink
0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef
longsymlinktestfrom: not supported by windows
--- FAIL: TestMkdirAllWithSymlink (0.00s)
        path_test.go:190: Symlink
C:\DOCUME~1\brainman\LOCALS~1\Temp\TestMkdirAllWithSymlink-907251403/link:
symlink dir
C:\DOCUME~1\brainman\LOCALS~1\Temp\TestMkdirAllWithSymlink-907251403/link: not
supported by windows
FAIL
FAIL    os      0.875s
ok      os/exec 3.828s
ok      os/signal       1.484s
ok      os/user 0.203s
ok      path    0.062s
--- FAIL: TestGlobSymlink (0.00s)
        match_test.go:192: symlink
C:\DOCUME~1\brainman\LOCALS~1\Temp\globsymlink287943679\test1
C:\DOCUME~1\brainman\LOCALS~1\Temp\globsymlink287943679\link1: not supported by
windows
--- FAIL: TestEvalSymlinks (0.02s)
        path_test.go:761:
Clean("C:\\DOCUME~1\\brainman\\LOCALS~1\\Temp\\evalsymlink912147081\\TEST")="C:\\DOCUME~1\\brainman\\LOCALS~1\\Temp\\evalsymlink912147081\\TEST",
want "C:\\DOCUME~1\\brainman\\LOCALS~1\\Temp\\evalsymlink912147081\\test"
        path_test.go:761:
Clean("C:\\DOCUME~1\\brainman\\LOCALS~1\\Temp\\evalsymlink912147081\\TEST/DIR")="C:\\DOCUME~1\\brainman\\LOCALS~1\\Temp\\evalsymlink912147081\\TEST\\DIR",
want "C:\\DOCUME~1\\brainman\\LOCALS~1\\Temp\\evalsymlink912147081\\test/dir"
FAIL
FAIL    path/filepath   0.766s
ok      reflect 0.125s
ok      regexp  1.594s
ok      regexp/syntax   0.703s
ok      runtime 17.094s
?       runtime/cgo     [no test files]
ok      runtime/debug   0.062s
ok      runtime/pprof   7.531s
?       runtime/race    [no test files]
ok      sort    0.125s
ok      strconv 0.547s
...

Is that expected?

Alex

https://codereview.appspot.com/86160044/diff/80001/src/pkg/path/filepath/path...
File src/pkg/path/filepath/path_test.go (left):

https://codereview.appspot.com/86160044/diff/80001/src/pkg/path/filepath/path...
src/pkg/path/filepath/path_test.go:965: func TestDriveLetterInEvalSymlinks(t
*testing.T) {
What happened to that test?

[mlf, 2014-06-23 20:02:31]

Something has changed along the way so I am investigating the os_test failures. 
Although I see different but similar failures.  I am getting incorrect function
rather than not supported by windows.
As for the path_test, the was a mistake to remove entirely.  I will add it back
but rework it a bit.  The final comparison needs to be done differently since
this code no longer relies on the OS to upper/lowercase the results.

[mlf, 2014-06-24 01:48:10]

Fixed the filepath/path_test.go -- had to add a Tolower to make the comparison
work since there is no normalization done anymore.

As far as getting the other tests to pass, it all comes down to OS, User and
permissions.
I am using Windows 7 with my id is an Administrator.  Creating
symlinks/hardlinks requires either running as an Administrator or granted Create
Symbolic Links policy. If you are using an id which is an Administrator, the
only option you have is to start a Command Prompt with Run as administrator. 
There isn't a way in code to make any of this easier, but depending on how you
invoke the CreateXXX functions, you will get different messages as you and I
have seen now.

[brainman, 2014-06-24 07:21:38]

On 2014/06/24 01:48:10, mlf wrote:
> Fixed the filepath/path_test.go -- had to add a Tolower to make the comparison
> work since there is no normalization done anymore.

I have reservation about that. filepath.EvalSymlink always returns same path
(https://codereview.appspot.com/5712044/#msg12). It happens naturally on unix,
but we had to make it work on windows. It is not documented, but it is
important. Some programs use that. I think cmd/go does. 

> As far as getting the other tests to pass, it all comes down to OS, User and
> permissions.

If it is fickle, then it shouldn't be part of standard library. Selected people
who need that functionality should ware the burden of maintaining that code. Do
you use that functionality (windows symlinks) anywhere in your code?

Also, since build still fails on my system:

ok      net/textproto   0.063s
ok      net/url 0.063s
--- FAIL: TestSymlink (0.00s)
        os_test.go:516: symlink "symlinktestto", "symlinktestfrom" failed:
symlink symlinktestto symlinktestfrom: not supported by windows
--- FAIL: TestLongSymlink (0.00s)
        os_test.go:573: symlink
"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef",
"longsymlinktestfrom" failed: symlink
0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef
longsymlinktestfrom: not supported by windows
--- FAIL: TestMkdirAllWithSymlink (0.00s)
        path_test.go:190: Symlink
C:\DOCUME~1\brainman\LOCALS~1\Temp\TestMkdirAllWithSymlink-376830047/link:
symlink dir
C:\DOCUME~1\brainman\LOCALS~1\Temp\TestMkdirAllWithSymlink-376830047/link: not
supported by windows
FAIL
FAIL    os      0.875s
ok      os/exec 3.984s
ok      os/signal       1.484s
ok      os/user 0.344s
ok      path    0.063s
--- FAIL: TestGlobSymlink (0.00s)
        match_test.go:192: symlink
C:\DOCUME~1\brainman\LOCALS~1\Temp\globsymlink752037027\test1
C:\DOCUME~1\brainman\LOCALS~1\Temp\globsymlink752037027\link1: not supported by
windows
--- FAIL: TestEvalSymlinks (0.02s)
        path_test.go:761:
Clean("C:\\DOCUME~1\\brainman\\LOCALS~1\\Temp\\evalsymlink728446157\\TEST")="C:\\DOCUME~1\\brainman\\LOCALS~1\\Temp\\evalsymlink728446157\\TEST",
want "C:\\DOCUME~1\\brainman\\LOCALS~1\\Temp\\evalsymlink728446157\\test"
        path_test.go:761:
Clean("C:\\DOCUME~1\\brainman\\LOCALS~1\\Temp\\evalsymlink728446157\\TEST/DIR")="C:\\DOCUME~1\\brainman\\LOCALS~1\\Temp\\evalsymlink728446157\\TEST\\DIR",
want "C:\\DOCUME~1\\brainman\\LOCALS~1\\Temp\\evalsymlink728446157\\test/dir"
FAIL
FAIL    path/filepath   0.750s
ok      reflect 0.125s
ok      regexp  1.688s

I don't see that we're making progress here. Sorry.

Alex

[mlf, 2014-06-24 11:15:24]

Lets take these as separate items.

1. EvalSymLink can easily be reverted to a mixture of what is done now to
evaluate the symlink and before to obtain a canonical path.

2. Its not that its fickle per say, Windows has very strong opinions about who
can create a symlink.  Anyone can read them, creating them is the trick.
See what happens if you run mklink.  You should get a similar error. 
Originally, I only wanted to be able to read/resolve symlinks, but that presents
a problem
when trying to create a test case since you need existing symlinks which is
present on some versions of Windows.
Doing a search, it seems this problem came up the last time
(https://codereview.appspot.com/4984050/).  I can do the same approach and track
when symlinks are suported or not.

[brainman, 2014-06-25 05:10:45]

On 2014/06/24 11:15:24, mlf wrote:
> 1. EvalSymLink can easily be reverted to a mixture of what is done now to
> evaluate the symlink and before to obtain a canonical path.

Sounds reasonable.

> 2. Its not that its fickle per say, Windows has very strong opinions about who
> can create a symlink.  Anyone can read them, creating them is the trick.
> See what happens if you run mklink.  You should get a similar error. 
> Originally, I only wanted to be able to read/resolve symlinks, but that
presents
> a problem
> when trying to create a test case since you need existing symlinks which is
> present on some versions of Windows.

I have never used / seen symlinks on windows. I suspect most other Go users are
the same. Do you use symlinks on windows? If we all agree that it is a rare
feature, then why bother wasting our energy on it. Your CL is not trivial,
potentially it introduces new bugs.

> Doing a search, it seems this problem came up the last time
> (https://codereview.appspot.com/4984050/).  I can do the same approach and
track
> when symlinks are suported or not.

I am not sure what you mean by "same approach", but, if you want to run test
only when all functions succeed, then it will work. But we might not be testing
code properly if testing code never gets executed for one reason or another.

Alex

[mlf, 2014-06-25 12:12:32]

While creating symlinks isn't common I agree, reading them however is a
different story.
If I look on my Windows 7 box, its used for backward compatibility with XP

07/14/2009  12:53 AM    <JUNCTION>     Documents and Settings [C:\Users]
....
08/09/2011  11:19 AM    <JUNCTION>     Application Data
[C:\Users\<username>\AppData\Roaming]
08/09/2011  11:19 AM    <JUNCTION>     Cookies
[C:\Users\<username>\AppData\Roaming\Microsoft\Windows\Cookies]
08/09/2011  11:19 AM    <JUNCTION>     Local Settings
[C:\Users\<username>\AppData\Local]
08/09/2011  11:19 AM    <JUNCTION>     My Documents
[C:\Users\<username>\Documents]
08/09/2011  11:19 AM    <JUNCTION>     NetHood
[C:\Users\<username>\AppData\Roaming\Microsoft\Windows\Network Shortcuts]

What I am thinking is to add a test to verify evalsymlink support for some of
these directories, that way the read side is tested even if the creation side
cannot.  I will also add a message implying what is being skipped.

[brainman, 2014-06-26 07:11:14]

On 2014/06/25 12:12:32, mlf wrote:
> ...
> What I am thinking is to add a test to verify evalsymlink support for some of
> these directories, that way the read side is tested even if the creation side
> cannot.  I will also add a message implying what is being skipped.

Lets see it.

Please, leave current windows filepath.EvalSymlink alone, just add symlink
resolution to it.

Thank you for your patience. :-)

Alex

[mlf, 2014-06-28 20:36:31]

I just made all the changes so the tests should pass whether you are on XP or
later and if you have the proper privileges for creating SymLinks.
If you run the tests with -v, you will see what is skipped.

Not happy with the naming/renaming of evalSymlinks but we can change that
easily.
I am also not sure how to handle the API breakage which we can also resolve by
adding more APIs and leaving the old ones alone.

[brainman, 2014-07-02 06:33:42]

Sorry, quite a few comments. It is big change.

But before you address my comments, please, explain something. I have this
program:

package main

import (
	"fmt"
	"log"
	"os"
	"path/filepath"
)

func list(dir string) error {
	d, err := os.Open(dir)
	if err != nil {
		return err
	}
	defer d.Close()

	files, err := d.Readdirnames(-1)
	if err != nil {
		return err
	}

	for _, name := range files {
		path := filepath.Join(dir, name)
		msg, err := filepath.EvalSymlinks(path)
		if err != nil {
			msg = fmt.Sprintf("FAILED: %v", err)
		}
		fmt.Printf("%v: %v\n", name, msg)
	}
	return nil
}

func main() {
	err := list(`c:\users\brainman`)
	if err != nil {
		log.Fatal(err)
	}
}

that outputs this

AppData: C:\Users\brainman\AppData
Application Data: C:\Users\brainman\Application Data
Contacts: C:\Users\brainman\Contacts
Cookies: C:\Users\brainman\Cookies
Desktop: C:\Users\brainman\Desktop
Documents: C:\Users\brainman\Documents
Downloads: C:\Users\brainman\Downloads
Favorites: C:\Users\brainman\Favorites
Links: C:\Users\brainman\Links
Local Settings: C:\Users\brainman\Local Settings
Music: C:\Users\brainman\Music
My Documents: C:\Users\brainman\My Documents
NetHood: C:\Users\brainman\NetHood
NTUSER.DAT: C:\Users\brainman\NTUSER.DAT
ntuser.dat.LOG1: C:\Users\brainman\ntuser.dat.LOG1
ntuser.dat.LOG2: C:\Users\brainman\ntuser.dat.LOG2
NTUSER.DAT{016888bd-6c6f-11de-8d1d-001e0bcde3ec}.TM.blf:
C:\Users\brainman\NTUSER.DAT{016888bd-6c6f-11de-8d1d-001e0bcde3ec}.TM.blf
NTUSER.DAT{016888bd-6c6f-11de-8d1d-001e0bcde3ec}.TMContainer00000000000000000001.regtrans-ms:
C:\Users\brainman\NTUSER.DAT{016888bd-6c6f-11de-8d1d-001e0bcde3ec}.TMContainer00000000000000000001.regtrans-ms
NTUSER.DAT{016888bd-6c6f-11de-8d1d-001e0bcde3ec}.TMContainer00000000000000000002.regtrans-ms:
C:\Users\brainman\NTUSER.DAT{016888bd-6c6f-11de-8d1d-001e0bcde3ec}.TMContainer00000000000000000002.regtrans-ms
ntuser.ini: C:\Users\brainman\ntuser.ini
ntuser.pol: C:\Users\brainman\ntuser.pol
Pictures: C:\Users\brainman\Pictures
PrintHood: C:\Users\brainman\PrintHood
Recent: C:\Users\brainman\Recent
Saved Games: C:\Users\brainman\Saved Games
Searches: C:\Users\brainman\Searches
SendTo: C:\Users\brainman\SendTo
Start Menu: C:\Users\brainman\Start Menu
Templates: C:\Users\brainman\Templates
Videos: C:\Users\brainman\Videos

But after your change, it outputs that

AppData: C:\Users\brainman\AppData
Application Data: FAILED: readlink c:\users\brainman\Application Data: Access is
denied.
Contacts: C:\Users\brainman\Contacts
Cookies: FAILED: readlink c:\users\brainman\Cookies: Access is denied.
Desktop: C:\Users\brainman\Desktop
Documents: C:\Users\brainman\Documents
Downloads: C:\Users\brainman\Downloads
Favorites: C:\Users\brainman\Favorites
Links: C:\Users\brainman\Links
Local Settings: FAILED: readlink c:\users\brainman\Local Settings: Access is
denied.
Music: C:\Users\brainman\Music
My Documents: FAILED: readlink c:\users\brainman\My Documents: Access is denied.
NetHood: FAILED: readlink c:\users\brainman\NetHood: Access is denied.
NTUSER.DAT: C:\Users\brainman\NTUSER.DAT
ntuser.dat.LOG1: C:\Users\brainman\ntuser.dat.LOG1
ntuser.dat.LOG2: C:\Users\brainman\ntuser.dat.LOG2
NTUSER.DAT{016888bd-6c6f-11de-8d1d-001e0bcde3ec}.TM.blf:
C:\Users\brainman\NTUSER.DAT{016888bd-6c6f-11de-8d1d-001e0bcde3ec}.TM.blf
NTUSER.DAT{016888bd-6c6f-11de-8d1d-001e0bcde3ec}.TMContainer00000000000000000001.regtrans-ms:
C:\Users\brainman\NTUSER.DAT{016888bd-6c6f-11de-8d1d-001e0bcde3ec}.TMContainer00000000000000000001.regtrans-ms
NTUSER.DAT{016888bd-6c6f-11de-8d1d-001e0bcde3ec}.TMContainer00000000000000000002.regtrans-ms:
C:\Users\brainman\NTUSER.DAT{016888bd-6c6f-11de-8d1d-001e0bcde3ec}.TMContainer00000000000000000002.regtrans-ms
ntuser.ini: C:\Users\brainman\ntuser.ini
ntuser.pol: C:\Users\brainman\ntuser.pol
Pictures: C:\Users\brainman\Pictures
PrintHood: FAILED: readlink c:\users\brainman\PrintHood: Access is denied.
Recent: FAILED: readlink c:\users\brainman\Recent: Access is denied.
Saved Games: C:\Users\brainman\Saved Games
Searches: C:\Users\brainman\Searches
SendTo: FAILED: readlink c:\users\brainman\SendTo: Access is denied.
Start Menu: FAILED: readlink c:\users\brainman\Start Menu: Access is denied.
Templates: FAILED: readlink c:\users\brainman\Templates: Access is denied.
Videos: C:\Users\brainman\Videos

Why suddenly all these errors? Are these OK? (I really hope these errors are
just bugs in your new code :-)).

Thank you.

Alex

https://codereview.appspot.com/86160044/diff/140001/src/pkg/os/file_windows.go
File src/pkg/os/file_windows.go (right):

https://codereview.appspot.com/86160044/diff/140001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:500: from, err := syscall.UTF16PtrFromString(newname)
I think your variable names "from "and "to" are confusing. I would keep them
close to original names. Maybe "o" and "n" or similar.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:502: return err
Isn't this supposed to be LinkError?

https://codereview.appspot.com/86160044/diff/140001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:506: return err
Isn't this supposed to be LinkError?

https://codereview.appspot.com/86160044/diff/140001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:510: 
Remove blank line.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:531: if !isAbs(oldname) {
Can you, please, explain why this song and dance?

https://codereview.appspot.com/86160044/diff/140001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:536: from, e := syscall.UTF16PtrFromString(destpath)
You can just do 
fi, err := Stat(destpath)
...
fi.IsDir()
here. No?

https://codereview.appspot.com/86160044/diff/140001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:538: return e
Isn't this supposed to be LinkError?

https://codereview.appspot.com/86160044/diff/140001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:557: 
Remove blank line.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/os/os_windows_tes...
File src/pkg/os/os_windows_test.go (right):

https://codereview.appspot.com/86160044/diff/140001/src/pkg/os/os_windows_tes...
src/pkg/os/os_windows_test.go:10: func init() {
Same changes as similar file in path/filepath.

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

https://codereview.appspot.com/86160044/diff/140001/src/pkg/os/types_windows....
src/pkg/os/types_windows.go:34: 
Remove blank line.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/os/types_windows....
src/pkg/os/types_windows.go:46: 
Remove blank line.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/path/filepath/pat...
File src/pkg/path/filepath/path_windows_test.go (right):

https://codereview.appspot.com/86160044/diff/140001/src/pkg/path/filepath/pat...
src/pkg/path/filepath/path_windows_test.go:17: const ERROR_PRIVILEGE_NOT_HELD
syscall.Errno = 1314
Move this into syscall. Together with other errors.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/path/filepath/pat...
src/pkg/path/filepath/path_windows_test.go:20: e := os.Symlink("test1", "test1")
s/e/err/

similar to other code in this file.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/path/filepath/pat...
src/pkg/path/filepath/path_windows_test.go:20: e := os.Symlink("test1", "test1")
Not in the current directory. Use tmp directory.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/path/filepath/pat...
src/pkg/path/filepath/path_windows_test.go:30: case syscall.EWINDOWS,
ERROR_PRIVILEGE_NOT_HELD:
Why is it OK to have "supportsSymlinks = true" for other errors?

https://codereview.appspot.com/86160044/diff/140001/src/pkg/syscall/syscall_w...
File src/pkg/syscall/syscall_windows.go (right):

https://codereview.appspot.com/86160044/diff/140001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:20: var supportLinks bool
This is not used anywhere. Why? Garbage?

https://codereview.appspot.com/86160044/diff/140001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:22: func init() {
As above.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:218: //sys	DeviceIoControl(handle Handle,
ioControlCode uint32, inBuffer *byte, inBufferSize uint32, outBuffer *byte,
outBufferSize uint32, bytesReturned *uint32, overlapped *byte) (err error)
s/overlapped *byte/overlapped *Overlapped/

https://codereview.appspot.com/86160044/diff/140001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:219: //sys	CreateSymbolicLink(from *uint16,
to *uint16, flags uint32) (err error) = CreateSymbolicLinkW
This function actually returns BOOLEAN (not BOOL). See syscall.TranslateName
http://golang.org/src/pkg/syscall/security_windows.go#L32 how to handle that.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:219: //sys	CreateSymbolicLink(from *uint16,
to *uint16, flags uint32) (err error) = CreateSymbolicLinkW
I think your "from "and "to" names are confusing. Just use names as per MS doco:
"symlinkfilename" and  "targetfilename".

https://codereview.appspot.com/86160044/diff/140001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:220: //sys	CreateHardLink(from *uint16, to
*uint16, reserved uintptr) (err error) = CreateHardLinkW
I think your "from "and "to" names are confusing. Just use names as per MS doco:
"filename" and "existingfilename".

https://codereview.appspot.com/86160044/diff/140001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:978: // If there is an error, it will be of
type *LinkError.
Your comment is wrong - error won't be "of type *LinkError". And there is no
"newname" parameter.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:981: if _, e :=
GetProcAddress(Handle(modkernel32.Handle()), "CreateSymbolicLinkW"); e != nil {
s/e/err/
everywhere.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:981: if _, e :=
GetProcAddress(Handle(modkernel32.Handle()), "CreateSymbolicLinkW"); e != nil {
You can use procCreateSymbolicLinkW.Find() here instead.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:1002: // If there is an error, it will be of
type *PathError.
Your comment is wrong again.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:1021: bytesReturned !=
uint32(8)+uint32(rdb.DataLength) {
uint32(8)??? I don't understand what is happening here. Please, just use
SymbolicLinkReparseBuffer fields as prescribed
(http://msdn.microsoft.com/en-us/library/ff552012.aspx) to retrieve your data.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/syscall/ztypes_wi...
File src/pkg/syscall/ztypes_windows.go (right):

https://codereview.appspot.com/86160044/diff/140001/src/pkg/syscall/ztypes_wi...
src/pkg/syscall/ztypes_windows.go:1072: type ReparseDataBuffer struct {
This is all wrong. I suppose you use REPARSE_DATA_BUFFER here. But then you will
be using SymbolicLinkReparseBuffer version of it:

typedef struct _REPARSE_DATA_BUFFER {
  ULONG  ReparseTag;
  USHORT ReparseDataLength;
  USHORT Reserved;
  struct {
    USHORT SubstituteNameOffset;
    USHORT SubstituteNameLength;
    USHORT PrintNameOffset;
    USHORT PrintNameLength;
    ULONG  Flags;
    WCHAR  PathBuffer[1];
 } SymbolicLinkReparseBuffer;
} REPARSE_DATA_BUFFER, *PREPARSE_DATA_BUFFER;

Please, keep the field names as is so it is less confusing when you use them.

Also, please, do not export this type out of syscall package (name it
reparseDataBuffer). Given that it contains union and has variable size, I am
worried we might need to change it later.

[mlf, 2014-07-02 18:33:16]

It looks to be a bug.
The CreateFile is failing.....  I will investigate this first before I look at
your other items.
I will also add a test which does exactly what your testcase is doing since it
seems to catch what the existing testcases do not.

Thanks for the feedback.

[mlf, 2014-07-03 00:11:33]

The more I look at this problem, the more I think the answers you are seeing are
actually correct.

All the files in question have the System and Hidden attributes applied.  If I
use Open or any other API, it too returns access denied.  If I attempt to get a
directory listing using command prompt, I get "File Not Found".  If I use the
"junction data", I see the directory contents.

Did find some documentation to explain these "special" junction points, see
http://pherricoxide.wordpress.com/2009/11/07/windows-vistawindows-7-junctions...
for a better explanation.  But it looks like its working as designed.

[brainman, 2014-07-03 02:55:50]

On 2014/07/03 00:11:33, mlf wrote:
> The more I look at this problem, the more I think the answers you are seeing
are
> actually correct.

I could be wrong , but I disagree. I have no problem with os.Readlink or
os.Open/Readdir failing, but I have problem with filepath.EvalSymlink failing.
filepath.EvalSymlink just provides you with "most correct / canonical" name for
the file. I think you should just ignore os.Readlink call failure in
filepath.EvalSymlink and continue on with the rest of function code.

I even think it is within the spirit of the article you provided: " ...
permissions have been set up so all the Junctions in Vista and Windows 7 can’t
be opened and listed, but the files inside can still be modified if you know the
exact path to them." filepath.EvalSymlink should give you that path. If listing
of that path later fails is irrelevant here.

I tried to simulate our scenario on Linux (not quite the same, but ...). Even
there filepath.EvalSymlink works:

$ pwd
/tmp
$ mkdir a
$ cd a
$ mkdir dir
$ chmod a-xr dir
$ ln -s dir link
$ ls -l
total 4
d-w------- 2 brainman brainman 4096 Jul  3 11:00 dir
lrwxrwxrwx 1 brainman brainman    3 Jul  3 11:02 link -> dir
$ ls -l dir/
ls: cannot open directory dir/: Permission denied
$ cat ../main.go
package main

import (
        "fmt"
        "log"
        "os"
        "path/filepath"
)

func list(dir string) error {
        d, err := os.Open(dir)
        if err != nil {
                return err
        }
        defer d.Close()

        files, err := d.Readdirnames(-1)
        if err != nil {
                return err
        }

        for _, name := range files {
                path := filepath.Join(dir, name)
                msg, err := filepath.EvalSymlinks(path)
                if err != nil {
                        msg = fmt.Sprintf("FAILED: %v", err)
                }
                fmt.Printf("%v: %v\n", name, msg)
        }
        return nil
}

func main() {
        err := list(`/tmp/a`)
        if err != nil {
                log.Fatal(err)
        }
}
$ go run ../main.go
link: /tmp/a/dir
dir: /tmp/a/dir
$

Alex

[mlf, 2014-07-03 03:13:49]

I will go with skipping on any error.

However, you testcase isn't the same.
Notice the symlink did resolve in your example, which it is not doing on
Windows.

I tried a slightly modified version:

$ ls -la
total 4056
drwxr-xr-x   5 fraenkel  wheel      170 Jul  2 23:11 .
drwxrwxrwt  27 root      wheel      918 Jul  2 23:01 ..
lrwxr-xr-x   1 root      wheel        1 Jul  2 23:11 b -> a

$ ./test
b: FAILED: lstat /private/tmp/t/a: no such file or directory

If I modify the code, we would never see something like this which is really
closer to what we are hitting.

[brainman, 2014-07-03 03:19:40]

On 2014/07/03 03:13:49, mlf wrote:
> ...
> I tried a slightly modified version:
> 
> $ ls -la
> total 4056
> drwxr-xr-x   5 fraenkel  wheel      170 Jul  2 23:11 .
> drwxrwxrwt  27 root      wheel      918 Jul  2 23:01 ..
> lrwxr-xr-x   1 root      wheel        1 Jul  2 23:11 b -> a
> 
> $ ./test
> b: FAILED: lstat /private/tmp/t/a: no such file or directory
> 
> If I modify the code, we would never see something like this which is really
> closer to what we are hitting.

Now that I see that, I am not sure that I am right. I will let you decide
whether to ignore error or not here. You seem to know more about links then I
do.

Alex

[mlf, 2014-07-07 04:07:02]

https://codereview.appspot.com/86160044/diff/140001/src/pkg/os/file_windows.go
File src/pkg/os/file_windows.go (right):

https://codereview.appspot.com/86160044/diff/140001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:531: if !isAbs(oldname) {
On 2014/07/02 06:33:40, brainman wrote:
> Can you, please, explain why this song and dance?

We are about to determine if this is a directory.  If I have a relative path, I
need to determine the fully qualified path otherwise its going to be based on
the current working directory which isn't right.

I will see if anything bad happens if its removed.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:536: from, e := syscall.UTF16PtrFromString(destpath)
On 2014/07/02 06:33:40, brainman wrote:
> You can just do 
> fi, err := Stat(destpath)
> ...
> fi.IsDir()
> here. No?

Stat would be wrong.  I wouldn't want the symlink chased. It would work in the
end.
I am trying to decide if this should all move into syscall.Symlink.  I struggle
with what belongs where.

Seems Stat/LStat both return errors when the paths are > 192 characters.  Seems
like a bad bug.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/path/filepath/pat...
File src/pkg/path/filepath/path_windows_test.go (right):

https://codereview.appspot.com/86160044/diff/140001/src/pkg/path/filepath/pat...
src/pkg/path/filepath/path_windows_test.go:30: case syscall.EWINDOWS,
ERROR_PRIVILEGE_NOT_HELD:
On 2014/07/02 06:33:41, brainman wrote:
> Why is it OK to have "supportsSymlinks = true" for other errors?

I am curious what the other errors are.  Right now I only know of 2, not
supported by windows (XP), and not enough privilege (the rest).  I am not
comfortable just blindly saying no symlink.  But we can change this.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/syscall/syscall_w...
File src/pkg/syscall/syscall_windows.go (right):

https://codereview.appspot.com/86160044/diff/140001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:20: var supportLinks bool
On 2014/07/02 06:33:42, brainman wrote:
> This is not used anywhere. Why? Garbage?

Correct.  Originally I was going to look at the Windows version but that wasn't
going to work if it was a privilege problem, so I punted to the test files
instead.

https://codereview.appspot.com/86160044/diff/140001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:1021: bytesReturned !=
uint32(8)+uint32(rdb.DataLength) {
On 2014/07/02 06:33:42, brainman wrote:
> uint32(8)??? I don't understand what is happening here. Please, just use
> SymbolicLinkReparseBuffer fields as prescribed
> (http://msdn.microsoft.com/en-us/library/ff552012.aspx) to retrieve your data.

Will do.

[brainman, 2014-07-07 10:40:03]

On 2014/07/07 04:07:02, mlf wrote:
> 
> I am trying to decide if this should all move into syscall.Symlink.  I
struggle
> with what belongs where.

I always try to limit syscall to real Windows APIs. syscall.Symlink and friends
are old leftovers that could not be removed now.

> 
> I am curious what the other errors are.  Right now I only know of 2, not
> supported by windows (XP), and not enough privilege (the rest).  I am not
> comfortable just blindly saying no symlink.  But we can change this.

No. Lets go with your strategy for now.

Alex

[mlf, 2014-07-07 12:24:44]

I am still tracking down 3 failures

TestLongSymlink -- failing on GetFileAttributesEx
TestMkdirAllWithSymlink  -- this seems to need the fixup in symlink for
abs/relative paths to pass
TestEvalSymlinks -- failing on GetFileAttributesEx

[mlf, 2014-07-08 03:33:20]

All testcases now pass.  Had to make the symlink function behave more unix like.

Did run across a bug on how errors are formatted.  If you get back an Errno = 2
which is a ERR_FILE_NOT_FOUND, the Error() prints "string too long" which really
confused me for quite some time.

[brainman, 2014-07-08 06:36:23]

On 2014/07/08 03:33:20, mlf wrote:
> All testcases now pass. ...

If your changes are ready for review again, you should say PTAL. Should I review
now?

> Did run across a bug on how errors are formatted.  If you get back an Errno =
2
> which is a ERR_FILE_NOT_FOUND, the Error() prints "string too long" which
really
> confused me for quite some time.

I am not clear what your problem is. Would you like to create a new issue here:
https://code.google.com/p/go/issues/entry

Thank you.

Alex

[mlf, 2014-07-08 11:58:20]

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

Please take another look.

[brainman, 2014-07-09 02:44:41]

I could not patch your CL into the tip. So I have used 0f766178488c instead.
Please update your changes to the tip.

It fails on my windows 7:

ok      net/url 0.036s
--- FAIL: TestSymlink (0.03s)
        os_test.go:534: stat "symlinktestfrom" failed: GetFileAttributesEx
mlinktesttosy: The system cannot find the file specified.
--- FAIL: TestLongSymlink (0.00s)
        os_test.go:591: after symlink
"23456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef01"
!=
"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
FAIL
FAIL    os      1.581s
ok      os/exec 5.518s
ok      os/signal       1.766s
ok      os/user 0.133s
ok      path    0.031s
--- FAIL: TestEvalSymlinks (0.01s)
        path_test.go:756: EvalSymlinks("test/link1") error: GetFileAttributesEx
\test..: The system cannot find the file specified.
        path_test.go:756: EvalSymlinks("test/link2") error: GetFileAttributesEx
C:\Users\brainman\AppData\Local\Temp\evalsymlink207883724\test\rdi: The system
cannot find the file specified.
        path_test.go:756: EvalSymlinks("test/link1/dir") error:
GetFileAttributesEx \test..: The system cannot find the file specified.
        path_test.go:756: EvalSymlinks("test/link2/..") error:
GetFileAttributesEx
C:\Users\brainman\AppData\Local\Temp\evalsymlink207883724\test\rdi: The system
cannot find the file specified.
        path_test.go:758:
Clean("C:\\Users\\brainman\\AppData\\Local\\Temp\\evalsymlink207883724\\test/dir/link3")="\\",
want "C:\\Users\\brainman\\AppData\\Local\\Temp\\evalsymlink207883724\\."
        path_test.go:756: EvalSymlinks("test/link2/link3/test") error:
GetFileAttributesEx
C:\Users\brainman\AppData\Local\Temp\evalsymlink207883724\test\rdi: The system
cannot find the file specified.
        path_test.go:756: EvalSymlinks("test/linkabs") error: readlink
C:\Users\brainman\AppData\Local\Temp\evalsymlink207883724\test\linkabs: The
system cannot find the file specified.
FAIL
FAIL    path/filepath   0.743s

I will wait for all tests to PASS before reviewing your code.

Alex

https://codereview.appspot.com/86160044/diff/200001/src/pkg/syscall/syscall_w...
File src/pkg/syscall/syscall_windows.go (right):

https://codereview.appspot.com/86160044/diff/200001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:971: func Symlink(oldpath, newpath string,
linkdir bool) error {
You cannot change this function prototype, because we promised not to change
exported thigs. See http://golang.org/doc/go1compat for details. BTW, if you run
all.bat, you will see it fails at the very end, because of your change.

https://codereview.appspot.com/86160044/diff/200001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:994: func Readlink(name string) (string,
error) {
Same as above.

[mlf, 2014-07-09 03:37:04]

I rebased on tip.

I fixed syscall so that the API test passes.
I verified that all tests pass on Windows 7 as a user and administrator.

PTAL

[minux, 2014-07-09 04:24:30]

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/syscall_w...
File src/pkg/syscall/syscall_windows.go (right):

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:972: // Symlink creates newname as a symbolic
link to oldpath.
what does linkdir do? please document it.
the docs talks about Symlink however the function is Symlink2.

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:996: func Readlink2(name string) (string,
error) {
this is similar enough Readlink, and I think we can just
use Readlink rather than invent a new one.

[brainman, 2014-07-09 05:09:24]

Still fails for me.

ok      net/url 0.039s
--- FAIL: TestSymlink (0.00s)
        os_test.go:534: stat "symlinktestfrom" failed: GetFileAttributesEx
mlinktesttosy: The system cannot find the file specified.
--- FAIL: TestLongSymlink (0.00s)
        os_test.go:591: after symlink
"23456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef01"
!=
"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
FAIL
FAIL    os      1.526s
ok      os/exec 5.945s
ok      os/signal       1.737s
ok      os/user 0.171s
ok      path    0.052s
--- FAIL: TestEvalSymlinks (0.01s)
        path_test.go:756: EvalSymlinks("test/link1") error: GetFileAttributesEx
\test..: The system cannot find the file specified.
        path_test.go:756: EvalSymlinks("test/link2") error: GetFileAttributesEx
C:\Users\brainman\AppData\Local\Temp\evalsymlink627754036\test\rdi: The system
cannot find the file specified.
        path_test.go:756: EvalSymlinks("test/link1/dir") error:
GetFileAttributesEx \test..: The system cannot find the file specified.
        path_test.go:756: EvalSymlinks("test/link2/..") error:
GetFileAttributesEx
C:\Users\brainman\AppData\Local\Temp\evalsymlink627754036\test\rdi: The system
cannot find the file specified.
        path_test.go:758:
Clean("C:\\Users\\brainman\\AppData\\Local\\Temp\\evalsymlink627754036\\test/dir/link3")="\\",
want "C:\\Users\\brainman\\AppData\\Local\\Temp\\evalsymlink627754036\\."
        path_test.go:756: EvalSymlinks("test/link2/link3/test") error:
GetFileAttributesEx
C:\Users\brainman\AppData\Local\Temp\evalsymlink627754036\test\rdi: The system
cannot find the file specified.
        path_test.go:756: EvalSymlinks("test/linkabs") error: readlink
C:\Users\brainman\AppData\Local\Temp\evalsymlink627754036\test\linkabs: The
system cannot find the file specified.
FAIL
FAIL    path/filepath   0.646s

https://codereview.appspot.com/86160044/diff/220001/src/pkg/os/os_test.go
File src/pkg/os/os_test.go (left):

https://codereview.appspot.com/86160044/diff/220001/src/pkg/os/os_test.go#old...
src/pkg/os/os_test.go:45: case "android":
Please don't touch unrelated code in this file.

https://codereview.appspot.com/86160044/diff/220001/src/pkg/os/os_windows_tes...
File src/pkg/os/os_windows_test.go (right):

https://codereview.appspot.com/86160044/diff/220001/src/pkg/os/os_windows_tes...
src/pkg/os/os_windows_test.go:15: err = os.Symlink(tmpfile.Name(),
tmpfile.Name())
Why are you creating a link to itself?

https://codereview.appspot.com/86160044/diff/220001/src/pkg/os/os_windows_tes...
src/pkg/os/os_windows_test.go:16: os.Remove(tmpfile.Name())
Move this up right after successful file creation.

https://codereview.appspot.com/86160044/diff/220001/src/pkg/path/filepath/pat...
File src/pkg/path/filepath/path_windows_test.go (right):

https://codereview.appspot.com/86160044/diff/220001/src/pkg/path/filepath/pat...
src/pkg/path/filepath/path_windows_test.go:23: err = os.Symlink(tmpfile.Name(),
tmpfile.Name())
Same problems as in "os".

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/syscall_w...
File src/pkg/syscall/syscall_windows.go (right):

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:973: func Symlink2(oldpath, newpath string,
linkdir bool) error {
I think you should move all that code into "os" package. Then It won't need to
be exported anywhere. If you do that, you would have to add:

func LoadCreateSymbolicLink() error {
        return procCreateSymbolicLinkW.Find()
}

here, so you can fail gracefully. See similar functions in syscall.

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:996: func Readlink2(name string) (string,
error) {
Like minux said. Do not create new function. Just ignore parameters you don't
use

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/zsyscall_...
File src/pkg/syscall/zsyscall_windows_386.go (right):

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/zsyscall_...
src/pkg/syscall/zsyscall_windows_386.go:1817: func CreateSymbolicLink(from
*uint16, to *uint16, flags uint32) (err error) {
This file is out of sync with your syscall_windows.go changes. Please,
regenerate.

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/zsyscall_...
File src/pkg/syscall/zsyscall_windows_amd64.go (right):

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/zsyscall_...
src/pkg/syscall/zsyscall_windows_amd64.go:1817: func CreateSymbolicLink(from
*uint16, to *uint16, flags uint32) (err error) {
This file is out of sync with your syscall_windows.go changes. Please,
regenerate.

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/ztypes_wi...
File src/pkg/syscall/ztypes_windows.go (right):

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/ztypes_wi...
src/pkg/syscall/ztypes_windows.go:1084: PathBuffer           *uint16
PathBuffer is not a pminter. It is defined as "WCHAR  PathBuffer[1]" on MSDN. So
it just

PathBuffer [1]uint16

[mlf, 2014-07-09 11:27:53]

Looks like I goofed the merge pretty bad.
As far as the testcase failing, its a path I am not testing which is no symlink
support at all which I haven't done but will now add.

Will let you know when I need the next review.

Thanks.

https://codereview.appspot.com/86160044/diff/220001/src/pkg/os/os_test.go
File src/pkg/os/os_test.go (left):

https://codereview.appspot.com/86160044/diff/220001/src/pkg/os/os_test.go#old...
src/pkg/os/os_test.go:45: case "android":
On 2014/07/09 05:09:24, brainman wrote:
> Please don't touch unrelated code in this file.

It was a mistake in the merge.  I will fix this.

https://codereview.appspot.com/86160044/diff/220001/src/pkg/os/os_windows_tes...
File src/pkg/os/os_windows_test.go (right):

https://codereview.appspot.com/86160044/diff/220001/src/pkg/os/os_windows_tes...
src/pkg/os/os_windows_test.go:15: err = os.Symlink(tmpfile.Name(),
tmpfile.Name())
On 2014/07/09 05:09:24, brainman wrote:
> Why are you creating a link to itself?

This is to test if symlinks are supported.  The easiest test is to symlink to
yourself.  Its either going to work or fail in one of the possible errors.

https://codereview.appspot.com/86160044/diff/220001/src/pkg/os/os_windows_tes...
src/pkg/os/os_windows_test.go:16: os.Remove(tmpfile.Name())
On 2014/07/09 05:09:24, brainman wrote:
> Move this up right after successful file creation.

The file needs to exist until after we attempt the symlink which is why its
here.

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/syscall_w...
File src/pkg/syscall/syscall_windows.go (right):

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:972: // Symlink creates newname as a symbolic
link to oldpath.
On 2014/07/09 04:24:29, minux wrote:
> what does linkdir do? please document it.
> the docs talks about Symlink however the function is Symlink2.

On Windows, the "symlink" function needs to know if its a directory or a file,
hence they require an extra parameter.

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:973: func Symlink2(oldpath, newpath string,
linkdir bool) error {
On 2014/07/09 05:09:24, brainman wrote:
> I think you should move all that code into "os" package. Then It won't need to
> be exported anywhere. If you do that, you would have to add:
> 
> func LoadCreateSymbolicLink() error {
>         return procCreateSymbolicLinkW.Find()
> }
> 
> here, so you can fail gracefully. See similar functions in syscall.

Ok.

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:996: func Readlink2(name string) (string,
error) {
On 2014/07/09 04:24:29, minux wrote:
> this is similar enough Readlink, and I think we can just
> use Readlink rather than invent a new one.

Ok.

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/ztypes_wi...
File src/pkg/syscall/ztypes_windows.go (right):

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/ztypes_wi...
src/pkg/syscall/ztypes_windows.go:1084: PathBuffer           *uint16
On 2014/07/09 05:09:24, brainman wrote:
> PathBuffer is not a pminter. It is defined as "WCHAR  PathBuffer[1]" on MSDN.
So
> it just
> 
> PathBuffer [1]uint16
If I do it this way, I assume the code I have to manage this part should remain
the same.  I was originally trying to leave it as a slice but trying to take
advantage of that fact didn't matter.

the [1]uint16 isn't really right since its more like []uint16.  The actual
length isn't known until after the call.

[minux, 2014-07-09 21:24:41]

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/syscall_w...
File src/pkg/syscall/syscall_windows.go (right):

https://codereview.appspot.com/86160044/diff/220001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:972: // Symlink creates newname as a symbolic
link to oldpath.
On 2014/07/09 11:27:53, mlf wrote:
> On 2014/07/09 04:24:29, minux wrote:
> > what does linkdir do? please document it.
> > the docs talks about Symlink however the function is Symlink2.
> On Windows, the "symlink" function needs to know if its a directory or a file,
> hence they require an extra parameter.
ok. i think isDir is more descriptive than linkdir.
anyway, please document what it's for in the document.

[brainman, 2014-07-10 00:06:04]

On 2014/07/09 11:27:53, mlf wrote:
> ...
> As far as the testcase failing, its a path I am not testing which is no
symlink
> support at all which I haven't done but will now add.

The tests fail on my Windows 7. Shouldn't symlinks work there?

> ...  The easiest test is to symlink to
> yourself.  Its either going to work or fail in one of the possible errors.

I am not sure about that (But I don't know much about this, so ...). Maybe just
keep things simple - create a file or directory, then create symlink to it with
completely different name. Keep it simple and do not look for trouble.

>
https://codereview.appspot.com/86160044/diff/220001/src/pkg/os/os_windows_tes...
> src/pkg/os/os_windows_test.go:16: os.Remove(tmpfile.Name())
> On 2014/07/09 05:09:24, brainman wrote:
> > Move this up right after successful file creation.
> 
> The file needs to exist until after we attempt the symlink which is why its
> here.

Sorry, I was meant to say:

Move this up right after successful file creation with "defer" in front of it.
It is safest / easiest to read code, when you defer disposal of any resource
right after it is been acquired.

> On 2014/07/09 05:09:24, brainman wrote:
> > PathBuffer is not a pminter. It is defined as "WCHAR  PathBuffer[1]" on
MSDN.
> So
> > it just
> > 
> > PathBuffer [1]uint16
> If I do it this way, I assume the code I have to manage this part should
remain
> the same. ...

I would replace "&rdb.PathBuffer" for "&rdb.PathBuffer[0]". I think "unsafe"
would allow you to do either, but I think later express situation better - you
want address of the first character of rdb.PathBuffer here.

> ... I was originally trying to leave it as a slice but trying to take
> advantage of that fact didn't matter.

No, you cannot use slice here. Windows does not understand Go slices (they are 3
field structures ...). Windows does understand Go arrays - these are same as C
arrays.

> 
> the [1]uint16 isn't really right since its more like []uint16.  The actual
> length isn't known until after the call.

No, I think "PathBuffer [1]uint16" is exactly what "WCHAR  PathBuffer[1]" is -
it is array of WCHAR of 1 element. The fact that could be more elements followed
is not reflected in "WCHAR  PathBuffer[1]" - it just semantic of that API that
developer must follow. We do that with help of "unsafe".

Alex

[mlf, 2014-07-10 04:07:09]

I believe I addressed everyone's issue.  I also fixed the poor merge on my part.
Hopefully this will also fix your test failures.  I hit a few which were a
subset of yours.

I was debating whether to push part of the Symlink code down into syscall but
currently I left it all in os like requested.

PTAL

[brainman, 2014-07-10 07:23:42]

Tests still fail. Now on my windows-xp:

ok      net/url 0.063s
panic: interface conversion: error is syscall.Errno, not *os.LinkError

goroutine 16 [running]:
runtime.panic(0x553a00, 0x10c35d00)
        C:/go/root/src/pkg/runtime/panic.c:279 +0xe9
os_test.init·1()
        C:/go/root/src/pkg/os/os_windows_test.go:22 +0x191
os_test.init()
        C:/go/root/src/pkg/os/path_test.go:220 +0x591
main.init()
        os/_test/_testmain.go:142 +0x42

goroutine 19 [finalizer wait]:
runtime.park(0x4141e0, 0x64d860, 0x64cbe9)
        C:/go/root/src/pkg/runtime/proc.c:1373 +0x9a
runtime.parkunlock(0x64d860, 0x64cbe9)
        C:/go/root/src/pkg/runtime/proc.c:1389 +0x3f
runfinq()
        C:/go/root/src/pkg/runtime/mgc0.c:2610 +0xc5
runtime.goexit()
        C:/go/root/src/pkg/runtime/proc.c:1449
FAIL    os      0.063s
ok      os/exec 3.703s
ok      os/signal       1.453s
ok      os/user 0.109s
ok      path    0.063s
panic: interface conversion: error is syscall.Errno, not *os.LinkError

goroutine 16 [running]:
runtime.panic(0x51e2c0, 0x10bd5b20)
        C:/go/root/src/pkg/runtime/panic.c:279 +0xe9
path/filepath_test.init·1()
        C:/go/root/src/pkg/path/filepath/path_windows_test.go:29 +0x191
path/filepath_test.init()
        C:/go/root/src/pkg/path/filepath/path_windows_test.go:113 +0x33f
main.init()
        path/filepath/_test/_testmain.go:90 +0x42

goroutine 19 [finalizer wait]:
runtime.park(0x414130, 0x5f4180, 0x5f3509)
        C:/go/root/src/pkg/runtime/proc.c:1373 +0x9a
runtime.parkunlock(0x5f4180, 0x5f3509)
        C:/go/root/src/pkg/runtime/proc.c:1389 +0x3f
runfinq()
        C:/go/root/src/pkg/runtime/mgc0.c:2610 +0xc5
runtime.goexit()
        C:/go/root/src/pkg/runtime/proc.c:1449
FAIL    path/filepath   0.063s

Alex

https://codereview.appspot.com/86160044/diff/240001/src/pkg/os/file_windows.go
File src/pkg/os/file_windows.go (right):

https://codereview.appspot.com/86160044/diff/240001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:521: return syscall.EWINDOWS
Use LinkError here.

https://codereview.appspot.com/86160044/diff/240001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:545: return err
Use LinkError here.

https://codereview.appspot.com/86160044/diff/240001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:549: return err
Use LinkError here.

https://codereview.appspot.com/86160044/diff/240001/src/pkg/syscall/zsyscall_...
File src/pkg/syscall/zsyscall_windows_386.go (right):

https://codereview.appspot.com/86160044/diff/240001/src/pkg/syscall/zsyscall_...
src/pkg/syscall/zsyscall_windows_386.go:1819: if r1 == 0 {
Please, regenerate this file.

https://codereview.appspot.com/86160044/diff/240001/src/pkg/syscall/zsyscall_...
File src/pkg/syscall/zsyscall_windows_amd64.go (right):

https://codereview.appspot.com/86160044/diff/240001/src/pkg/syscall/zsyscall_...
src/pkg/syscall/zsyscall_windows_amd64.go:1807: if r1 == 0 {
Please, regenerate this file.

[mlf, 2014-07-10 10:50:50]

How do I regenerate zsyscall_windows?

I was just running all.bat and assumed it would regenerate as needed but that
doesn't seem to be the case.

[mlf, 2014-07-10 11:24:25]

On 2014/07/10 10:50:50, mlf wrote:
> How do I regenerate zsyscall_windows?
> 
> I was just running all.bat and assumed it would regenerate as needed but that
> doesn't seem to be the case.

Found it.

[mlf, 2014-07-11 02:11:44]

Hello golang-codereviews@googlegroups.com, bradfitz@golang.org,
alex.brainman@gmail.com, gobot@golang.org, rsc@golang.org, minux@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[brainman, 2014-07-11 07:08:05]

LGTM

I will wait for another review before submitting (this changes quite a bit of
code).

Please, change description (use "hg change 86160044" command) to:

>>>
os: Implement symlink support for Windows

Fixes Issue 5750.
<<<

The "Fixes Issue 5750." line is important. It will automatically close issue
5750 once we submit your change.

Thank you very much for sticking all the way. It is quite complicated change.

Alex

[bradfitz, 2014-07-14 22:40:27]

Random drive-by comments.

https://codereview.appspot.com/86160044/diff/280001/src/pkg/os/file_windows.go
File src/pkg/os/file_windows.go (right):

https://codereview.appspot.com/86160044/diff/280001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:525: pathbuf := []byte(oldname)
you can skip these two string->[]byte->string allocations if there's no '/' byte
to begin with.

or you can at least skip one if you note when you did any work in the loop. but
better to ditch both.

https://codereview.appspot.com/86160044/diff/280001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:526: for i, _ := range pathbuf {
don't need ", _"  But I'd make it "i, b" and change pathbuf[i] to b below

https://codereview.appspot.com/86160044/diff/280001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:537: destpath += `\` + oldname
don't use += here to build a string. just do it all at once on the previous
line: it's more efficient.

https://codereview.appspot.com/86160044/diff/280001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:552: var flags uint32 = 0
drop = 0

https://codereview.appspot.com/86160044/diff/280001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:577: return vol + dir
so if dir == "" this looks like "C:." instead of "C:"?  I don't know Windows,
but that looks funny. Are those the same? Which is more canonical?

https://codereview.appspot.com/86160044/diff/280001/src/pkg/path/filepath/pat...
File src/pkg/path/filepath/path_windows.go (left):

https://codereview.appspot.com/86160044/diff/280001/src/pkg/path/filepath/pat...
src/pkg/path/filepath/path_windows.go:7: import (
unrelated change. I'd remove from this CL.

https://codereview.appspot.com/86160044/diff/280001/src/pkg/path/filepath/sym...
File src/pkg/path/filepath/symlink.go (right):

https://codereview.appspot.com/86160044/diff/280001/src/pkg/path/filepath/sym...
src/pkg/path/filepath/symlink.go:27: if c < 0x80 && os.IsPathSeparator(uint8(c))
{
At top here:

const utf8RuneSelf = 0x80

Would read better than a literal here.

[mlf, 2014-07-15 01:43:40]

https://codereview.appspot.com/86160044/diff/280001/src/pkg/os/file_windows.go
File src/pkg/os/file_windows.go (right):

https://codereview.appspot.com/86160044/diff/280001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:525: pathbuf := []byte(oldname)
The moment I find a single /, I have to do some kind of magic so I can change
the value.
I would like to use strings.Replace instead which does exactly what you want and
then handles the heavy lifting. I can defer the conversion to bytes and back to
strings once I see the first / if that's preferred.

https://codereview.appspot.com/86160044/diff/280001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:577: return vol + dir
More dangers with Windows.
If we use your two values and append a path separator we end up with C:\ and
C:.\ which are very different.  The first is the root of the C drive the second
is the current working directory on the C drive.

[mlf, 2014-07-15 02:08:07]

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

Please take another look.

[brainman, 2014-07-16 06:46:34]

One more nit, and I will submit.

Thank you.

Alex

https://codereview.appspot.com/86160044/diff/330001/src/pkg/os/file_windows.go
File src/pkg/os/file_windows.go (right):

https://codereview.appspot.com/86160044/diff/330001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:524: // '/' does not work in link's content
This new code makes it too complicated now. I loose track of the main
functionality - symlinks. Please, move all that code into new function
(toBackSlash or something).

[mlf, 2014-07-16 10:44:27]

PTAL

https://codereview.appspot.com/86160044/diff/330001/src/pkg/os/file_windows.go
File src/pkg/os/file_windows.go (right):

https://codereview.appspot.com/86160044/diff/330001/src/pkg/os/file_windows.g...
src/pkg/os/file_windows.go:524: // '/' does not work in link's content
I decided to follow the naming convention that filepath uses, fromSlash.

[brainman, 2014-07-17 07:02:55]

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

os: Implement symlink support for Windows

Fixes Issue 5750.

https://code.google.com/p/go/issues/detail?id=5750

os: Separate windows from posix. Implement windows support.
path/filepath: Use the same implementation as other platforms
syscall: Add/rework new APIs for Windows

LGTM=alex.brainman
R=golang-codereviews, alex.brainman, gobot, rsc, minux
CC=golang-codereviews
https://codereview.appspot.com/86160044

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

[gobot, 2014-07-17 08:04:38]

This CL appears to have broken the plan9-386-cnielsen builder.
See http://build.golang.org/log/deb22ee25f7fa78825cf8f7c8b2618b57ccd8a19