|
|
Descriptionsyscall: add GetsockoptInt, GetsockoptInet4Addr for windows
Patch Set 1 #Patch Set 2 : diff -r 5c4859bc123f https://code.google.com/p/go #Patch Set 3 : diff -r 4965beed4492 https://code.google.com/p/go #
Total comments: 4
Patch Set 4 : diff -r 91c1c2d6e2ff https://code.google.com/p/go #
Total comments: 3
MessagesTotal messages: 13
Hello alex.brainman@gmail.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
Your new test fails: --- FAIL: TestIPv4SocketOptions (0.00 seconds) socket_windows_test.go:15: syscall.Socket failed: Either the application has not called WSAStartup, or WSAStartup failed. FAIL Alex http://codereview.appspot.com/6506063/diff/3/src/pkg/syscall/socket_windows_t... File src/pkg/syscall/socket_windows_test.go (right): http://codereview.appspot.com/6506063/diff/3/src/pkg/syscall/socket_windows_t... src/pkg/syscall/socket_windows_test.go:12: func TestIPv4SocketOptions(t *testing.T) { I do not know what all these options do. I do not think I am qualified to review this. http://codereview.appspot.com/6506063/diff/3/src/pkg/syscall/socket_windows_t... src/pkg/syscall/socket_windows_test.go:23: syscall.SetNonblock(s, true) syscall.SetNonblock is NOOP. Please, remove it. http://codereview.appspot.com/6506063/diff/3/src/pkg/syscall/syscall_windows.go File src/pkg/syscall/syscall_windows.go (right): http://codereview.appspot.com/6506063/diff/3/src/pkg/syscall/syscall_windows.... src/pkg/syscall/syscall_windows.go:594: func GetsockoptInt(fd Handle, level, opt int) (int, error) { It is 4 lines of code. Are you sure, we must have this function in syscall? http://codereview.appspot.com/6506063/diff/3/src/pkg/syscall/syscall_windows.... src/pkg/syscall/syscall_windows.go:733: func GetsockoptInet4Addr(fd Handle, level, opt int) ([4]byte, error) { Same. And this function is new.
Sign in to reply to this message.
Thanks for your review. On 2012/09/04 06:50:09, brainman wrote: > Your new test fails: > > --- FAIL: TestIPv4SocketOptions (0.00 seconds) > socket_windows_test.go:15: syscall.Socket failed: Either the application > has not called WSAStartup, or WSAStartup failed. > FAIL ah, I missed those two, also looks like procgetsockopt doesn't exist in syscall entries, hmm. > src/pkg/syscall/syscall_windows.go:594: func GetsockoptInt(fd Handle, level, opt > int) (int, error) { > It is 4 lines of code. Are you sure, we must have this function in syscall? As same as other platforms, that function handles several int-type (for now 32-bit) options for both IPv4 and IPv6, I think so.
Sign in to reply to this message.
On 2012/09/04 09:07:26, mikio wrote: > > > > --- FAIL: TestIPv4SocketOptions (0.00 seconds) > > socket_windows_test.go:15: syscall.Socket failed: Either the application > > has not called WSAStartup, or WSAStartup failed. > > FAIL > > ah, I missed those two, also looks like procgetsockopt > doesn't exist in syscall entries, hmm. > As per Microsoft documentation, you must call WSAStartup before you do anything else: http://msdn.microsoft.com/en-us/library/windows/desktop/ms742213%28v=vs.85%29... "... The WSAStartup function must be the first Windows Sockets function called by an application or DLL ..." Our net package does call WSAStartup under the covers. So, if you move your new test there, it should avoid this problem. > As same as other platforms, that function handles several > int-type (for now 32-bit) options for both IPv4 and IPv6, > I think so. I think we all agreed that syscall packages for different os platforms do not have to look the same. I think, if you want some "commonality" for os independent code, you should expose it somewhere outside of syscall. Alex
Sign in to reply to this message.
Thanks for the comments. On Wed, Sep 5, 2012 at 9:23 AM, <alex.brainman@gmail.com> wrote: >> As same as other platforms, that function handles several >> int-type (for now 32-bit) options for both IPv4 and IPv6, >> I think so. > > I think we all agreed that syscall packages for different os platforms > do not have to look the same. Yes. > I think, if you want some "commonality" > for os independent code, you should expose it somewhere outside of > syscall. I guess it's not commonality, I just need a sort of int-type options aggregator depends on windows, so just for windows. But... another side of me agree with you, don't mess the package syscall anymore... hmm... If you feel something uncomfortable on this then I will stop to consider this approach and will re-implement those platform-dependent socket options stuff to net, go.net/ipv4 and go.net/ipv6 perhaps or wherever appropriate packages.
Sign in to reply to this message.
On 2012/09/05 04:08:45, mikio wrote: > > I guess it's not commonality, I just need a sort of int-type options > aggregator depends on windows, so just for windows. I do not understand your explanation. Feel free to try again, if you like. > If you feel something uncomfortable on this then I will stop to consider > this approach and will re-implement those platform-dependent socket > options stuff to net, go.net/ipv4 and go.net/ipv6 perhaps or wherever > appropriate packages. If it saves some code somewhere else, then I have nothing against it. But the CL only adds new code that is not used anywhere in the Go tree. Perhaps, if you want to make this change as part of that other bigger change, then things will become clearer for reviewers. Alex
Sign in to reply to this message.
On 2012/09/05 04:30:16, brainman wrote: > If it saves some code somewhere else, then I have nothing against it. But the CL > only adds new code that is not used anywhere in the Go tree. Perhaps, if you > want to make this change as part of that other bigger change, then things will > become clearer for reviewers. Got it, thanks for your insight and sorry for the lack of explanation. This CL is a part of CL 6482044 that provides IP-level socket options for IPv4 and ensures windows IP-level socket options for net and go.net/ipv4 packages. Does this make sense?
Sign in to reply to this message.
Hello alex.brainman@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Sorry, but I still fail to see the need for these changes. Perhaps, we could keep the test, but since I do not understand what it does, I can't say if it is good idea or not. Alex http://codereview.appspot.com/6506063/diff/8002/src/pkg/syscall/socket_window... File src/pkg/syscall/socket_windows_test.go (right): http://codereview.appspot.com/6506063/diff/8002/src/pkg/syscall/socket_window... src/pkg/syscall/socket_windows_test.go:46: if err := syscall.SetsockoptIPMreq(fd, syscall.IPPROTO_IP, syscall.IP_ADD_MEMBERSHIP, mreq); err != nil { s/fd/s/ otherwise it fails to build # GOOS=windows go test -c # syscall_test ./socket_windows_test.go:46: undefined: fd http://codereview.appspot.com/6506063/diff/8002/src/pkg/syscall/syscall_windo... File src/pkg/syscall/syscall_windows.go (right): http://codereview.appspot.com/6506063/diff/8002/src/pkg/syscall/syscall_windo... src/pkg/syscall/syscall_windows.go:712: func GetsockoptInt(fd Handle, level, opt int) (int, error) { This function is not used by go.net. Why do we need to implement it? http://codereview.appspot.com/6506063/diff/8002/src/pkg/syscall/syscall_windo... src/pkg/syscall/syscall_windows.go:729: func GetsockoptInet4Addr(fd Handle, level, opt int) ([4]byte, error) { This is a new function. You said that this CL simplify code in go.net, but looking there: # grep GetsockoptInet4Addr * -r ipv4/sockopt_bsd.go: a, err := syscall.GetsockoptInet4Addr(fd, syscall.IPPROTO_IP, syscall.IP_MULTICAST_IF) ipv4/sockopt_windows.go: a, err := syscall.GetsockoptInet4Addr(fd, syscall.IPPROTO_IP, syscall.IP_MULTICAST_IF) # I can see it is called once from ipv4/sockopt_windows.go. You might as well implement it in go.net then.
Sign in to reply to this message.
On Fri, Sep 7, 2012 at 10:55 AM, <alex.brainman@gmail.com> wrote: > Sorry, but I still fail to see the need for these changes. Perhaps, we > could keep the test, but since I do not understand what it does, I can't > say if it is good idea or not. > otherwise it fails to build oops, sorry. > I can see it is called once from ipv4/sockopt_windows.go. You might as > well implement it in go.net then. yup, looks like that's the point. I now plan to implement go.net/ipv4 and go.net/ipv6. those two will use Get/SetsockoptInt, SetsockoptIPMreq, IPv6Mreq and siblings. I think we have two options. a) syscall provides Setsockopt and Getsockopt other optional thin wrappers like GetsockoptInt, SetsockoptIPMreq, IPv6Mreq, blah blah should be implemented in each package b) syscall provides a bit detailed GetsockoptInt, SetsockoptIPMreq, etc (b) works well for Unix families; BSD bros, a cousin Darwin and a second cousin Liunx but I'm not sure for Windows families including 2000, XP, Vista and newcomers. Do you have any suggestions or ideas?
Sign in to reply to this message.
On 2012/09/07 04:31:12, brainman wrote: > Once something is in the main Go tree, it is hard to change it. I would keep > everything in http://go.net. ack, thanks!
Sign in to reply to this message.
|