code review 4357052: os: New Open API.

os: New Open API. We replace the current Open with: OpenFile(name, flag, perm) // same as old Open Open(name) // same as old Open(name, O_RDONLY, 0) Create(name) // same as old Open(name, O_RDWR|O_TRUNC|O_CREAT, 0666) This CL includes a gofix module and full code updates: all.bash passes. (There may be a few comments I missed.) The interesting packages are: gofix os Everything else is automatically generated except for hand tweaks to: src/pkg/io/ioutil/ioutil.go src/pkg/io/ioutil/tempfile.go src/pkg/crypto/tls/generate_cert.go src/cmd/goyacc/goyacc.go src/cmd/goyacc/units.y

[r, 2011-04-04 22:14:13]

Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/

[bradfitzgoog, 2011-04-04 22:17:05]

http://codereview.appspot.com/4357052/diff/1/src/pkg/os/file_windows.go File src/pkg/os/file_windows.go (right): http://codereview.appspot.com/4357052/diff/1/src/pkg/os/file_windows.go#newco... src/pkg/os/file_windows.go:150: if e == syscall.ERROR_NOMORE_FILES { I think this was a mistake.

[rsc, 2011-04-04 22:45:44]

I stopped reading after misc/goplay/goplay.go. Please add that as a test case. http://codereview.appspot.com/4357052/diff/1/misc/goplay/goplay.go File misc/goplay/goplay.go (right): http://codereview.appspot.com/4357052/diff/1/misc/goplay/goplay.go#newcode86 misc/goplay/goplay.go:86: f, err := os.OpenFile(src, os.O_CREAT|os.O_WRONLY|os.O_TRUNC, 0666) I think gofix should rewrite this to os.Create. http://codereview.appspot.com/4357052/diff/1/src/cmd/gofix/osopen.go File src/cmd/gofix/osopen.go (right): http://codereview.appspot.com/4357052/diff/1/src/cmd/gofix/osopen.go#newcode9 src/cmd/gofix/osopen.go:9: //	"go/token" delete http://codereview.appspot.com/4357052/diff/1/src/cmd/gofix/osopen.go#newcode17 src/cmd/gofix/osopen.go:17: TODO http://codereview.appspot.com/4245064 Hijacker can fill in now http://codereview.appspot.com/4357052/diff/1/src/cmd/gofix/osopen.go#newcode50 src/cmd/gofix/osopen.go:50: call.Fun = &ast.SelectorExpr{sel.X, &ast.Ident{ident.Pos(), "Create", ident.Obj}} This is fine but so is sel.Sel.Name = "Create" http://codereview.appspot.com/4357052/diff/1/src/cmd/gofix/osopen.go#newcode84 src/cmd/gofix/osopen.go:84: return isPkgDot(flag, "os", "O_CREAT") || isPkgDot(flag, "os", "O_CREATE")

[rsc, 2011-04-04 22:52:21]

Also, it may be worth warning (call warn) about calls to Open with O_CREAT that are missing O_TRUNC. 9 times out of 10 they are bugs so we should rewrite them to os.Create, but it's worth printing a warning at the same time. warn(p, "rewrote os.Open with O_CREATE but not O_TRUNC to os.Create") 

