code review 4590050: net, syscall: interface for windows

net, syscall: interface for windows

[mattn, 2011-06-10 09:50:52]

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

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

[mikio, 2011-06-10 12:24:03]

CL description: net, syscall: interface for windows might be better.

http://codereview.appspot.com/4590050/diff/4001/src/pkg/net/interface_windows.go
File src/pkg/net/interface_windows.go (right):

http://codereview.appspot.com/4590050/diff/4001/src/pkg/net/interface_windows...
src/pkg/net/interface_windows.go:5: // Network interface identification for BSD
variants
s/BSD variants/Windows/

http://codereview.appspot.com/4590050/diff/4001/src/pkg/net/interface_windows...
src/pkg/net/interface_windows.go:17: iff_up          = 1 << 0
Please hold on these changes until CL4602044 get merged.

http://codereview.appspot.com/4590050/diff/4001/src/pkg/net/interface_windows...
src/pkg/net/interface_windows.go:25: func (ifi *Interface) IsUp() bool {
Likewise.

[mattn, 2011-06-10 12:34:53]

Thank you for your review.

http://codereview.appspot.com/4590050/diff/4001/src/pkg/net/interface_windows.go
File src/pkg/net/interface_windows.go (right):

http://codereview.appspot.com/4590050/diff/4001/src/pkg/net/interface_windows...
src/pkg/net/interface_windows.go:5: // Network interface identification for BSD
variants
On 2011/06/10 12:24:03, mikioh wrote:
> s/BSD variants/Windows/

Done.

http://codereview.appspot.com/4590050/diff/4001/src/pkg/net/interface_windows...
src/pkg/net/interface_windows.go:17: iff_up          = 1 << 0
On 2011/06/10 12:24:03, mikioh wrote:
> Please hold on these changes until CL4602044 get merged.

I think so. :)

http://codereview.appspot.com/4590050/diff/4001/src/pkg/net/interface_windows...
src/pkg/net/interface_windows.go:25: func (ifi *Interface) IsUp() bool {
On 2011/06/10 12:24:03, mikioh wrote:
> Likewise.

ok

[mattn, 2011-06-13 00:56:50]

Hello golang-dev@googlecode.com, mikioh.mikioh@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[brainman, 2011-06-14 02:57:10]

On 2011/06/13 00:56:50, mattn wrote:
> Hello mailto:golang-dev@googlecode.com, mailto:mikioh.mikioh@gmail.com (cc:
> mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

If we submit your change, no go program will be able to run on Windows 2000:

$ ./8.out.exe
panic: syscall: could not GetProcAddress for GetAdaptersAddresses

runtime.panic+0x9e /root/hg/go/src/pkg/runtime/proc.c:1060
        runtime.panic(0x47f13c, 0x109d01a0)
syscall.getSysProcAddr+0xa0 /root/hg/go/src/pkg/syscall/syscall_windows.go:94
        syscall.getSysProcAddr(0x77340000, 0x4bcd90, 0x14, 0x77990d96, 0x43c765,
...)
syscall.init+0xe01 /root/hg/go/src/pkg/syscall/exec_windows.go:-1899
        syscall.init()
os.init+0x4b /root/hg/go/src/pkg/os/signal_windows.go:26
        os.init()
strings.init+0x45 /root/hg/go/src/pkg/strings/strings.go:572
        strings.init()
regexp.init+0x46 /root/hg/go/src/pkg/regexp/regexp.go:1487
        regexp.init()
main.init+0x41 /root/hg/go/src/pkg/net/_testmain.go:43
        main.init()
runtime.mainstart+0x5 /root/hg/go/src/pkg/runtime/386/asm.s:91
        runtime.mainstart()
runtime.goexit /root/hg/go/src/pkg/runtime/proc.c:178
        runtime.goexit()
----- goroutine created by -----
_rt0_386+0xbf /root/hg/go/src/pkg/runtime/386/asm.s:80

Perhaps, you could find a way to do what you're trying to do that is compatible
with Windows 2000.

Alternatively, we could:

- Decide that we do not support Windows 2000 anymore;
- Redesign way we load dlls in syscall: at this moment we just load them all at
startup, we could change it to load them selectively, on "as used" basis, but I
do not know of a simple and efficient way of doing it yet.

Alex

[mattn, 2011-06-14 08:48:21]

I updated CL.

All of function that manipulating Interface/Addr are confined to
interface_windows.go

This should be working on Win2K, WinXp, or later.

[rsc, 2011-06-14 15:25:48]

Thanks for fixing this.

http://codereview.appspot.com/4590050/diff/9006/src/pkg/net/interface_windows.go
File src/pkg/net/interface_windows.go (right):

http://codereview.appspot.com/4590050/diff/9006/src/pkg/net/interface_windows...
src/pkg/net/interface_windows.go:270: //sys	getAdaptersAddresses(family uint32,
flags uint32, reserved uintptr, adapterAddresses *_IP_ADAPTER_ADDRESSES,
sizeOfPointer *uint32) (errcode uint32) = iphlpapi.GetAdaptersAddresses
I'm confused.  These comments only have an effect in syscall, but this is
package net.  What's going on?  Is there a file missing?

[mattn, 2011-06-15 00:12:11]

I did't do really checking.

I checked this patch with specifyign isWin2k = true forcely for Windows2000.
I hope that someone can check it. :)

http://codereview.appspot.com/4590050/diff/9006/src/pkg/net/interface_windows.go
File src/pkg/net/interface_windows.go (right):

