code review 4440057: os/user: new package to look up users

os/user: new package to look up users

Only for Unix presently. Other operating systems
are stubbed out, as well as arm (lacks cgo).

[bradfitz, 2011-04-20 23:34:08]

Hello rsc, r (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-04-20 23:50:05]

Russ,

Please review my cgo stuff thoroughly. It passes my tests, but it might be
getting lucky.

I couldn't find docs on a lot of the cgo (C.CString, C.GoString, etc) so I
was just copying bits of pieces from various places and looking at the
generated code.

On Wed, Apr 20, 2011 at 4:34 PM, <bradfitz@golang.org> wrote:

> Reviewers: rsc, r,
>
> Message:
> Hello rsc, r (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> os/user: new package to look up users
>
> Only for Unix presently. Other operating systems
> are stubbed out, as well as arm (lacks cgo).
>
> Please review this at http://codereview.appspot.com/4440057/
>
> Affected files:
>  M src/pkg/Makefile
>  A src/pkg/os/user/Makefile
>  A src/pkg/os/user/lookup_stubs.go
>  A src/pkg/os/user/lookup_unix.go
>  A src/pkg/os/user/user.go
>  A src/pkg/os/user/user_test.go
>
>
>

[bradfitz, 2011-04-21 19:19:14]

Hello rsc, r (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-04-21 19:19:30]

*** Abandoned ***

[bradfitz, 2011-04-21 19:22:41]

PTAL (not abandoned)

This now works on linux and darwin.  Will test freebsd next.

[bradfitz, 2011-04-21 19:38:28]

Hello rsc, r (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-04-21 19:39:18]

Works on freebsd, darwin and linux now.

This is another reminder why I prefer Go to C.

On Thu, Apr 21, 2011 at 12:38 PM, <bradfitz@golang.org> wrote:

> Hello rsc, r (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/4440057/
>

[r, 2011-04-21 20:39:32]

seems fine but i am not a cgo adept

http://codereview.appspot.com/4440057/diff/11001/src/pkg/os/user/lookup_unix.go
File src/pkg/os/user/lookup_unix.go (right):

http://codereview.appspot.com/4440057/diff/11001/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:28: // LookupId looks up a user by username. If
the user isn't found,
s/isn't/cannot be/ here and below

http://codereview.appspot.com/4440057/diff/11001/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:29: // os.ENOENT is returned.
is this overspecifying the error return?  we usually don't document so
specifically.

http://codereview.appspot.com/4440057/diff/11001/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:53: if bufSize <= 0 || bufSize > 1<<20 {
this test can go up a line

http://codereview.appspot.com/4440057/diff/11001/src/pkg/os/user/user.go
File src/pkg/os/user/user.go (right):

http://codereview.appspot.com/4440057/diff/11001/src/pkg/os/user/user.go#newc...
src/pkg/os/user/user.go:8: // User is a user account.
s/is/represents/

[bradfitz, 2011-04-21 21:45:16]

Hello rsc, r (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitzgoog, 2011-04-21 21:45:42]

Done, except removing ENOENT.  I thought we did specify that one in
particular in places.

PTAL.


On Thu, Apr 21, 2011 at 1:39 PM, <r@golang.org> wrote:

> seems fine but i am not a cgo adept
>
>
>
>
http://codereview.appspot.com/4440057/diff/11001/src/pkg/os/user/lookup_unix.go
> File src/pkg/os/user/lookup_unix.go (right):
>
>
>
http://codereview.appspot.com/4440057/diff/11001/src/pkg/os/user/lookup_unix....
> src/pkg/os/user/lookup_unix.go:28: // LookupId looks up a user by
> username. If the user isn't found,
> s/isn't/cannot be/ here and below
>
>
>
http://codereview.appspot.com/4440057/diff/11001/src/pkg/os/user/lookup_unix....
> src/pkg/os/user/lookup_unix.go:29: // os.ENOENT is returned.
> is this overspecifying the error return?  we usually don't document so
> specifically.
>
>
>
http://codereview.appspot.com/4440057/diff/11001/src/pkg/os/user/lookup_unix....
> src/pkg/os/user/lookup_unix.go:53: if bufSize <= 0 || bufSize > 1<<20 {
> this test can go up a line
>
> http://codereview.appspot.com/4440057/diff/11001/src/pkg/os/user/user.go
> File src/pkg/os/user/user.go (right):
>
>
>
http://codereview.appspot.com/4440057/diff/11001/src/pkg/os/user/user.go#newc...
> src/pkg/os/user/user.go:8: // User is a user account.
> s/is/represents/
>
>
> http://codereview.appspot.com/4440057/
>

[rsc, 2011-04-21 21:48:48]

On Thu, Apr 21, 2011 at 17:45, Brad Fitzpatrick <bradfitz@google.com> wrote:
> Done, except removing ENOENT.  I thought we did specify that one in
> particular in places.

I'm sure we don't, because it's much more useful to get
    "/etc/passwd" file not found
than
    file not found

It's the canonical reason os.Error is an interface.

Russ

[rsc, 2011-04-21 21:53:08]

http://codereview.appspot.com/4440057/diff/9008/src/pkg/os/user/lookup_unix.go
File src/pkg/os/user/lookup_unix.go (right):

http://codereview.appspot.com/4440057/diff/9008/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:29: // os.ENOENT is returned.
See previous mail.  This is exactly what error types are for.

type UnknownUsernameError string

func (e UnknownUsernameError) String() string { return "unknown username " +
string(e) }

That will format as "unknown username bradfitz"
(very helpful) instead of "no such file or directory"
(arguably wrong, certainly confusing).

http://codereview.appspot.com/4440057/diff/9008/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:35: // os.ENOENT is returned.
same

[rsc, 2011-04-21 22:00:36]

http://codereview.appspot.com/4440057/diff/9008/src/pkg/os/user/lookup_unix.go
File src/pkg/os/user/lookup_unix.go (right):

http://codereview.appspot.com/4440057/diff/9008/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:78: return nil, fmt.Errorf("user: getpwuid_r
returned %d", rv)
rv is, at least according to the man page I found, an error number.

this test should be inside the if/else so it can blame
the right function

return nil, fmt.Errorf("user: lookup %s: %s", username, os.Errno(rv))

etc

http://codereview.appspot.com/4440057/diff/9008/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:81: return nil, os.ENOENT
this will probably have to move inside the if to get a
good error too.

http://codereview.appspot.com/4440057/diff/9008/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:88: HomeDirectory: C.GoString(pwd.pw_dir),
we're not super consistent but Dir is more common, so HomeDir.

http://codereview.appspot.com/4440057/diff/9008/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:91: // says: "It is expected to be a comma
separated list of
s/says/say/

http://codereview.appspot.com/4440057/diff/9008/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:94: if strings.Contains(u.Name, ",") {
if i := strings.Index(u.Name, ","); i >= 0 {
    u.Name = u.Name[:i]
}

[bradfitz, 2011-04-21 23:00:25]

http://codereview.appspot.com/4440057/diff/9008/src/pkg/os/user/lookup_unix.go
File src/pkg/os/user/lookup_unix.go (right):

http://codereview.appspot.com/4440057/diff/9008/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:29: // os.ENOENT is returned.
On 2011/04/21 21:53:08, rsc wrote:
> See previous mail.  This is exactly what error types are for.
> 
> type UnknownUsernameError string
> 
> func (e UnknownUsernameError) String() string { return "unknown username " +
> string(e) }
> 
> That will format as "unknown username bradfitz"
> (very helpful) instead of "no such file or directory"
> (arguably wrong, certainly confusing).

Done.

http://codereview.appspot.com/4440057/diff/9008/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:35: // os.ENOENT is returned.
On 2011/04/21 21:53:08, rsc wrote:
> same

Done.

http://codereview.appspot.com/4440057/diff/9008/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:78: return nil, fmt.Errorf("user: getpwuid_r
returned %d", rv)
On 2011/04/21 22:00:36, rsc wrote:
> rv is, at least according to the man page I found, an error number.
> 
> this test should be inside the if/else so it can blame
> the right function
> 
> return nil, fmt.Errorf("user: lookup %s: %s", username, os.Errno(rv))
> 
> etc

Done.

http://codereview.appspot.com/4440057/diff/9008/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:81: return nil, os.ENOENT
On 2011/04/21 22:00:36, rsc wrote:
> this will probably have to move inside the if to get a
> good error too.

Done.

http://codereview.appspot.com/4440057/diff/9008/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:88: HomeDirectory: C.GoString(pwd.pw_dir),
On 2011/04/21 22:00:36, rsc wrote:
> we're not super consistent but Dir is more common, so HomeDir.

Done.

http://codereview.appspot.com/4440057/diff/9008/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:91: // says: "It is expected to be a comma
separated list of
On 2011/04/21 22:00:36, rsc wrote:
> s/says/say/

Done.

http://codereview.appspot.com/4440057/diff/9008/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:94: if strings.Contains(u.Name, ",") {
On 2011/04/21 22:00:36, rsc wrote:
> if i := strings.Index(u.Name, ","); i >= 0 {
>     u.Name = u.Name[:i]
> }

Done.

[bradfitz, 2011-04-21 23:00:32]

Hello rsc, r, bradfitzwork (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-04-22 16:18:24]

LGTM

http://codereview.appspot.com/4440057/diff/16001/src/pkg/os/user/lookup_unix.go
File src/pkg/os/user/lookup_unix.go (right):

http://codereview.appspot.com/4440057/diff/16001/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:29: // UnknownUserError is returned.
// the returned error is of type UnknownUserError.

http://codereview.appspot.com/4440057/diff/16001/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:35: // UnknownUserIdError is returned.
// the returned error is of type UnknownUserIdError.

[bradfitz, 2011-04-22 16:30:35]

*** Submitted as http://code.google.com/p/go/source/detail?r=544094ecf115 ***

os/user: new package to look up users

Only for Unix presently. Other operating systems
are stubbed out, as well as arm (lacks cgo).

R=rsc, r, bradfitzwork
CC=golang-dev
http://codereview.appspot.com/4440057