[go] os: Windows: MkdirAll should work with extended-length form path
Mansour Rahimi (Gerrit)
Mansour Rahimi has uploaded this change for review.
os: Windows: MkdirAll should work with extended-length form path
MkdirAll recursively calls itself for parent directory of given path. In
case of Windows extended-length form (with \\?\ prefix), it cannot
manage to find the parent at the end. With removing the prefix (\\?\),
it would be just a normal directory structure (and Mkdir takes care
of long directory).
Updates #22230
Change-Id: I363660b262588c5382ea829773d3b6005ab8df3c
---
M src/os/path.go
M src/os/path_test.go
2 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/src/os/path.go b/src/os/path.go
index eb996e5..3d7c49d 100644
--- a/src/os/path.go
+++ b/src/os/path.go
@@ -27,6 +27,11 @@
return &PathError{"mkdir", path, syscall.ENOTDIR}
}
+ if runtime.GOOS == "windows" && len(path) >= 4 && path[:4] == `\\?\` {
+ // Remove the extended-length form prefix (\\?\).
+ path = path[4:]
+ }
+
// Slow path: make sure parent exists and then call Mkdir for path.
i := len(path)
for i > 0 && IsPathSeparator(path[i-1]) { // Skip trailing path separator.
diff --git a/src/os/path_test.go b/src/os/path_test.go
index f58c7e7..e6861bc 100644
--- a/src/os/path_test.go
+++ b/src/os/path_test.go
@@ -73,6 +73,13 @@
if err != nil {
t.Fatalf("MkdirAll %q: %s", path, err)
}
+
+ // Make directory with extended-length form (\\?\-prefixed).
+ path = `\\?\` + tmpDir + `\_TestMkdirAll_\dir\`
+ err = MkdirAll(path, 0777)
+ if err != nil {
+ t.Fatalf("MkdirAll %q: %s", path, err)
+ }
}
}
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
2 comments:
Patch Set #1, Line 7:
os: Windows: MkdirAll should work with extended-length form pathThe topic line should describe the change. See existing examples. I would write a different version but to be honest I'm not really clear on what is changing here. The next paragraph also fails to describe the change. This about writing it in the form "Calling MkdirAll on paths like X failed. This change fixes that by doing Y."
Patch Set #1, Line 30:
if runtime.GOOS == "windows" && len(path) >= 4 && path[:4] == `\\?\` {This would be the first test of the form GOOS == "windows" in this file. We have path_windows.go and path_unix.go to handle cases where we need differences.
Let me add that I don't understand how this could possibly be correct, but I also don't understand Windows path names.
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
Thanks for the comments
2 comments:
Patch Set #1, Line 7:
os: Windows: MkdirAll should work with extended-length form path
The topic line should describe the change. See existing examples. […]
Will change it to:
os: Windows: fix MkdirAll to support path in extended-length formAnd will try to explain more.
Patch Set #1, Line 30:
if runtime.GOOS == "windows" && len(path) >= 4 && path[:4] == `\\?\` {
This would be the first test of the form GOOS == "windows" in this file. We have path_windows. […]
Well, I saw GOOS check on L101 of this file, and thought it should be ok.
Will move it to path_windows.go.
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
Mansour Rahimi uploaded patch set #2 to this change.
os: Windows: fix MkdirAll to support path in extended-length form
Calling MkdirAll on paths in extended-length form (\\?\-prefixed)
failed.
MkdirAll calls itself recursively with parent directory of given path in
its parameter. It finds parent directory by looking for delimiter in
the path, and taking the left part. When path is in extended-length form,
it finds empty path at the end.
Here is a sample of path in extended-length form:
\\?\c:\foo\bar
This change fixes that by removing extended-length form prefix (\\?\),
before looking for the parent directory.
Updates #22230
Change-Id: I363660b262588c5382ea829773d3b6005ab8df3c
---
M src/os/export_windows_test.go
M src/os/path.go
M src/os/path_plan9.go
M src/os/path_test.go
M src/os/path_unix.go
M src/os/path_windows.go
M src/os/path_windows_test.go
7 files changed, 43 insertions(+), 0 deletions(-)
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
Please take a look.
2 comments:
Patch Set #1, Line 7:
os: Windows: fix MkdirAll to support path in extended-length formWill change it to: […]
Done
Patch Set #1, Line 30:
path = removePathPrefix(path)Well, I saw GOOS check on L101 of this file, and thought it should be ok. […]
Done
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
Mansour Rahimi uploaded patch set #3 to this change.
os: Windows: fix MkdirAll to support path in extended-length form
Calling MkdirAll on paths in extended-length form (\\?\-prefixed)
failed.
MkdirAll calls itself recursively with parent directory of given path in
its parameter. It finds parent directory by looking for delimiter in
the path, and taking the left part. When path is in extended-length form,
it finds empty path at the end.
Here is a sample of path in extended-length form:
\\?\c:\foo\bar
This change fixes that by removing extended-length form prefix (\\?\),
before looking for the parent directory.
Updates #22230
Change-Id: I363660b262588c5382ea829773d3b6005ab8df3c
---
M src/os/export_windows_test.go
M src/os/path.go
M src/os/path_plan9.go
M src/os/path_test.go
M src/os/path_unix.go
M src/os/path_windows.go
M src/os/path_windows_test.go
7 files changed, 43 insertions(+), 0 deletions(-)
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Thank you for trying to fix this.
Alex
2 comments:
Patch Set #3, Line 30:
path = removePathPrefix(path)I don't think this will work, because MkdirAll calls itself recursively. So removePathPrefix will remove \\?\ even in the middle of file path. I don't think we want that.
This is also broken if user passes \\?\xxx. Will your new code just delete ./xxx ? Is that what we want?
Patch Set #3, Line 78:
path = `\\?\` + tmpDir + `\_TestMkdirAll_\dir\`This will not work if tmpDir is relative path, or contains "." or "/" and others - see os.fixLongPath function for details. Did you verify that tmpDir is suitable?
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
Thanks Alex for the comments.
2 comments:
Patch Set #3, Line 30:
path = removePathPrefix(path)
I don't think this will work, because MkdirAll calls itself recursively. […]
As far as I understand from MSDN documentation, \\?\-prefix is not allowed in middle of path (Anyway I'm looking for a Windows VM to confirm this). Documentation also says "you cannot use the "\\?\" prefix with a relative path".
So these paths should not be correct:
\\?\c:\foo\\?\c:\foo\bar
\\?\c:\foo\\?\bar
\\?\foo\barNew code will remove the prefix (\\?\xxx -> xxx):
\\?\c:\foo\bar -> c:\foo\bar
Patch Set #3, Line 78:
path = `\\?\` + tmpDir + `\_TestMkdirAll_\dir\`
This will not work if tmpDir is relative path, or contains "." or "/" and others - see os. […]
TempDir() uses syscall to get the temp directory, and if it's Windows runtime, the temp path should be suitable (e.g. c:\temp), but agree that it's better to verify.
So I'll fixLongPath(tmpDir) to make sure it's suitable, then if it's not started with \\?\, will add the prefix.
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
1 comment:
Patch Set #3, Line 30:
path = removePathPrefix(path)
As far as I understand from MSDN documentation, \\?\-prefix is not allowed in middle of path (Anyway […]
I got your point!
Even if it's not valid, we can get that path in parameter:
\\?\c:\foo\\?\bar
And ends in creating an invalid directory:
c:\foobarThe problem is, we don't validate path in MkdirAll, and postpone it to syscall (in CreateDirectory).
So maybe it's better to add a basic validation of the path in MkdirAll, by looking for more occurrences of prefix (\\?\) in path, and reject it as an invalid path.
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
Mansour Rahimi uploaded patch set #4 to this change.
os: Windows: fix MkdirAll to support path in extended-length form
Calling MkdirAll on paths in extended-length form (\\?\-prefixed)
failed.
MkdirAll calls itself recursively with parent directory of given path in
its parameter. It finds parent directory by looking for delimiter in
the path, and taking the left part. When path is in extended-length form,
it finds empty path at the end.
Here is a sample of path in extended-length form:
\\?\c:\foo\bar
This change fixes that by removing extended-length form prefix (\\?\),
before looking for the parent directory.
Updates #22230
Change-Id: I363660b262588c5382ea829773d3b6005ab8df3c
---
M src/os/export_windows_test.go
M src/os/path.go
M src/os/path_plan9.go
M src/os/path_test.go
M src/os/path_unix.go
M src/os/path_windows.go
M src/os/path_windows_test.go
7 files changed, 72 insertions(+), 0 deletions(-)
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
1 comment:
Patch Set #4, Line 221:
return nil, &PathError{"mkdir", path, syscall.ENOTDIR}This error (and the below one) should be ERROR_INVALID_NAME (0x7B), but cannot find it in zerrors_windows.go.
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
3 comments:
Patch Set #3, Line 30:
path, err = removePathPrefix(path)... we can get that path in parameter:
\\?\c:\foo\\?\bar
Yes, we can get all weird file paths in here. I suggest you add these to new tests.
And ends in creating an invalid directory:
c:\foobarThe problem is, we don't validate path in MkdirAll, and postpone it
to syscall (in CreateDirectory).
So maybe it's better to add a basic validation of the path in
MkdirAll, by looking for more occurrences of prefix (\\?\) in path,
and reject it as an invalid path.
How are you going to validate paths prefixed with \\?\ ? Maybe you should use GetFullPathName for these paths to have them cleaned up? I googled and found http://arsenmk.blogspot.com.au/2015/12/handling-long-paths-on-windows.html maybe you could do something similar.
You also seems to be calling removePathPrefix many times, because MkdirAll calls itself. Why? Shouldn't you validate path once at the very start?
Patch Set #3, Line 78:
if len(tmpDir) < 4 || tmpDir[:4] != prefix {
So I'll fixLongPath(tmpDir) to make sure it's suitable, then if
it's not started with \\?\, will add the prefix.
SGTM. Please do.
Patch Set #4, Line 221:
return nil, &PathError{"mkdir", path, syscall.ENOTDIR}
This error (and the below one) should be ERROR_INVALID_NAME (0x7B), but cannot find it in zerrors_wi […]
If you need ERROR_INVALID_NAME and similar, add them to internal/syscall/windows/syscall_windows.go. But I am not convinced you need them yet.
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
2 comments:
Patch Set #3, Line 78:
if len(tmpDir) < 4 || tmpDir[:4] != prefix {
> So I'll fixLongPath(tmpDir) to make sure it's suitable, then if […]
Using fixLongPath(tmpDir) is not working. It would return on short path length (< 248) immediately. So for a path like c:\temp\foo\..\ will keep it as is. I'm trying to use the path validation recommended by you, here as well.
Patch Set #4, Line 221:
return nil, &PathError{"mkdir", path, syscall.ENOTDIR}If you need ERROR_INVALID_NAME and similar, add them to internal/syscall/windows/syscall_windows.go. […]
You're right, I don't need it anymore.
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
Mansour Rahimi uploaded patch set #5 to this change.
os: Windows: fix MkdirAll to support path in extended-length form
Calling MkdirAll on paths in extended-length form (\\?\-prefixed)
failed.
MkdirAll calls itself recursively with parent directory of given path in
its parameter. It finds parent directory by looking for delimiter in
the path, and taking the left part. When path is in extended-length form,
it finds empty path at the end.
Here is a sample of path in extended-length form:
\\?\c:\foo\bar
This change fixes that by removing extended-length form prefix (\\?\),
before looking for the parent directory.
Updates #22230
Change-Id: I363660b262588c5382ea829773d3b6005ab8df3c
---
M src/os/export_windows_test.go
M src/os/path.go
M src/os/path_plan9.go
M src/os/path_test.go
M src/os/path_unix.go
M src/os/path_windows.go
M src/os/path_windows_test.go
7 files changed, 91 insertions(+), 1 deletion(-)
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
1 comment:
Patch Set #5, Line 79:
n, err := syscall.GetFullPathName(tmpDir, 0, nil, nil)Why I got undefined syscall.GetFullPathName? Am I missing something?
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
2 comments:
Patch Set #3, Line 78:
if len(tmpDir) < 4 || tmpDir[:4] != prefix {
Using fixLongPath(tmpDir) is not working. It would return on short path length (< 248) immediately. […]
SGTM
Patch Set #5, Line 79:
n, err := syscall.GetFullPathName(tmpDir, 0, nil, nil)
Why I got undefined syscall. […]
I am not sure what you are asking? syscall.GetFullPathName does exist, but only on windows. Do you build this on windows? What command do you run and what exact error message is?
I also just realized, that you could use syscall.FullPath instead. It already does all string into *uint16 and back conversion and buffer allocation and all.
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
1 comment:
Patch Set #5, Line 79:
n, err := syscall.GetFullPathName(tmpDir, 0, nil, nil)
I am not sure what you are asking? syscall.GetFullPathName does exist, but only on windows. Do you build this on windows? What command do you run and what exact error message is?
No, I'm on Debian (have an XP VM for testing), and running all.bash. I believe we should move this part of test, in path_windows_test.go.
Here is the failed build:
$ ./all.bash
Building Go cmd/dist using /usr/local/go.
Building Go toolchain1 using /usr/local/go.
Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1.
Building Go toolchain2 using go_bootstrap and Go toolchain1.
Building Go toolchain3 using go_bootstrap and Go toolchain2.
Building packages and commands for linux/amd64.##### Testing packages.
ok archive/tar 0.078s
ok archive/zip 1.404s
.
.
.
# os_test
os/path_test.go:79:14: undefined: syscall.GetFullPathName
os/path_test.go:84:13: undefined: syscall.GetFullPathName
os/path_test.go:88:22: undefined: syscall.UTF16ToString
ok net/http 3.681s
ok net/http/cgi 0.565s
.
.
.
FAIL os [build failed]
ok os/exec 0.593s
ok os/signal 4.534s
.
.
.
2018/01/16 23:51:47 Failed: exit status 2
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
Mansour Rahimi uploaded patch set #6 to this change.
os: Windows: fix MkdirAll to support path in extended-length form
Calling MkdirAll on paths in extended-length form (\\?\-prefixed)
failed.
MkdirAll calls itself recursively with parent directory of given path in
its parameter. It finds parent directory by looking for delimiter in
the path, and taking the left part. When path is in extended-length form,
it finds empty path at the end.
Here is a sample of path in extended-length form:
\\?\c:\foo\bar
This change fixes that by removing extended-length form prefix (\\?\),
before looking for the parent directory.
Updates #22230
Change-Id: I363660b262588c5382ea829773d3b6005ab8df3c
---
M src/os/export_windows_test.go
M src/os/path.go
M src/os/path_plan9.go
M src/os/path_unix.go
M src/os/path_windows.go
M src/os/path_windows_test.go
6 files changed, 91 insertions(+), 1 deletion(-)
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Slowly getting there.
Thank you.
Alex
11 comments:
Patch Set #6, Line 7:
os: Windows: fix MkdirAll to support path in extended-length formos: make MkdirAll support path in extended-length form
Patch Set #6, Line 23:
Updates #22230s/Updates/Fixes/
No? I think there is nothing else to do in issue #22230.
Patch Set #6, Line 21:
path, err := removePathPrefix(path)Maybe add short comment explaining that removePathPrefix is really only used on windows. So people who don't use windows don't even need to go look at the code.
Patch Set #6, Line 26:
err = mkdirAll(path, perm)Just
return mkdirAll(path, perm)
Patch Set #5, Line 79:
func TestRemoveAll(t *testing.T) {
No, I'm on Debian (have an XP VM for testing), and running
all.bash. I believe we should move this part of test, in
path_windows_test.go.
Yes, windows specific test should go into windows specific source file.
os/path_test.go:79:14: undefined: syscall.GetFullPathName
You are building path_test.go on linux, and linux version of Go does not have syscall.GetFullPathName function.
Patch Set #6, Line 211:
func removePathPrefix(path string) (string, error) {Maybe
s/removePathPrefix/removeExtendedLenPathPrefix/
to make function name less general?
And we need comment here, otherwise future readers will be guessing about what this function does.
Patch Set #6, Line 214:
n, err := GetFullPathName(path, 0, nil, nil)This does not build on windows:
```
$ GOOS=windows go test -c -o /dev/null os
# os
os/path_windows.go:214:13: undefined: GetFullPathName
os/path_windows.go:216:4: cannot use nil as type string in return argument
os/path_windows.go:221:4: cannot use nil as type string in return argument
os/path_windows.go:221:16: undefined: GetLastError
# os
os/path_windows.go:214:13: undefined: GetFullPathName
os/path_windows.go:216:4: cannot use nil as type string in return argument
os/path_windows.go:221:4: cannot use nil as type string in return argument
os/path_windows.go:221:16: undefined: GetLastError
$
```I suggest you use syscall.FullPath here
```
$ GOOS=windows go doc syscall.FullPath
func FullPath(name string) (path string, err error)
FullPath retrieves the full path of the specified file.
```
File src/os/path_windows_test.go:
Patch Set #6, Line 71:
t.Errorf("#%d: Incorrect error result (did fail? %v, expected: %v)", i, err == nil, test.ok)I think you want to print the error too - it might be useful debugging test failures.
Patch Set #6, Line 79:
TestMkdirAll(We already have TestMkdirAll function in os test package
s/TestMkdirAll/TestMkdirAllExtendedLength/
Patch Set #6, Line 85:
t.Skipf("MkdirAll (extended-length), skipping on %s, cannot get temp full path", runtime.GOOS)Why skipping this failure? syscall.FullPath call should succeed. No?
Don't you want to delete %TMP%\_TestMkdirAll_\dir after the test?
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
Mansour Rahimi uploaded patch set #7 to this change.
os: make MkdirAll support path in extended-length form
Calling MkdirAll on paths in extended-length form (\\?\-prefixed)
failed.
MkdirAll calls itself recursively with parent directory of given path in
its parameter. It finds parent directory by looking for delimiter in
the path, and taking the left part. When path is in extended-length form,
it finds empty path at the end.
Here is a sample of path in extended-length form:
\\?\c:\foo\bar
This change fixes that by removing extended-length form prefix (\\?\),
before looking for the parent directory.
Fixes #22230
Change-Id: I363660b262588c5382ea829773d3b6005ab8df3c
---
M src/os/export_windows_test.go
M src/os/path.go
M src/os/path_plan9.go
M src/os/path_unix.go
M src/os/path_windows.go
M src/os/path_windows_test.go
6 files changed, 90 insertions(+), 4 deletions(-)
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
Patch Set 6:
(11 comments)
Slowly getting there.
Thank you.
Alex
I'm learning a lot, and it's fun.
Thanks for your patience Alex.
11 comments:
Patch Set #6, Line 7:
os: make MkdirAll support path in extended-length form
os: make MkdirAll support path in extended-length form
Ack
Patch Set #6, Line 23:
Fixes #22230s/Updates/Fixes/ […]
Ack
Just one thing.
I guess we still need to check if there is UNC\ after prefix, and remove it as well, correct?
Patch Set #6, Line 21:
// removeExtendedLenPathPrefix is only used on Windows.Maybe add short comment explaining that removePathPrefix is really only used on windows. […]
Ack
Just […]
Ack
Patch Set #5, Line 79:
func TestRemoveAll(t *testing.T) {
> No, I'm on Debian (have an XP VM for testing), and running […]
Ack
Maybe […]
Ack
Patch Set #6, Line 214:
// (\\?\-prefixed and is a valid path), and remove the prefix.This does not build on windows: […]
Ack
BTW this doesn't seem to work for me:
GOOS=windows go test -c -o /dev/null os
It shows nothing, even if I add some intentional compilation error, like not closed ")". Just weird!
File src/os/path_windows_test.go:
I think you want to print the error too - it might be useful debugging test failures.
Ack
Patch Set #6, Line 79:
ir := os.TempWe already have TestMkdirAll function in os test package […]
Ack
Why skipping this failure? syscall.FullPath call should succeed. […]
Ack
Patch Set #6, Line 93:
defer os.RemoveAll(tmpDir + `\_TestMkdirAllExtendedLength_\dir`)
Don't you want to delete %TMP%\_TestMkdirAll_\dir after the test?
Ack
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
More comments.
Thank you.
Alex
Patch set 7:Run-TryBot +1
5 comments:
Patch Set #6, Line 23:
Fixes #22230Ack […]
I don't understand your question, please, try again.
Patch Set #6, Line 214:
// (\\?\-prefixed and is a valid path), and remove the prefix.BTW this doesn't seem to work for me:
GOOS=windows go test -c -o /dev/null os
It shows nothing, even if I add some intentional compilation error,
like not closed ")". Just weird!
I get an error message if my code has syntax error. Can you show me the steps to reproduce this?
File src/os/path_windows_test.go:
Patch Set #7, Line 70:
t.Errorf("#%d: Incorrect error result = %v; (did fail? %v, expected: %v)", i, err, err == nil, test.ok)This will be confusing with os.RemoveExtendedLenPathPrefix error message embedded in the middle of another error message. Maybe just make that all simpler
```
got, err := os.RemoveExtendedLenPathPrefix(test.in)
if err != nil {
if test.ok {
t.Errorf('#%d: os.RemoveExtendedLenPathPrefix(%q) failed: %v', test.in, err)
}
continue
}
if !test.ok {
t.Errorf('#%d: os.RemoveExtendedLenPathPrefix(%q) succeeded (returns %q), but expected to fail', test.in, got, err)
continue
}
if got != test.want {
t.Errorf("#%d: removeExtendedLenPathPrefix(%q) = %q; want %q", i, test.in, got, test.want)
}
```Patch Set #7, Line 84:
Errorfs/Errorf/Fatalf/
There is no point to continue test, if your tmpDir is not set correctly.
Patch Set #7, Line 93:
tmpDir + `\_TestMkdirAllExtendedLength_\dir`)s/tmpDir + `\_TestMkdirAllExtendedLength_\dir`/tmpDir + `\_TestMkdirAllExtendedLength_`/
otherwise your test will still leave %TMP%\_TestMkdirAllExtendedLength_ directory. No?
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
Gobot Gobot (Gerrit)
Build is still in progress...
This change failed on windows-386-2008:
See https://storage.googleapis.com/go-build-log/8e2674bf/windows-386-2008_190d15f9.log
Consult https://build.golang.org/ to see whether it's a new failure. Other builds still in progress; subsequent failure notices suppressed until final report.
Alex Brainman (Gerrit)
Build is still in progress...
This change failed on windows-386-2008:
See https://storage.googleapis.com/go-build-log/8e2674bf/windows-386-2008_190d15f9.logConsult https://build.golang.org/ to see whether it's a new
failure. Other builds still in progress; subsequent failure notices
suppressed until final report.
The error is
```
C:\workdir\go\src\os\path_windows.go:220:4: cannot use nil as type string in return argument
```
You obviously did not compile this on Windows. I will let you sort this out.
Alex
Patch set 7:-Run-TryBot
Gobot Gobot (Gerrit)
2 of 18 TryBots failed:
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/8e2674bf/windows-386-2008_190d15f9.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/8e2674bf/windows-amd64-2016_07205317.log
Consult https://build.golang.org/ to see whether they are new failures.
Patch set 7:TryBot-Result -1
Mansour Rahimi (Gerrit)
Patch Set 7: -Run-TryBot
Build is still in progress...
This change failed on windows-386-2008:
See https://storage.googleapis.com/go-build-log/8e2674bf/windows-386-2008_190d15f9.logConsult https://build.golang.org/ to see whether it's a new
failure. Other builds still in progress; subsequent failure notices
suppressed until final report.The error is
```
C:\workdir\go\src\os\path_windows.go:220:4: cannot use nil as type string in return argument
```
You obviously did not compile this on Windows. I will let you sort this out.Alex
Ah! sorry, was struggling with GOOS=windows and missed this.
5 comments:
Patch Set #6, Line 23:
Fixes #22230
I don't understand your question, please, try again.
What if we got this?
\\?\UNC\server\share\fooI assume, it is a valid path, and will end in:
UNC\server\share\fooIt seems, we should remove "UNC" too.
Patch Set #6, Line 214:
// (\\?\-prefixed and is a valid path), and remove the prefix.
> BTW this doesn't seem to work for me: […]
Yes, for instance, I use same code here (L214 has error, since GetFullPathName() is undefined, and we cannot pass nil as zero string), and run this command:
mnr@debian:~/workspace/go/src/os$ GOOS=windows go test -c -o /dev/null osBut I get no error or something else.
File src/os/path_windows_test.go:
Patch Set #7, Line 70:
t.Errorf("#%d: Incorrect error result = %v; (did fail? %v, expected: %v)", i, err, err == nil, test.ok)
This will be confusing with os. […]
Ack
Patch Set #7, Line 84:
Errorf
s/Errorf/Fatalf/ […]
Ack
Patch Set #7, Line 93:
tmpDir + `\_TestMkdirAllExtendedLength_\dir`)
s/tmpDir + `\_TestMkdirAllExtendedLength_\dir`/tmpDir + `\_TestMkdirAllExtendedLength_`/ […]
Ack
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
Mansour Rahimi uploaded patch set #8 to this change.
os: make MkdirAll support path in extended-length form
Calling MkdirAll on paths in extended-length form (\\?\-prefixed)
failed.
MkdirAll calls itself recursively with parent directory of given path in
its parameter. It finds parent directory by looking for delimiter in
the path, and taking the left part. When path is in extended-length form,
it finds empty path at the end.
Here is a sample of path in extended-length form:
\\?\c:\foo\bar
This change fixes that by removing extended-length form prefix (\\?\),
before looking for the parent directory.
Fixes #22230
Change-Id: I363660b262588c5382ea829773d3b6005ab8df3c
---
M src/os/export_windows_test.go
M src/os/path.go
M src/os/path_plan9.go
M src/os/path_unix.go
M src/os/path_windows.go
M src/os/path_windows_test.go
6 files changed, 97 insertions(+), 4 deletions(-)
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
Mansour Rahimi uploaded patch set #9 to this change.
Mansour Rahimi (Gerrit)
I've got my VM (Win XP) up and running, and tried to built on windows.
Negative tests are failing in TestRemoveExtendedLenPathPrefix. It seems, syscall.FullPath doesn't work correctly!
```
--- FAIL: TestRemoveExtendedLenPathPrefix (0.00s)
path_windows_test.go:76: #7: os.RemoveExtendedLenPathPrefix("\\\\?\\ ?") succeeded (returns " ?"), but expected to fail
path_windows_test.go:76: #8: os.RemoveExtendedLenPathPrefix("\\\\?\\cc:\\") succeeded (returns "cc:\\"), but expected to fail
path_windows_test.go:76: #9: os.RemoveExtendedLenPathPrefix("\\\\?\\foo\\bar") succeeded (returns "foo\\bar"), but expected to fail
path_windows_test.go:76: #10: os.RemoveExtendedLenPathPrefix("\\\\?\\c:\\foo\\..\\bar") succeeded (returns "c:\\foo\\..\\bar"), but expected to fail
path_windows_test.go:76: #11: os.RemoveExtendedLenPathPrefix("\\\\?\\c:\\foo\\\\?\\bar") succeeded (returns "c:\\foo\\\\?\\bar"), but expected to fail
path_windows_test.go:76: #12: os.RemoveExtendedLenPathPrefix("\\\\?\\c:\\foo\\\\?\\c:\\foo\\bar") succeeded (returns "c:\\foo\\\\?\\c:\\foo\\bar"), but expected to fail
FAIL
FAIL os 2.393s
```
Mansour Rahimi (Gerrit)
Well, it seems GetFullPathNameW doesn't care at all, if it's a correct path structure or not. Then we cannot use syscall.FullPath to validate the path:
```
C:\>python
>>> from ctypes import *
>>> print windll.kernel32.GetFullPathNameW("\\\\?\\foo\\",0,None,None)
25
>>> print windll.kernel32.GetLastError()
0
```
In the other side, CreateDirectoryW validates the path structure, and returns the correct error code (123 - ERROR_INVALID_NAME - The filename, directory name, or volume label syntax is incorrect):
```
C:\>python
>>> from ctypes import *
>>> print windll.kernel32.CreateDirectoryW("\\\\?\\foo\\",0)
0
>>> print windll.kernel32.GetLastError()
123
```
I'm afraid, they didn't expose this validation as a separate API, so maybe we can use syscall.Mkdir to validate the path structure (even if it sounds crazy, at least it's better than a complex regex)
Alex Brainman (Gerrit)
Well, it seems GetFullPathNameW doesn't care at all, if it's a
correct path structure or not. Then we cannot use syscall.FullPath
to validate the path:
I was afraid of that. :-)
In the other side, CreateDirectoryW validates the path structure,
and returns the correct error code (123 - ERROR_INVALID_NAME
...
I'm afraid, they didn't expose this validation as a separate API,
so maybe we can use syscall.Mkdir to validate the path structure
(even if it sounds crazy, at least it's better than a complex
regex)
Yes, I tried this change:
```
diff --git a/src/os/path.go b/src/os/path.go
index eb996e5..ec6a793 100644
--- a/src/os/path.go
+++ b/src/os/path.go
@@ -39,8 +39,10 @@ func MkdirAll(path string, perm FileMode) error {
}
if j > 1 {
- // Create parent
- err = MkdirAll(path[0:j-1], perm)
+ // Create parent.
+ // Pass trailing path separator to MkdirAll, so our
+ // algorithm works for paths, like \\?\c:\
+ err = MkdirAll(path[0:j], perm)
if err != nil {
return err
}
```
and that works for os.MkdirAll(`\\?\D:\test`, 0777) mentioned on issue #22230. I suspect my change will also work for `\\?\UNC\server\share\foo` that you mention. Perhaps if you add new tests you might discover some problems with my approach. but perhaps it might work with some variations.
Alex
Alex Brainman (Gerrit)
1 comment:
Patch Set #6, Line 214:
n, err := GetFullPathName(path, 0, nil, nil)Yes, for instance, I use same code here (L214 has error, since GetFullPathName() is undefined, and w […]
Here is what I do, to get your Patch Set 7 and build os windows test on linux:
```
$ cd $GOROOT/src
$ git fetch https://go.googlesource.com/go refs/changes/95/86295/7 && git
checkout FETCH_HEAD
remote: Counting objects: 1339, done
remote: Finding sources: 100% (471/471)
remote: Total 471 (delta 213), reused 426 (delta 213)
Receiving objects: 100% (471/471), 921.05 KiB | 0 bytes/s, done.
Resolving deltas: 100% (213/213), completed with 61 local objects.
From https://go.googlesource.com/go
* branch refs/changes/95/86295/7 -> FETCH_HEAD
Previous HEAD position was 165e752... sync: consistently use article "a" for RWMutex
HEAD is now at 8e2674b... os: make MkdirAll support path in extended-length form
$ ./make.bash
Building Go cmd/dist using /home/a/go1.4.
Building Go toolchain1 using /home/a/go1.4.
Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1.
Building Go toolchain2 using go_bootstrap and Go toolchain1.
Building Go toolchain3 using go_bootstrap and Go toolchain2.
Building packages and commands for linux/amd64.
---
Installed Go for linux/amd64 in /home/a/go
Installed commands in /home/a/go/bin
$ GOOS=windows go test -c -o /dev/null os
# os
os/path_windows.go:220:4: cannot use nil as type string in return argument
# os
os/path_windows.go:220:4: cannot use nil as type string in return argument
$
```
Don't you get the error like I do?Alex
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
1 comment:
Patch Set #6, Line 214:
// (\\?\-prefixed and is a valid path), and remove the prefix.Here is what I do, to get your Patch Set 7 and build os windows test on linux: […]
No error :(
```
/go/src$ git fetch https://go.googlesource.com/go refs/changes/95/86295/7 && git checkout FETCH_HEAD
From https://go.googlesource.com/go
* branch refs/changes/95/86295/7 -> FETCH_HEAD
Note: checking out 'FETCH_HEAD'.
You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:git checkout -b <new-branch-name>
HEAD is now at 8e2674bfad... os: make MkdirAll support path in extended-length form
/go/src$ ./make.bash
Building Go cmd/dist using /usr/local/go.
Building Go toolchain1 using /usr/local/go.
Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1.
Building Go toolchain2 using go_bootstrap and Go toolchain1.
Building Go toolchain3 using go_bootstrap and Go toolchain2.
Building packages and commands for linux/amd64.
---
Installed Go for linux/amd64 in /home/mansour/workspace/go
Installed commands in /home/mansour/workspace/go/bin
/go/src$ GOOS=windows go test -c -o /dev/null os
/go/src$
/go/src$
/go/src$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/mansour/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build902138843=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
/go/src$
```
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
Patch Set 9:
I didn't get you.
How is this gonna work without removing the prefix?
Let's say for this path:
\\?\D:\test
Current MkdirAll implementation would get these paths in each step:
```
\\?\D:\test
\\?\D:
\\?
\
mkdir \: Access is denied.
```
And with your change:
```
\\?\D:\test
\\?\D:\
\\?\
\\
mkdir \\: The filename, directory name, or volume label syntax is incorrect.
```
Alex Brainman (Gerrit)
I didn't get you.
How is this gonna work without removing the prefix?
Let's say for this path:
\\?\D:\test
If I change Go like:
```
diff --git a/src/os/path.go b/src/os/path.go
index eb996e5..b0ace12 100644
--- a/src/os/path.go
+++ b/src/os/path.go
@@ -18,6 +18,7 @@ import (
// If path is already a directory, MkdirAll does nothing
// and returns nil.
func MkdirAll(path string, perm FileMode) error {
+ println("DEBUG1: ", path)
// Fast path: if we can tell whether path is a directory or file, stop with success or error.
dir, err := Stat(path)
if err == nil {
@@ -26,6 +27,7 @@ func MkdirAll(path string, perm FileMode) error {
}
return &PathError{"mkdir", path, syscall.ENOTDIR}
}
+ println("DEBUG2:")
// Slow path: make sure parent exists and then call Mkdir for path.
i := len(path)
@@ -39,8 +41,10 @@ func MkdirAll(path string, perm FileMode) error {
}
if j > 1 {
- // Create parent
- err = MkdirAll(path[0:j-1], perm)
+ // Create parent.
+ // Pass trailing path separator to MkdirAll, so our
+ // algorithm works for paths, like \\?\c:\
+ err = MkdirAll(path[0:j], perm)
if err != nil {
return err
}
```
and then run this program:
```
package main
import (
"log"
"os"
)
func main() {
dir := "\\\\?\\c:\\test"_, err := os.Stat(dir)
if !os.IsNotExist(err) {
log.Fatalf("%v directory already exists", dir)
}
err = os.MkdirAll(dir, 0777)
if err != nil {
log.Fatalf("MkdirAll(%q) failed: %v", dir, err)
}
defer os.Remove(dir)
}
```
I see my program output:
```
C:\>u:\test
DEBUG1: \\?\c:\test
DEBUG2:
DEBUG1: \\?\c:\
C:\>
```
What do you see?
(Make sure you fix your build process be fore answering my question.)
Alex
1 comment:
Patch Set #6, Line 214:
// (\\?\-prefixed and is a valid path), and remove the prefix.
No error :( ``` /go/src$ git fetch https://go.googlesource.com/go refs/changes/95/86295/7 && git checkout FETCH_HEAD From https://go.googlesource.com/go
- branch refs/changes/95/86295/7 -> FETCH_HEAD
When you are changing Go itself, you must build Go from source. So you must follow these instructons https://golang.org/doc/install/source It says:
```
The Go tool chain is written in Go. To build it, you need a Go compiler installed. The scripts that do the initial build of the tools look for an existing Go tool chain in $GOROOT_BOOTSTRAP. If unset, the default value of GOROOT_BOOTSTRAP is $HOME/go1.4.
```So you msut have already installed version of Go (it needs to be go1.4 or high). That already installed version of Go must be installed in a directory listed in GOROOT_BOOTSTRAP variable. Do you have GOROOT_BOOTSTRAP set? Also it is very important that Go installed in $GOROOT_BOOTSTRAP is not pointed by your GOROOT variable, and not listed in you PATH - that old version of Go will be only used to build your current Go and nothing else.
Looking at your output, I am confused. Where is your current Go? Where is you old Go?
According to
/go/src$ ./make.bash
it looks like your current Go is installed in /go. Is that correct?
According to
GOROOT="/usr/local/go"
it looks like your current Go is installed in /usr/local/go. But it cannot be installed in two different places simultaneously.
I suspect you have Go already installed in /usr/local/go, and then you checked out current Go into /go, but you are not really using /go, you are using /usr/local/go.
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
What do you see?
(Make sure you fix your build process be fore answering my question.)
Now same result, thanks! :)
I see what you did there. Fast path checking will catch \\?\c:\.
```
// Fast path: if we can tell whether path is a directory or file, stop with success or error.
```
Let me test it more.
1 comment:
Patch Set #6, Line 214:
// (\\?\-prefixed and is a valid path), and remove the prefix.
I suspect you have Go already installed in /usr/local/go, and then you checked out current Go into /go, but you are not really using /go, you are using /usr/local/go.
Yes, I had Go installed in /usr/local/go :(
Thanks a lot for pointing me to the right direction. I configured everything from scratch, and now it's working.
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
1 comment:
Patch Set #6, Line 214:
n, err := GetFullPathName(path, 0, nil, nil)Yes, I had Go installed in /usr/local/go :(
That would have been fine, if you used Go for your own projects. But if you want to make changes to the Go itself, you have to have different setup.
Alex
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
Mansour Rahimi uploaded patch set #10 to this change.
os: make MkdirAll support path in extended-length form
Calling MkdirAll on paths in extended-length form (\\?\-prefixed)
failed.
MkdirAll calls itself recursively with parent directory of given path in
its parameter. It finds parent directory by looking for delimiter in
the path, and taking the left part. When path is in extended-length form,
it finds empty path at the end.
Here is a sample of path in extended-length form:
\\?\c:\foo\bar
This change fixes that by passing trailing path separator to MkdirAll (so
it works for path like \\?\c:\).
Fixes #22230
Change-Id: I363660b262588c5382ea829773d3b6005ab8df3c
---
M src/os/path.go
M src/os/path_windows_test.go
2 files changed, 29 insertions(+), 2 deletions(-)
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
and that works for os.MkdirAll(`\\?\D:\test`, 0777) mentioned on issue #22230. I suspect my change will also work for `\\?\UNC\server\share\foo` that you mention. Perhaps if you add new tests you might discover some problems with my approach. but perhaps it might work with some variations.
Yes, it works perfectly, even for paths like `\\?\UNC\server\share\foo`.
Alex Brainman (Gerrit)
Thank you for your patience. I think we are getting close.
Alex
Patch set 10:Run-TryBot +1
5 comments:
Patch Set #10, Line 45:
err = MkdirAll(path[0:j], perm)Ian, this LGTM. Are you OK with us adding this Windows specific logic in here? Is comment good enough?
File src/os/path_windows_test.go:
Patch Set #10, Line 55:
FullPath (temp directory) %q: %sJust
s/FullPath (temp directory) %q: %s/FullPath(%q) fails: %v/
This will be less confusing when it fails. The failure message will have line number included anyway, so you can always find where it failed.
Patch Set #10, Line 62:
MkdirAll (extended-length) %q: %sSame
/MkdirAll (extended-length) %q: %s/MkdirAll(%q) failed: %v/
Patch Set #10, Line 64:
defer os.RemoveAll(tmpDir + `\_TestMkdirAllExtendedLength_`)I suspect cleaning temp directory here might be too late. Imaging that os.MkdirAll on line 60 starts its job and creates tmpDir + `\_TestMkdirAllExtendedLength_` directory, but then fails while creating `dir` inside of it, then your code on line 64 will not run.
If you change line 50 with:
```
tempDir, err := ioutil.TempDir("", "TestMkdirAllExtendedLength")
if err != nil {
t.Fatal(err)
}
defer os.RemoveAll(tempDir)```
that should take care of that situation.This test also cannot be run in parallel (because it uses the same temp directory), but if you follow my advice, your test can run in parallel with no problems.
Patch Set #10, Line 69:
MkdirAll (extended-length) %q: no errorSame
/MkdirAll (extended-length) %q: no error/MkdirAll(%q) should have failed, but did not/
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
TryBots beginning. Status page: https://farmer.golang.org/try?commit=2b5cefc4
Gobot Gobot (Gerrit)
TryBots are happy.
Patch set 10:TryBot-Result +1
Mansour Rahimi (Gerrit)
Mansour Rahimi uploaded patch set #11 to this change.
os: make MkdirAll support path in extended-length form
Calling MkdirAll on paths in extended-length form (\\?\-prefixed)
failed.
MkdirAll calls itself recursively with parent directory of given path in
its parameter. It finds parent directory by looking for delimiter in
the path, and taking the left part. When path is in extended-length form,
it finds empty path at the end.
Here is a sample of path in extended-length form:
\\?\c:\foo\bar
This change fixes that by passing trailing path separator to MkdirAll (so
it works for path like \\?\c:\).
Fixes #22230
Change-Id: I363660b262588c5382ea829773d3b6005ab8df3c
---
M src/os/path.go
M src/os/path_windows_test.go
2 files changed, 35 insertions(+), 2 deletions(-)
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
4 comments:
File src/os/path_windows_test.go:
Patch Set #10, Line 55:
oveAll(tempDir)Just […]
Ack
Same […]
Ack
I suspect cleaning temp directory here might be too late. Imaging that os. […]
Ack
Same […]
Ack
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
Mansour Rahimi uploaded patch set #12 to this change.
os: make MkdirAll support path in extended-length form
Calling MkdirAll on paths in extended-length form (\\?\-prefixed)
failed.
MkdirAll calls itself recursively with parent directory of given path in
its parameter. It finds parent directory by looking for delimiter in
the path, and taking the left part. When path is in extended-length form,
it finds empty path at the end.
Here is a sample of path in extended-length form:
\\?\c:\foo\bar
This change fixes that by passing trailing path separator to MkdirAll (so
it works for path like \\?\c:\).
Fixes #22230
Change-Id: I363660b262588c5382ea829773d3b6005ab8df3c
---
M src/os/path.go
M src/os/path_windows_test.go
2 files changed, 34 insertions(+), 2 deletions(-)
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Mansour Rahimi (Gerrit)
Mansour Rahimi uploaded patch set #13 to this change.
os: make MkdirAll support path in extended-length form
Calling MkdirAll on paths in extended-length form (\\?\-prefixed)
failed.
MkdirAll calls itself recursively with parent directory of given path in
its parameter. It finds parent directory by looking for delimiter in
the path, and taking the left part. When path is in extended-length form,
it finds empty path at the end.
Here is a sample of path in extended-length form:
\\?\c:\foo\bar
This change fixes that by passing trailing path separator to MkdirAll (so
it works for path like \\?\c:\).
Fixes #22230
Change-Id: I363660b262588c5382ea829773d3b6005ab8df3c
---
M src/os/path.go
M src/os/path_windows_test.go
2 files changed, 34 insertions(+), 2 deletions(-)
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
1 comment:
Patch Set #10, Line 45:
err = MkdirAll(path[0:j], perm)
Ian, this LGTM. […]
I'm OK with that for 1.11.
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
LGTM
Thank you very much.
Alex
Patch set 13:Run-TryBot +1Code-Review +2
Gobot Gobot (Gerrit)
TryBots beginning. Status page: https://farmer.golang.org/try?commit=0ddec525
Gobot Gobot (Gerrit)
TryBots are happy.
Patch set 13:TryBot-Result +1
Alex Brainman (Gerrit)
Alex Brainman merged this change by Mansour Rahimi.
os: make MkdirAll support path in extended-length form
Calling MkdirAll on paths in extended-length form (\\?\-prefixed)
failed.
MkdirAll calls itself recursively with parent directory of given path in
its parameter. It finds parent directory by looking for delimiter in
the path, and taking the left part. When path is in extended-length form,
it finds empty path at the end.
Here is a sample of path in extended-length form:
\\?\c:\foo\bar
This change fixes that by passing trailing path separator to MkdirAll (so
it works for path like \\?\c:\).
Fixes #22230
Change-Id: I363660b262588c5382ea829773d3b6005ab8df3c
Reviewed-on: https://go-review.googlesource.com/86295
Reviewed-by: Alex Brainman <alex.b...@gmail.com>
Run-TryBot: Alex Brainman <alex.b...@gmail.com>
TryBot-Result: Gobot Gobot <go...@golang.org>
---
M src/os/path.go
M src/os/path_windows_test.go
2 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/src/os/path.go b/src/os/path.go
index eb996e5..ec6a793 100644
--- a/src/os/path.go
+++ b/src/os/path.go
@@ -39,8 +39,10 @@
}
if j > 1 {
- // Create parent
- err = MkdirAll(path[0:j-1], perm)
+ // Create parent.
+ // Pass trailing path separator to MkdirAll, so our
+ // algorithm works for paths, like \\?\c:\
+ err = MkdirAll(path[0:j], perm)
if err != nil {
return err
}
diff --git a/src/os/path_windows_test.go b/src/os/path_windows_test.go
index cce0bdd..00a3e63 100644
--- a/src/os/path_windows_test.go
+++ b/src/os/path_windows_test.go
@@ -5,8 +5,10 @@
package os_test
import (
+ "io/ioutil"
"os"
"strings"
+ "syscall"
"testing"
)
@@ -44,3 +46,31 @@
}
}
}
+
+func TestMkdirAllExtendedLength(t *testing.T) {
+ tmpDir, err := ioutil.TempDir("", "TestMkdirAllExtendedLength")
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer os.RemoveAll(tmpDir)
+
+ const prefix = `\\?\`
+ if len(tmpDir) < 4 || tmpDir[:4] != prefix {
+ fullPath, err := syscall.FullPath(tmpDir)
+ if err != nil {
+ t.Fatalf("FullPath(%q) fails: %v", tmpDir, err)
+ }
+ tmpDir = prefix + fullPath
+ }
+ path := tmpDir + `\dir\`
+ err = os.MkdirAll(path, 0777)
+ if err != nil {
+ t.Fatalf("MkdirAll(%q) failed: %v", path, err)
+ }
+
+ path = path + `.\dir2`
+ err = os.MkdirAll(path, 0777)
+ if err == nil {
+ t.Fatalf("MkdirAll(%q) should have failed, but did not", path)
+ }
+}
To view, visit change 86295. To unsubscribe, or for help writing mail filters, visit settings.