code review 4521053: os/user: Windows support

os/user: Windows support

Fixes issue 1789.

[brainman, 2011-05-10 13:15:45]

It's a start. Lets try to make it as simple as possible.

Alex

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

http://codereview.appspot.com/4521053/diff/2001/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:17: type UserId int
I suggest you name it Uid, then users will refer to it as user.Uid, not
user.UserId from outside of package. Also, your your User struct will look like
this:

type User struct {
    Uid               // user id
    Gid               // primary group id
    Username string
    Name     string
    HomeDir  string
}

http://codereview.appspot.com/4521053/diff/2001/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:20: type GroupId int
Same, name it Gid.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:23: // User represents a user account in Unix
systems.
This can stay in user.go.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/mkerrors_wind...
File src/pkg/syscall/mkerrors_windows.sh (right):

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/mkerrors_wind...
src/pkg/syscall/mkerrors_windows.sh:181: printf("\tEWOULDBLOCK\n");
I have no idea why you've change this file. I don't think you need to change
even one line in this file.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
File src/pkg/syscall/syscall_windows.go (left):

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:130: //sys	GetComputerName(buf *uint16, n
*uint32) (errno int) = GetComputerNameW
Please use \t between sys and function names. You, probably, can't see
misalignment in your editor, but surely, you can see it here in codereview.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:134: //sys   LookupAccountName(systemName
*uint16, accountName *uint16, sid *uint8, sidSize *uint32, domainName *uint16,
domainNameSize *uint32, nameUse *int32) (errno int) =
advapi32.LookupAccountNameW
s/uint8/byte/
just to be like everything else.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:135: //sys   LookupAccountSid(systemName
*uint16, sid *uint8, username *uint16, usernameSize *uint32, domainName *uint16,
domainNameSize *uint32, nameUse *int32) (errno int) = advapi32.LookupAccountSidW
s/uint8/byte/

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:136: //sys   ConvertSidToStringSid(sid
*uint8, stringSid *uint32) (errno int) =  advapi32.ConvertSidToStringSidW
s/uint8/byte/
s/stringSid *uint32/stringSid **uint16/

from the manual:
StringSid
A pointer to a variable that receives a pointer to a null-terminated SID string.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:149: //sys   OpenProcessToken(pseudoHandle
int32, access uint32, token *int32) (errno int) = advapi32.OpenProcessToken
s/pseudoHandle int32/processHandle int32/
why pseudoHandle?

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:384: func UserName() (name string, errno int)
{
Please move this function to user package.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:385: // from Lmcons.h
func UserName() (name string, errno int) {
	b := make([]uint16, 40)
	n := uint32(len(b))
	e := GetUserName(&b[0], &n)
	if e == ERROR_INSUFFICIENT_BUFFER {
		b := make([]uint16, n)
		e = GetUserName(&b[0], &n)
	}
	if e != 0 {
		return "", e
	}
	return string(utf16.Decode(b[:n])), 0
}

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:397: type ExtendedNameFormat int32
Do not need this type at all, just use int or int32, whatever appropriate.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:399: const (
Move these consts to ztypes_windows.go

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:412: func UserNameEx(format
ExtendedNameFormat) (name string, errno int) {
Please move this function to user package.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:425: func UserProfileDirectory() (name
string, errno int) {
Please move this function to user package.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:455: type SID []uint8
s/uint8/byte/

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:459: type SID_NAME_USE int32
Do not need this type at all, just use int or int32, whatever appropriate.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:461: const (
Move these consts to ztypes_windows.go

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:476: func SidToStringSid(sid SID) (string,
int) {
Please move this function to user package.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:489: // The Window specific account
information
I'm not sure you need this struct, if you move all these functions into user
package, I suspect many things will get less verbose.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:499: func AccountName(name string) (*Account,
int) {
Please move this function to user package.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:512: e := LookupAccountName(nil,
&accountName[0], &sid[0], &sidSize, &domainName[0], &domainNameSize, &nameUse)
You shouldn't allocate huge buffers of 8192 for output parameters, but be
reasonable - 100. You should make buffer bigger and call second time if first
buffer is not big enough.

From the manual:
If the function fails because the buffer is too small, this variable receives
the required buffer size, including the terminating null character.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:527: func AccountSid(sid SID) (*Account, int)
{
Please move this function to user package.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/zerrors_windo...
File src/pkg/syscall/zerrors_windows_386.go (right):

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/zerrors_windo...
src/pkg/syscall/zerrors_windows_386.go:6: // Go names for Windows errors.
I don't know why this file is changed. This file is generated automatically by
running ./mkall.sh with GOOS=windows and GOARCH=386. Even assuming all your
changes in mkerrors_windows.sh are needed, this file would look very
differently. I don't think you need to change one line in this file.

[pjmlp, 2011-05-11 09:30:26]

Hi,

here are my comments. Please let me know your view on them, so that I can
republish my changes.

On a side note I find Go's code has many single letter variables and magic
numbers, but I try to follow the conventions in place. Please let me know if
there is also something there to change.

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

http://codereview.appspot.com/4521053/diff/2001/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:17: type UserId int
On 2011/05/10 13:15:45, brainman wrote:
> I suggest you name it Uid, then users will refer to it as user.Uid, not
> user.UserId from outside of package. Also, your your User struct will look
like
> this:
> 
> type User struct {
>     Uid               // user id
>     Gid               // primary group id
>     Username string
>     Name     string
>     HomeDir  string
> }

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:20: type GroupId int
On 2011/05/10 13:15:45, brainman wrote:
> Same, name it Gid.

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:23: // User represents a user account in Unix
systems.
On 2011/05/10 13:15:45, brainman wrote:
> This can stay in user.go.

I cannot, because Uid and Gid fields are os specific, as such user.go won't
compile.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/mkerrors_wind...
File src/pkg/syscall/mkerrors_windows.sh (right):

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/mkerrors_wind...
src/pkg/syscall/mkerrors_windows.sh:181: printf("\tEWOULDBLOCK\n");
On 2011/05/10 13:15:45, brainman wrote:
> I have no idea why you've change this file. I don't think you need to change
> even one line in this file.

These hacks are required when compiling on Windows. Somehow the errno.h file
provided by mingw does not provide the same set of error values as in Linux.
Leading to a compile failure of the syscall package after calling mkall.sh.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
File src/pkg/syscall/syscall_windows.go (left):

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:130: //sys	GetComputerName(buf *uint16, n
*uint32) (errno int) = GetComputerNameW
On 2011/05/10 13:15:45, brainman wrote:
> Please use \t between sys and function names. You, probably, can't see
> misalignment in your editor, but surely, you can see it here in codereview.

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:134: //sys   LookupAccountName(systemName
*uint16, accountName *uint16, sid *uint8, sidSize *uint32, domainName *uint16,
domainNameSize *uint32, nameUse *int32) (errno int) =
advapi32.LookupAccountNameW
On 2011/05/10 13:15:45, brainman wrote:
> s/uint8/byte/
> just to be like everything else.

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:135: //sys   LookupAccountSid(systemName
*uint16, sid *uint8, username *uint16, usernameSize *uint32, domainName *uint16,
domainNameSize *uint32, nameUse *int32) (errno int) = advapi32.LookupAccountSidW
On 2011/05/10 13:15:45, brainman wrote:
> s/uint8/byte/

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:135: //sys   LookupAccountSid(systemName
*uint16, sid *uint8, username *uint16, usernameSize *uint32, domainName *uint16,
domainNameSize *uint32, nameUse *int32) (errno int) = advapi32.LookupAccountSidW
On 2011/05/10 13:15:45, brainman wrote:
> s/uint8/byte/

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:135: //sys   LookupAccountSid(systemName
*uint16, sid *uint8, username *uint16, usernameSize *uint32, domainName *uint16,
domainNameSize *uint32, nameUse *int32) (errno int) = advapi32.LookupAccountSidW
On 2011/05/10 13:15:45, brainman wrote:
> s/uint8/byte/

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:136: //sys   ConvertSidToStringSid(sid
*uint8, stringSid *uint32) (errno int) =  advapi32.ConvertSidToStringSidW
On 2011/05/10 13:15:45, brainman wrote:
> s/uint8/byte/
> s/stringSid *uint32/stringSid **uint16/
> 
> from the manual:
> StringSid
> A pointer to a variable that receives a pointer to a null-terminated SID
string.

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:136: //sys   ConvertSidToStringSid(sid
*uint8, stringSid *uint32) (errno int) =  advapi32.ConvertSidToStringSidW
On 2011/05/10 13:15:45, brainman wrote:
> s/uint8/byte/
> s/stringSid *uint32/stringSid **uint16/
> 
> from the manual:
> StringSid
> A pointer to a variable that receives a pointer to a null-terminated SID
string.

stringSid kept as *uint32. I was following LocalFree declaration. As this string
needs to be freed by it.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:149: //sys   OpenProcessToken(pseudoHandle
int32, access uint32, token *int32) (errno int) = advapi32.OpenProcessToken
On 2011/05/10 13:15:45, brainman wrote:
> s/pseudoHandle int32/processHandle int32/
> why pseudoHandle?

