code review 4129042: net: There's no need for prefixBefore() function.

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

[ola, 2011-02-01 16:09:16]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc, 2011-02-01 19:16:52]

It's not true that prefixBefore is unused.
Look at ipraw_test.go, which calls ListenIP on "ip4:icmp".

Russ

[ola, 2011-02-01 21:36:53]

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

[rsc, 2011-02-01 21:44:24]

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

[ola, 2011-02-02 09:58:29]

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

[rsc, 2011-02-09 05:29:10]

> 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

[ola, 2011-02-10 16:46:57]

*** Abandoned ***

[ola, 2011-02-10 16:51:55]

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 ?

[ola, 2011-02-10 18:43:21]

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/

[rsc, 2011-02-11 19:58:52]

LGTM

[rsc, 2011-02-11 20:00:03]

LGTM

But please complete a CLA as described at
http://golang.org/doc/contribute.html#copyright

Thanks.

[rsc, 2011-02-16 20:03:51]

*** 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>