[r2, 2011-04-04 22:54:14]

 On Apr 4, 2011, at 3:45 PM, rsc@golang.org wrote: > I stopped reading after misc/goplay/goplay.go. > Please add that as a test case. > I believe it does, but that misc was missed because it's not under src. I rewrote it once in an earlier pass. Will verify when I get back to that machine. > http://codereview.appspot.com/4357052/diff/1/misc/goplay/goplay.go > File misc/goplay/goplay.go (right): > > http://codereview.appspot.com/4357052/diff/1/misc/goplay/goplay.go#newcode86 > misc/goplay/goplay.go:86: f, err := os.OpenFile(src, > os.O_CREAT|os.O_WRONLY|os.O_TRUNC, 0666) > I think gofix should rewrite this to os.Create. > > http://codereview.appspot.com/4357052/diff/1/src/cmd/gofix/osopen.go > File src/cmd/gofix/osopen.go (right): > > http://codereview.appspot.com/4357052/diff/1/src/cmd/gofix/osopen.go#newcode9 > src/cmd/gofix/osopen.go:9: //	"go/token" > delete > > http://codereview.appspot.com/4357052/diff/1/src/cmd/gofix/osopen.go#newcode17 > src/cmd/gofix/osopen.go:17: TODO http://codereview.appspot.com/4245064 > Hijacker > can fill in now > > http://codereview.appspot.com/4357052/diff/1/src/cmd/gofix/osopen.go#newcode50 > src/cmd/gofix/osopen.go:50: call.Fun = &ast.SelectorExpr{sel.X, > &ast.Ident{ident.Pos(), "Create", ident.Obj}} > This is fine but so is > > sel.Sel.Name = "Create" Really? That seems unsafe - shared state and all that. Where does the ast docs say I can rewrite a node like that? > http://codereview.appspot.com/4357052/diff/1/src/cmd/gofix/osopen.go#newcode84 > src/cmd/gofix/osopen.go:84: return isPkgDot(flag, "os", "O_CREAT") > || isPkgDot(flag, "os", "O_CREATE") > > http://codereview.appspot.com/4357052/ 

[rsc, 2011-04-04 22:56:11]

> Really?  That seems unsafe - shared state and all that.  Where does the ast docs say I can rewrite a node like that? The AST is a tree, not a dag. There can't be any cross linking of expression nodes, because each node contains its position within the source text. The docs could probably be more clear, yes, but you are already rewriting the AST in that function. I'm just suggesting a slightly lower-level rewrite. Russ 

[r, 2011-04-04 22:56:17]

http://codereview.appspot.com/4357052/diff/1/src/cmd/gofix/osopen.go File src/cmd/gofix/osopen.go (right): http://codereview.appspot.com/4357052/diff/1/src/cmd/gofix/osopen.go#newcode84 src/cmd/gofix/osopen.go:84: return isPkgDot(flag, "os", "O_CREAT") On 2011/04/04 22:45:44, rsc wrote: > || isPkgDot(flag, "os", "O_CREATE") aha. dammit.

[r2, 2011-04-04 22:57:13]

 On Apr 4, 2011, at 3:56 PM, Russ Cox wrote: >> Really? That seems unsafe - shared state and all that. Where does the ast docs say I can rewrite a node like that? > > The AST is a tree, not a dag. There can't be any cross linking > of expression nodes, because each node contains its > position within the source text. > > The docs could probably be more clear, yes, but you are > already rewriting the AST in that function. I'm just suggesting > a slightly lower-level rewrite. OK, happy to be more expedient. I was just being extra careful. -rob 

[r, 2011-04-05 04:53:36]

Hello golang-dev@googlegroups.com, bradfitzwork, rsc, r2 (cc: golang-dev@googlegroups.com), Please take another look.

[r2, 2011-04-05 04:54:54]

Updated and all comments addressed. I added a copule of warnings about dangerous conditions to gofix. -rob 

[r2, 2011-04-05 04:56:32]

New addendum to CL desc:	Everything else is automatically generated except for hand tweaks to:	src/pkg/io/ioutil/ioutil.go	src/pkg/io/ioutil/tempfile.go	src/pkg/crypto/tls/generate_cert.go	src/cmd/goyacc/goyacc.go	src/cmd/goyacc/units.y 

[rsc, 2011-04-05 05:15:04]

LGTM Well that's a win.

[r, 2011-04-05 06:42:19]

*** Submitted as http://code.google.com/p/go/source/detail?r=6c96d1022285 *** os: New Open API. We replace the current Open with: OpenFile(name, flag, perm) // same as old Open Open(name) // same as old Open(name, O_RDONLY, 0) Create(name) // same as old Open(name, O_RDWR|O_TRUNC|O_CREAT, 0666) This CL includes a gofix module and full code updates: all.bash passes. (There may be a few comments I missed.) The interesting packages are: gofix os Everything else is automatically generated except for hand tweaks to: src/pkg/io/ioutil/ioutil.go src/pkg/io/ioutil/tempfile.go src/pkg/crypto/tls/generate_cert.go src/cmd/goyacc/goyacc.go src/cmd/goyacc/units.y R=golang-dev, bradfitzwork, rsc, r2 CC=golang-dev http://codereview.appspot.com/4357052

