Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: inconsistent cross-platform behavior: os.OpenFile/os.Write* #21322

Closed
prasannavl opened this issue Aug 6, 2017 · 13 comments
Closed

os: inconsistent cross-platform behavior: os.OpenFile/os.Write* #21322

prasannavl opened this issue Aug 6, 2017 · 13 comments

Comments

@prasannavl
Copy link

@prasannavl prasannavl commented Aug 6, 2017

What version of Go are you using (go version)?

go version go1.8.3 windows/amd64
go version go1.8.3 linux/amd64

What operating system and processor architecture are you using (go env)?

Skipping as the details are covered below.

What did you do?

In Windows, the following program will happily end up writing to the file with no issues.

package main

import "os"
import "fmt"

func main() {
	flag := os.O_CREATE
	file, err := os.OpenFile("test.log", flag, os.FileMode(0644))
	if err != nil {
		fmt.Println(err)
	}
	defer file.Close()
	n, err := file.WriteString("Hello")
	if err != nil {
		fmt.Println(err)
	}
	fmt.Printf("Written: %v\r\n", n)
	fmt.Println("Good-bye.")
}

However, in linux this will fail to write with a bad file descriptor error, since there's no write permission that's given. The os.O_RDWR/WRONLY is required during OpenFile for the write to succeed.

Possible solutions

  1. Document it.
  2. Do run time checks to ensure consistent behavior.
@davecheney

This comment has been minimized.

Copy link
Contributor

@davecheney davecheney commented Aug 6, 2017

@prasannavl

This comment has been minimized.

Copy link
Author

@prasannavl prasannavl commented Aug 6, 2017

@davecheney, you can't, if you need to create the file it doesn't exist, or just append, which is quite common.. logging for instance.

@ALTree ALTree changed the title Inconsistent cross-platform behavior: os.OpenFile/os.Write* os: inconsistent cross-platform behavior: os.OpenFile/os.Write* Aug 6, 2017
@prasannavl

This comment has been minimized.

Copy link
Author

@prasannavl prasannavl commented Aug 6, 2017

Note: This is because, on Windows, the syscall package actually goes one step further here: https://github.com/golang/go/blob/master/src/syscall/syscall_windows.go#L269

To ensure that when APPEND flag is given, it also add the write perm. Should this behavior be added in Linux also? It make perfect sense semantically.

@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Aug 9, 2017
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 9, 2017

To save looking through the link, the issue is that on Windows syscall.Open sets the write flag for the Windows file when the mode includes the syscall.O_CREAT flag. This does not happen for syscall.Open on other operating systems.

We could change Windows, which could potentially break existing working programs on Windows.

We could change other systems in the syscall package. This would be annoying to code but would be unlikely to break anybody. It would remove a small and probably unimportant layer of protection from programs that use os.O_CREATE|os.O_EXCL, without os.O_WRONLY to get an unwriteable lock file.

We could make the similar change in the os package, which would be less annoying but have the same minor drawbacks.

We could simply document it.

We could ignore it.

@prasannavl

This comment has been minimized.

Copy link
Author

@prasannavl prasannavl commented Aug 28, 2017

I think a better way to handle this would be to take a page from how Rust does this. OpenFile doesn't take any arguments other than the path, with sensible defaults, and OpenOptions - a builder pattern that handles the OS specific ways.

I think this is far superior design. I find myself have to specify a sensible default for fileMode which is a completely useless parameter in Windows.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Aug 28, 2017

@ianlancetaylor I don't see anything sinister in any of your proposals.

I like your last suggestion (to ignore this issue) the best. Go on Windows does many things differently then on other OSes. And this is just one of them. I don't see why it is important for the program above to behave the same on different OSes. Last os.FileMode(0644) parameter is meaningless on Windows - why we should care about it?

Alex

@prasannavl

This comment has been minimized.

Copy link
Author

@prasannavl prasannavl commented Sep 1, 2017

I don't see why it is important for the program above to behave the same on different OSes

That's rather a saddening comment from a team member of a language which happens to have cross-platform as one of it's core values.

Last os.FileMode(0644) parameter is meaningless on Windows - why we should care about it?

