code review 3790044: net: windows/amd64 port

net: windows/amd64 port

[vcc, 2010-12-31 05:16:20]

Hello golang-dev@googlegroups.com,

I'd like you to review this change.

[rsc, 2011-01-19 19:12:19]

Thanks for putting this together, but I'd like to see
a lot less copying of code.  We should be able to
share everything that was shared before.  It's okay
if we have to change one or two signatures to make
that happen.

http://codereview.appspot.com/3790044/diff/2001/src/pkg/net/ipsock_windows_38...
File src/pkg/net/ipsock_windows_386.go (right):

http://codereview.appspot.com/3790044/diff/2001/src/pkg/net/ipsock_windows_38...
src/pkg/net/ipsock_windows_386.go:14: // Should we try to use the IPv4 socket
interface if we're
Again, too much code duplication.

[vcc, 2011-01-28 16:20:14]

PTAL

[rsc, 2011-01-31 20:01:30]

I'd still prefer to see less code duplication.

http://codereview.appspot.com/3790044/diff/8001/src/pkg/net/Makefile
File src/pkg/net/Makefile (right):

http://codereview.appspot.com/3790044/diff/8001/src/pkg/net/Makefile#newcode40
src/pkg/net/Makefile:40: GOFILES_linux=\
This list is wrong.  It lists many files multiple times.

http://codereview.appspot.com/3790044/diff/8001/src/pkg/net/ipsock_windows.go
File src/pkg/net/ipsock_windows.go (right):

