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: make os.OpenFile() 'perm' argument variadic to match POSIX `open` signature #35647

Open
Binary-Eater opened this issue Nov 17, 2019 · 5 comments
Milestone

Comments

@Binary-Eater
Copy link

@Binary-Eater Binary-Eater commented Nov 17, 2019

What are you proposing?

The os.OpenFile interface takes perm FileMode as a required argument. Since perm is only relevant in os.O_CREATE (basing this statement off of POSIX open), it would make more sense for it to be a variadic/optional argument with a signature like the one below.

func OpenFile(name string, flag int, perm ...FileMode) (*File, error)

Why are you proposing this?

SYNOPSIS
       #include <sys/stat.h>
       #include <fcntl.h>

       int open(const char *path, int oflag, ...);
       int openat(int fd, const char *path, int oflag, ...);

The proposed change to the os.OpenFile function signature will make it resemble the POSIX function signature for the open system call.

@ALTree

This comment has been minimized.

Copy link
Member

@ALTree ALTree commented Nov 17, 2019

Thanks for the suggestion.

This would break backward compatibility, so it's not something that can be done for Go1.

https://golang.org/doc/go1compat

It is intended that programs written to the Go 1 specification will continue to compile and run correctly, unchanged, over the lifetime of that specification.

It may be considered for Go2, I guess.

@ALTree ALTree added this to the Unplanned milestone Nov 17, 2019
@ALTree ALTree added the Go2 label Nov 17, 2019
@deanveloper

This comment has been minimized.

Copy link

@deanveloper deanveloper commented Nov 17, 2019

I do not like this idea, personally. Variadic arguments in C are strictly seen as "additional arguments" and really don't have any additional meaning. This means that variadic arguments in C are used sometimes to express optional arguments, as they are with open.

In Go, variadic arguments are represented as a slice, which brings the convention that many arguments should be acceptable as a valid input. In this case, we would only want a single FileMode argument to be passed as a bitflag (if we want to keep it close to the POSIX syscall).

I think requiring the bitflag is a bit more "loyal" to the original syscall than allowing multiple FileModes to be passed.

@Binary-Eater

This comment has been minimized.

Copy link
Author

@Binary-Eater Binary-Eater commented Nov 17, 2019

@deanveloper doesn't the POSIX system call itself theoretically allow for multiple mode params to be passed though only the first one has any meaning?

For example, the following compiles and runs on Linux 5.3.10-arch1-1 with gcc 9.2.0 with glibc 2.30.

#include <fcntl.h>
#include <sys/stat.h>
#include <unistd.h>

int main(int argc, char *argv[]) {
  int fd = open(
                "/tmp/test.txt",
                O_CREAT|O_WRONLY|O_TRUNC,
                S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH,
                S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH,
                S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH,
                S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH,
                S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH
           );
  if (fd != -1)
  {
    close(fd);
  }
  return 0;
}

I noticed that the interfaces for open differ when viewing man -S2 open versus man -S3 open. What I assume the Linux manual page -S2 is implying is that the two signatures listed are the only two of any meaning so any argument after mode has no meaning. However, the POSIX manual page -S3 intends to leave the arguments after oflags as variadic hence the actual interface has variadic arguments that are ignored. Feel free to correct me if my understanding is mistaken.

@Binary-Eater

This comment has been minimized.

Copy link
Author

@Binary-Eater Binary-Eater commented Nov 18, 2019

@ALTree I wanted to ask about the 'backward compatibility' requirement mentioned in the link you provided for Go1.

compile and run correctly, unchanged

Does unchanged include the way parameters are passed too?

If users invoke os.OpenFile like below, this change shouldn't affect previous code from compiling, but perm the user provided would be passed into a slice with the change.

os.OpenFile("notes.txt", os.O_RDWR|os.O_CREATE, 0755)

Compatibility is at the source level. Binary compatibility for
compiled packages is not guaranteed between releases. After a point
release, Go source will need to be recompiled to link against the
new release.

For the line Compatibility is at the source level., I guess its ensuring that function signatures remain the same for provided interfaces even if a change to the signature would not affect backwards compilation?

Thanks.

@deanveloper

This comment has been minimized.

Copy link

@deanveloper deanveloper commented Nov 18, 2019

I noticed that the interfaces for open differ when viewing man -S2 open versus man -S3 open. What I assume the Linux manual page -S2 is implying is that the two signatures listed are the only two of any meaning so any argument after mode has no meaning. However, the POSIX manual page -S3 intends to leave the arguments after oflags as variadic hence the actual interface has variadic arguments that are ignored. Feel free to correct me if my understanding is mistaken.

You are not mistaken, that's pretty much spot on IIRC.

Note that if you supply any more than one argument for the variadic arguments, they will be ignored, and it's not really logical for them to be anything but that. This is because C does not specify a length for it's variadic arguments, just as it does with arrays. The way it can even tell that you supplied a variadic argument is if you specified the O_CREAT flag in the oflag argument. If you specified that flag, it will pull a single element from the va_list and uses that as the mode flag in the syscall.

That's getting a bit off topic though. Getting back to the proposal - I personally think it's better to leave OpenFile as-is, however I can see the justification for making it variadic.

For the line Compatibility is at the source level., I guess its ensuring that function signatures remain the same for provided interfaces even if a change to the signature would not affect backwards compilation?

var x func(string, int, os.FileMode) (*os.File, error) = os.Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.