|
|
|
net: 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/ #
Total messages: 12
|
ola
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change.
|
14 years, 11 months ago (2011年02月01日 16:09:16 UTC) #1 | ||||||||||||||||||||||||||||||
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change.
It's not true that prefixBefore is unused. Look at ipraw_test.go, which calls ListenIP on "ip4:icmp". Russ
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
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
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
> 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
*** Abandoned ***
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 ?
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/
LGTM
LGTM But please complete a CLA as described at http://golang.org/doc/contribute.html#copyright Thanks.
*** 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>