golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.29k stars 17.7k forks source link

proposal: os/v2: Chown expects int, but os/user uses strings #6495

Open gopherbot opened 11 years ago

gopherbot commented 11 years ago
[Noting down confusion from IRC]

Chown takes ints, os/user Lookup returns strings. If someone wants to "chown jdoe
file", they're supposed to have platform-specific code to strconv from os/lookup
strings to ints?

I know backwards-compatibility means changes are hard, but this API does not seem ideal.

func Chown(name string, uid, gid int) error
http://golang.org/pkg/os/#Chown

type User struct { Uid string; ... }
http://golang.org/pkg/os/user/#User

I understand different platforms do different things -- but surely, if a platform has
Chown that operates on ints, surely it should have an API that produces ints too. Or a
cross-platform User abstraction that can be passed to Chown (where Chown exists).
bradfitz commented 11 years ago

Comment 1:

I agree this is not ideal.  But we can't change this until Go 2.x.
Marking long-term.

Labels changed: added go2, removed priority-triage.

Status changed to LongTerm.

gopherbot commented 11 years ago

Comment 2:

Also, how would one do this on Windows? It uses the file_posix.go implementation of
Chown (does not have an all-errors stub like plan9), but on Windows, os/user contains
SIDs, not stringified ints. It seems a chown call is not possible on Windows without
syscall.
alexbrainman commented 11 years ago

Comment 3:

What are you trying to do? Chown is Unix API. What is the task you are trying to
accomplish on Windows?
Alex
gopherbot commented 11 years ago

Comment 4:

Not sure what chown does on Windows, but consider:
1. Windows claims POSIX compatibility
2. The implementation of os.Chown http://golang.org/src/pkg/os/file_posix.go#L98
   is explicitly used on Windows: http://golang.org/src/pkg/os/file_posix.go#L5
   -- that is, it's not a stub like on Plan 9:
   http://golang.org/src/pkg/os/file_plan9.go#L408
alexbrainman commented 11 years ago

Comment 5:

> ... 1. Windows claims POSIX compatibility
You should take it up with Microsoft.
> 2. The implementation of os.Chown http://golang.org/src/pkg/os/file_posix.go#L98
>    is explicitly used on Windows: http://golang.org/src/pkg/os/file_posix.go#L5
>    -- that is, it's not a stub like on Plan 9:
>    http://golang.org/src/pkg/os/file_plan9.go#L408
Oh, it is the same as plan9:
http://golang.org/src/pkg/syscall/syscall_windows.go#L901
And you didn't tell us what you are trying to accomplish with Chown on windows. I don't
see us doing anything until we have some plan.
Alex
rsc commented 11 years ago

Comment 6:

Labels changed: added go1.3maybe.

rsc commented 10 years ago

Comment 7:

Labels changed: added release-none, removed go1.3maybe.

rsc commented 10 years ago

Comment 8:

Labels changed: added repo-main.

rsc commented 7 years ago

cf #8537

ianlancetaylor commented 6 years ago

The os package does provide Getuid and Getgid functions that return int (currently, at least), and it seems reasonable that those functions should work well with Chown. So it seems that we shouldn't change Chown. Note that all these functions are Unix-specific and do not work on Windows.

So the question is whether we should add a conversion function from the os/user package representation (string) to the os package representation (int). And, if so, where should that function live.

Perhaps the os/user package should have Unix-specific functions UnixIDFromString and StringFromUnixID that do the conversion between the os/user types and the os types.

It's not clear whether there is any corresponding requirement on Windows.

bradfitz commented 6 years ago

Rather than focus on how to fix this particular mismatch, I'd rethink the whole os package. There's a ton of weird C/Unix junk in there that does feel like Go and doesn't work nicely on Windows.

I mean, Chown lets you change the ownership, but there's nothing in the standard library to even get the ownership (easily). You have to type-assert the os.FileInfo interface into a non-portable *syscall.Stat_t which is never documented in the os package.

bradfitz commented 5 years ago

os/user was designed to work with Windows, hence the strings. But os.Chown etc return ErrWindows on Windows.

I'm thinking we should create new interface types for OS users and groups (similar to os.Signal), and then concrete implementations with int & string underlying types, something like:

package xxx

type User interface{
    unexported()
} 

type UserID int
func (UserID) unexported() {}

type UserName string
func (UserName) unexported() {}

And then use the xxx.User as the type for os/user and os.Chown/etc.

wade-welles commented 4 years ago

Seems weird to be forced to make string comparisons when:

// On Windows, it returns -1.
func Getuid() int { return syscall.Getuid() }

// Geteuid returns the numeric effective user id of the caller.
//
// On Windows, it returns -1.
func Geteuid() int { return syscall.Geteuid() }

You are really going to cripple everyone but windows for the sake of windows bad choices? Just make a separate library for windows and leave the standard library efficient as possible.