On 2013/07/24 15:06:10, mikio wrote: > PTAL > > > how much else in this ...
10 years, 8 months ago
(2013-07-25 11:11:59 UTC)
#4
On 2013/07/24 15:06:10, mikio wrote:
> PTAL
>
> > how much else in this diff is just providing distraction?
>
> Not so much, isolated, thx.
Hi Mikio,
I'm having trouble following this CL. Can I please ask you to do several things.
1. Unless they are critical to the change, please revert the nil checks. Please
open a discussion on the mailing list to discuss this change in behavior, from
my understanding Go takes a very liberal approach to cushioning the user against
passing nil pointer values.
2. Can refactoring of listenerSockaddr be split out into another CL.
3. And this is related to point 1, and please excuse me if I have misunderstood
this change, it feels like nil is being used as a signal inside the net package.
I'm uncomfortable about this change as I expressed in point 1.
Thanks again
Dave
On Thu, Jul 25, 2013 at 8:11 PM, <dave@cheney.net> wrote: > 1. Unless they are ...
10 years, 8 months ago
(2013-07-25 14:34:00 UTC)
#5
On Thu, Jul 25, 2013 at 8:11 PM, <dave@cheney.net> wrote:
> 1. Unless they are critical to the change, please revert the nil checks.
I guess you are saying about net.sockaddr stuff, right?
Unfortunately they are a bit important stuff; same as it was, and as
you supposed,
internal socket preflight helpers take nil as a signal that indicates
"a passed address
is already determined by exposed API surface whether wildcard stuff or
not. Now we
understand that the address, net.sockaddr (not syscall.Sockaddr), is
really nil address
for target operations such as syscall.Listen, syscall.Connect".
> 3. And this is related to point 1, and please excuse me if I have
> misunderstood this change, it feels like nil is being used as a signal
> inside the net package. I'm uncomfortable about this change as I
> expressed in point 1.
I'm happy if you have a nice counterproposal.
> Please open a discussion on the mailing list to discuss this change in
> behavior, from my understanding Go takes a very liberal approach to
> cushioning the user against passing nil pointer values.
Will do so if you want.
> 2. Can refactoring of listenerSockaddr be split out into another CL.
Not sure how to do that, any suggestions?
Thanks for your comments, I'll take another pass in the morning. On 26/07/2013, at 0:33, ...
10 years, 8 months ago
(2013-07-25 14:54:27 UTC)
#6
Thanks for your comments, I'll take another pass in the morning.
On 26/07/2013, at 0:33, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> On Thu, Jul 25, 2013 at 8:11 PM, <dave@cheney.net> wrote:
>
>> 1. Unless they are critical to the change, please revert the nil checks.
>
> I guess you are saying about net.sockaddr stuff, right?
>
> Unfortunately they are a bit important stuff; same as it was, and as
> you supposed,
> internal socket preflight helpers take nil as a signal that indicates
> "a passed address
> is already determined by exposed API surface whether wildcard stuff or
> not. Now we
> understand that the address, net.sockaddr (not syscall.Sockaddr), is
> really nil address
> for target operations such as syscall.Listen, syscall.Connect".
>
>> 3. And this is related to point 1, and please excuse me if I have
>> misunderstood this change, it feels like nil is being used as a signal
>> inside the net package. I'm uncomfortable about this change as I
>> expressed in point 1.
>
> I'm happy if you have a nice counterproposal.
>
>> Please open a discussion on the mailing list to discuss this change in
>> behavior, from my understanding Go takes a very liberal approach to
>> cushioning the user against passing nil pointer values.
>
> Will do so if you want.
>
>> 2. Can refactoring of listenerSockaddr be split out into another CL.
>
> Not sure how to do that, any suggestions?
On Thu, Jul 25, 2013 at 11:54 PM, Dave Cheney <dave@cheney.net> wrote: > Thanks for ...
10 years, 8 months ago
(2013-07-25 15:00:49 UTC)
#7
On Thu, Jul 25, 2013 at 11:54 PM, Dave Cheney <dave@cheney.net> wrote:
> Thanks for your comments, I'll take another pass in the morning.
Thanks too. I just remembered that just a year ago Russ rejected the CL
I wrote and modified w/ your suggestions. Ah, that patch included something
like how could we deal a net.Addr; whether a wildcard address+port or not, use
of nil as a signal or not, blah blah blah. Hm... ;)
Yup. I'm ashamed to admit I proposed something very icky using types nils. On 26/07/2013, ...
10 years, 8 months ago
(2013-07-25 15:26:39 UTC)
#8
Yup. I'm ashamed to admit I proposed something very icky using types nils.
On 26/07/2013, at 1:00, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> On Thu, Jul 25, 2013 at 11:54 PM, Dave Cheney <dave@cheney.net> wrote:
>
>> Thanks for your comments, I'll take another pass in the morning.
>
> Thanks too. I just remembered that just a year ago Russ rejected the CL
> I wrote and modified w/ your suggestions. Ah, that patch included something
> like how could we deal a net.Addr; whether a wildcard address+port or not,
use
> of nil as a signal or not, blah blah blah. Hm... ;)
On Sun, Jul 28, 2013 at 11:37 AM, <dave@cheney.net> wrote: > I am having difficulty ...
10 years, 8 months ago
(2013-07-28 02:50:53 UTC)
#10
On Sun, Jul 28, 2013 at 11:37 AM, <dave@cheney.net> wrote:
> I am having difficulty reviewing this change as I cannot determine a
> theme. Can you please try to break this CL up into smaller parts. I have
> marked some places in the diff where I believe these changes can be made
> independently.
Ah, you need almost all transitions, toAddr -> sockaddr, internetSocket to
new methods, unixSocket -> new methods, right?
Can't you do the socketaddr interface move and the other move without changing their contents ...
10 years, 8 months ago
(2013-07-28 03:10:17 UTC)
#11
Can't you do the socketaddr interface move and the other move without changing
their contents ?
On 28/07/2013, at 12:50, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> On Sun, Jul 28, 2013 at 11:37 AM, <dave@cheney.net> wrote:
>
>> I am having difficulty reviewing this change as I cannot determine a
>> theme. Can you please try to break this CL up into smaller parts. I have
>> marked some places in the diff where I believe these changes can be made
>> independently.
>
> Ah, you need almost all transitions, toAddr -> sockaddr, internetSocket to
> new methods, unixSocket -> new methods, right?
On Sun, Jul 28, 2013 at 12:10 PM, Dave Cheney <dave@cheney.net> wrote: > Can't you ...
10 years, 8 months ago
(2013-07-28 03:22:45 UTC)
#12
On Sun, Jul 28, 2013 at 12:10 PM, Dave Cheney <dave@cheney.net> wrote:
> Can't you do the socketaddr interface move and the other move without changing
their contents ?
Without ephemeral scaffolds, no.
But let me try it.
Or, is it possible to refractor those methods in place, the propose a follow up ...
10 years, 8 months ago
(2013-07-28 03:32:26 UTC)
#13
Or, is it possible to refractor those methods in place, the propose a follow up
CL that will reduce the duplication.
On 28/07/2013, at 13:22, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> On Sun, Jul 28, 2013 at 12:10 PM, Dave Cheney <dave@cheney.net> wrote:
>
>> Can't you do the socketaddr interface move and the other move without
changing their contents ?
>
> Without ephemeral scaffolds, no.
> But let me try it.
10 years, 7 months ago
(2013-08-16 01:21:51 UTC)
#14
https://codereview.appspot.com/8726051/diff/140001/src/pkg/net/iprawsock_posi...
File src/pkg/net/iprawsock_posix.go (right):
https://codereview.appspot.com/8726051/diff/140001/src/pkg/net/iprawsock_posi...
src/pkg/net/iprawsock_posix.go:185: fd, err := socket(family, syscall.SOCK_RAW,
proto, ipv6only, net, func(fd *netFD) error {
These closures will appear in stack traces and should be made top-level
functions with proper names.
Also there is a lot of redundant here. The second argument to socket implies the
kind of operation in the closure, and laddr, raddr, deadline could be passed to
socket. The only variable is sockaddrToIP, which could be passed as well. And
then we're back at the current internetSocket, which is noticeably shorter.
Usually refactor means things get shorter but here things seem to be getting
longer.
What's the rationale for the change?
On Friday, August 16, 2013 10:21:51 AM UTC+9, rsc wrote: Usually refactor means things get ...
10 years, 7 months ago
(2013-08-16 04:26:50 UTC)
#15
On Friday, August 16, 2013 10:21:51 AM UTC+9, rsc wrote:
Usually refactor means things get shorter but here things seem to be
> getting longer.
>
> What's the rationale for the change?
>
I just want to collapse internetSocket and unixSocket because they take
a tons of args and make things unclear; so just want to make socket ops
a bit clear w/o adding new types and allocations.
Hm, let me sleep on it.
The old code was fd, err := internetSocket(net, laddr, raddr, deadline, syscall.SOCK_RAW, proto, "dial", sockaddrToIP) ...
10 years, 7 months ago
(2013-08-16 05:40:03 UTC)
#16
The old code was
fd, err := internetSocket(net, laddr, raddr, deadline, syscall.SOCK_RAW,
proto, "dial", sockaddrToIP)
The new code is:
family, ipv6only := favoriteAddrFamily(net, laddr, raddr, "dial")
fd, err := socket(family, syscall.SOCK_RAW, proto, ipv6only, net, func(fd
*netFD) error {
return fd.dial(laddr, raddr, deadline, sockaddrToIP)
})
The same arguments are all still there, but now they are split across three
calls (plus the closure body), and that chunk is repeated at least four
times. I don't think that's any clearer.
If you were worried about the meaning of the arguments being unclear you
could make internetSocket take a struct and use named initializers, but I
think it would just be redundant, not clearer:
fd, err := internetSocket(socketArgs{
net: net,
laddr: laddr,
deadline: deadline,
socktype: syscall.SOCK_RAW,
proto: proto,
op: "dial",
sockaddrConv: sockaddrToIP,
})
I certainly admit it is complex and that there are more arguments than
there should be in a typical call. But in this case I think it is because
the API we are calling into is fundamentally complex. I don't think there's
a clear way to make it better.
Russ
Thanks. Leave it as it is for now and will revisit when someone wants to ...
10 years, 7 months ago
(2013-08-18 10:30:47 UTC)
#17
Thanks. Leave it as it is for now and will revisit when someone
wants to implement TCP fast-open, SCTP or MP-TCP.
On 2013/08/16 05:40:03, rsc wrote:
> The old code was
>
> fd, err := internetSocket(net, laddr, raddr, deadline, syscall.SOCK_RAW,
> proto, "dial", sockaddrToIP)
>
> The new code is:
>
> family, ipv6only := favoriteAddrFamily(net, laddr, raddr, "dial")
> fd, err := socket(family, syscall.SOCK_RAW, proto, ipv6only, net, func(fd
> *netFD) error {
> return fd.dial(laddr, raddr, deadline, sockaddrToIP)
> })
>
> The same arguments are all still there, but now they are split across three
> calls (plus the closure body), and that chunk is repeated at least four
> times. I don't think that's any clearer.
>
> If you were worried about the meaning of the arguments being unclear you
> could make internetSocket take a struct and use named initializers, but I
> think it would just be redundant, not clearer:
>
> fd, err := internetSocket(socketArgs{
> net: net,
> laddr: laddr,
> deadline: deadline,
> socktype: syscall.SOCK_RAW,
> proto: proto,
> op: "dial",
> sockaddrConv: sockaddrToIP,
> })
>
> I certainly admit it is complex and that there are more arguments than
> there should be in a typical call. But in this case I think it is because
> the API we are calling into is fundamentally complex. I don't think there's
> a clear way to make it better.
>
> Russ
Issue 8726051: code review 8726051: net: refactor sockets
(Closed)
Created 10 years, 11 months ago by mikio
Modified 10 years, 7 months ago
Reviewers: golang-dev, bradfitz, dfc, rsc
Base URL:
Comments: 4