http://codereview.appspot.com/3790044/diff/8001/src/pkg/net/ipsock_windows.go...
src/pkg/net/ipsock_windows.go:94: func getip(fd syscall.HANDLE, remote bool) (ip
[]byte, port int, ok bool) {
This is the only use of HANDLE in this file.  
Much of the rest of the code looks like it is copied
verbatim from ipsock.go and did not need changes.
It seems like a better approach would be to put just getip
into its own file and then not duplicate everything else.

Having two copies will cause maintenance headaches
down the road as the package changes.

http://codereview.appspot.com/3790044/diff/8001/src/pkg/net/sock_windows.go
File src/pkg/net/sock_windows.go (right):

http://codereview.appspot.com/3790044/diff/8001/src/pkg/net/sock_windows.go#n...
src/pkg/net/sock_windows.go:24: func socket(net string, f, p, t int, la, ra
syscall.Sockaddr, toAddr func(syscall.Sockaddr) Addr) (fd *netFD, err os.Error)
{
Same comment about code duplication.

http://codereview.appspot.com/3790044/diff/8001/src/pkg/net/sock_windows.go#n...
src/pkg/net/sock_windows.go:71: func setsockoptInt(fd syscall.HANDLE, level, opt
int, value int) os.Error {
I'd suggest making this take fd *netFD, like setReadBuffer does,
and then you can avoid saying HANDLE.

[vcc, 2011-07-13 13:45:36]

PTAL

PASS test.

[rsc, 2011-07-13 19:44:10]

LGTM

fantastic

[rsc, 2011-07-13 19:44:24]

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

net: windows/amd64 port

R=rsc, alex.brainman
CC=golang-dev
http://codereview.appspot.com/3790044

Committer: Russ Cox <rsc@golang.org>

[brainman, 2011-07-14 01:41:46]

Sorry, I'm late. But I have some questions.

Alex

http://codereview.appspot.com/3790044/diff/16001/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

http://codereview.appspot.com/3790044/diff/16001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:496: e = syscall.Setsockopt(s, syscall.SOL_SOCKET,
syscall.SO_UPDATE_ACCEPT_CONTEXT, (*byte)(unsafe.Pointer(&fd.sysfd)),
int32(unsafe.Sizeof(fd.sysfd)))
I can't see any difference between old and new code. Can you? Except that new
code is less readable.

http://codereview.appspot.com/3790044/diff/16001/src/pkg/syscall/ztypes_windo...
File src/pkg/syscall/ztypes_windows_amd64.go (right):

http://codereview.appspot.com/3790044/diff/16001/src/pkg/syscall/ztypes_windo...
src/pkg/syscall/ztypes_windows_amd64.go:13: Description  [WSADESCRIPTION_LEN +
1]byte
I think this is wrong. Here is:

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

Where did you find your information?

http://codereview.appspot.com/3790044/diff/16001/src/pkg/syscall/ztypes_windo...
src/pkg/syscall/ztypes_windows_amd64.go:21: Port    uint16
Same here. I think it should be:

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

Where did you find your information?

[rsc, 2011-07-14 03:02:04]

>
http://codereview.appspot.com/3790044/diff/16001/src/pkg/net/fd_windows.go#ne...
> src/pkg/net/fd_windows.go:496: e = syscall.Setsockopt(s,
> syscall.SOL_SOCKET, syscall.SO_UPDATE_ACCEPT_CONTEXT,
> (*byte)(unsafe.Pointer(&fd.sysfd)), int32(unsafe.Sizeof(fd.sysfd)))
> I can't see any difference between old and new code. Can you? Except
> that new code is less readable.

I think it's because fd.sysfd is 64 bits on 64-bit
but before it was calling SetsockoptInt.

I don't know about the rest.

[vcc, 2011-07-14 03:23:37]

2011/7/14  <alex.brainman@gmail.com>:
> Sorry, I'm late. But I have some questions.
>
> Alex
>
>
> http://codereview.appspot.com/3790044/diff/16001/src/pkg/net/fd_windows.go
> File src/pkg/net/fd_windows.go (right):
>
>
http://codereview.appspot.com/3790044/diff/16001/src/pkg/net/fd_windows.go#ne...
> src/pkg/net/fd_windows.go:496: e = syscall.Setsockopt(s,
> syscall.SOL_SOCKET, syscall.SO_UPDATE_ACCEPT_CONTEXT,
> (*byte)(unsafe.Pointer(&fd.sysfd)), int32(unsafe.Sizeof(fd.sysfd)))
> I can't see any difference between old and new code. Can you? Except
> that new code is less readable.

as Russ says, fd.sysfd is Handle, 64 bit.

>
>
http://codereview.appspot.com/3790044/diff/16001/src/pkg/syscall/ztypes_windo...
> File src/pkg/syscall/ztypes_windows_amd64.go (right):
>
>
http://codereview.appspot.com/3790044/diff/16001/src/pkg/syscall/ztypes_windo...
> src/pkg/syscall/ztypes_windows_amd64.go:13: Description
> [WSADESCRIPTION_LEN + 1]byte
> I think this is wrong. Here is:
>
> http://msdn.microsoft.com/en-us/library/ms741563%28v=vs.85%29.aspx
>
> Where did you find your information?
>
>
http://codereview.appspot.com/3790044/diff/16001/src/pkg/syscall/ztypes_windo...
> src/pkg/syscall/ztypes_windows_amd64.go:21: Port    uint16
> Same here. I think it should be:
>
> http://msdn.microsoft.com/en-us/library/ms740150%28v=vs.85%29.aspx
>
> Where did you find your information?

search .h in mingw-w64 or vc++, you will find the define:

typedef struct WSAData {
        WORD                    wVersion;
        WORD                    wHighVersion;
#ifdef _WIN64
        unsigned short          iMaxSockets;
        unsigned short          iMaxUdpDg;
        char FAR *              lpVendorInfo;
        char                    szDescription[WSADESCRIPTION_LEN+1];
        char                    szSystemStatus[WSASYS_STATUS_LEN+1];
#else
        char                    szDescription[WSADESCRIPTION_LEN+1];
        char                    szSystemStatus[WSASYS_STATUS_LEN+1];
        unsigned short          iMaxSockets;
        unsigned short          iMaxUdpDg;
        char FAR *              lpVendorInfo;
#endif
} WSADATA;

struct  servent {
        char    FAR * s_name;           /* official service name */
        char    FAR * FAR * s_aliases;  /* alias list */
#ifdef _WIN64
        char    FAR * s_proto;          /* protocol to use */
        short   s_port;                 /* port # */
#else
        short   s_port;                 /* port # */
        char    FAR * s_proto;          /* protocol to use */
#endif
};

>
> http://codereview.appspot.com/3790044/
>

[brainman, 2011-07-14 04:37:03]

On 2011/07/14 03:02:04, rsc wrote:
> >
> I think it's because fd.sysfd is 64 bits on 64-bit
> but before it was calling SetsockoptInt.
> 

You're correct. I take back my argument.

Alex

[brainman, 2011-07-14 04:39:25]

On 2011/07/14 03:23:37, vcc wrote:
> 
> search .h in mingw-w64 or vc++, you will find the define:
> 

I did. You are correct.

Just wandering why they shuffled these fields around? ...
Perhaps to confuse use <g>!

Thanks for explaining. LGTM

Alex