code review 12023043: net: add dial, listenStream and listenDatagram methods to netFD

net: add dial, listenStream and listenDatagram methods to netFD

This CL refactors the existing listenerSockaddr function into several
methods on netFD.

This is in preparation for runtime-integrated network pollster for BSD
variants.

Update issue 5199

[mikio, 2013-08-03 07:06:46]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[mikio, 2013-08-03 14:18:31]

ptal

[dfc, 2013-08-04 01:32:40]

LGTM. Please wait for Alex or Dmitry to try it on Windows systems.

Some typos in the description, a suggested replacement:

This CL refactors up the existing listenerSockaddr function into several methods
on *netFD.

https://codereview.appspot.com/12023043/diff/33001/src/pkg/net/sock_posix.go
File src/pkg/net/sock_posix.go (right):

https://codereview.appspot.com/12023043/diff/33001/src/pkg/net/sock_posix.go#...
src/pkg/net/sock_posix.go:93: 
The setXXX names aren't the best, but I can't think of a better suggestion at
the moment. Please add a comment to this method and it's friends below
describing their intent.

https://codereview.appspot.com/12023043/diff/33001/src/pkg/net/sock_posix.go#...
src/pkg/net/sock_posix.go:180: fd.setAddr(toAddr(lsa), nil)
suggestion, could you return the err on line 179 on line 181 ?

[mikio, 2013-08-04 03:19:51]

On Sun, Aug 4, 2013 at 10:32 AM,  <dave@cheney.net> wrote

> The setXXX names aren't the best, but I can't think of a better
> suggestion at the moment. Please add a comment to this method and it's
> friends below describing their intent.

In the end of a series of CLs  we'll use those like:

builder := func(fd *netFD) {
    return fd.setDialer(laddr, raddr, ...)
}

Hm, buildDialer? constructDialer? setDialer?

> src/pkg/net/sock_posix.go:180: fd.setAddr(toAddr(lsa), nil)
> suggestion, could you return the err on line 179 on line 181 ?

Is that a critical condition case? I don't think so. But very welcome
If you can describe the behavior of syscall.Getsockname for several
address families across over platforms.

[mikio, 2013-08-04 04:09:07]

PTAL

https://codereview.appspot.com/12023043/diff/33001/src/pkg/net/sock_posix.go
File src/pkg/net/sock_posix.go (right):

https://codereview.appspot.com/12023043/diff/33001/src/pkg/net/sock_posix.go#...
src/pkg/net/sock_posix.go:93: 
On 2013/08/04 01:32:40, dfc wrote:
> The setXXX names aren't the best, but I can't think of a better suggestion at
> the moment. Please add a comment to this method and it's friends below
> describing their intent.

Done.

https://codereview.appspot.com/12023043/diff/33001/src/pkg/net/sock_posix.go#...
src/pkg/net/sock_posix.go:180: fd.setAddr(toAddr(lsa), nil)
On 2013/08/04 01:32:40, dfc wrote:
> suggestion, could you return the err on line 179 on line 181 ?

thanks but no thanks.

[dfc, 2013-08-04 04:15:21]

LGTM. Thank you for adding commentary to those functions, I made a small
suggestion. Please wait for someone to verify win32 works.

https://codereview.appspot.com/12023043/diff/33001/src/pkg/net/sock_posix.go
File src/pkg/net/sock_posix.go (right):

https://codereview.appspot.com/12023043/diff/33001/src/pkg/net/sock_posix.go#...
src/pkg/net/sock_posix.go:180: fd.setAddr(toAddr(lsa), nil)
> thanks but no thanks.

fair enough

https://codereview.appspot.com/12023043/diff/46001/src/pkg/net/sock_posix.go
File src/pkg/net/sock_posix.go (right):

https://codereview.appspot.com/12023043/diff/46001/src/pkg/net/sock_posix.go#...
src/pkg/net/sock_posix.go:94: // setDialer sets fd up as a dialer.
// setDialer configures fd as a dialer.

I do wonder if we need to explain what a dialer is. I considered ... configures
fd as a connecting socket, but that was verbose and not always accurate. I think
this rewording will do for now.

https://codereview.appspot.com/12023043/diff/46001/src/pkg/net/sock_posix.go#...
src/pkg/net/sock_posix.go:133: // setStreamListener sets fd up as a stream
listener.
s/sets fd up/configures fd/

https://codereview.appspot.com/12023043/diff/46001/src/pkg/net/sock_posix.go#...
src/pkg/net/sock_posix.go:150: // setDatagramListener sets fd up as a datagram
listener.
same

[dfc, 2013-08-04 04:16:40]

Final nit on the description.

This CL refactors up the existing listenerSockaddr function into several
methods on netFD.

s/up//

[mikio, 2013-08-04 04:19:42]

> src/pkg/net/sock_posix.go:180: fd.setAddr(toAddr(lsa), nil)
>> thanks but no thanks.
>
> fair enough