Since it does nothing in windows, you can effectively ignore fileMode - and it's fairly common to develop on windows, and deploy on linux. (I have been doing this for as long as I can remember and now thanks to WSL - I barely ever use linux on desktops - and I suspect there would be a good number of us)

Now, you write os.OpenFile("somefile", os.O_APPEND | os.O_CREATE, 0) - It works perfectly on Windows. Deploy it on Linux, everything falls over silently (that's worse) - You'll get a file that will NEVER be written to. Despite being a static language, the lack of static enums and no modes in the std lib doesn't help the case.

Also, above point, again.

@mattn

This comment has been minimized.

Copy link
Member

@mattn mattn commented Sep 1, 2017

I prefer to simply document it. package os's purpose is not to hide every detail, just to provide a consistent interface.

https://groups.google.com/forum/#!msg/golang-dev/TlLZoZjJuCk/oQKXUkzGhoEJ

@prasannavl

This comment has been minimized.

Copy link
Author

@prasannavl prasannavl commented Sep 1, 2017

@mattn, I agree with the comment on the link and simply documenting it as an immediate resolution to the issue as well - I can take this up, and submit a CL in a few days when I get time.

That being said, I also wonder if chalking it as we cannot deal with every detail, is the best strategy here especially when a std lib apis like Rust's has clearly demonstrated a better (and simpler) design that covers both sides of the design. A builder pattern like that, will also end up removing a bunch of complicated flags case handling in windows and make the overall code nicer and more importantly predictable and intuitively correct - which are 2 things I find very important in std lib design.

I wonder if fileMode being a complete dummy parameter on windows, can even be considered a "consistent interface" here.

Besides, I wouldn't have a problem with this, if file handing was simply delegated to the OS on all the OS - which implies consistent and predictable behavior. But here, Go's std library doesn't do that as well. It tries to - neither here nor there - which makes things unpredictable and hard to determine.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Sep 8, 2017

That's rather a saddening comment from a team member of a language which happens to have cross-platform as one of it's core values.

Cross-platform is important, but there are other things that Go developers care about too.

Since it does nothing in windows, you can effectively ignore fileMode - and it's fairly common to develop on windows, and deploy on linux.

It would be nice if we could just deploy code on another OS and it will work. But I doubt it is possible without real testing on real OS.

I agree with the comment on the link and simply documenting it as an immediate resolution to the issue as well

I am not sure I like any new documentation here. What are you going to say? I don't see that documentation affecting many people, but everyone will have to read and comprehend new text.

I wonder if fileMode being a complete dummy parameter on windows, can even be considered a "consistent interface" here.

Besides, I wouldn't have a problem with this, if file handing was simply delegated to the OS on all the OS - which implies consistent and predictable behavior. But here, Go's std library doesn't do that as well. It tries to - neither here nor there - which makes things unpredictable and hard to determine.

You are talking about things that cannot be changed now. Maybe for Go2? Make suggestion for Go2, if you have some great ideas.

Alex

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 30, 2017

I do think that OpenFile and these flags are a bit too low-level, which is why we introduced os.Open and os.Create. It's possible we need another so that OpenFile is essentially never needed; that is, needed only when you really need low-level system flags. But that's not where we are today.

The current docs for the constants say:

Flags to OpenFile wrapping those of the underlying system. Not all flags may be implemented on a given system.

Let's add: "Each call to OpenFile should specify exactly one of O_RDONLY, O_WRONLY, or O_RDWR."

@rsc rsc added the NeedsFix label Oct 30, 2017
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 30, 2017

Change https://golang.org/cl/74550 mentions this issue: os: clarify that OpenFile reqires one of O_RDONLY/O_WRONLY/O_RDWR

@gopherbot gopherbot closed this in 94d9371 Oct 31, 2017
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 1, 2017

Change https://golang.org/cl/75250 mentions this issue: os: rearrange OpenFile Flags doc

gopherbot pushed a commit that referenced this issue Nov 1, 2017
Updates #21322

Change-Id: Ib03ee9dbe1b44c2fecd51f2f2c23a88482158e7e
Reviewed-on: https://go-review.googlesource.com/75250
Reviewed-by: Rob Pike <r@golang.org>
@golang golang locked and limited conversation to collaborators Nov 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.