Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(363)

Issue 83690045: code review 83690045: net: detect changes to /etc/resolv.conf. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 12 months ago by creack
Modified:
9 years, 10 months ago
Reviewers:
ruiu, iant, josharian
CC:
golang-codereviews, iant, josharian, mikio, axaxs, gobot, rsc
Visibility:
Public.

Description

net: detect changes to /etc/resolv.conf. Implement the changes as suggested by rsc. Fixes issue 6670.

Patch Set 1 #

Patch Set 2 : diff -r ddfee9dfca20 https://code.google.com/p/go #

Patch Set 3 : diff -r ddfee9dfca20 https://code.google.com/p/go #

Patch Set 4 : diff -r ddfee9dfca20 https://code.google.com/p/go #

Total comments: 8

Patch Set 5 : diff -r 88b3b2fa4dde https://code.google.com/p/go #

Patch Set 6 : diff -r 88b3b2fa4dde https://code.google.com/p/go #

Patch Set 7 : diff -r 88b3b2fa4dde https://code.google.com/p/go #

Patch Set 8 : diff -r 88b3b2fa4dde https://code.google.com/p/go #

Patch Set 9 : diff -r 88b3b2fa4dde https://code.google.com/p/go #

Total comments: 3

Patch Set 10 : diff -r 88b3b2fa4dde https://code.google.com/p/go #

Patch Set 11 : diff -r 88b3b2fa4dde https://code.google.com/p/go #

Patch Set 12 : diff -r 88b3b2fa4dde https://code.google.com/p/go #

Total comments: 3

Patch Set 13 : diff -r b3405f9c2e32 https://code.google.com/p/go #

Patch Set 14 : diff -r b3405f9c2e32 https://code.google.com/p/go #

Patch Set 15 : diff -r b3405f9c2e32 https://code.google.com/p/go #

Patch Set 16 : diff -r 4c91e8fa0ebf https://code.google.com/p/go #

Patch Set 17 : diff -r 4c91e8fa0ebf https://code.google.com/p/go #

Patch Set 18 : diff -r 1bf549d894c9 https://code.google.com/p/go #

Patch Set 19 : diff -r 1bf549d894c9 https://code.google.com/p/go #

Patch Set 20 : diff -r 1bf549d894c9 https://code.google.com/p/go #

Patch Set 21 : diff -r f8b50ad4cac4 https://code.google.com/p/go #

Patch Set 22 : diff -r 1d8991d40dd3 https://code.google.com/p/go #

Patch Set 23 : diff -r 1d8991d40dd3 https://code.google.com/p/go #

Total comments: 2

Patch Set 24 : diff -r 9fd0fe734a42 https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+203 lines, -29 lines) Patch
M src/pkg/net/dnsclient_unix.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 8 chunks +71 lines, -29 lines 0 comments Download
M src/pkg/net/dnsclient_unix_test.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +132 lines, -0 lines 0 comments Download

Messages