http://codereview.appspot.com/4590050/diff/9006/src/pkg/net/interface_windows...
src/pkg/net/interface_windows.go:270: //sys	getAdaptersAddresses(family uint32,
flags uint32, reserved uintptr, adapterAddresses *_IP_ADAPTER_ADDRESSES,
sizeOfPointer *uint32) (errcode uint32) = iphlpapi.GetAdaptersAddresses
On 2011/06/14 15:25:48, rsc wrote:
> I'm confused.  These comments only have an effect in syscall, but this is
> package net.  What's going on?  Is there a file missing?
> 

Windows2000 don't have GetAdaptersAddres(), But it have GetIpAddrTable().
And windows200 don't support IPv6 APIs.

Then net package should choose APIs as checking the API is enabled.
I generated API wrapper with using mksyscall_windows.pl . And I left it for
re-gerateing wrapper of APIs.
But I won't change it. I'll remove it.

[mattn, 2011-06-15 00:55:24]

On 2011/06/15 00:12:11, mattn wrote:
> I did't do really checking.
> 
> I checked this patch with specifyign isWin2k = true forcely for Windows2000.
> I hope that someone can check it. :)
> 
>
http://codereview.appspot.com/4590050/diff/9006/src/pkg/net/interface_windows.go
> File src/pkg/net/interface_windows.go (right):
> 
>
http://codereview.appspot.com/4590050/diff/9006/src/pkg/net/interface_windows...
> src/pkg/net/interface_windows.go:270: //sys	getAdaptersAddresses(family
uint32,
> flags uint32, reserved uintptr, adapterAddresses *_IP_ADAPTER_ADDRESSES,
> sizeOfPointer *uint32) (errcode uint32) = iphlpapi.GetAdaptersAddresses
> On 2011/06/14 15:25:48, rsc wrote:
> > I'm confused.  These comments only have an effect in syscall, but this is
> > package net.  What's going on?  Is there a file missing?
> > 
> 
> Windows2000 don't have GetAdaptersAddres(), But it have GetIpAddrTable().
> And windows200 don't support IPv6 APIs.
> 
> Then net package should choose APIs as checking the API is enabled.
> I generated API wrapper with using mksyscall_windows.pl . And I left it for
> re-gerateing wrapper of APIs.
> But I won't change it. I'll remove it.

rsc, If you think that this have better to left for future re-generating, I'll
keep it.

[mattn, 2011-06-15 01:08:46]

Hello mikioh.mikioh@gmail.com, alex.brainman@gmail.com, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[brainman, 2011-06-15 01:32:48]

I tested your "win2k" version here. And it works just fine. TestInterface* tests
list my interfaces and addresses correctly.

Why don't you make your "win2k" version to be used everywhere. I think it is
good enough for now (none of windows net functions do ip6 anyway).

And, please, put everything back into syscall package, now that you've found
functions that will work for you on any windows os. Let's do what all other
packages do.

Thank you.

Alex

[mattn, 2011-06-15 01:52:59]

On 2011/06/15 01:32:48, brainman wrote:
> I tested your "win2k" version here. And it works just fine. TestInterface*
tests
> list my interfaces and addresses correctly.

Thank you for checking.

> Why don't you make your "win2k" version to be used everywhere. I think it is
> good enough for now (none of windows net functions do ip6 anyway).

GetIfTable/GetIfEntry don't work with IPv6. If some functions will be fixed to
work on IPv6, this part will be work well.

> And, please, put everything back into syscall package, now that you've found
> functions that will work for you on any windows os. Let's do what all other
> packages do.

interfaceTable2 is using GetAdaptersAddresses() yet. And windows2000 don't have
GetAdaptersAddresses().
When the API is not found, syscall occur error and panic. net package is calling
API dynamically.
i.e. it shouldn't back into syscall.

Thanks.

- Yasuhiro Matsumoto

[brainman, 2011-06-15 02:08:56]

On 2011/06/15 01:52:59, mattn wrote:
> 
> interfaceTable2 is using GetAdaptersAddresses() ...

I propose we forget about GetAdaptersAddresses() for now. I think it is more
important to keep code simple and compatible with as many OSes used as we can at
this moment. Your proposed code is far from simple. If you compare just size of
your source code to other OSes:

 4023 interface_bsd.go     
 4343 interface_linux.go   
16906 interface_windows.go 

it is not even close. Also, since you moved all your code from syscall package,
your code won't use any of our "os api code generation" facilities and have to
be handcrafted from no on. What about 64bit version?

> i.e. it shouldn't back into syscall.

I don't agree.

Alex

[mattn, 2011-06-15 02:30:55]

On 2011/06/15 02:08:56, brainman wrote:
> On 2011/06/15 01:52:59, mattn wrote:
> > 
> > interfaceTable2 is using GetAdaptersAddresses() ...
> 
> I propose we forget about GetAdaptersAddresses() for now. I think it is more
> important to keep code simple and compatible with as many OSes used as we can
at
> this moment. Your proposed code is far from simple. If you compare just size
of
> your source code to other OSes:
> 
>  4023 interface_bsd.go     
>  4343 interface_linux.go   
> 16906 interface_windows.go 
> 
> it is not even close. Also, since you moved all your code from syscall
package,
> your code won't use any of our "os api code generation" facilities and have to
> be handcrafted from no on. What about 64bit version?
> 
> > i.e. it shouldn't back into syscall.
> 
> I don't agree.

