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.
>