Issue 4129042: code review 4129042: net: There's no need for prefixBefore() function.
Descriptionnet: There's no need for prefixBefore() function. net or netProto argument are a choice of constants strings. It's not necessary to search a prefix in it. Actual code permit to write 'Dial("tcp:foobarbazquux", "", host) which may lead to confusion. ListenPacket() is modified to permit a net:proto string Patch Set 1 : code review 4129042: net: There's no need for prefixBefore() function. #Patch Set 2 : diff -r 3972eb64f7cd https://go.googlecode.com/hg/ #
MessagesTotal messages: 12
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change. Sign in to reply to this message.
It's not true that prefixBefore is unused. Look at ipraw_test.go, which calls ListenIP on "ip4:icmp". Russ Sign in to reply to this message.
On Tue, Feb 1, 2011 at 8:16 PM, Russ Cox <rsc@golang.org> wrote: > It's not true that prefixBefore is unused. > Look at ipraw_test.go, which calls ListenIP on "ip4:icmp". > Argument netProto is processed by netProtoSplit() on ListenIP() which behave the same as prefixBefore() if there is only one ':' -- Olivier Antoine Sign in to reply to this message.
On Tue, Feb 1, 2011 at 16:36, Olivier ANTOINE <olivier.antoine@gmail.com> wrote: > On Tue, Feb 1, 2011 at 8:16 PM, Russ Cox <rsc@golang.org> wrote: >> >> It's not true that prefixBefore is unused. >> Look at ipraw_test.go, which calls ListenIP on "ip4:icmp". > > Argument netProto is processed by netProtoSplit() on ListenIP() which behave > the same as prefixBefore() if there is only one ':' Okay, but what about ListenPacket("ip4:icmp", laddr)? I agree that it could possibly go in Dial, but maybe there should be a stricter check? Russ Sign in to reply to this message.
On Tue, Feb 1, 2011 at 10:44 PM, Russ Cox <rsc@golang.org> wrote: > On Tue, Feb 1, 2011 at 16:36, Olivier ANTOINE <olivier.antoine@gmail.com> > wrote: > > On Tue, Feb 1, 2011 at 8:16 PM, Russ Cox <rsc@golang.org> wrote: > >> > >> It's not true that prefixBefore is unused. > >> Look at ipraw_test.go, which calls ListenIP on "ip4:icmp". > > > > Argument netProto is processed by netProtoSplit() on ListenIP() which > behave > > the same as prefixBefore() if there is only one ':' > > Okay, but what about ListenPacket("ip4:icmp", laddr)? > Is it allowed ? The documentation for this function states that the "network string net must be a packet-oriented network: "udp", "udp4", "udp6", or "unixgram". :) prefixBefore() just discard the proto part (":icmp" in your example). So ListenPacket("ip4:icmp", laddr) is strictly equivalent to ListenPacket("ip4", laddr) which is strictly equivalent to ListenPacket("ip4:foo", laddr) according to the code. If we care about that proto part, we should probably use netProtoSplit() on top of ListenPacket() and rename the 'net' argument to 'netProto' to be consistent, just like in ListenIP() and DialIP does. But is it usefull ? I agree that it could possibly go in Dial, but maybe there > should be a stricter check? netProtoSplit() do an internal check in order to see if the proto part of the netProto argument is valid. We can use it everywhere it's needed. -- Olivier Antoine Sign in to reply to this message.
> Is it allowed ? The documentation for this function states that the "network > string net must be a packet-oriented network: "udp", "udp4", "udp6", or > "unixgram". :) > prefixBefore() just discard the proto part (":icmp" in your example). So > ListenPacket("ip4:icmp", laddr) is strictly equivalent to > ListenPacket("ip4", laddr) which is strictly equivalent to > ListenPacket("ip4:foo", laddr) according to the code. If we care about that > proto part, we should probably use netProtoSplit() on top of ListenPacket() > and rename the 'net' argument to 'netProto' to be consistent, just like in > ListenIP() and DialIP does. But is it usefull ? I would like ListenPacket("ip4:anything", laddr) to be equivalent to ListenIP("ip4:anything", packet). I don't think the parameter name needs to change. I would be happy, though, to see the : trigger unknown network names for all the other networks. Russ Sign in to reply to this message.
*** Abandoned *** Sign in to reply to this message.
On 2011/02/09 05:29:10, rsc wrote: > > Is it allowed ? The documentation for this function states that the "network > > string net must be a packet-oriented network: "udp", "udp4", "udp6", or > > "unixgram". :) > > prefixBefore() just discard the proto part (":icmp" in your example). So > > ListenPacket("ip4:icmp", laddr) is strictly equivalent to > > ListenPacket("ip4", laddr) which is strictly equivalent to > > ListenPacket("ip4:foo", laddr) according to the code. If we care about that > > proto part, we should probably use netProtoSplit() on top of ListenPacket() > > and rename the 'net' argument to 'netProto' to be consistent, just like in > > ListenIP() and DialIP does. But is it usefull ? > > I would like ListenPacket("ip4:anything", laddr) to be > equivalent to ListenIP("ip4:anything", packet). > I don't think the parameter name needs to change. > I would be happy, though, to see the : trigger > unknown network names for all the other networks. > > Russ Argh ! I can't upload a new patchset ! 'hg mail 4129042' sends some weirds things to codereview and I don't know how to fix… Do I need to make a new codereview ? Sign in to reply to this message.
Hello golang-dev@googlegroups.com, rsc (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/ Sign in to reply to this message.
LGTM Sign in to reply to this message.
LGTM But please complete a CLA as described at http://golang.org/doc/contribute.html#copyright Thanks. Sign in to reply to this message.
*** Submitted as 24f5f929d319 *** net: reject invalid net:proto network names R=rsc CC=golang-dev http://codereview.appspot.com/4129042 Committer: Russ Cox <rsc@golang.org> Sign in to reply to this message. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