IIRC, at first Russ tried, then other heldens who ported net stuff
to BSDs, Windows tried. Now the code stays as it is, I won't try.

[remyoudompheng, 2013-08-04 06:43:56]

On 2013/08/04 04:19:42, mikio wrote:
> > src/pkg/net/sock_posix.go:180: fd.setAddr(toAddr(lsa), nil)
> >> thanks but no thanks.
> >
> > fair enough
> 
> IIRC, at first Russ tried, then other heldens who ported net stuff
> to BSDs, Windows tried. Now the code stays as it is, I won't try.

Can you explain why we would need this CL at all ?

Also, since this is a refactoring patch, the names of the methods are a
essential part of it, and I am pretty dissatisfied by the current names.

[mikio, 2013-08-04 08:02:52]

> Can you explain why we would need this CL at all ?

This CL is a part of CL 8264043+8608044.

For pulling up pending connections from the kernel on BSDs with
runtime-integrated network pollster, I think we need to keep the
order of calling functions;

syscall.Socket
net.runtime_pollInit
syscall.Listen
net.runtime_pollOpen

> Also, since this is a refactoring patch, the names of the methods are a
> essential part of it, and I am pretty dissatisfied by the current names.

Do you have any good alternatives?

[mikio, 2013-08-04 09:51:23]

PTAL

changed method names into dial, listenStream and listenDatagram.

[brainman, 2013-08-05 04:40:09]

I just tested it on windows and it builds fine. But I would be wary make changes
here - different OSes rely on it to be the way it is. Making this code "better"
might break all these assumptions. I hope we have enough test to cover all
angles.

Alex

[mikio, 2013-08-05 07:06:27]

On Mon, Aug 5, 2013 at 1:40 PM,  <alex.brainman@gmail.com> wrote:

> I just tested it on windows and it builds fine. But I would be wary make
> changes here - different OSes rely on it to be the way it is. Making
> this code "better" might break all these assumptions. I hope we have
> enough test to cover all angles.

Thank you for your confirmation.

Sure, certainly the devil lies in between layers; own or runtime pollster
and netFD, netFD and Conn/Listener, connection setup/teardown and
inflight ops. I'll go carefully.

Will submitting later.

[remyoudompheng, 2013-08-05 07:09:48]

On 2013/08/04 08:02:52, mikio wrote:
> > Can you explain why we would need this CL at all ?
> 
> This CL is a part of CL 8264043+8608044.

I see it's part of the CL but that doesn't tell *why* a refactoring is needed.
Why would the old code not work?

Rémy.

[mikio, 2013-08-05 07:14:41]

On Mon, Aug 5, 2013 at 4:09 PM,  <remyoudompheng@gmail.com> wrote:

> I see it's part of the CL but that doesn't tell *why* a refactoring is
> needed. Why would the old code not work?

I think the existing code has no space to put new controls like:
https://codereview.appspot.com/8608044/

What do you think?

[mikio, 2013-08-06 21:15:59]

*** Submitted as https://code.google.com/p/go/source/detail?r=51d2970d9348 ***

net: add dial, listenStream and listenDatagram methods to netFD

This CL refactors the existing listenerSockaddr function into several
methods on netFD.

This is in preparation for runtime-integrated network pollster for BSD
variants.

Update issue 5199

R=golang-dev, dave, alex.brainman, dvyukov, remyoudompheng
CC=golang-dev
https://codereview.appspot.com/12023043

[bradfitz, 2013-08-06 21:21:34]

Was everybody happy with this?  I saw a conditional LGTM and some silence.

I admit that the net code is changing around too quickly lately for me to
keep track of where it's at, or where it's going (if anybody knows)

Where is the roadmap for the upcoming CLs?  Do you have an idea of what the
end picture looks like?



On Tue, Aug 6, 2013 at 2:15 PM, <mikioh.mikioh@gmail.com> wrote:

> *** Submitted as
>
https://code.google.com/p/go/**source/detail?r=51d2970d9348<https://code.goog...
>
> net: add dial, listenStream and listenDatagram methods to netFD
>
> This CL refactors the existing listenerSockaddr function into several
> methods on netFD.
>
> This is in preparation for runtime-integrated network pollster for BSD
> variants.
>
> Update issue 5199
>
> R=golang-dev, dave, alex.brainman, dvyukov, remyoudompheng
> CC=golang-dev
>
https://codereview.appspot.**com/12023043<https://codereview.appspot.com/1202...
>
>
>
https://codereview.appspot.**com/12023043/<https://codereview.appspot.com/120...
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[mikio, 2013-08-06 21:33:03]

On Wed, Aug 7, 2013 at 6:21 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> Where is the roadmap for the upcoming CLs?

Will update:
https://groups.google.com/d/msg/golang-dev/hkygwgxDQ-s/HUQJGAomOksJ

> Do you have an idea of what the end picture looks like?

https://codereview.appspot.com/8264043/ is that.