|
|
Descriptionos: 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
Patch Set 1 #Patch Set 2 : diff -r 1eaf29392348 https://code.google.com/p/go #Patch Set 3 : diff -r 1eaf29392348 https://code.google.com/p/go #Patch Set 4 : diff -r e0ad7e329637 https://code.google.com/p/go #Patch Set 5 : diff -r e0ad7e329637 https://code.google.com/p/go #
Total comments: 1
Patch Set 6 : diff -r 75040398cb4e https://code.google.com/p/go #Patch Set 7 : diff -r 4af9a710ed44 https://code.google.com/p/go #Patch Set 8 : diff -r 4af9a710ed44 https://code.google.com/p/go #
Total comments: 32
Patch Set 9 : diff -r 1a9d124153b9 https://code.google.com/p/go #Patch Set 10 : diff -r 991519645363 https://code.google.com/p/go #Patch Set 11 : diff -r 991519645363 https://code.google.com/p/go #
Total comments: 2
Patch Set 12 : diff -r eae7f816eb6c https://code.google.com/p/go #
Total comments: 19
Patch Set 13 : diff -r d6c7ec5112ec https://code.google.com/p/go #
Total comments: 5
Patch Set 14 : diff -r c0a68bcf19ae https://code.google.com/p/go #Patch Set 15 : diff -r c0a68bcf19ae https://code.google.com/p/go #
Total comments: 9
Patch Set 16 : diff -r 13af066c1267 https://code.google.com/p/go #Patch Set 17 : diff -r 13af066c1267 https://code.google.com/p/go #Patch Set 18 : diff -r 13af066c1267 https://code.google.com/p/go #
Total comments: 2
Patch Set 19 : diff -r 418b1fb34050 https://code.google.com/p/go #
MessagesTotal messages: 52
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
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. >
Sign in to reply to this message.
R=alex.brainman@gmail.com (assigned by bradfitz@golang.org)
Sign in to reply to this message.
R=close Please run 'hg mail' once 1.3 is out to reopen this CL for 1.4.
Sign in to reply to this message.
I tried to apply this to current tip, but no luck. Alex
Sign in to reply to this message.
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?
Sign in to reply to this message.
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?
Sign in to reply to this message.
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
Sign in to reply to this message.
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'
Sign in to reply to this message.
Figured it out... This should be something that can be applied now.
Sign in to reply to this message.
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?
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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).
Sign in to reply to this message.
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.
Sign in to reply to this message.
*** 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>
Sign in to reply to this message.
This CL appears to have broken the plan9-386-cnielsen builder. See http://build.golang.org/log/deb22ee25f7fa78825cf8f7c8b2618b57ccd8a19
Sign in to reply to this message.
|