[rog, 2011-04-05 08:40:58]

http://codereview.appspot.com/4357052/diff/11001/misc/dashboard/builder/exec.go File misc/dashboard/builder/exec.go (right): http://codereview.appspot.com/4357052/diff/11001/misc/dashboard/builder/exec.... misc/dashboard/builder/exec.go:52: f, err := os.Create(logfile) Did you really intend to lose the append semantics from this? http://codereview.appspot.com/4357052/diff/11001/src/pkg/crypto/tls/generate_... File src/pkg/crypto/tls/generate_cert.go (right): http://codereview.appspot.com/4357052/diff/11001/src/pkg/crypto/tls/generate_... src/pkg/crypto/tls/generate_cert.go:62: keyOut, err := os.OpenFile("key.pem", os.O_WRONLY|os.O_CREAT, 0600) |os.O_TRUNC ? http://codereview.appspot.com/4357052/diff/11001/src/pkg/os/inotify/inotify_l... File src/pkg/os/inotify/inotify_linux_test.go (right): http://codereview.appspot.com/4357052/diff/11001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux_test.go:54: _, err = os.OpenFile(testFile, os.O_WRONLY|os.O_CREATE, 0666) os.Create (BTW it looks like a bug that the file isn't removed afterwards)

[rsc, 2011-04-05 14:52:41]

> http://codereview.appspot.com/4357052/diff/11001/misc/dashboard/builder/exec.... > misc/dashboard/builder/exec.go:52: f, err := os.Create(logfile) > Did you really intend to lose the append semantics from this? Sent a CL. > http://codereview.appspot.com/4357052/diff/11001/src/pkg/crypto/tls/generate_... > File src/pkg/crypto/tls/generate_cert.go (right): > > http://codereview.appspot.com/4357052/diff/11001/src/pkg/crypto/tls/generate_... > src/pkg/crypto/tls/generate_cert.go:62: keyOut, err := > os.OpenFile("key.pem", os.O_WRONLY|os.O_CREAT, 0600) > |os.O_TRUNC ? Included in the CL. > http://codereview.appspot.com/4357052/diff/11001/src/pkg/os/inotify/inotify_l... > File src/pkg/os/inotify/inotify_linux_test.go (right): > > http://codereview.appspot.com/4357052/diff/11001/src/pkg/os/inotify/inotify_l... > src/pkg/os/inotify/inotify_linux_test.go:54: _, err = > os.OpenFile(testFile, os.O_WRONLY|os.O_CREATE, 0666) > os.Create > (BTW it looks like a bug that the file isn't removed afterwards) The test file is in _obj. It will get deleted during make clean. Russ 

[rog, 2011-04-05 15:08:23]

On 5 April 2011 15:52, Russ Cox <rsc@golang.org> wrote: >> (BTW it looks like a bug that the file isn't removed afterwards) > > The test file is in _obj.  It will get deleted during make clean. but if you run gotest twice in that directory i think the second time would fail. as far as i can tell, gotest doesn't remove _obj. if that's the case, i still think that the file should be explicitly deleted. 

[rsc, 2011-04-05 15:08:48]

okay; send a CL On Tue, Apr 5, 2011 at 11:08, roger peppe <rogpeppe@gmail.com> wrote: > On 5 April 2011 15:52, Russ Cox <rsc@golang.org> wrote: >>> (BTW it looks like a bug that the file isn't removed afterwards) >> >> The test file is in _obj.  It will get deleted during make clean. > > but if you run gotest twice in that directory i think the second > time would fail. as far as i can tell, gotest doesn't remove _obj. > > if that's the case, i still think that the file should be explicitly deleted. >