Issue 8334044: pkg:syscall: fix a bug in the shuffling of file descrip... (Closed)
|
Created:
11 years, 11 months ago by cnicolaou Modified:
11 years, 10 months ago Reviewers:
CC:
iant, iant2, r, bradfitz, golang-dev Visibility:
Public. |
Descriptionsyscall: fix a bug in the shuffling of file descriptors in StartProcess on Linux.
Patch Set 1 #Patch Set 2 : diff -r 56b90ebca903 https://code.google.com/p/go #
Total comments: 14
Patch Set 3 : diff -r 2593f9be47d0 https://code.google.com/p/go #
MessagesTotal messages: 18
The fix looks good. I wouldn't mind another set of eyes on the test, though. Phew. https://codereview.appspot.com/8334044/diff/2001/src/pkg/syscall/exec_linux.go File src/pkg/syscall/exec_linux.go (right): https://codereview.appspot.com/8334044/diff/2001/src/pkg/syscall/exec_linux.g... src/pkg/syscall/exec_linux.go:49: if int(ufd) > nextfd { i would reverse this because nextfd is the target and the order is then the same on the next line if nextfd < int(ufd)
Sign in to reply to this message.
https://codereview.appspot.com/8334044/diff/2001/src/pkg/os/exec/exec_test.go File src/pkg/os/exec/exec_test.go (right): https://codereview.appspot.com/8334044/diff/2001/src/pkg/os/exec/exec_test.go... src/pkg/os/exec/exec_test.go:244: w.Write([]byte(data[i])) Can use w.WriteString here. https://codereview.appspot.com/8334044/diff/2001/src/pkg/os/exec/exec_test.go... src/pkg/os/exec/exec_test.go:282: case <-time.After(1 * time.Second): s/1/5/ We have some very slow builders. https://codereview.appspot.com/8334044/diff/2001/src/pkg/os/exec/exec_test.go... src/pkg/os/exec/exec_test.go:319: Seems like an unrelated change. https://codereview.appspot.com/8334044/diff/2001/src/pkg/os/exec/exec_test.go... src/pkg/os/exec/exec_test.go:332: Seems like an unrelated change. https://codereview.appspot.com/8334044/diff/2001/src/pkg/os/exec/exec_test.go... src/pkg/os/exec/exec_test.go:357: Unrelated change. https://codereview.appspot.com/8334044/diff/2001/src/pkg/os/exec/exec_test.go... src/pkg/os/exec/exec_test.go:574: case <-time.After(1000 * time.Millisecond): Bump this timeout too, for slow builders.
Sign in to reply to this message.
Hello iant@golang.org, iant@google.com, r@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
I've made the changes. https://codereview.appspot.com/8334044/diff/2001/src/pkg/os/exec/exec_test.go File src/pkg/os/exec/exec_test.go (right): https://codereview.appspot.com/8334044/diff/2001/src/pkg/os/exec/exec_test.go... src/pkg/os/exec/exec_test.go:244: w.Write([]byte(data[i])) On 2013/04/30 16:30:11, iant wrote: > Can use w.WriteString here. Done. And the one above for _LAST. https://codereview.appspot.com/8334044/diff/2001/src/pkg/os/exec/exec_test.go... src/pkg/os/exec/exec_test.go:282: case <-time.After(1 * time.Second): On 2013/04/30 16:30:11, iant wrote: > s/1/5/ > > We have some very slow builders. Done. https://codereview.appspot.com/8334044/diff/2001/src/pkg/os/exec/exec_test.go... src/pkg/os/exec/exec_test.go:319: On 2013/04/30 16:30:11, iant wrote: > Seems like an unrelated change. Done. https://codereview.appspot.com/8334044/diff/2001/src/pkg/os/exec/exec_test.go... src/pkg/os/exec/exec_test.go:332: On 2013/04/30 16:30:11, iant wrote: > Seems like an unrelated change. Done. https://codereview.appspot.com/8334044/diff/2001/src/pkg/os/exec/exec_test.go... src/pkg/os/exec/exec_test.go:357: On 2013/04/30 16:30:11, iant wrote: > Unrelated change. Done. https://codereview.appspot.com/8334044/diff/2001/src/pkg/os/exec/exec_test.go... src/pkg/os/exec/exec_test.go:574: case <-time.After(1000 * time.Millisecond): On 2013/04/30 16:30:11, iant wrote: > Bump this timeout too, for slow builders. Done. https://codereview.appspot.com/8334044/diff/2001/src/pkg/syscall/exec_linux.go File src/pkg/syscall/exec_linux.go (right): https://codereview.appspot.com/8334044/diff/2001/src/pkg/syscall/exec_linux.g... src/pkg/syscall/exec_linux.go:49: if int(ufd) > nextfd { On 2013/04/30 15:54:54, r wrote: > i would reverse this because nextfd is the target and the order is then the same > on the next line > if nextfd < int(ufd) Done.
Sign in to reply to this message.
CL description, first line: syscall: fix StartProcess shuffling of file descriptors on Linux (or something. but drop /^pkg:/ and mention Linux) On Tue, Apr 30, 2013 at 10:46 AM, <cnicolaou@google.com> wrote: > Reviewers: iant, iant2, r, > > Message: > Hello iant@golang.org, iant@google.com, r@golang.org (cc: > golang-dev@googlegroups.com), > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > pkg:syscall: fix a bug in the shuffling of file descriptors in > StartProcess. > > Please review this at https://codereview.appspot.**com/8334044/<https://codereview.appspot.com/8334... > > Affected files: > M src/pkg/os/exec/exec_test.go > M src/pkg/syscall/exec_linux.go > > > -- > > ---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... > . > > >
Sign in to reply to this message.
done. It looks to me as if the same problem exists on plan9, bsd - how do you handle things like this? I presume if I check in the new test it may well fail on those systems, but I don't have access to them to fix them etc? On Tue, Apr 30, 2013 at 10:53 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > CL description, first line: > > syscall: fix StartProcess shuffling of file descriptors on Linux > > (or something. but drop /^pkg:/ and mention Linux) > > > > On Tue, Apr 30, 2013 at 10:46 AM, <cnicolaou@google.com> wrote: >> >> Reviewers: iant, iant2, r, >> >> Message: >> Hello iant@golang.org, iant@google.com, r@golang.org (cc: >> golang-dev@googlegroups.com), >> >> I'd like you to review this change to >> https://code.google.com/p/go >> >> >> Description: >> pkg:syscall: fix a bug in the shuffling of file descriptors in >> StartProcess. >> >> Please review this at https://codereview.appspot.com/8334044/ >> >> Affected files: >> M src/pkg/os/exec/exec_test.go >> M src/pkg/syscall/exec_linux.go >> >> >> >> -- >> >> ---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. >> For more options, visit https://groups.google.com/groups/opt_out. >> >> >
Sign in to reply to this message.
Don't worry about plan9. They don't have builders running yet, so they don't count. We can get you access to any of the builder machines for testing, though. Or, it's also common to fix it blind and submit, hoping for the best, as long as you're around to rollback or fix quickly if the tests don't pass. On Tue, Apr 30, 2013 at 11:01 AM, Cosmos Nicolaou <cnicolaou@google.com>wrote: > done. It looks to me as if the same problem exists on plan9, bsd - how > do you handle things like this? I presume if I check in the new test > it may well fail on those systems, but I don't have access to them to > fix them etc? > > On Tue, Apr 30, 2013 at 10:53 AM, Brad Fitzpatrick <bradfitz@golang.org> > wrote: > > CL description, first line: > > > > syscall: fix StartProcess shuffling of file descriptors on Linux > > > > (or something. but drop /^pkg:/ and mention Linux) > > > > > > > > On Tue, Apr 30, 2013 at 10:46 AM, <cnicolaou@google.com> wrote: > >> > >> Reviewers: iant, iant2, r, > >> > >> Message: > >> Hello iant@golang.org, iant@google.com, r@golang.org (cc: > >> golang-dev@googlegroups.com), > >> > >> I'd like you to review this change to > >> https://code.google.com/p/go > >> > >> > >> Description: > >> pkg:syscall: fix a bug in the shuffling of file descriptors in > >> StartProcess. > >> > >> Please review this at https://codereview.appspot.com/8334044/ > >> > >> Affected files: > >> M src/pkg/os/exec/exec_test.go > >> M src/pkg/syscall/exec_linux.go > >> > >> > >> > >> -- > >> > >> ---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. > >> For more options, visit https://groups.google.com/groups/opt_out. > >> > >> > > >
Sign in to reply to this message.
LGTM Wait for r.
Sign in to reply to this message.
LGTM thanks.
Sign in to reply to this message.
exec_test.go:270: Read: EOF exec_test.go:283: Read timedout is what I got on darwin -rob
Sign in to reply to this message.
ok, i have a fix for bsd.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=b9afdc08225a *** syscall: fix a bug in the shuffling of file descriptors in StartProcess on Linux. R=iant, iant, r, bradfitz CC=golang-dev https://codereview.appspot.com/8334044 Committer: Rob Pike <r@golang.org>
Sign in to reply to this message.
Whoa, what: go build math: wait: bad address http://build.golang.org/log/8ffe851878f9d1562b959264c7bddc34c3cdd394 On Tue, Apr 30, 2013 at 11:43 AM, Rob Pike <r@golang.org> wrote: > ok, i have a fix for bsd. >
Sign in to reply to this message.
On Tue, Apr 30, 2013 at 11:57 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > Whoa, what: > > go build math: wait: bad address > http://build.golang.org/log/8ffe851878f9d1562b959264c7bddc34c3cdd394 That is disturbing, and I don't know what is going on, but I don't see how it could be related to this CL. Ian
Sign in to reply to this message.
>On Tue, Apr 30, 2013 at 11:07 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > Don't worry about plan9. They don't have builders running yet, so they > don't count. > > We can get you access to any of the builder machines for testing, though. sure, can you do that so that I can fix this up on all of the machine types. Darwin seems to work fine for me though. Thanks. Cheers, Cos. > > Or, it's also common to fix it blind and submit, hoping for the best, as > long as you're around to rollback or fix quickly if the tests don't pass. > > > > > On Tue, Apr 30, 2013 at 11:01 AM, Cosmos Nicolaou <cnicolaou@google.com> > wrote: >> >> done. It looks to me as if the same problem exists on plan9, bsd - how >> do you handle things like this? I presume if I check in the new test >> it may well fail on those systems, but I don't have access to them to >> fix them etc? >> >> On Tue, Apr 30, 2013 at 10:53 AM, Brad Fitzpatrick <bradfitz@golang.org> >> wrote: >> > CL description, first line: >> > >> > syscall: fix StartProcess shuffling of file descriptors on Linux >> > >> > (or something. but drop /^pkg:/ and mention Linux) >> > >> > >> > >> > On Tue, Apr 30, 2013 at 10:46 AM, <cnicolaou@google.com> wrote: >> >> >> >> Reviewers: iant, iant2, r, >> >> >> >> Message: >> >> Hello iant@golang.org, iant@google.com, r@golang.org (cc: >> >> golang-dev@googlegroups.com), >> >> >> >> I'd like you to review this change to >> >> https://code.google.com/p/go >> >> >> >> >> >> Description: >> >> pkg:syscall: fix a bug in the shuffling of file descriptors in >> >> StartProcess. >> >> >> >> Please review this at https://codereview.appspot.com/8334044/ >> >> >> >> Affected files: >> >> M src/pkg/os/exec/exec_test.go >> >> M src/pkg/syscall/exec_linux.go >> >> >> >> >> >> >> >> -- >> >> >> >> ---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. >> >> For more options, visit https://groups.google.com/groups/opt_out. >> >> >> >> >> > > >
Sign in to reply to this message.
On Tue, Apr 30, 2013 at 1:26 PM, Cosmos Nicolaou <cnicolaou@google.com> wrote: >>On Tue, Apr 30, 2013 at 11:07 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: >> Don't worry about plan9. They don't have builders running yet, so they >> don't count. >> >> We can get you access to any of the builder machines for testing, though. > > sure, can you do that so that I can fix this up on all of the machine > types. Darwin seems to work fine for me though. Thanks. We've disabled the test rather than worry about it for 1.1. Ian
Sign in to reply to this message.
sure, I just saw that. I'm still happy to try and fix it - I don't think it'll be too hard once I have access to the build machines. If that's a hassle then I can just take care of it later when you guys have time to set me up with access. Thanks. Cheers, Cos. On Tue, Apr 30, 2013 at 1:30 PM, Ian Lance Taylor <iant@google.com> wrote: > On Tue, Apr 30, 2013 at 1:26 PM, Cosmos Nicolaou <cnicolaou@google.com> wrote: >>>On Tue, Apr 30, 2013 at 11:07 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: >>> Don't worry about plan9. They don't have builders running yet, so they >>> don't count. >>> >>> We can get you access to any of the builder machines for testing, though. >> >> sure, can you do that so that I can fix this up on all of the machine >> types. Darwin seems to work fine for me though. Thanks. > > We've disabled the test rather than worry about it for 1.1. > > Ian
Sign in to reply to this message.
fwiw I've tracked this down to being another symptom of https://code.google.com/p/go/issues/detail?id=2603. I'll work on a workaround for this test and look into a fix for 2603. Cheers, Cos. On 2013/04/30 20:34:31, cnicolaou wrote: > sure, I just saw that. I'm still happy to try and fix it - I don't > think it'll be too hard once I have access to the build machines. If > that's a hassle then I can just take care of it later when you guys > have time to set me up with access. Thanks. > > Cheers, Cos. > > On Tue, Apr 30, 2013 at 1:30 PM, Ian Lance Taylor <mailto:iant@google.com> wrote: > > On Tue, Apr 30, 2013 at 1:26 PM, Cosmos Nicolaou <mailto:cnicolaou@google.com> > wrote: > >>>On Tue, Apr 30, 2013 at 11:07 AM, Brad Fitzpatrick <mailto:bradfitz@golang.org> > wrote: > >>> Don't worry about plan9. They don't have builders running yet, so they > >>> don't count. > >>> > >>> We can get you access to any of the builder machines for testing, though. > >> > >> sure, can you do that so that I can fix this up on all of the machine > >> types. Darwin seems to work fine for me though. Thanks. > > > > We've disabled the test rather than worry about it for 1.1. > > > > Ian
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