Temporary name I forget to properly fix afterwards.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:384: func UserName() (name string, errno int)
{
On 2011/05/10 13:15:45, brainman wrote:
> Please move this function to user package.

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:385: // from Lmcons.h
On 2011/05/10 13:15:45, brainman wrote:
> func UserName() (name string, errno int) {
> 	b := make([]uint16, 40)
> 	n := uint32(len(b))
> 	e := GetUserName(&b[0], &n)
> 	if e == ERROR_INSUFFICIENT_BUFFER {
> 		b := make([]uint16, n)
> 		e = GetUserName(&b[0], &n)
> 	}
> 	if e != 0 {
> 		return "", e
> 	}
> 	return string(utf16.Decode(b[:n])), 0
> }

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:397: type ExtendedNameFormat int32
On 2011/05/10 13:15:45, brainman wrote:
> Do not need this type at all, just use int or int32, whatever appropriate.

Just wanted to simulate type safe enums.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:399: const (
On 2011/05/10 13:15:45, brainman wrote:
> Move these consts to ztypes_windows.go

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:412: func UserNameEx(format
ExtendedNameFormat) (name string, errno int) {
On 2011/05/10 13:15:45, brainman wrote:
> Please move this function to user package.

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:425: func UserProfileDirectory() (name
string, errno int) {
On 2011/05/10 13:15:45, brainman wrote:
> Please move this function to user package.

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:455: type SID []uint8
On 2011/05/10 13:15:45, brainman wrote:
> s/uint8/byte/

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:459: type SID_NAME_USE int32
On 2011/05/10 13:15:45, brainman wrote:
> Do not need this type at all, just use int or int32, whatever appropriate.

Just wanted to simulate type safe enums.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:461: const (
On 2011/05/10 13:15:45, brainman wrote:
> Move these consts to ztypes_windows.go

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:476: func SidToStringSid(sid SID) (string,
int) {
On 2011/05/10 13:15:45, brainman wrote:
> Please move this function to user package.

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:489: // The Window specific account
information
On 2011/05/10 13:15:45, brainman wrote:
> I'm not sure you need this struct, if you move all these functions into user
> package, I suspect many things will get less verbose.

Originally I thought about using multiple return values, but then the return
values list was just too big.

But you're right, in package user it is not be required, I rewrote the code.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:499: func AccountName(name string) (*Account,
int) {
On 2011/05/10 13:15:45, brainman wrote:
> Please move this function to user package.

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:512: e := LookupAccountName(nil,
&accountName[0], &sid[0], &sidSize, &domainName[0], &domainNameSize, &nameUse)
On 2011/05/10 13:15:45, brainman wrote:
> You shouldn't allocate huge buffers of 8192 for output parameters, but be
> reasonable - 100. You should make buffer bigger and call second time if first
> buffer is not big enough.
> 
> From the manual:
> If the function fails because the buffer is too small, this variable receives
> the required buffer size, including the terminating null character.

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:512: e := LookupAccountName(nil,
&accountName[0], &sid[0], &sidSize, &domainName[0], &domainNameSize, &nameUse)
On 2011/05/10 13:15:45, brainman wrote:
> You shouldn't allocate huge buffers of 8192 for output parameters, but be
> reasonable - 100. You should make buffer bigger and call second time if first
> buffer is not big enough.
> 
> From the manual:
> If the function fails because the buffer is too small, this variable receives
> the required buffer size, including the terminating null character.

Changed all the function calls to first query for buffer sizes, and only
afterwards fetch the data.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:527: func AccountSid(sid SID) (*Account, int)
{
On 2011/05/10 13:15:45, brainman wrote:
> Please move this function to user package.

Done.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/zerrors_windo...
File src/pkg/syscall/zerrors_windows_386.go (right):

http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/zerrors_windo...
src/pkg/syscall/zerrors_windows_386.go:6: // Go names for Windows errors.
On 2011/05/10 13:15:45, brainman wrote:
> I don't know why this file is changed. This file is generated automatically by
> running ./mkall.sh with GOOS=windows and GOARCH=386. Even assuming all your
> changes in mkerrors_windows.sh are needed, this file would look very
> differently. I don't think you need to change one line in this file.

This is the result of calling mingw.sh with no arguments in mingw. Should I do
it with the arguments you use on your comment?

[brainman, 2011-05-11 12:46:47]

On 2011/05/11 09:30:26, pjmlp wrote:
> 
> here are my comments. Please let me know your view on them, so that I can
> republish my changes.

Cool. Please, use "hg main 4521053" to publish your changes once you're ready.
This will also send email to golang-dev list, so other people will see your
changes and can comment, if they care.

> On 2011/05/10 13:15:45, brainman wrote:
> > This can stay in user.go.
> 
> I cannot, because Uid and Gid fields are os specific, as such user.go won't
> compile.

You can put this into user.go

type User struct {
    Uid              // user id
    Gid              // primary group id
    Username string
    Name     string
    HomeDir  string
}

You can put this into lookup_unix.go

type Uid int
type Gid int

You can put this into lookup_windows.go

type Uid syscall.SID
type Gid syscall.SID

or whatever.

> On 2011/05/10 13:15:45, brainman wrote:
> > I have no idea why you've change this file. I don't think you need to change
> > even one line in this file.
> 
> These hacks are required when compiling on Windows. ...

I realised that, but only after I hit "send" button. Please, just revert changes
to zerrors_windows.go file for now. We could deal with that problem in a
separate CL http://code.google.com/p/go/issues/detail?id=1799.

>
http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
> src/pkg/syscall/syscall_windows.go:136: //sys   ConvertSidToStringSid(sid
> *uint8, stringSid *uint32) (errno int) =  advapi32.ConvertSidToStringSidW
> On 2011/05/10 13:15:45, brainman wrote:
> > s/uint8/byte/
> > s/stringSid *uint32/stringSid **uint16/
> > 
> > from the manual:
> > StringSid
> > A pointer to a variable that receives a pointer to a null-terminated SID
> string.
> 
> stringSid kept as *uint32. I was following LocalFree declaration. As this
string
> needs to be freed by it.
> 

I understand. But this

pstr := (*[8192]uint16)(unsafe.Pointer(uintptr(str)))

is not pretty. I just hope my suggestion will simplify this code. Line with
LocalFree will have single type conversion - no big deal.

> On 2011/05/10 13:15:45, brainman wrote:
> > Do not need this type at all, just use int or int32, whatever appropriate.
> 
> Just wanted to simulate type safe enums.
> 

It's simple enough here. You won't make mistake <g>.

> 
> Changed all the function calls to first query for buffer sizes, and only
> afterwards fetch the data.
> 

Please, don't do that. Here are some issues you're fighting against:

1) syscalls into Windows are expensive - you have to do first call, it is OK to
have second call, but only in "extreme" cases; two syscalls every time is
wasteful;

2) allocating big blocks of memory could be expensive - even if you're
allocating on the stack, since go use segmented stacks, big allocations on stack
will trigger stack split - memory allocation and some stack management;

3) you don't want to end up with code that doesn't return complete data or
doesn't work in "extreme" cases (like username is way too long or something).

So the approach is, use reasonably small buffer, that will cover *most* of
scenarios. If that fails, make second call, but now you should know how big your
buffer should be. So now you never use more buffer space than you need to, and
also make second syscall only on rare occasions.

Alex

[bradfitz, 2011-05-13 01:21:48]

A few comments.

I'm mostly relying on brainman to handle this review.

We could also split it into two CLs, one for the syscalls/etc and one for the
user package, if that's easier.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/os/user/Makefile
File src/pkg/os/user/Makefile (right):

http://codereview.appspot.com/4521053/diff/2001/src/pkg/os/user/Makefile#newc...
src/pkg/os/user/Makefile:23: #GOFILES+=lookup_stubs.go
we'll still need lookup_stubs for e.g. plan9 ... can't just comment this all
out. :)

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

http://codereview.appspot.com/4521053/diff/2001/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:23: // User represents a user account in Unix
systems.
On 2011/05/11 09:30:26, pjmlp wrote:
> On 2011/05/10 13:15:45, brainman wrote:
> > This can stay in user.go.
> 
> I cannot, because Uid and Gid fields are os specific, as such user.go won't
> compile.

Hm, it should? Sure the Makefile is correct?

http://codereview.appspot.com/4521053/diff/2001/src/pkg/os/user/lookup_window...
File src/pkg/os/user/lookup_windows.go (right):

http://codereview.appspot.com/4521053/diff/2001/src/pkg/os/user/lookup_window...
src/pkg/os/user/lookup_windows.go:34: return nil, UnknownUserError(username)
Are you sure this is the only care case?  What if I'm user "A" (not an
Administrator) and I'm trying to lookup details of user "B"?  Isn't that a
permission problem of some sort, at least in some cases?   UnknownUserError is
probably the wrong error type in that case.

http://codereview.appspot.com/4521053/diff/2001/src/pkg/os/user/lookup_window...
src/pkg/os/user/lookup_windows.go:40: fmt.Printf("Could not access the username
data, assuming an empty value.")
is this debug stuff?  log.Printf is more suitable, but even then the log message
should start with "os/user: could not access.."

http://codereview.appspot.com/4521053/diff/2001/src/pkg/os/user/lookup_window...
src/pkg/os/user/lookup_windows.go:47: return nil, fmt.Errorf("Could not access
username directory")
"os/user: could not acccess ...."

http://codereview.appspot.com/4521053/diff/2001/src/pkg/os/user/lookup_window...
src/pkg/os/user/lookup_windows.go:71: fmt.Printf("Could not access the username
data, assuming an empty value.")
as above

http://codereview.appspot.com/4521053/diff/2001/src/pkg/os/user/lookup_window...
src/pkg/os/user/lookup_windows.go:78: return nil, fmt.Errorf("Could not access
username directory")
also

[pjmlp, 2011-05-16 12:53:32]

Hello bradfitz, brainman (cc: golang-dev@googlegroups.com),

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

[pjmlp, 2011-05-16 12:57:37]

Hi,

tried to follow most of the last recomendations. Commenting here, because I
forgot to add the comment on the changes submission email.

I noticed that some bindings in the syscall package actually require two return
values, fixed that.

Most of the code is now in os.user package.

Now some initial values for the buffers is used.

The full username is being correctly retrieved now.

A new issue surfaced, how to get the SID for the Gid. For the time being I made
Uid == Gid.

On 2011/05/11 12:46:47, brainman wrote:
> On 2011/05/11 09:30:26, pjmlp wrote:
> > 
> > here are my comments. Please let me know your view on them, so that I can
> > republish my changes.
> 
> Cool. Please, use "hg main 4521053" to publish your changes once you're ready.
> This will also send email to golang-dev list, so other people will see your
> changes and can comment, if they care.
> 
> > On 2011/05/10 13:15:45, brainman wrote:
> > > This can stay in user.go.
> > 
> > I cannot, because Uid and Gid fields are os specific, as such user.go won't
> > compile.
> 
> You can put this into user.go
> 
> type User struct {
>     Uid              // user id
>     Gid              // primary group id
>     Username string
>     Name     string
>     HomeDir  string
> }
> 
> You can put this into lookup_unix.go
> 
> type Uid int
> type Gid int
> 
> You can put this into lookup_windows.go
> 
> type Uid syscall.SID
> type Gid syscall.SID
> 
> or whatever.
> 
> > On 2011/05/10 13:15:45, brainman wrote:
> > > I have no idea why you've change this file. I don't think you need to
change
> > > even one line in this file.
> > 
> > These hacks are required when compiling on Windows. ...
> 
> I realised that, but only after I hit "send" button. Please, just revert
changes
> to zerrors_windows.go file for now. We could deal with that problem in a
> separate CL http://code.google.com/p/go/issues/detail?id=1799.
> 
> >
>
http://codereview.appspot.com/4521053/diff/2001/src/pkg/syscall/syscall_windo...
> > src/pkg/syscall/syscall_windows.go:136: //sys   ConvertSidToStringSid(sid
> > *uint8, stringSid *uint32) (errno int) =  advapi32.ConvertSidToStringSidW
> > On 2011/05/10 13:15:45, brainman wrote:
> > > s/uint8/byte/
> > > s/stringSid *uint32/stringSid **uint16/
> > > 
> > > from the manual:
> > > StringSid
> > > A pointer to a variable that receives a pointer to a null-terminated SID
> > string.
> > 
> > stringSid kept as *uint32. I was following LocalFree declaration. As this
> string
> > needs to be freed by it.
> > 
> 
> I understand. But this
> 
> pstr := (*[8192]uint16)(unsafe.Pointer(uintptr(str)))
> 
> is not pretty. I just hope my suggestion will simplify this code. Line with
> LocalFree will have single type conversion - no big deal.
> 
> > On 2011/05/10 13:15:45, brainman wrote:
> > > Do not need this type at all, just use int or int32, whatever appropriate.
> > 
> > Just wanted to simulate type safe enums.
> > 
> 
> It's simple enough here. You won't make mistake <g>.
> 
> > 
> > Changed all the function calls to first query for buffer sizes, and only
> > afterwards fetch the data.
> > 
> 
> Please, don't do that. Here are some issues you're fighting against:
> 
> 1) syscalls into Windows are expensive - you have to do first call, it is OK
to
> have second call, but only in "extreme" cases; two syscalls every time is
> wasteful;
> 
> 2) allocating big blocks of memory could be expensive - even if you're
> allocating on the stack, since go use segmented stacks, big allocations on
stack
> will trigger stack split - memory allocation and some stack management;
> 
> 3) you don't want to end up with code that doesn't return complete data or
> doesn't work in "extreme" cases (like username is way too long or something).
> 
> So the approach is, use reasonably small buffer, that will cover *most* of
> scenarios. If that fails, make second call, but now you should know how big
your
> buffer should be. So now you never use more buffer space than you need to, and
> also make second syscall only on rare occasions.
> 
> Alex

[bradfitz, 2011-05-16 23:42:31]

Please do all the syscall changes in a separate CL.

Also, don't change the line endings to Windows-style (on e.g.
http://codereview.appspot.com/4521053/patch/8003/10002)

[pjmlp, 2011-05-17 07:59:01]

On 2011/05/16 23:42:31, bradfitz wrote:
> Please do all the syscall changes in a separate CL.
> 
> Also, don't change the line endings to Windows-style (on e.g.
> http://codereview.appspot.com/4521053/patch/8003/10002)

syscall changes are now on
http://codereview.appspot.com/4528073

[pjmlp, 2011-05-17 08:03:29]

Hello bradfitz, brainman (cc: golang-dev@googlegroups.com),

Please take another look.

[pjmlp, 2011-05-17 08:07:42]

Hello bradfitz, brainman (cc: golang-dev@googlegroups.com),

Please take another look.

[mattn, 2011-05-18 00:52:26]

On 2011/05/17 08:07:42, pjmlp wrote:
> Hello bradfitz, brainman (cc: mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Do you forget to upload file in syscall ?

------------------
8g  -o _go_.8 user.go  lookup_windows.go
user.go:14: invalid recursive type Uid
user.go:15: invalid recursive type Gid
lookup_windows.go:15: undefined: syscall.SID
lookup_windows.go:18: undefined: syscall.SID
lookup_windows.go:24: assignment count mismatch: 2 = 0
lookup_windows.go:31: undefined: syscall.NameDisplay
lookup_windows.go:55: undefined: syscall.SID
lookup_windows.go:155: undefined: syscall.SID
lookup_windows.go:168: undefined: syscall.SID
lookup_windows.go:200: undefined: syscall.SID
lookup_windows.go:55: too many errors

------------------

[brainman, 2011-05-18 05:57:22]

Shouldn't we have tests going for Windows too?

I don't think it was good idea to have syscall changes in a separate CL.

Alex

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
File src/pkg/os/user/lookup_windows.go (right):

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:28: 
Remove all blank lines from short functions.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:31: name, err =
UserNameEx(syscall.NameDisplay)
Do not need vars, just do
name, err := UserNameEx(syscall.NameDisplay)

Same everywhere else.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:85: func UserName() (name string, err
os.Error) {
I don't think you want this function. There is no equivalent unix version. It is
also not used by anything inside here.

Perhaps in the future we should have something like that, but this should be
coordinated with other platforms. Let's delay this for later CL.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:101: func UserNameEx(format int32) (name
string, err os.Error) {
This function will be visibly from outside of user package. I think you want it
lower case.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:102: //var n uint32 = syscall.UNLEN + 1
Remove this comment.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:114: return string(utf16.Decode(b[0:n])), nil
s/0:n/:n/

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:118: func UserProfileDirectory() (name string,
err os.Error) {
This function will be visibly from outside of user package. I think you want it
lower case.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:136: // not enough space, get the buffer size
From GetUserProfileDirectory manual:

If the buffer specified by lpProfileDir is not large enough or lpProfileDir is
NULL, the function fails and this parameter receives the necessary buffer size,
including the terminating null character.

Do not call GetUserProfileDirectory(token, nil, &n), after first call to
GetUserProfileDirectory, n should have size you want. Do similar to UserNameEx.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:151: return string(utf16.Decode(b[0:n])), nil
s/0:n/:n/

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:155: func SidToStringSid(sid syscall.SID)
(string, os.Error) {
Maybe this should be called:

func (uid UID) String() string

This way your UID would look good in fmt.Printf and others.

Not sure about this one, just thinking aloud.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:177: domainName := make([]uint16,
domainNameSize)
Why are you fetching domain name if you're not using it for anything.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:179: if e == syscall.ERROR_INSUFFICIENT_BUFFER
{
Similar to UserProfileDirectory, sidSize and domainNameSize contain correct
sizes. It's not clear from the manual, but you could see it is set, if you start
with really small buffers (3-5 bytes).

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:207: domainName := make([]uint16,
domainNameSize)
Why are you fetching domain name if you're not using it for anything. Maybe you
should return domain name as part of username or something.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:210: if e == syscall.ERROR_INSUFFICIENT_BUFFER
{
Similar to UserProfileDirectory, usernameSize and domainNameSize contain correct
sizes. It's not clear from the manual, but you could see it is set, if you start
with really small buffers (3-5 bytes).

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/user.go
File src/pkg/os/user/user.go (left):

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/user.go#oldc...
src/pkg/os/user/user.go:15: Gid      int // primary group id
I take my suggestion back. Let's not complicate things. Let's:

type User struct {
  Uid Id // user id
  Gid Id // primary group id
  Username string
  Name string
  HomeDir string
}

[brainman, 2011-05-18 05:58:59]

On 2011/05/18 00:52:26, mattn wrote:
> 
> Do you forget to upload file in syscall ?
> 

Brad asked to put it in a separate CL (http://codereview.appspot.com/4528073/).

Alex

[pjmlp, 2011-05-18 08:30:31]

I took care of most of the comments. Will upload a new change set. Please note
that I cannot test Unix builds.

Regarding the unit tests, as I understand user_test.go is used for all OS,
right?

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
File src/pkg/os/user/lookup_windows.go (right):

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:28: 
On 2011/05/18 05:57:22, brainman wrote:
> Remove all blank lines from short functions.

Done.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:31: name, err =
UserNameEx(syscall.NameDisplay)
On 2011/05/18 05:57:22, brainman wrote:
> Do not need vars, just do
> name, err := UserNameEx(syscall.NameDisplay)
> 
> Same everywhere else.

I fixed this partially, the main problem being that variables cannot be reused
in := assignments, hence the few vars you see.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:85: func UserName() (name string, err
os.Error) {
On 2011/05/18 05:57:22, brainman wrote:
> I don't think you want this function. There is no equivalent unix version. It
is
> also not used by anything inside here.
> 
> Perhaps in the future we should have something like that, but this should be
> coordinated with other platforms. Let's delay this for later CL.

Done.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:101: func UserNameEx(format int32) (name
string, err os.Error) {
On 2011/05/18 05:57:22, brainman wrote:
> This function will be visibly from outside of user package. I think you want
it
> lower case.

Why not? should user package only offer the same set of public functions across
OS?

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:102: //var n uint32 = syscall.UNLEN + 1
On 2011/05/18 05:57:22, brainman wrote:
> Remove this comment.

Done.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:114: return string(utf16.Decode(b[0:n])), nil
On 2011/05/18 05:57:22, brainman wrote:
> s/0:n/:n/

Done.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:118: func UserProfileDirectory() (name string,
err os.Error) {
On 2011/05/18 05:57:22, brainman wrote:
> This function will be visibly from outside of user package. I think you want
it
> lower case.

I might even remove it. Still not clear how to get this information for any
user, besides the current process owner.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:136: // not enough space, get the buffer size
On 2011/05/18 05:57:22, brainman wrote:
> From GetUserProfileDirectory manual:
> 
> If the buffer specified by lpProfileDir is not large enough or lpProfileDir is
> NULL, the function fails and this parameter receives the necessary buffer
size,
> including the terminating null character.
> 
> Do not call GetUserProfileDirectory(token, nil, &n), after first call to
> GetUserProfileDirectory, n should have size you want. Do similar to
UserNameEx.

Done.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:151: return string(utf16.Decode(b[0:n])), nil
On 2011/05/18 05:57:22, brainman wrote:
> s/0:n/:n/

Done.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:155: func SidToStringSid(sid syscall.SID)
(string, os.Error) {
On 2011/05/18 05:57:22, brainman wrote:
> Maybe this should be called:
> 
> func (uid UID) String() string
> 
> This way your UID would look good in fmt.Printf and others.
> 
> Not sure about this one, just thinking aloud.

For it, maybe we need to cache the string representation, to avoid going down to
OS level, every time String() gets called.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:177: domainName := make([]uint16,
domainNameSize)
On 2011/05/18 05:57:22, brainman wrote:
> Why are you fetching domain name if you're not using it for anything.

The function fails otherwise, even if it is documented that it should work
without asking for the domain.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:179: if e == syscall.ERROR_INSUFFICIENT_BUFFER
{
On 2011/05/18 05:57:22, brainman wrote:
> Similar to UserProfileDirectory, sidSize and domainNameSize contain correct
> sizes. It's not clear from the manual, but you could see it is set, if you
start
> with really small buffers (3-5 bytes).

Reduced down to 10.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:207: domainName := make([]uint16,
domainNameSize)
On 2011/05/18 05:57:22, brainman wrote:
> Why are you fetching domain name if you're not using it for anything. Maybe
you
> should return domain name as part of username or something.

As replied above, function seems to fail if no domain is given.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:210: if e == syscall.ERROR_INSUFFICIENT_BUFFER
{
On 2011/05/18 05:57:22, brainman wrote:
> Similar to UserProfileDirectory, usernameSize and domainNameSize contain
correct
> sizes. It's not clear from the manual, but you could see it is set, if you
start
> with really small buffers (3-5 bytes).

Reduced down to 10.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/user.go
File src/pkg/os/user/user.go (left):

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/user.go#oldc...
src/pkg/os/user/user.go:15: Gid      int // primary group id
On 2011/05/18 05:57:22, brainman wrote:
> I take my suggestion back. Let's not complicate things. Let's:
> 
> type User struct {
>   Uid Id // user id
>   Gid Id // primary group id
>   Username string
>   Name string
>   HomeDir string
> }

Done.

[pjmlp, 2011-05-18 08:36:23]

Hello bradfitz, brainman, mattn (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2011-05-18 12:29:25]

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
File src/pkg/os/user/lookup_windows.go (right):

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:101: func UserNameEx(format int32) (name
string, err os.Error) {
On 2011/05/18 08:30:31, pjmlp wrote:
> ... should user package only offer the same set of public functions across
> OS?

Yes. Maybe later, but this time round, we'll implement equivalent of "unix"
version. Just to keep things simple.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:118: func UserProfileDirectory() (name string,
err os.Error) {
On 2011/05/18 08:30:31, pjmlp wrote:
> I might even remove it. ...

That is OK, but you don't want it to be visible from outside. Please, make it
lower case.

http://codereview.appspot.com/4521053/diff/14001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:155: func SidToStringSid(sid syscall.SID)
(string, os.Error) {
On 2011/05/18 08:30:31, pjmlp wrote:
> On 2011/05/18 05:57:22, brainman wrote:
> > Maybe this should be called:
> > 
> > func (uid UID) String() string
> > 
> > This way your UID would look good in fmt.Printf and others.
> > 
> > Not sure about this one, just thinking aloud.
> 

> For it, maybe we need to cache the string representation, ...

Caching is, probably, OK, but not now. Let's keep it simple.

But, please, rename it to lower case, or have it named as I suggested.

http://codereview.appspot.com/4521053/diff/12002/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:25: if er != 0 {
Replace these 2 lines with
name, err := UserNameEx(syscall.NameDisplay)

http://codereview.appspot.com/4521053/diff/12002/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:30: var err os.Error
Replace these 2 lines with
dir, err := UserProfileDirectory()

... same everywhere else.

http://codereview.appspot.com/4521053/diff/12002/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:69: if err != nil {
Remove blank line.

http://codereview.appspot.com/4521053/diff/12002/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:161: defer
syscall.LocalFree(uint32(uintptr(unsafe.Pointer(str))))
Please, make these both to be 100, like the function before.

[bradfitzgoog, 2011-05-18 14:11:02]

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

http://codereview.appspot.com/4521053/diff/12002/src/pkg/os/user/user.go#newc...
src/pkg/os/user/user.go:15: Uid      Id // user id
We're not going to go down this painful road where basic types are different
depending on the OS.

Structs and code should compile on different systems, even if it doesn't run.

Keep these as ints but feel free to add a new field:

SID  <whatevertype>  // The Windows SID, generally blank on POSIX systems.

Generally?  Perhaps somebody wants to use it to interop with a Windows system,
or Samba, etc.

Then just add a new LookupSID function for Windows that returns an error on
Posix.

And have LookupId return an error on Windows.

Portable code can switch on runtime.GOOS and support multiple OSes without
Makefile shenanigans.

[brainman, 2011-05-19 02:54:04]

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

http://codereview.appspot.com/4521053/diff/12002/src/pkg/os/user/user.go#newc...
src/pkg/os/user/user.go:15: Uid      Id // user id
On 2011/05/18 14:11:02, bradfitzgoog wrote:
> 
> ...
> 

I have no idea what we're trying to do here then. I'll just step back and help
when asked. You drive it.

Alex

[bradfitz, 2011-05-19 20:21:41]

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

http://codereview.appspot.com/4521053/diff/12002/src/pkg/os/user/user.go#newc...
src/pkg/os/user/user.go:15: Uid      Id // user id
On 2011/05/19 02:54:04, brainman wrote:
> On 2011/05/18 14:11:02, bradfitzgoog wrote:
> > 
> > ...
> > 
> 
> I have no idea what we're trying to do here then. I'll just step back and help
> when asked. You drive it.

Sorry, what I meant is that it should be possible to write code which compiles
unchanged for different GOOS={windows,posix,darwin}, without doing tricks like
having different Go files in your makefiles for different OSes.  That's
generally only done when it's very necessary.

What I was thinking we'd do for os/user was something like:

type User struct {
    // Where applicable: (e.g. POSIX systems)
    Uid int  // userid
    Gid int  // primary group id, 

    // Where applicable: (e.g. Windows)
    SID string // the Windows xxxxx

    Username string
    Name     string
    HomeDir  string
}

Then for Windows:

func LookupId(uid int) (*User, os.Error) {
   return nil, os.NewError("user: Windows does not support lookup by userid")
}

For Posix:

func LookupSID(sid string) (*User, os.Error) {
   return nil, os.NewError("user: username look by SID not supported")
}

etc

Make sense?

I'm only commenting on the top-level API design.  I'd still like you to continue
to review the Windows-specific parts if you could (syscall and implementation
here, as I don't know anything about Windows).

[pjmlp, 2011-05-19 20:41:26]

On 2011/05/19 20:21:41, bradfitz wrote:
> http://codereview.appspot.com/4521053/diff/12002/src/pkg/os/user/user.go
> File src/pkg/os/user/user.go (right):
> 
>
http://codereview.appspot.com/4521053/diff/12002/src/pkg/os/user/user.go#newc...
> src/pkg/os/user/user.go:15: Uid      Id // user id
> On 2011/05/19 02:54:04, brainman wrote:
> > On 2011/05/18 14:11:02, bradfitzgoog wrote:
> > > 
> > > ...
> > > 
> > 
> > I have no idea what we're trying to do here then. I'll just step back and
help
> > when asked. You drive it.
> 
> Sorry, what I meant is that it should be possible to write code which compiles
> unchanged for different GOOS={windows,posix,darwin}, without doing tricks like
> having different Go files in your makefiles for different OSes.  That's
> generally only done when it's very necessary.
> 
> What I was thinking we'd do for os/user was something like:
> 
> type User struct {
>     // Where applicable: (e.g. POSIX systems)
>     Uid int  // userid
>     Gid int  // primary group id, 
> 
>     // Where applicable: (e.g. Windows)
>     SID string // the Windows xxxxx
> 
>     Username string
>     Name     string
>     HomeDir  string
> }
> 
> Then for Windows:
> 
> func LookupId(uid int) (*User, os.Error) {
>    return nil, os.NewError("user: Windows does not support lookup by userid")
> }
> 
> For Posix:
> 
> func LookupSID(sid string) (*User, os.Error) {
>    return nil, os.NewError("user: username look by SID not supported")
> }
> 
> etc
> 
> Make sense?
> 
> I'm only commenting on the top-level API design.  I'd still like you to
continue
> to review the Windows-specific parts if you could (syscall and implementation
> here, as I don't know anything about Windows).

I actually already did that. Only used panic instead of os.NewError.
I will replace it.

So far I have not uploaded the latest changes, because I am still looking for a
way to map a SID to the user fullname.

The GetUsernameEx() function cannot be used, since it only works for the process
owner. So I have come to the conclusion that it does not make sense to call it.

Windows makes it very hard to fetch this information, since the APIs change
depending on where the information is stored (local, domain controler,
ActiveDirectory, LDAP).

Another question, is there a way to make the tests filename OS dependent, like
with the normal code?

[bradfitz, 2011-05-19 20:49:46]

On Thu, May 19, 2011 at 1:41 PM, <paulo.jpinto@gmail.com> wrote:

> On 2011/05/19 20:21:41, bradfitz wrote:
>
> http://codereview.appspot.com/4521053/diff/12002/src/pkg/os/user/user.go
>
>> File src/pkg/os/user/user.go (right):
>>
>
>
>
>
http://codereview.appspot.com/4521053/diff/12002/src/pkg/os/user/user.go#newc...
>
>> src/pkg/os/user/user.go:15: Uid      Id // user id
>> On 2011/05/19 02:54:04, brainman wrote:
>> > On 2011/05/18 14:11:02, bradfitzgoog wrote:
>> > >
>> > > ...
>> > >
>> >
>> > I have no idea what we're trying to do here then. I'll just step
>>
> back and help
>
>> > when asked. You drive it.
>>
>
>  Sorry, what I meant is that it should be possible to write code which
>>
> compiles
>
>> unchanged for different GOOS={windows,posix,darwin}, without doing
>>
> tricks like
>
>> having different Go files in your makefiles for different OSes.
>>
> That's
>
>> generally only done when it's very necessary.
>>
>
>  What I was thinking we'd do for os/user was something like:
>>
>
>  type User struct {
>>     // Where applicable: (e.g. POSIX systems)
>>     Uid int  // userid
>>     Gid int  // primary group id,
>>
>
>      // Where applicable: (e.g. Windows)
>>     SID string // the Windows xxxxx
>>
>
>      Username string
>>     Name     string
>>     HomeDir  string
>> }
>>
>
>  Then for Windows:
>>
>
>  func LookupId(uid int) (*User, os.Error) {
>>    return nil, os.NewError("user: Windows does not support lookup by
>>
> userid")
>
>> }
>>
>
>  For Posix:
>>
>
>  func LookupSID(sid string) (*User, os.Error) {
>>    return nil, os.NewError("user: username look by SID not supported")
>> }
>>
>
>  etc
>>
>
>  Make sense?
>>
>
>  I'm only commenting on the top-level API design.  I'd still like you
>>
> to continue
>
>> to review the Windows-specific parts if you could (syscall and
>>
> implementation
>
>> here, as I don't know anything about Windows).
>>
>
> I actually already did that. Only used panic instead of os.NewError.
> I will replace it.
>
> So far I have not uploaded the latest changes, because I am still
> looking for a way to map a SID to the user fullname.
>
> The GetUsernameEx() function cannot be used, since it only works for the
> process owner. So I have come to the conclusion that it does not make
> sense to call it.
>

I wouldn't worry about it.  Just document somewhere that some fields (and
perhaps which) may be missing if you're not the same owner or
root/Administrator.


> Another question, is there a way to make the tests filename OS
> dependent, like with the normal code?


Just do:

func TestWindowsStuff(t *testing.T) {
  if runtime.GOOS != "windows" {
      t.Logf("skip; not on Windows")
      return
   }
   ....
}

No Makefile stuff.

[brainman, 2011-05-20 02:44:47]

On 2011/05/19 20:21:41, bradfitz wrote:
> 
> Sorry, what I meant is that it should be possible to write code which compiles
> unchanged for different GOOS={windows,posix,darwin}, without doing tricks like
> having different Go files in your makefiles for different OSes.  That's
> generally only done when it's very necessary.
> 
> What I was thinking we'd do for os/user was something like:
> 
> type User struct {
>     // Where applicable: (e.g. POSIX systems)
>     Uid int  // userid
>     Gid int  // primary group id, 
> 
>     // Where applicable: (e.g. Windows)
>     SID string // the Windows xxxxx
> 
>     Username string
>     Name     string
>     HomeDir  string
> }
> 
> Then for Windows:
> 
> func LookupId(uid int) (*User, os.Error) {
>    return nil, os.NewError("user: Windows does not support lookup by userid")
> }
> 
> For Posix:
> 
> func LookupSID(sid string) (*User, os.Error) {
>    return nil, os.NewError("user: username look by SID not supported")
> }
> 
> etc
> 
> Make sense?
> 

I'm looking at it from our user's point of view: "What can I do with this
package?"

My answer would be:

1) I could find, who is running current process:

u := user.GetCurrentUser()

2) I could find "things" about that user:

fmt.Printf("%v\n", u)
fmt.Printf("My username is %s, and my userid is %s.\n", u.Username, u.Uid)
fmt.Printf("My name is %s, and I live at %s.\n", u.Name, u.HomeDir)

All that code should be os-independent. I don't think we want our users to write
code that is os-dependent, unless they chose to themselves. I don't think we
want our users to write:

if runtime.GOOS == "windows" {
  fmt.Printf("My user id is %s.\n", u.SID)
} else {
  fmt.Printf("My user id is %d.\n", u.Uid)
}

I'm not sure what other functionality you want from this package, so it is hard
for me to comment. Perhaps security models for different OSes are too different,
so we won't be able to have single API.

As to implementation side of it, it has to be os-dependent, whether you want it
or not. So we have to keep implementations in different source file and select
right one with Makefile when building. That is what we've done so far.

> ...  I'd still like you to continue
> to review the Windows-specific parts if you could (syscall and implementation
> here, as I don't know anything about Windows).

Happy to do what I can.

Alex

[pjmlp, 2011-05-20 13:50:53]

Hi I'll be uploading the new version shortly.

- Changed again the User struct;
- Brought back the UserName() function to ease the tests implementation;
- Currently the full username on Windows is an empty string;

I have found this example on MSDN about retrieving the real username, but it
fails on my company network. Not sure about other environments.

http://msdn.microsoft.com/en-us/library/aa370284%28v=VS.85%29.aspx

http://codereview.appspot.com/4521053/diff/12002/src/pkg/os/user/lookup_windo...
File src/pkg/os/user/lookup_windows.go (right):

http://codereview.appspot.com/4521053/diff/12002/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:25: name, err =
UserNameEx(syscall.NameDisplay)
On 2011/05/18 12:29:25, brainman wrote:
> Replace these 2 lines with
> name, err := UserNameEx(syscall.NameDisplay)

Done. I only had like that because on my actual Go source code, I got a
redeclaration error.

Since I synced with tip it works. Have I missed this language change?

http://codereview.appspot.com/4521053/diff/12002/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:30: dir, err = UserProfileDirectory()
On 2011/05/18 12:29:25, brainman wrote:
> Replace these 2 lines with
> dir, err := UserProfileDirectory()
> 
> ... same everywhere else.

Done.

http://codereview.appspot.com/4521053/diff/12002/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:69: 
On 2011/05/18 12:29:25, brainman wrote:
> Remove blank line.

Done.

http://codereview.appspot.com/4521053/diff/12002/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:161: var domainNameSize uint32 = 10
On 2011/05/18 12:29:25, brainman wrote:
> Please, make these both to be 100, like the function before.

Done.

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

http://codereview.appspot.com/4521053/diff/12002/src/pkg/os/user/user.go#newc...
src/pkg/os/user/user.go:15: Uid      Id // user id
On 2011/05/18 14:11:02, bradfitzgoog wrote:
> We're not going to go down this painful road where basic types are different
> depending on the OS.
> 
> Structs and code should compile on different systems, even if it doesn't run.
> 
> Keep these as ints but feel free to add a new field:
> 
> SID  <whatevertype>  // The Windows SID, generally blank on POSIX systems.
> 
> Generally?  Perhaps somebody wants to use it to interop with a Windows system,
> or Samba, etc.
> 
> Then just add a new LookupSID function for Windows that returns an error on
> Posix.
> 
> And have LookupId return an error on Windows.
> 
> Portable code can switch on runtime.GOOS and support multiple OSes without
> Makefile shenanigans.

Ok, I'll include an attempt to do this on my next patch submission.

[pjmlp, 2011-05-20 13:51:18]

Hello bradfitz@golang.org, alex.brainman@gmail.com, mattn.jp@gmail.com,
bradfitz@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-05-20 15:52:33]

This is getting close.

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

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:16: // Represents an OS independent user
identifier
I think this comment is old.  It's no longer used for POSIX.  Also:

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:17: type Id int
here Id is an int value, but...

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:44: // LookupSID looks up a user by Id. If the
user cannot be found,
these docs assume the user knows what a SID is, which zero Unix people know and
very few Windows people even know.

Be sure to mention Windows in here.

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:46: // This function panics if called on Unix
systems
doesn't panic.  just returns an error.

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:104: SID:      nil,
... here SID is a pointer.

Maybe the "Id" type should just be:

type WindowsSID string

... on both posix and windows.

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_windo...
File src/pkg/os/user/lookup_windows.go (right):

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:68: func UserName() (name string, err
os.Error) {
this function isn't exported on the unix side.  Did you mean to make this
lowercase?

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

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user.go#newc...
src/pkg/os/user/user.go:15: Uid      int // user id, only valid in POSIX
systems, 0 otherwise
no need to say "0 otherwise".

You could also group these with a shared comment:

     // For POSIX systems:
     Uid   int  // user id
     Gid   int  // primary group id

     // For Windows:
     SID   Id   // What is this? What is its type?

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user.go#newc...
src/pkg/os/user/user.go:19: Name     string // An empty string on Windows
platforms for the time being
Name  string // User's name, if available.

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user_test.go
File src/pkg/os/user/user_test.go (right):

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user_test.go...
src/pkg/os/user/user_test.go:74: if err != nil && err.String() != "user: Windows
does not support lookup by userid" {
error checking by string comparison is a bit gross, but in a test I suppose it's
not the end of the world.

I wonder if you should just return syscall.EWINDOWS here instead, and check for
that exact value?

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user_test.go...
src/pkg/os/user/user_test.go:79: username, err := UserName()
UserName shouldn't be part of the public API.  At least, it isn't now.

make this userName for now?  maybe we can unhide it later.

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user_test.go...
src/pkg/os/user/user_test.go:89: t.Errorf("expected Uid of 0 got %d", u.Uid)
As a unix person, Uid/Gid of 0 have a very distinct meaning.  I wonder if -1
might be a better "not implemented" value over 0?  Anybody have preferences?

[pjmlp, 2011-05-20 16:41:52]

Currently I am not able to upload my changes, because I am on another PC, but I
can already give my feedback to your comments.

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

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:16: // Represents an OS independent user
identifier
On 2011/05/20 15:52:33, bradfitz wrote:
> I think this comment is old.  It's no longer used for POSIX.  Also:

You're right, sorry about that.

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:17: type Id int
On 2011/05/20 15:52:33, bradfitz wrote:
> here Id is an int value, but...

Sorry about that. Not being able to compile on Linux for the time being, opens
the door for such issues.

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:44: // LookupSID looks up a user by Id. If the
user cannot be found,
On 2011/05/20 15:52:33, bradfitz wrote:
> these docs assume the user knows what a SID is, which zero Unix people know
and
> very few Windows people even know.
> 
> Be sure to mention Windows in here.

Done.

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:46: // This function panics if called on Unix
systems
On 2011/05/20 15:52:33, bradfitz wrote:
> doesn't panic.  just returns an error.

Done.

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_windo...
File src/pkg/os/user/lookup_windows.go (right):

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:68: func UserName() (name string, err
os.Error) {
On 2011/05/20 15:52:33, bradfitz wrote:
> this function isn't exported on the unix side.  Did you mean to make this
> lowercase?

No, it needs to be public. Otherwise there is no way to implement similar test
case for Windows. Or should the function be private to the tests file?

syscall.getuid() won't work on Windows, the companion function would be either a
getsid() or the UserName() function.

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

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user.go#newc...
src/pkg/os/user/user.go:15: Uid      int // user id, only valid in POSIX
systems, 0 otherwise
On 2011/05/20 15:52:33, bradfitz wrote:
> no need to say "0 otherwise".
> 
> You could also group these with a shared comment:
> 
>      // For POSIX systems:
>      Uid   int  // user id
>      Gid   int  // primary group id
> 
>      // For Windows:
>      SID   Id   // What is this? What is its type?

I'll do it.

Regarding the Id, maybe I am spoiled with years of software engineering, but I
thought it should be enough for the users to know that user.Id is an black box
ADT.

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user.go#newc...
src/pkg/os/user/user.go:19: Name     string // An empty string on Windows
platforms for the time being
On 2011/05/20 15:52:33, bradfitz wrote:
> Name  string // User's name, if available.

Done.

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user_test.go
File src/pkg/os/user/user_test.go (right):

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user_test.go...
src/pkg/os/user/user_test.go:74: if err != nil && err.String() != "user: Windows
does not support lookup by userid" {
On 2011/05/20 15:52:33, bradfitz wrote:
> error checking by string comparison is a bit gross, but in a test I suppose
it's
> not the end of the world.
> 
> I wonder if you should just return syscall.EWINDOWS here instead, and check
for
> that exact value?

Ah ok. I'm only doing this at the unit test level, but I also don't like it as
it is now.

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user_test.go...
src/pkg/os/user/user_test.go:79: username, err := UserName()
On 2011/05/20 15:52:33, bradfitz wrote:
> UserName shouldn't be part of the public API.  At least, it isn't now.
> 
> make this userName for now?  maybe we can unhide it later.

As I said in another comment I need it to retrieve the username, and Go
developers on Windows will need it too.

They won't be able to call syscall.getuid() to get the process owner. Are they
forced to wrap the Win32 function?

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user_test.go...
src/pkg/os/user/user_test.go:89: t.Errorf("expected Uid of 0 got %d", u.Uid)
On 2011/05/20 15:52:33, bradfitz wrote:
> As a unix person, Uid/Gid of 0 have a very distinct meaning.  I wonder if -1
> might be a better "not implemented" value over 0?  Anybody have preferences?

You are right, I forgot about it.

[bradfitz, 2011-05-20 16:58:42]

On Fri, May 20, 2011 at 9:41 AM, <paulo.jpinto@gmail.com> wrote:

> Currently I am not able to upload my changes, because I am on another
> PC, but I can already give my feedback to your comments.
>
>
>
>
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix.go
> File src/pkg/os/user/lookup_unix.go (right):
>
>
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix....
> src/pkg/os/user/lookup_unix.go:16: // Represents an OS independent user
> identifier
> On 2011/05/20 15:52:33, bradfitz wrote:
>
>> I think this comment is old.  It's no longer used for POSIX.  Also:
>>
>
> You're right, sorry about that.
>
>
>
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix....
> src/pkg/os/user/lookup_unix.go:17: type Id int
> On 2011/05/20 15:52:33, bradfitz wrote:
>
>> here Id is an int value, but...
>>
>
> Sorry about that. Not being able to compile on Linux for the time being,
> opens the door for such issues.
>
>
>
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix....
> src/pkg/os/user/lookup_unix.go:44: // LookupSID looks up a user by Id.
> If the user cannot be found,
> On 2011/05/20 15:52:33, bradfitz wrote:
>
>> these docs assume the user knows what a SID is, which zero Unix people
>>
> know and
>
>> very few Windows people even know.
>>
>
>  Be sure to mention Windows in here.
>>
>
> Done.
>
>
>
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix....
> src/pkg/os/user/lookup_unix.go:46: // This function panics if called on
> Unix systems
> On 2011/05/20 15:52:33, bradfitz wrote:
>
>> doesn't panic.  just returns an error.
>>
>
> Done.
>
>
>
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_windo...
> File src/pkg/os/user/lookup_windows.go (right):
>
>
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_windo...
> src/pkg/os/user/lookup_windows.go:68: func UserName() (name string, err
> os.Error) {
> On 2011/05/20 15:52:33, bradfitz wrote:
>
>> this function isn't exported on the unix side.  Did you mean to make
>>
> this
>
>> lowercase?
>>
>
> No, it needs to be public. Otherwise there is no way to implement
> similar test case for Windows. Or should the function be private to the
> tests file?
>

You can call lowercase stuff from tests in the same package.


> syscall.getuid() won't work on Windows, the companion function would be
> either a getsid() or the UserName() function.


If you really need a new public function in the user package, it has to go
in both posix & windows, not just one.

Maybe we can add:

    func Current() (*User, os.Error)

... to both.

Would that work?


>
>
> http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user.go
> File src/pkg/os/user/user.go (right):
>
>
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user.go#newc...
> src/pkg/os/user/user.go:15: Uid      int // user id, only valid in POSIX
> systems, 0 otherwise
> On 2011/05/20 15:52:33, bradfitz wrote:
>
>> no need to say "0 otherwise".
>>
>
>  You could also group these with a shared comment:
>>
>
>       // For POSIX systems:
>>      Uid   int  // user id
>>      Gid   int  // primary group id
>>
>
>       // For Windows:
>>      SID   Id   // What is this? What is its type?
>>
>
> I'll do it.
>
> Regarding the Id, maybe I am spoiled with years of software engineering,
> but I thought it should be enough for the users to know that user.Id is
> an black box ADT.
>
>
>
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user.go#newc...
> src/pkg/os/user/user.go:19: Name     string // An empty string on
> Windows platforms for the time being
> On 2011/05/20 15:52:33, bradfitz wrote:
>
>> Name  string // User's name, if available.
>>
>
> Done.
>
>
>
> http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user_test.go
> File src/pkg/os/user/user_test.go (right):
>
>
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user_test.go...
> src/pkg/os/user/user_test.go:74: if err != nil && err.String() != "user:
> Windows does not support lookup by userid" {
> On 2011/05/20 15:52:33, bradfitz wrote:
>
>> error checking by string comparison is a bit gross, but in a test I
>>
> suppose it's
>
>> not the end of the world.
>>
>
>  I wonder if you should just return syscall.EWINDOWS here instead, and
>>
> check for
>
>> that exact value?
>>
>
> Ah ok. I'm only doing this at the unit test level, but I also don't like
> it as it is now.
>
>
>
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user_test.go...
> src/pkg/os/user/user_test.go:79: username, err := UserName()
> On 2011/05/20 15:52:33, bradfitz wrote:
>
>> UserName shouldn't be part of the public API.  At least, it isn't now.
>>
>
>  make this userName for now?  maybe we can unhide it later.
>>
>
> As I said in another comment I need it to retrieve the username, and Go
> developers on Windows will need it too.
>
> They won't be able to call syscall.getuid() to get the process owner.
> Are they forced to wrap the Win32 function?
>
>
>
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user_test.go...
> src/pkg/os/user/user_test.go:89: t.Errorf("expected Uid of 0 got %d",
> u.Uid)
> On 2011/05/20 15:52:33, bradfitz wrote:
>
>> As a unix person, Uid/Gid of 0 have a very distinct meaning.  I wonder
>>
> if -1
>
>> might be a better "not implemented" value over 0?  Anybody have
>>
> preferences?
>
> You are right, I forgot about it.
>
>
> http://codereview.appspot.com/4521053/
>

[pjmlp, 2011-05-20 17:00:40]

On 2011/05/20 16:58:42, bradfitz wrote:
> On Fri, May 20, 2011 at 9:41 AM, <mailto:paulo.jpinto@gmail.com> wrote:
> 
> > Currently I am not able to upload my changes, because I am on another
> > PC, but I can already give my feedback to your comments.
> >
> >
> >
> >
> >
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix.go
> > File src/pkg/os/user/lookup_unix.go (right):
> >
> >
> >
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix....
> > src/pkg/os/user/lookup_unix.go:16: // Represents an OS independent user
> > identifier
> > On 2011/05/20 15:52:33, bradfitz wrote:
> >
> >> I think this comment is old.  It's no longer used for POSIX.  Also:
> >>
> >
> > You're right, sorry about that.
> >
> >
> >
> >
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix....
> > src/pkg/os/user/lookup_unix.go:17: type Id int
> > On 2011/05/20 15:52:33, bradfitz wrote:
> >
> >> here Id is an int value, but...
> >>
> >
> > Sorry about that. Not being able to compile on Linux for the time being,
> > opens the door for such issues.
> >
> >
> >
> >
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix....
> > src/pkg/os/user/lookup_unix.go:44: // LookupSID looks up a user by Id.
> > If the user cannot be found,
> > On 2011/05/20 15:52:33, bradfitz wrote:
> >
> >> these docs assume the user knows what a SID is, which zero Unix people
> >>
> > know and
> >
> >> very few Windows people even know.
> >>
> >
> >  Be sure to mention Windows in here.
> >>
> >
> > Done.
> >
> >
> >
> >
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_unix....
> > src/pkg/os/user/lookup_unix.go:46: // This function panics if called on
> > Unix systems
> > On 2011/05/20 15:52:33, bradfitz wrote:
> >
> >> doesn't panic.  just returns an error.
> >>
> >
> > Done.
> >
> >
> >
> >
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_windo...
> > File src/pkg/os/user/lookup_windows.go (right):
> >
> >
> >
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/lookup_windo...
> > src/pkg/os/user/lookup_windows.go:68: func UserName() (name string, err
> > os.Error) {
> > On 2011/05/20 15:52:33, bradfitz wrote:
> >
> >> this function isn't exported on the unix side.  Did you mean to make
> >>
> > this
> >
> >> lowercase?
> >>
> >
> > No, it needs to be public. Otherwise there is no way to implement
> > similar test case for Windows. Or should the function be private to the
> > tests file?
> >
> 
> You can call lowercase stuff from tests in the same package.
> 
> 
> > syscall.getuid() won't work on Windows, the companion function would be
> > either a getsid() or the UserName() function.
> 
> 
> If you really need a new public function in the user package, it has to go
> in both posix & windows, not just one.
> 
> Maybe we can add:
> 
>     func Current() (*User, os.Error)
> 
> ... to both.
> 
> Would that work?
> 
> 
> >
> >
> > http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user.go
> > File src/pkg/os/user/user.go (right):
> >
> >
> >
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user.go#newc...
> > src/pkg/os/user/user.go:15: Uid      int // user id, only valid in POSIX
> > systems, 0 otherwise
> > On 2011/05/20 15:52:33, bradfitz wrote:
> >
> >> no need to say "0 otherwise".
> >>
> >
> >  You could also group these with a shared comment:
> >>
> >
> >       // For POSIX systems:
> >>      Uid   int  // user id
> >>      Gid   int  // primary group id
> >>
> >
> >       // For Windows:
> >>      SID   Id   // What is this? What is its type?
> >>
> >
> > I'll do it.
> >
> > Regarding the Id, maybe I am spoiled with years of software engineering,
> > but I thought it should be enough for the users to know that user.Id is
> > an black box ADT.
> >
> >
> >
> >
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user.go#newc...
> > src/pkg/os/user/user.go:19: Name     string // An empty string on
> > Windows platforms for the time being
> > On 2011/05/20 15:52:33, bradfitz wrote:
> >
> >> Name  string // User's name, if available.
> >>
> >
> > Done.
> >
> >
> >
> >
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user_test.go
> > File src/pkg/os/user/user_test.go (right):
> >
> >
> >
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user_test.go...
> > src/pkg/os/user/user_test.go:74: if err != nil && err.String() != "user:
> > Windows does not support lookup by userid" {
> > On 2011/05/20 15:52:33, bradfitz wrote:
> >
> >> error checking by string comparison is a bit gross, but in a test I
> >>
> > suppose it's
> >
> >> not the end of the world.
> >>
> >
> >  I wonder if you should just return syscall.EWINDOWS here instead, and
> >>
> > check for
> >
> >> that exact value?
> >>
> >
> > Ah ok. I'm only doing this at the unit test level, but I also don't like
> > it as it is now.
> >
> >
> >
> >
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user_test.go...
> > src/pkg/os/user/user_test.go:79: username, err := UserName()
> > On 2011/05/20 15:52:33, bradfitz wrote:
> >
> >> UserName shouldn't be part of the public API.  At least, it isn't now.
> >>
> >
> >  make this userName for now?  maybe we can unhide it later.
> >>
> >
> > As I said in another comment I need it to retrieve the username, and Go
> > developers on Windows will need it too.
> >
> > They won't be able to call syscall.getuid() to get the process owner.
> > Are they forced to wrap the Win32 function?
> >
> >
> >
> >
>
http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user_test.go...
> > src/pkg/os/user/user_test.go:89: t.Errorf("expected Uid of 0 got %d",
> > u.Uid)
> > On 2011/05/20 15:52:33, bradfitz wrote:
> >
> >> As a unix person, Uid/Gid of 0 have a very distinct meaning.  I wonder
> >>
> > if -1
> >
> >> might be a better "not implemented" value over 0?  Anybody have
> >>
> > preferences?
> >
> > You are right, I forgot about it.
> >
> >
> > http://codereview.appspot.com/4521053/
> >

The Current idea seems nice, I'll do as suggested.

[pjmlp, 2011-05-22 19:01:28]

Hello bradfitz@golang.org, alex.brainman@gmail.com, mattn.jp@gmail.com,
bradfitz@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[pjmlp, 2011-05-22 19:03:30]

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user_test.go
File src/pkg/os/user/user_test.go (right):

http://codereview.appspot.com/4521053/diff/17004/src/pkg/os/user/user_test.go...
src/pkg/os/user/user_test.go:74: if err != nil && err.String() != "user: Windows
does not support lookup by userid" {
On 2011/05/20 16:41:52, pjmlp wrote:
> On 2011/05/20 15:52:33, bradfitz wrote:
> > error checking by string comparison is a bit gross, but in a test I suppose
> it's
> > not the end of the world.
> > 
> > I wonder if you should just return syscall.EWINDOWS here instead, and check
> for
> > that exact value?
> 
> Ah ok. I'm only doing this at the unit test level, but I also don't like it as
> it is now.

I just submitted a new changeset, but left this test as it is, because I can
only refer to syscall.EWINDOWS on Windows builds.

[brainman, 2011-05-23 06:23:23]

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_window...
File src/pkg/os/user/lookup_windows.go (right):

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_window...
src/pkg/os/user/lookup_windows.go:35: // Returns the information about the
process owner
In Windows process, different threads could be executed as different users. It
happens inside servers a lot, when server thread changes its "current user" to
impersonates someone else on behalf of a client connected to the thread.

I'm not sure how/if we can support this behavior.

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_window...
src/pkg/os/user/lookup_windows.go:36: func Current() (*User, os.Error) {
I would use OpenProcessToken/OpenThreadToken + GetTokenInformation to retrieve
current user details instead, it gives you more information, and, probably,
faster to boot.

Then I would write a single function that

func getUserBySID(sid syscall.SID) (*User, os.Error)

which I would use here and in both Lookup and LookupSID. I think, it will make
your code clear, shorter and less repetitive.

[pjmlp, 2011-05-23 07:21:11]

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_window...
File src/pkg/os/user/lookup_windows.go (right):

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_window...
src/pkg/os/user/lookup_windows.go:35: // Returns the information about the
process owner
On 2011/05/23 06:23:24, brainman wrote:
> In Windows process, different threads could be executed as different users. It
> happens inside servers a lot, when server thread changes its "current user" to
> impersonates someone else on behalf of a client connected to the thread.
> 
> I'm not sure how/if we can support this behavior.

I am aware of this. But how would this play with goroutines as well?

Anyway the Current() was introduced, because I needed something similar like
getuid() for the unit tests. Just to follow the UNIX decisions.

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_window...
src/pkg/os/user/lookup_windows.go:36: func Current() (*User, os.Error) {
On 2011/05/23 06:23:24, brainman wrote:
> I would use OpenProcessToken/OpenThreadToken + GetTokenInformation to retrieve
> current user details instead, it gives you more information, and, probably,
> faster to boot.
> 
> Then I would write a single function that
> 
> func getUserBySID(sid syscall.SID) (*User, os.Error)
> 
> which I would use here and in both Lookup and LookupSID. I think, it will make
> your code clear, shorter and less repetitive.

I don't get your point, in Lookup you need to map an username to a SID, how
would getUserBySID() help?

[brainman, 2011-05-23 07:56:30]

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_window...
File src/pkg/os/user/lookup_windows.go (right):

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_window...
src/pkg/os/user/lookup_windows.go:35: // Returns the information about the
process owner
On 2011/05/23 07:21:11, pjmlp wrote:
> 
> ... But how would this play with goroutines as well?
> 

I don't know. There is runtime.LockOSThread(), but, it is too hacky ...

> Anyway the Current() was introduced, because I needed something similar like

I understand. I just want to add that there are 2 distinct Current(): one for
process, another for thread.

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_window...
src/pkg/os/user/lookup_windows.go:36: func Current() (*User, os.Error) {
On 2011/05/23 07:21:11, pjmlp wrote:
> 
> ..., in Lookup you need to map an username to a SID, how
> would getUserBySID() help?

You would still use syscall.LookupAccountName for that, but at least everything
else will be done in one getUserBySID() function.

But I'm not fussed about that. It just looks convoluted to me, perhaps, your
function names are not expressive enough.

[pjmlp, 2011-05-23 08:54:39]

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_window...
File src/pkg/os/user/lookup_windows.go (right):

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_window...
src/pkg/os/user/lookup_windows.go:36: func Current() (*User, os.Error) {
On 2011/05/23 07:56:31, brainman wrote:
> On 2011/05/23 07:21:11, pjmlp wrote:
> > 
> > ..., in Lookup you need to map an username to a SID, how
> > would getUserBySID() help?
> 
> You would still use syscall.LookupAccountName for that, but at least
everything
> else will be done in one getUserBySID() function.
> 
> But I'm not fussed about that. It just looks convoluted to me, perhaps, your
> function names are not expressive enough.

I wonder how many ms we would gain by that. I mean by doing a OpenProcessToken +
GetTokenInformation + getUserBySID, instead of the current scenario.

Still, I would follow your decision.

[brainman, 2011-05-23 11:16:51]

On 2011/05/23 08:54:39, pjmlp wrote:
> ...
> I wonder how many ms we would gain by that. ...
> 

I'm just guessing here, but I would think that "user database" is kept somewhere
on the network. So, I suspect, Windows would have to make a network round trip
or two (dns, domain controlers, ...) to answer some of your questions.

Alex

[pjmlp, 2011-05-23 12:03:42]

On 2011/05/23 11:16:51, brainman wrote:
> On 2011/05/23 08:54:39, pjmlp wrote:
> > ...
> > I wonder how many ms we would gain by that. ...
> > 
> 
> I'm just guessing here, but I would think that "user database" is kept
somewhere
> on the network. So, I suspect, Windows would have to make a network round trip
> or two (dns, domain controlers, ...) to answer some of your questions.
> 
> Alex

It depends on the configuration. If you are using domains, then yes. A LDAP,
Kerberos or ActiveDirectory server will be required to talk to.

If using SAM (local authentication) then not.

Anyway Windows also caches the network answers for some time.

[bradfitz, 2011-05-23 18:50:45]

Please update the CL description too, like:

os/user: Windows support

(text, if necessary)

Fixes issue nnnnn

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

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:14: 
extra blank line

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:41: // LookupSID looks up a user by Id. If the
user cannot be found,
s/by Id/by its Windows SID/.

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:43: // This function panics if called on Unix
systems
doesn't panic

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:45: return nil, os.NewError("user: LookupSID  not
supported on POSIX")
extra space

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:105: SID:      "",
unnecessary. this is the default value.

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_window...
File src/pkg/os/user/lookup_windows.go (right):

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_window...
src/pkg/os/user/lookup_windows.go:35: // Returns the information about the
process owner
On 2011/05/23 07:56:31, brainman wrote:
> On 2011/05/23 07:21:11, pjmlp wrote:
> > 
> > ... But how would this play with goroutines as well?
> > 
> 
> I don't know. There is runtime.LockOSThread(), but, it is too hacky ...
> 
> > Anyway the Current() was introduced, because I needed something similar like
> 
> I understand. I just want to add that there are 2 distinct Current(): one for
> process, another for thread.

I wouldn't worry about this yet. There will be many other complications to deal
with when/if that time comes, with Go running multiple Windows users in the same
process.

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

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/user.go#newco...
src/pkg/os/user/user.go:15: // For POSIX systems
is this gofmt'd?

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/user.go#newco...
src/pkg/os/user/user.go:33: return "user: unknown user SID " + string(e)
is this guaranteed to be a readable string?  or will it contain non-ASCII
characters?

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/user.go#newco...
src/pkg/os/user/user.go:54: type UserError string
is this error type worth it?  doesn't seem like it.

[pjmlp, 2011-05-23 20:45:19]

Thanks for the feedback, providing changes in a second.

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

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:14: 
On 2011/05/23 18:50:45, bradfitz wrote:
> extra blank line

Done.

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:41: // LookupSID looks up a user by Id. If the
user cannot be found,
On 2011/05/23 18:50:45, bradfitz wrote:
> s/by Id/by its Windows SID/.

Done.

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:43: // This function panics if called on Unix
systems
On 2011/05/23 18:50:45, bradfitz wrote:
> doesn't panic

Done.

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:45: return nil, os.NewError("user: LookupSID  not
supported on POSIX")
On 2011/05/23 18:50:45, bradfitz wrote:
> extra space

Done.

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/lookup_unix.g...
src/pkg/os/user/lookup_unix.go:105: SID:      "",
On 2011/05/23 18:50:45, bradfitz wrote:
> unnecessary. this is the default value.

Done.

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

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/user.go#newco...
src/pkg/os/user/user.go:15: // For POSIX systems
On 2011/05/23 18:50:45, bradfitz wrote:
> is this gofmt'd?

sorry no

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/user.go#newco...
src/pkg/os/user/user.go:33: return "user: unknown user SID " + string(e)
On 2011/05/23 18:50:45, bradfitz wrote:
> is this guaranteed to be a readable string?  or will it contain non-ASCII
> characters?

Not sure. I wanted to parallel UnknownUserIdError

http://codereview.appspot.com/4521053/diff/4005/src/pkg/os/user/user.go#newco...
src/pkg/os/user/user.go:54: type UserError string
On 2011/05/23 18:50:45, bradfitz wrote:
> is this error type worth it?  doesn't seem like it.

How to map all possible syscall errors? Unknown... does not seem a fit

[pjmlp, 2011-05-24 07:34:26]

Hello bradfitz@golang.org, alex.brainman@gmail.com, mattn.jp@gmail.com,
bradfitz@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[pjmlp, 2011-05-24 07:36:32]

On 2011/05/23 11:16:51, brainman wrote:
> On 2011/05/23 08:54:39, pjmlp wrote:
> > ...
> > I wonder how many ms we would gain by that. ...
> > 
> 
> I'm just guessing here, but I would think that "user database" is kept
somewhere
> on the network. So, I suspect, Windows would have to make a network round trip
> or two (dns, domain controlers, ...) to answer some of your questions.
> 
> Alex

I am yet to make this modification. Should I redo the way Current() is working?

--
Paulo

[brainman, 2011-05-24 07:39:59]

On 2011/05/24 07:36:32, pjmlp wrote:
> 
> ... Should I redo the way Current() is working?
> 

Let's leave it for now if it works. Let's get something / anything done. We can
change it later if we like.

Alex

[pjmlp, 2011-06-07 14:18:13]

On 2011/05/24 07:39:59, brainman wrote:
> On 2011/05/24 07:36:32, pjmlp wrote:
> > 
> > ... Should I redo the way Current() is working?
> > 
> 
> Let's leave it for now if it works. Let's get something / anything done. We
can
> change it later if we like.
> 
> Alex

Any update on this?

[bradfitz, 2011-06-07 15:09:20]

Looks like the Windows SID type is still a []byte too in the other CL.  I
thought we were changing that to a string.

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

http://codereview.appspot.com/4521053/diff/12008/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:42: // This returns an error if called on Unix
systems
missing period at the end.

http://codereview.appspot.com/4521053/diff/12008/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:47: // Current returns the user information about
the process owner
missing period at the end.

http://codereview.appspot.com/4521053/diff/12008/src/pkg/os/user/lookup_windo...
File src/pkg/os/user/lookup_windows.go (right):

http://codereview.appspot.com/4521053/diff/12008/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:35: // Returns the information about the
process owner
period

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

http://codereview.appspot.com/4521053/diff/12008/src/pkg/os/user/user.go#newc...
src/pkg/os/user/user.go:28: // UnknownUserIdError is returned by LookupSID when
doc doesn't match type name.  IdError vs SIDError.

also, is this even used?

http://codereview.appspot.com/4521053/diff/12008/src/pkg/os/user/user.go#newc...
src/pkg/os/user/user.go:54: type UserError string
I'd remove this type and just use an os.NewError or fmt.Errorf to make the error
you need whenever.

[pjmlp, 2011-06-10 19:52:24]

Will upload new CL.

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

http://codereview.appspot.com/4521053/diff/12008/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:42: // This returns an error if called on Unix
systems
On 2011/06/07 15:09:20, bradfitz wrote:
> missing period at the end.

Done.

http://codereview.appspot.com/4521053/diff/12008/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:47: // Current returns the user information about
the process owner
On 2011/06/07 15:09:20, bradfitz wrote:
> missing period at the end.

Done.

http://codereview.appspot.com/4521053/diff/12008/src/pkg/os/user/lookup_windo...
File src/pkg/os/user/lookup_windows.go (right):

http://codereview.appspot.com/4521053/diff/12008/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:35: // Returns the information about the
process owner
On 2011/06/07 15:09:20, bradfitz wrote:
> period

Done.

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

http://codereview.appspot.com/4521053/diff/12008/src/pkg/os/user/user.go#newc...
src/pkg/os/user/user.go:28: // UnknownUserIdError is returned by LookupSID when
On 2011/06/07 15:09:20, bradfitz wrote:
> doc doesn't match type name.  IdError vs SIDError.
> 
> also, is this even used?

Yes, on Windows. It was a copy paste error, fixed now.

http://codereview.appspot.com/4521053/diff/12008/src/pkg/os/user/user.go#newc...
src/pkg/os/user/user.go:54: type UserError string
On 2011/06/07 15:09:20, bradfitz wrote:
> I'd remove this type and just use an os.NewError or fmt.Errorf to make the
error
> you need whenever.

Replaced by os.NewError. Thanks for the hint, I still don't know much of the
standard library.

[pjmlp, 2011-06-10 19:57:27]

Hello bradfitz@golang.org, alex.brainman@gmail.com, mattn.jp@gmail.com,
bradfitz@google.com (cc: golang-dev@googlegroups.com),

Please take another look.