Ah, I understood your said. I agreed.
We should re-generate zsyscall_windows_xxx.go for 64bit.

I'll update interface_windows.go for moving "loading APIs", "structure", "const
definitions" to syscall package.
I will take some times for the changes, Sorry.

BTW, We may have to think about delay loading of APIs.

[brainman, 2011-06-15 02:39:38]

On 2011/06/15 02:30:55, mattn wrote:
> 
> BTW, We may have to think about delay loading of APIs.

Very good idea.

This program:

package main

func main() {
  println("Hello")
}

will not use any syscalls, but will load every dll we use in all go packages. I
don't think it is very cheap.

If you can think of a simple solution, I'm happy to listen.

Alex

[mattn, 2011-06-15 04:52:10]

On 2011/06/15 02:39:38, brainman wrote:
> On 2011/06/15 02:30:55, mattn wrote:
> > 
> > BTW, We may have to think about delay loading of APIs.
> 
> Very good idea.
> 
> This program:
> 
> package main
> 
> func main() {
>   println("Hello")
> }
> 
> will not use any syscalls, but will load every dll we use in all go packages.
I
> don't think it is very cheap.
> 
> If you can think of a simple solution, I'm happy to listen.
> 
> Alex

ok, lets discuss in another topic.

[mattn, 2011-06-15 04:52:32]

On 2011/06/15 02:08:56, brainman wrote:
> On 2011/06/15 01:52:59, mattn wrote:
> > 
> > interfaceTable2 is using GetAdaptersAddresses() ...
> 
> I propose we forget about GetAdaptersAddresses() for now. I think it is more
> important to keep code simple and compatible with as many OSes used as we can
at
> this moment. Your proposed code is far from simple. If you compare just size
of
> your source code to other OSes:
> 
>  4023 interface_bsd.go     
>  4343 interface_linux.go   
> 16906 interface_windows.go 
> 
> it is not even close. Also, since you moved all your code from syscall
package,
> your code won't use any of our "os api code generation" facilities and have to
> be handcrafted from no on. What about 64bit version?
> 
> > i.e. it shouldn't back into syscall.
> 
> I don't agree.
> 
> Alex

I uploaded changes.

[mattn, 2011-06-15 04:52:44]

Hello mikioh.mikioh@gmail.com, alex.brainman@gmail.com, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[mikio, 2011-06-15 10:59:25]

http://codereview.appspot.com/4590050/diff/13007/src/pkg/net/interface_window...
File src/pkg/net/interface_windows.go (right):

http://codereview.appspot.com/4590050/diff/13007/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:18: if ifi == nil {
Interface struct no longer takes any method.
Please refer to other OSes.

