code review 15960047: net: handle single-line non-\n-terminated files correct...

net: handle single-line non-\n-terminated files correctly in readLine

Fixes issue 6646.

[josharian, 2013-10-23 17:47:04]

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

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

[bradfitz, 2013-10-23 17:48:14]

Not a fix without a test.



On Wed, Oct 23, 2013 at 10:47 AM, <josharian@gmail.com> wrote:

> Reviewers: rsc,
>
> Message:
> Hello rsc@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> net: handle single-line non-\n-terminated files correctly in readLine
>
> Fixes issue 6646.
>
> Please review this at
https://codereview.appspot.**com/15960047/<https://codereview.appspot.com/159...
>
> Affected files (+1, -1 lines):
>   src/pkg/net/parse.go
>
>
> Index: src/pkg/net/parse.go
> ==============================**==============================**=======
> --- a/src/pkg/net/parse.go
> +++ b/src/pkg/net/parse.go
> @@ -54,7 +54,7 @@
>                 if n >= 0 {
>                         f.data = f.data[0 : ln+n]
>                 }
> -               if err == io.EOF {
> +               if err == io.EOF || err == io.ErrUnexpectedEOF {
>                         f.atEOF = true
>                 }
>         }
>
>
> --
>
> ---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...
> .
>

[rsc, 2013-10-23 17:54:02]

You can test the reading of /etc/hosts instead of the reading of
/etc/resolv.conf. See TestLookupStaticHost.

[josharian, 2013-10-23 17:55:33]

> You can test the reading of /etc/hosts instead of the reading of
> /etc/resolv.conf. See TestLookupStaticHost.

Oh great, I missed that. Thanks. Coming soon...

-josh

[josharian, 2013-10-23 18:13:30]

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

Please take another look.

[bradfitz, 2013-10-23 18:17:10]

https://codereview.appspot.com/15960047/diff/20002/src/pkg/net/hosts_test.go
File src/pkg/net/hosts_test.go (right):

https://codereview.appspot.com/15960047/diff/20002/src/pkg/net/hosts_test.go#...
src/pkg/net/hosts_test.go:68: hostsPath = p
nit that probably doesn't matter: this isn't restored if the goroutine dies with
runtime.Goexist (via t.Fatalf) above.

but you could instead do, at the top:

defer func(orig string) {
   hostsPath = orig
}(hostsPath)
hostsPath = "testdata/hosts_singleline"

[josharian, 2013-10-23 18:18:24]

On 2013/10/23 18:17:10, bradfitz wrote:
> https://codereview.appspot.com/15960047/diff/20002/src/pkg/net/hosts_test.go
> File src/pkg/net/hosts_test.go (right):
> 
>
https://codereview.appspot.com/15960047/diff/20002/src/pkg/net/hosts_test.go#...
> src/pkg/net/hosts_test.go:68: hostsPath = p
> nit that probably doesn't matter: this isn't restored if the goroutine dies
with
> runtime.Goexist (via t.Fatalf) above.
> 
> but you could instead do, at the top:
> 
> defer func(orig string) {
>    hostsPath = orig
> }(hostsPath)
> hostsPath = "testdata/hosts_singleline"

Yeah, I noticed that and couldn't decide whether it mattered. Since you noticed
too, I'll do it. :)

[rsc, 2013-10-23 18:18:52]

https://codereview.appspot.com/15960047/diff/20002/src/pkg/net/hosts_test.go
File src/pkg/net/hosts_test.go (right):

https://codereview.appspot.com/15960047/diff/20002/src/pkg/net/hosts_test.go#...
src/pkg/net/hosts_test.go:61: if len(ips) != 1 {
you can avoid brad's comment about Fatalf by merging the tests:

if len(ips) != 1 || ips[0] != "127.0.0.2" {
    t.Errorf("lookupStaticHost = %v, want %v", ips, []string{"127.0.0.2"})
}
hostsPath = p

[josharian, 2013-10-23 18:22:13]

> you can avoid brad's comment about Fatalf by merging the tests:

The other test has the same problem (there's a return in the middle of
it). Will merge the tests and fix the problem.

[josharian, 2013-10-23 18:29:21]

Fixed the corresponding problem in TestLookupStaticHost; kept the
defer for TestSingleLineHostsFile for symmetry. Merged the tests in
TestSingleLineHostsFile per rsc anyway, since it reads better. Thanks!

Please take another look.


On Wed, Oct 23, 2013 at 11:21 AM, Josh Bleecher Snyder
<josharian@gmail.com> wrote:
>> you can avoid brad's comment about Fatalf by merging the tests:
>
> The other test has the same problem (there's a return in the middle of
> it). Will merge the tests and fix the problem.

[rsc, 2013-10-23 18:32:48]

The other test did not have the problem, because it did not call Fatalf.

[rsc, 2013-10-23 18:33:41]

https://codereview.appspot.com/15960047/diff/90001/src/pkg/net/hosts_test.go
File src/pkg/net/hosts_test.go (right):

https://codereview.appspot.com/15960047/diff/90001/src/pkg/net/hosts_test.go#...
src/pkg/net/hosts_test.go:36: defer func(orig string) {
this is unnecessarily complex; please put it back the way it was.

https://codereview.appspot.com/15960047/diff/90001/src/pkg/net/hosts_test.go#...
src/pkg/net/hosts_test.go:60: defer func(orig string) {
same

[josharian, 2013-10-23 18:35:43]

> The other test did not have the problem, because it did not call Fatalf.

What about the return statement halfway through the test?

-josh

[bradfitz, 2013-10-23 18:36:06]

On Wed, Oct 23, 2013 at 11:32 AM, Russ Cox <rsc@golang.org> wrote:

> The other test did not have the problem, because it did not call Fatalf.
>

It used return, though.  Maybe it should be continue.

[josharian, 2013-10-23 18:52:46]

Third time's a charm? Please take another look.


> this is unnecessarily complex; please put it back the way it was.

Done.


> It used return, though.  Maybe it should be continue.

continue looks right, and tests out ok locally. Will send a separate
CL for that, to keep this one on topic.


-josh

[josharian, 2013-10-24 22:11:15]

On 2013/10/23 18:52:46, josharian wrote:
> Third time's a charm? Please take another look.
> 
> 
> > this is unnecessarily complex; please put it back the way it was.
> 
> Done.
> 
> 
> > It used return, though.  Maybe it should be continue.
> 
> continue looks right, and tests out ok locally. Will send a separate
> CL for that, to keep this one on topic.
> 
> 
> -josh


I finally won the Rietveld lottery and no longer have old chunk mismatches on
this. Sorry about the delay.

-josh

[bradfitz, 2013-10-25 04:13:58]

LGTM

Russ?

[rsc, 2013-10-28 23:31:31]

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

net: handle single-line non-\n-terminated files correctly in readLine

Fixes issue 6646.

R=rsc, bradfitz
CC=golang-dev
https://codereview.appspot.com/15960047

Committer: Russ Cox <rsc@golang.org>