Total messages: 48
creack
Hello golang-codereviews@googlegroups.com (cc: golang-codereviews@googlegroups.com, rsc@golang.org), I'd like you to review this change to https://code.google.com/p/go
9 years, 12 months ago (2014-04-03 16:27:36 UTC) #1
iant
https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix.go File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix.go#newcode168 src/pkg/net/dnsclient_unix.go:168: cfg.ch = make(chan struct{}) This should be a buffered ...
9 years, 12 months ago (2014-04-03 23:47:14 UTC) #2
josharian
Thanks for working on this! I like how simple this is. It'd be nice to ...
9 years, 12 months ago (2014-04-03 23:54:14 UTC) #3
creack
On 2014/04/03 23:47:14, iant wrote: > https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix.go > File src/pkg/net/dnsclient_unix.go (right): > > https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix.go#newcode168 > ...
9 years, 12 months ago (2014-04-03 23:55:54 UTC) #4
josharian
On 2014/04/03 23:55:54, creack wrote: > On 2014/04/03 23:47:14, iant wrote: > > > https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix.go ...
9 years, 12 months ago (2014-04-04 00:01:53 UTC) #5
creack
On 2014/04/03 23:54:14, josharian wrote: > Thanks for working on this! I like how simple ...
9 years, 12 months ago (2014-04-04 00:02:24 UTC) #6
josharian
> https://codereview.appspot.com/83690045/diff/60001/src/pkg/net/dnsclient_unix.go#newcode265 > > src/pkg/net/dnsclient_unix.go:265: onceLoadConfig.Do(loadConfig) > > Do you want to send to cfg.ch ...
9 years, 12 months ago (2014-04-04 18:30:06 UTC) #7
creack
Hello golang-codereviews@googlegroups.com, iant@golang.org, josharian@gmail.com (cc: golang-codereviews@googlegroups.com, rsc@golang.org), Please take another look.
9 years, 12 months ago (2014-04-05 06:21:48 UTC) #8
mikio
please try to run "CGO_ENABLED=0 go test net -v -cpu=2,4,8" and see what happens with ...
9 years, 12 months ago (2014-04-05 10:11:54 UTC) #9
creack
On 2014/04/05 10:11:54, mikio wrote: > please try to run "CGO_ENABLED=0 go test net -v ...
9 years, 11 months ago (2014-04-05 14:46:47 UTC) #10
axaxs
Are we considering for 1.3, or going to wait to merge into other logic fixes ...
9 years, 11 months ago (2014-04-05 17:08:03 UTC) #11
creack
On 2014/04/05 17:08:03, axaxs wrote: > Are we considering for 1.3, or going to wait ...
9 years, 11 months ago (2014-04-05 17:10:22 UTC) #12
mikio
> CGO_ENABLED=0 go test net -v -cpu=2,4,8 > ... > PASS > ok net 163.139s ...
9 years, 11 months ago (2014-04-05 17:35:35 UTC) #13
josharian
From a quick glance, looks like one of the inner lookups uses "/etc/resolv.conf" instead resolvConfPath ...
9 years, 11 months ago (2014-04-05 17:45:13 UTC) #14
creack
On 2014/04/05 17:45:13, josharian wrote: > From a quick glance, looks like one of the ...
9 years, 11 months ago (2014-04-05 17:55:12 UTC) #15
creack
On 2014/04/05 17:35:35, mikio wrote: > > CGO_ENABLED=0 go test net -v -cpu=2,4,8 > > ...
9 years, 11 months ago (2014-04-05 18:01:53 UTC) #16
creack
On 2014/04/05 18:01:53, creack wrote: > On 2014/04/05 17:35:35, mikio wrote: > > > CGO_ENABLED=0 ...
9 years, 11 months ago (2014-04-05 18:19:26 UTC) #17
axaxs
I can see that, but thinking on it more - what this does is essentially ...
9 years, 11 months ago (2014-04-05 23:13:43 UTC) #18
creack
On 2014/04/05 23:13:43, axaxs wrote: > I can see that, but thinking on it more ...
9 years, 11 months ago (2014-04-05 23:21:03 UTC) #19
creack
Hello golang-codereviews@googlegroups.com, iant@golang.org, josharian@gmail.com, mikioh.mikioh@gmail.com, alex@lx.lc (cc: golang-codereviews@googlegroups.com, rsc@golang.org), Please take another look.
9 years, 11 months ago (2014-04-05 23:27:23 UTC) #20
josharian
Getting close! * The implementation looks good, modulo nitpicks below. * I don't see how ...
9 years, 11 months ago (2014-04-07 17:38:43 UTC) #21
creack
Hello golang-codereviews@googlegroups.com, iant@golang.org, josharian@gmail.com, mikioh.mikioh@gmail.com, alex@lx.lc (cc: golang-codereviews@googlegroups.com, rsc@golang.org), Please take another look.
9 years, 11 months ago (2014-04-13 11:23:50 UTC) #22
josharian
The tests are failing for me: $ go test net --- FAIL: TestReloadResolvConfChange (0.57 seconds) ...
9 years, 11 months ago (2014-04-14 17:50:39 UTC) #23
creack
My bad, I ran only my tests, forgot to rerun the whole one. And I ...
9 years, 11 months ago (2014-04-14 17:53:00 UTC) #24
josharian
> And I did signed the CLA. Do I need to add something to the ...
9 years, 11 months ago (2014-04-14 18:03:20 UTC) #25
iant
On Mon, Apr 14, 2014 at 11:03 AM, <josharian@gmail.com> wrote: >> And I did signed ...
9 years, 11 months ago (2014-04-14 18:26:47 UTC) #26
creack
Hello golang-codereviews@googlegroups.com, iant@golang.org, josharian@gmail.com, mikioh.mikioh@gmail.com, alex@lx.lc (cc: golang-codereviews@googlegroups.com, rsc@golang.org), Please take another look.
9 years, 11 months ago (2014-04-15 05:17:34 UTC) #27
josharian
I'm afraid that the tests still fail for me: $ go test net --- FAIL: ...
9 years, 11 months ago (2014-04-15 16:55:09 UTC) #28
creack
Hmm, weird. My previous patch was time based so obviously racy, but now, everything is ...
9 years, 11 months ago (2014-04-15 16:57:03 UTC) #29
josharian
> Hmm, weird. My previous patch was time based so obviously racy, but now, > ...
9 years, 11 months ago (2014-04-15 17:33:20 UTC) #30
creack
Hello golang-codereviews@googlegroups.com, iant@golang.org, josharian@gmail.com, mikioh.mikioh@gmail.com, alex@lx.lc (cc: golang-codereviews@googlegroups.com, rsc@golang.org), Please take another look.
9 years, 11 months ago (2014-04-16 16:31:38 UTC) #31
josharian
On 2014/04/16 16:31:38, creack wrote: > Hello mailto:golang-codereviews@googlegroups.com, mailto:iant@golang.org, mailto:josharian@gmail.com, > mailto:mikioh.mikioh@gmail.com, mailto:alex@lx.lc (cc: mailto:golang-codereviews@googlegroups.com, ...
9 years, 11 months ago (2014-04-16 18:42:06 UTC) #32
josharian
TestReloadResolvConfFail is failing for me because cfg.mtime doesn't get reset; an old cfg.mtime prevented an ...
9 years, 11 months ago (2014-04-18 16:15:26 UTC) #33
creack
Hello golang-codereviews@googlegroups.com, iant@golang.org, josharian@gmail.com, mikioh.mikioh@gmail.com, alex@lx.lc (cc: golang-codereviews@googlegroups.com, rsc@golang.org), Please take another look.
9 years, 11 months ago (2014-04-29 18:55:54 UTC) #34
josharian
LGTM but as I'm now too familiar with this code, I'd like another reviewer to ...
9 years, 11 months ago (2014-04-29 22:11:14 UTC) #35
josharian
Oh, and thanks for your tenacity in seeing this through. Much appreciated. -josh
9 years, 11 months ago (2014-04-29 22:12:28 UTC) #36
mikio
looks like introducing "type resolvConf struct" and using it not only for reading resolv.conf but ...
9 years, 11 months ago (2014-04-30 00:46:26 UTC) #37
gobot
R=iant@golang.org (assigned by mikioh.mikioh@gmail.com)
9 years, 11 months ago (2014-04-30 00:47:39 UTC) #38
axaxs
Sorry it has been a while since I've looked at this myself. It's late here ...
9 years, 11 months ago (2014-04-30 02:05:50 UTC) #39
creack
The quit chan is used only in tests. It allows the tests to start and ...
9 years, 10 months ago (2014-05-05 19:55:15 UTC) #40
iant
LGTM
9 years, 10 months ago (2014-05-14 22:25:34 UTC) #41
iant
*** Submitted as https://code.google.com/p/go/source/detail?r=afc520948686 *** net: detect changes to /etc/resolv.conf. Implement the changes as suggested ...
9 years, 10 months ago (2014-05-15 00:11:11 UTC) #42
ruiu
The third argument of loadConfig() is always nil, so the code to read from it ...
9 years, 10 months ago (2014-05-15 01:58:48 UTC) #43
mikio
On Thu, May 15, 2014 at 10:58 AM, <ruiu@google.com> wrote: > The third argument of ...
9 years, 10 months ago (2014-05-15 02:09:36 UTC) #44
creack
If you have a better idea for testing, I'd love to remove that parameter On ...
9 years, 10 months ago (2014-05-15 02:24:02 UTC) #45
mikio
if i were you, i'd use resolveconf instead of keeping fake resolvconf-ish stuff. the test ...
9 years, 10 months ago (2014-05-15 04:19:40 UTC) #46
ruiu
On Wed, May 14, 2014 at 7:09 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote: > On Thu, ...
9 years, 10 months ago (2014-05-15 04:32:06 UTC) #47
mikio
9 years, 10 months ago (2014-05-15 04:40:02 UTC) #48
On Thu, May 15, 2014 at 1:31 PM, Rui Ueyama <ruiu@google.com> wrote:

> As you might have already discussed, you might want to do the same goroutine
> file polling operation on /etc/hosts as well. I'm curious why we use a
> goroutine for /etc/resolv.conf but don't for /etc/hosts.

because we've been waiting for fsevnet-like stuff long time. but in
go1.4, eventually... hope so.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b