http://codereview.appspot.com/4590050/diff/13007/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:114: if row.OperStatus ==
syscall.IF_OPER_STATUS_CONNECTED || row.OperStatus ==
syscall.IF_OPER_STATUS_OPERATIONAL {
Not sure Windows but no need to check AdminStatus here?
If MS did implement MIB-like network interface management,
there will be administratively down but operational up state.

[mattn, 2011-06-15 11:16:09]

Hello mikioh.mikioh@gmail.com, alex.brainman@gmail.com, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[mattn, 2011-06-15 11:16:40]

Thank you

http://codereview.appspot.com/4590050/diff/13007/src/pkg/net/interface_window...
File src/pkg/net/interface_windows.go (right):

http://codereview.appspot.com/4590050/diff/13007/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:18: if ifi == nil {
On 2011/06/15 10:59:25, mikioh wrote:
> Interface struct no longer takes any method.
> Please refer to other OSes.

Done.

http://codereview.appspot.com/4590050/diff/13007/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:114: if row.OperStatus ==
syscall.IF_OPER_STATUS_CONNECTED || row.OperStatus ==
syscall.IF_OPER_STATUS_OPERATIONAL {
On 2011/06/15 10:59:25, mikioh wrote:
> Not sure Windows but no need to check AdminStatus here?
> If MS did implement MIB-like network interface management,
> there will be administratively down but operational up state.

Done.

[vincent.vanackere, 2011-06-15 12:43:29]

On Wed, Jun 15, 2011 at 4:08 AM,  <alex.brainman@gmail.com> wrote:
> On 2011/06/15 01:52:59, mattn wrote:
>
>> interfaceTable2 is using GetAdaptersAddresses() ...
>
> I propose we forget about GetAdaptersAddresses() for now. I think it is
> more important to keep code simple and compatible with as many OSes used
> as we can at this moment. Your proposed code is far from simple. If you
> compare just size of your source code to other OSes:

Sorry for coming so late into this discussion, but since Microsoft has
officially ended support for Windows 2000 since July 13, 2010, don't
you think it would be better to instead simplify the code the other
way by giving up Windows 2000 compatibility... ?  AFAICT a lot of
Windows software vendors have also already stopped supporting Win2K at
all.
Windows XP should be a much less constraining common denominator and
has been available since October 2001...

Vincent

[mikio, 2011-06-15 13:24:22]

http://codereview.appspot.com/4590050/diff/12009/src/pkg/net/interface_window...
File src/pkg/net/interface_windows.go (right):

http://codereview.appspot.com/4590050/diff/12009/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:71: if row.OperStatus ==
syscall.IF_OPER_STATUS_CONNECTED || row.OperStatus ==
syscall.IF_OPER_STATUS_OPERATIONAL || row.AdminStatus ==
syscall.MIB_IF_ADMIN_STATUS_UP {
Please refer to MIB_IFROW and MIB_IF_ROW2 structure properly.

if you look at the IFROW, if-statement should be:
if row.AdminStatus == MIB_IF_ADMIN_STATUS_UP &&
(row.OperStatus == MIB_IF_OPER_STATUS_CONNECTED ||
row.OperStatus == MIB_IF_OPER_STATUS_OPERATIONAL) {
// Not sure MIB_IF_OPER_STATUS_OPERATIONAL is equiv to UP... what's this?

if you look at the IF_ROW2, if-statement should be:
if row.AdminStatus == NET_IF_ADMIN_STATUS_UP &&
if row.OperStatus == IfOperStatusUp {

[mattn, 2011-06-15 17:02:52]

http://codereview.appspot.com/4590050/diff/12009/src/pkg/net/interface_window...
File src/pkg/net/interface_windows.go (right):

http://codereview.appspot.com/4590050/diff/12009/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:71: if row.OperStatus ==
syscall.IF_OPER_STATUS_CONNECTED || row.OperStatus ==
syscall.IF_OPER_STATUS_OPERATIONAL || row.AdminStatus ==
syscall.MIB_IF_ADMIN_STATUS_UP {
On 2011/06/15 13:24:22, mikioh wrote:
> Please refer to MIB_IFROW and MIB_IF_ROW2 structure properly.
> 
> if you look at the IFROW, if-statement should be:
> if row.AdminStatus == MIB_IF_ADMIN_STATUS_UP &&
> (row.OperStatus == MIB_IF_OPER_STATUS_CONNECTED ||
> row.OperStatus == MIB_IF_OPER_STATUS_OPERATIONAL) {
> // Not sure MIB_IF_OPER_STATUS_OPERATIONAL is equiv to UP... what's this?
> 
> if you look at the IF_ROW2, if-statement should be:
> if row.AdminStatus == NET_IF_ADMIN_STATUS_UP &&
> if row.OperStatus == IfOperStatusUp {

You can see the spec of OperStatus at:
http://msdn.microsoft.com/en-us/library/aa366836(v=vs.85).aspx

And dwAdminStatus at:
http://msdn.microsoft.com/en-us/library/aa366362(v=vs.85).aspx

It should equiv Up with your first code. I had mis-reading that. I'll fix in
later.
(I'm not from windows now)

Thanks.

[mattn, 2011-06-16 00:31:02]

http://codereview.appspot.com/4590050/diff/12009/src/pkg/net/interface_window...
File src/pkg/net/interface_windows.go (right):

http://codereview.appspot.com/4590050/diff/12009/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:71: if row.OperStatus ==
syscall.IF_OPER_STATUS_CONNECTED || row.OperStatus ==
syscall.IF_OPER_STATUS_OPERATIONAL || row.AdminStatus ==
syscall.MIB_IF_ADMIN_STATUS_UP {
On 2011/06/15 17:02:52, mattn wrote:
> On 2011/06/15 13:24:22, mikioh wrote:
> > Please refer to MIB_IFROW and MIB_IF_ROW2 structure properly.
> > 
> > if you look at the IFROW, if-statement should be:
> > if row.AdminStatus == MIB_IF_ADMIN_STATUS_UP &&
> > (row.OperStatus == MIB_IF_OPER_STATUS_CONNECTED ||
> > row.OperStatus == MIB_IF_OPER_STATUS_OPERATIONAL) {
> > // Not sure MIB_IF_OPER_STATUS_OPERATIONAL is equiv to UP... what's this?
> > 
> > if you look at the IF_ROW2, if-statement should be:
> > if row.AdminStatus == NET_IF_ADMIN_STATUS_UP &&
> > if row.OperStatus == IfOperStatusUp {
> 
> You can see the spec of OperStatus at:
> http://msdn.microsoft.com/en-us/library/aa366836%28v=vs.85%29.aspx
> 
> And dwAdminStatus at:
> http://msdn.microsoft.com/en-us/library/aa366362%28v=vs.85%29.aspx
> 
> It should equiv Up with your first code. I had mis-reading that. I'll fix in
> later.
> (I'm not from windows now)
> 
> Thanks.

Done.

[mattn, 2011-06-16 00:31:34]

Hello mikioh.mikioh@gmail.com, alex.brainman@gmail.com, rsc@golang.org,
vincent.vanackere@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[mikio, 2011-06-17 01:05:45]

LGTM.

I'll leave it to Windows experts because I saw the reliable 
evidence of many people still love Windows yesterday.

http://codereview.appspot.com/4590050/diff/19008/src/pkg/net/interface_window...
File src/pkg/net/interface_windows.go (right):

http://codereview.appspot.com/4590050/diff/19008/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:50: piftable :=
(*syscall.MIB_IFTABLE)(unsafe.Pointer(&heap[0]))
no hungarian notation pls.

http://codereview.appspot.com/4590050/diff/19008/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:56: var ptable *syscall.IP_INTERFACE_NAME_INFO
no hungarian notation pls.

http://codereview.appspot.com/4590050/diff/19008/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:88: if
tables[n].AccessType&syscall.IF_ACCESS_POINT_TO_MULTI_POINT != 0 {
Is this the right way to determine a multicast access capability 
on Windows?

I believe NBMA/P-MP are different capabilities on different 
concept. For example, InARP/NHRP can help us (I mean, layer 3 
guys) to make a broadcast, multicast access over NBMA/P-MP 
links. I'd like to suggest that if you are not sure this stuff, 
leave it or use it just as it is.

http://codereview.appspot.com/4590050/diff/19008/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:131: pipinfo :=
(*syscall.MIB_IPADDRTABLE)(unsafe.Pointer(&heap[0]))
no hungarian notation pls.

[mattn, 2011-06-17 06:38:07]

I found the way to get interface flags.
Then, I updated drastic.

On 2011/06/17 01:05:45, mikioh wrote:
> LGTM.
> 
> I'll leave it to Windows experts because I saw the reliable 
> evidence of many people still love Windows yesterday.
> 
>
http://codereview.appspot.com/4590050/diff/19008/src/pkg/net/interface_window...
> File src/pkg/net/interface_windows.go (right):
> 
>
http://codereview.appspot.com/4590050/diff/19008/src/pkg/net/interface_window...
> src/pkg/net/interface_windows.go:50: piftable :=
> (*syscall.MIB_IFTABLE)(unsafe.Pointer(&heap[0]))
> no hungarian notation pls.
> 
>
http://codereview.appspot.com/4590050/diff/19008/src/pkg/net/interface_window...
> src/pkg/net/interface_windows.go:56: var ptable
*syscall.IP_INTERFACE_NAME_INFO
> no hungarian notation pls.
> 
>
http://codereview.appspot.com/4590050/diff/19008/src/pkg/net/interface_window...
> src/pkg/net/interface_windows.go:88: if
> tables[n].AccessType&syscall.IF_ACCESS_POINT_TO_MULTI_POINT != 0 {
> Is this the right way to determine a multicast access capability 
> on Windows?
> 
> I believe NBMA/P-MP are different capabilities on different 
> concept. For example, InARP/NHRP can help us (I mean, layer 3 
> guys) to make a broadcast, multicast access over NBMA/P-MP 
> links. I'd like to suggest that if you are not sure this stuff, 
> leave it or use it just as it is.
> 
>
http://codereview.appspot.com/4590050/diff/19008/src/pkg/net/interface_window...
> src/pkg/net/interface_windows.go:131: pipinfo :=
> (*syscall.MIB_IPADDRTABLE)(unsafe.Pointer(&heap[0]))
> no hungarian notation pls.

[mikio, 2011-06-17 10:42:32]

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
File src/pkg/net/interface_windows.go (right):

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:16: func bytePtrToString(p *uint8) string {
Seems like not the right place here.
syscall/syscall.go or syscall/syscall_windows.go?

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:25: func utf16PtrToString(p *uint16) string {
Seems like not the right place here.
syscall/syscall_windows.go?

[mattn, 2011-06-17 10:49:38]

On 2011/06/17 10:42:32, mikioh wrote:
>
http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
> File src/pkg/net/interface_windows.go (right):
> 
>
http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
> src/pkg/net/interface_windows.go:16: func bytePtrToString(p *uint8) string {
> Seems like not the right place here.
> syscall/syscall.go or syscall/syscall_windows.go?
> 
>
http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
> src/pkg/net/interface_windows.go:25: func utf16PtrToString(p *uint16) string {
> Seems like not the right place here.
> syscall/syscall_windows.go?

bytePtrToString/utf16PtrToString is using pointer. not slice or array.
It seems that is bits dangerous for me.

Russ, brainman: how do you think about?

[mikio, 2011-06-17 12:30:42]

> bytePtrToString/utf16PtrToString is using pointer. not slice or array.
> It seems that is bits dangerous for me.

To me, there is no reason to make a new function that calls 
MultiByteToWideChar inside and returns string. Am I mssing
something?

[mikio, 2011-06-17 13:07:15]

> To me, there is no reason to make a new...

no reason you won't make a...

[mattn, 2011-06-17 15:21:16]

On 2011/06/17 13:07:15, mikioh wrote:
> > To me, there is no reason to make a new...
> 
> no reason you won't make a...

In my environment, MultiByteToWideChar/GetCP works for converting text from
cp932 to utf-8. Maybe, you also?

For ascii bytes which is like IP string(ex: "192.168.1.1"), it don't need to
call MultiByteToWideChar/GetCP. Then it call bytePtrToString.
And MultiByteToWideChar convert bytes encoded with locale to widechar(UCS-2).
not string. Then I added utf16PtrToString.

Certainty, it is not in need of stay at net package.

I want to hear other's minds. :)

レビューありがとうございます。

[brainman, 2011-06-22 01:30:37]

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
File src/pkg/net/interface_windows.go (right):

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:54: }
Replace the above by a call to something like this:

func getAdapterList() (*syscall.IP_ADAPTER_INFO, os.Error) {
        b := make([]byte, 1000)
        l := uint32(len(b))
        a := (*syscall.IP_ADAPTER_INFO)(unsafe.Pointer(&b[0]))
        e := syscall.GetAdaptersInfo(a, &l)
        if e == syscall.ERROR_BUFFER_OVERFLOW {
                b = make([]byte, l)
                a = (*syscall.IP_ADAPTER_INFO)(unsafe.Pointer(&b[0]))
                e = syscall.GetAdaptersInfo(a, &l)
        }
        if e != 0 {
                return nil, os.NewSyscallError("GetAdaptersInfo", int(e))
        }
        return a, nil
}

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:56: s, err := syscall.Socket(syscall.AF_INET,
syscall.SOCK_DGRAM, syscall.IPPROTO_UDP)
Again, I think your code will look simpler, if you put following into a separate
function, like

func getInterfaces() ([]syscall.INTERFACE_INFO, os.Error)

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:68: }
You should truncate iinfo to the length populated by WSAIoctl. From the manual:

>>>
The number of interfaces returned (number of structures returned in the buffer
pointed to by lpvOutBuffer parameter) can be determined based on the actual
length of the output buffer returned in lpcbBytesReturned parameter.
<<<

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:70: for ai != nil {
for ; ai != nil; ai = ai.Next {

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:112: name := string(row.Descr[:row.DescrLen-1])
I don't think it is appropriate you use row.Descr for the "name" field. I take
it "name" should uniquely identify an interface, I'm not sure if row.Descr will
do that.

I don't have an alternative, so leave it up to you.

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:113: wcl :=
syscall.MultiByteToWideChar(syscall.GetACP(), syscall.MB_PRECOMPOSED,
&row.Descr[0], int32(row.DescrLen), nil, 0)
I would put that in a separate function, something like

function MultiByteToString(...) (string, errno int)

Maybe even have it in syscall, so others can use it too.

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:113: wcl :=
syscall.MultiByteToWideChar(syscall.GetACP(), syscall.MB_PRECOMPOSED,
&row.Descr[0], int32(row.DescrLen), nil, 0)
Please, don't call syscall.MultiByteToWideChar with empty return buffer. Try
give it something reasonable first time round.

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:154: }
Replace the above by a call to getAdapterList, as before.

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:156: for ai != nil {
for ; ai != nil; ai = ai.Next {

http://codereview.appspot.com/4590050/diff/38001/src/pkg/syscall/syscall_wind...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4590050/diff/38001/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:471: //sys	WSAIoctl(s int32, iocc uint32,
inbuf uintptr, cbif uint32, outbuf uintptr, cbob uint32, cbbr *uint32,
overlapped *Overlapped, completionRoutine uintptr) (errno int) [failretval==-1]
= ws2_32.WSAIoctl
s/inbuf uintptr/inbuf *byte/
s/outbuf uintptr/outbuf *byte/

then you could use variables like
b := make([]byte, 100)
without unsafe.Pointer typecast.

http://codereview.appspot.com/4590050/diff/38001/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:494: //sys	MultiByteToWideChar(cp uint32,
flags uint32, mbs *byte, mbl int32, wcs *uint16, wcl int32) (written int32) =
kernel32.MultiByteToWideChar
s/written int32/written uint32, errno int/

http://codereview.appspot.com/4590050/diff/38001/src/pkg/syscall/ztypes_windo...
File src/pkg/syscall/ztypes_windows_386.go (right):

http://codereview.appspot.com/4590050/diff/38001/src/pkg/syscall/ztypes_windo...
src/pkg/syscall/ztypes_windows_386.go:584: type SockaddrGen [24]byte //
TODO(mattn)
Put TODO on the line above and provide a description of what todo.

http://codereview.appspot.com/4590050/diff/38001/src/pkg/syscall/ztypes_windo...
src/pkg/syscall/ztypes_windows_386.go:586: type INTERFACE_INFO struct {
s/INTERFACE_INFO/InterfaceInfo/

like all other types in here.

Same for all new types you've introduced.

[mattn, 2011-06-22 02:11:23]

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
File src/pkg/net/interface_windows.go (right):

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:16: func bytePtrToString(p *uint8) string {
On 2011/06/17 10:42:32, mikioh wrote:
> Seems like not the right place here.
> syscall/syscall.go or syscall/syscall_windows.go?

I left this for below

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:25: func utf16PtrToString(p *uint16) string {
On 2011/06/17 10:42:32, mikioh wrote:
> Seems like not the right place here.
> syscall/syscall_windows.go?

Removed

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:54: }
On 2011/06/22 01:30:37, brainman wrote:
> Replace the above by a call to something like this:
> 
> func getAdapterList() (*syscall.IP_ADAPTER_INFO, os.Error) {
>         b := make([]byte, 1000)
>         l := uint32(len(b))
>         a := (*syscall.IP_ADAPTER_INFO)(unsafe.Pointer(&b[0]))
>         e := syscall.GetAdaptersInfo(a, &l)
>         if e == syscall.ERROR_BUFFER_OVERFLOW {
>                 b = make([]byte, l)
>                 a = (*syscall.IP_ADAPTER_INFO)(unsafe.Pointer(&b[0]))
>                 e = syscall.GetAdaptersInfo(a, &l)
>         }
>         if e != 0 {
>                 return nil, os.NewSyscallError("GetAdaptersInfo", int(e))
>         }
>         return a, nil
> }

Done.

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:56: s, err := syscall.Socket(syscall.AF_INET,
syscall.SOCK_DGRAM, syscall.IPPROTO_UDP)
On 2011/06/22 01:30:37, brainman wrote:
> Again, I think your code will look simpler, if you put following into a
separate
> function, like
> 
> func getInterfaces() ([]syscall.INTERFACE_INFO, os.Error)

Done.

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:68: }
On 2011/06/22 01:30:37, brainman wrote:
> You should truncate iinfo to the length populated by WSAIoctl. From the
manual:
> 
> >>>
> The number of interfaces returned (number of structures returned in the buffer
> pointed to by lpvOutBuffer parameter) can be determined based on the actual
> length of the output buffer returned in lpcbBytesReturned parameter.
> <<<

Done.

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:70: for ai != nil {
On 2011/06/22 01:30:37, brainman wrote:
> for ; ai != nil; ai = ai.Next {

Done.

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:112: name := string(row.Descr[:row.DescrLen-1])
On 2011/06/22 01:30:37, brainman wrote:
> I don't think it is appropriate you use row.Descr for the "name" field. I take
> it "name" should uniquely identify an interface, I'm not sure if row.Descr
will
> do that.
> 
> I don't have an alternative, so leave it up to you.

Done.

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:113: wcl :=
syscall.MultiByteToWideChar(syscall.GetACP(), syscall.MB_PRECOMPOSED,
&row.Descr[0], int32(row.DescrLen), nil, 0)
On 2011/06/22 01:30:37, brainman wrote:
> Please, don't call syscall.MultiByteToWideChar with empty return buffer. Try
> give it something reasonable first time round.

Done.

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:154: }
On 2011/06/22 01:30:37, brainman wrote:
> Replace the above by a call to getAdapterList, as before.

Done.

http://codereview.appspot.com/4590050/diff/38001/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:156: for ai != nil {
On 2011/06/22 01:30:37, brainman wrote:
> for ; ai != nil; ai = ai.Next {

Done.

[mattn, 2011-06-22 02:11:50]

Hello mikioh.mikioh@gmail.com, alex.brainman@gmail.com, rsc@golang.org,
vincent.vanackere@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2011-06-22 05:47:45]

http://codereview.appspot.com/4590050/diff/36007/src/pkg/net/interface_window...
File src/pkg/net/interface_windows.go (right):

http://codereview.appspot.com/4590050/diff/36007/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:35: return nil,
os.NewSyscallError("GetAdaptersInfo", int(e))
s/int(e)/e/

http://codereview.appspot.com/4590050/diff/36007/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:43: return nil, os.NewSyscallError("Socket",
int(e))
s/int(e)/e/

http://codereview.appspot.com/4590050/diff/36007/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:52: return nil, os.NewSyscallError("WSAIoctl",
int(e))
s/int(e)/e/

http://codereview.appspot.com/4590050/diff/36007/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:82: return nil,
os.NewSyscallError("GetIfEntry", int(e))
s/int(e)/e/

http://codereview.appspot.com/4590050/diff/36007/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:116: name :=
bytePtrToString(&ai.AdapterName[0])
This is what my name is: "{F2CEBFA0-4E73-4791-AA3A-56CC7BC19F88}". I don't think
it is better then what you had before. But I have no better suggestion, so I'll
leave it up to you to decide.

http://codereview.appspot.com/4590050/diff/36007/src/pkg/syscall/syscall_wind...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4590050/diff/36007/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:471: //sys	WSAIoctl(s int32, iocc uint32,
inbuf uintptr, cbif uint32, outbuf uintptr, cbob uint32, cbbr *uint32,
overlapped *Overlapped, completionRoutine uintptr) (errno int) [failretval==-1]
= ws2_32.WSAIoctl
s/inbuf uintptr/inbuf *byte/
s/outbuf uintptr/outbuf *byte/

then you could use variables like b := make([]byte, 100) without unsafe.Pointer
typecast.

http://codereview.appspot.com/4590050/diff/36007/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:492: //sys	GetIfEntry(pIfRow *MIB_IFROW)
(errcode uint32) = iphlpapi.GetIfEntry
s/errcode uint32/errcode int/

All our errors are int.

http://codereview.appspot.com/4590050/diff/36007/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:493: //sys	GetAdaptersInfo(ai
*IP_ADAPTER_INFO, ol *uint32) (errcode uint32) = iphlpapi.GetAdaptersInfo
s/errcode uint32/errcode int/

http://codereview.appspot.com/4590050/diff/36007/src/pkg/syscall/ztypes_windo...
File src/pkg/syscall/ztypes_windows_386.go (right):

http://codereview.appspot.com/4590050/diff/36007/src/pkg/syscall/ztypes_windo...
src/pkg/syscall/ztypes_windows_386.go:577: type SockaddrGen [24]byte //
TODO(mattn)
Put TODO on the line above and provide a description of what todo.

http://codereview.appspot.com/4590050/diff/36007/src/pkg/syscall/ztypes_windo...
src/pkg/syscall/ztypes_windows_386.go:579: type INTERFACE_INFO struct {
s/INTERFACE_INFO/InterfaceInfo/

like all other types in here.

Same for all new types you've introduced.

[mattn, 2011-06-22 06:15:35]

http://codereview.appspot.com/4590050/diff/36007/src/pkg/net/interface_window...
File src/pkg/net/interface_windows.go (right):

http://codereview.appspot.com/4590050/diff/36007/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:35: return nil,
os.NewSyscallError("GetAdaptersInfo", int(e))
On 2011/06/22 05:47:45, brainman wrote:
> s/int(e)/e/

Done.

http://codereview.appspot.com/4590050/diff/36007/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:43: return nil, os.NewSyscallError("Socket",
int(e))
On 2011/06/22 05:47:45, brainman wrote:
> s/int(e)/e/

Done.

http://codereview.appspot.com/4590050/diff/36007/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:52: return nil, os.NewSyscallError("WSAIoctl",
int(e))
On 2011/06/22 05:47:45, brainman wrote:
> s/int(e)/e/

Done.

http://codereview.appspot.com/4590050/diff/36007/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:82: return nil,
os.NewSyscallError("GetIfEntry", int(e))
On 2011/06/22 05:47:45, brainman wrote:
> s/int(e)/e/

Done.

http://codereview.appspot.com/4590050/diff/36007/src/pkg/net/interface_window...
src/pkg/net/interface_windows.go:116: name :=
bytePtrToString(&ai.AdapterName[0])
On 2011/06/22 05:47:45, brainman wrote:
> This is what my name is: "{F2CEBFA0-4E73-4791-AA3A-56CC7BC19F88}". I don't
think
> it is better then what you had before. But I have no better suggestion, so
I'll
> leave it up to you to decide.

This should be ID. And we can remove MultiByteToWideChar/GetACP from syscall.
Then I want to keep this line(using AdapterName).

http://codereview.appspot.com/4590050/diff/36007/src/pkg/syscall/syscall_wind...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4590050/diff/36007/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:471: //sys	WSAIoctl(s int32, iocc uint32,
inbuf uintptr, cbif uint32, outbuf uintptr, cbob uint32, cbbr *uint32,
overlapped *Overlapped, completionRoutine uintptr) (errno int) [failretval==-1]
= ws2_32.WSAIoctl
On 2011/06/22 05:47:45, brainman wrote:
> s/inbuf uintptr/inbuf *byte/
> s/outbuf uintptr/outbuf *byte/
> 
> then you could use variables like b := make([]byte, 100) without
unsafe.Pointer
> typecast.

Done.

http://codereview.appspot.com/4590050/diff/36007/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:492: //sys	GetIfEntry(pIfRow *MIB_IFROW)
(errcode uint32) = iphlpapi.GetIfEntry
On 2011/06/22 05:47:45, brainman wrote:
> s/errcode uint32/errcode int/
> 
> All our errors are int.

Done.

http://codereview.appspot.com/4590050/diff/36007/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:493: //sys	GetAdaptersInfo(ai
*IP_ADAPTER_INFO, ol *uint32) (errcode uint32) = iphlpapi.GetAdaptersInfo
On 2011/06/22 05:47:45, brainman wrote:
> s/errcode uint32/errcode int/

Done.

http://codereview.appspot.com/4590050/diff/36007/src/pkg/syscall/ztypes_windo...
File src/pkg/syscall/ztypes_windows_386.go (right):

http://codereview.appspot.com/4590050/diff/36007/src/pkg/syscall/ztypes_windo...
src/pkg/syscall/ztypes_windows_386.go:577: type SockaddrGen [24]byte //
TODO(mattn)
On 2011/06/22 05:47:45, brainman wrote:
> Put TODO on the line above and provide a description of what todo.

Done.

http://codereview.appspot.com/4590050/diff/36007/src/pkg/syscall/ztypes_windo...
src/pkg/syscall/ztypes_windows_386.go:579: type INTERFACE_INFO struct {
On 2011/06/22 05:47:45, brainman wrote:
> s/INTERFACE_INFO/InterfaceInfo/
> 
> like all other types in here.
> 
> Same for all new types you've introduced.

Done.

[mattn, 2011-06-22 06:16:09]

Hello mikioh.mikioh@gmail.com, alex.brainman@gmail.com, rsc@golang.org,
vincent.vanackere@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2011-06-22 07:08:44]

LGTM.

http://codereview.appspot.com/4590050/diff/33004/src/pkg/syscall/syscall_wind...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4590050/diff/33004/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:471: //sys	WSAIoctl(s int32, iocc uint32,
inbuf *byte, cbif uint32, outbuf uintptr, cbob uint32, cbbr *uint32, overlapped
*Overlapped, completionRoutine uintptr) (errno int) [failretval==-1] =
ws2_32.WSAIoctl
s/outbuf uintptr/outbuf *byte/

http://codereview.appspot.com/4590050/diff/33004/src/pkg/syscall/ztypes_windo...
File src/pkg/syscall/ztypes_windows_386.go (right):

http://codereview.appspot.com/4590050/diff/33004/src/pkg/syscall/ztypes_windo...
src/pkg/syscall/ztypes_windows_386.go:631: type MibIpAddrRow struct {
Don't need that now. Please remove.

[mattn, 2011-06-22 07:17:07]

http://codereview.appspot.com/4590050/diff/33004/src/pkg/syscall/syscall_wind...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4590050/diff/33004/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:471: //sys	WSAIoctl(s int32, iocc uint32,
inbuf *byte, cbif uint32, outbuf uintptr, cbob uint32, cbbr *uint32, overlapped
*Overlapped, completionRoutine uintptr) (errno int) [failretval==-1] =
ws2_32.WSAIoctl
On 2011/06/22 07:08:44, brainman wrote:
> s/outbuf uintptr/outbuf *byte/

Done.

http://codereview.appspot.com/4590050/diff/33004/src/pkg/syscall/ztypes_windo...
File src/pkg/syscall/ztypes_windows_386.go (right):

http://codereview.appspot.com/4590050/diff/33004/src/pkg/syscall/ztypes_windo...
src/pkg/syscall/ztypes_windows_386.go:631: type MibIpAddrRow struct {
On 2011/06/22 07:08:44, brainman wrote:
> Don't need that now. Please remove.

Done.

[brainman, 2011-06-22 23:50:19]

More comments anyone?
If no more takers, I'll submit this.

Alex

[rsc, 2011-06-22 23:52:23]

LGTM

I'm happy if everyone else is.

[brainman, 2011-06-22 23:55:06]

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

net, syscall: interface for windows

R=mikioh.mikioh, alex.brainman, rsc, vincent.vanackere
CC=golang-dev
http://codereview.appspot.com/4590050

Committer: Alex Brainman <alex.brainman@gmail.com>