code review 6445049: misc/cgo/test: add test for issue 3871: cgo setgid hang...

misc/cgo/test: add test for issue 3871: cgo setgid hang on GNU/Linux

[iant, 2012-07-27 05:49:34]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitzgoog, 2012-07-27 06:11:54]

LGTM but I'd do more than 2 seconds.

ARM builders always surprise me with their slowness.
 On Jul 26, 2012 10:49 PM, <iant@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> misc/cgo/test: add test for issue 3871: cgo setgid hang on GNU/Linux
>
> Please review this at
http://codereview.appspot.com/**6445049/<http://codereview.appspot.com/6445049/>
>
> Affected files:
>   M misc/cgo/test/basic.go
>   M misc/cgo/test/cgo_test.go
>
>
> Index: misc/cgo/test/basic.go
> ==============================**==============================**=======
> --- a/misc/cgo/test/basic.go
> +++ b/misc/cgo/test/basic.go
> @@ -11,6 +11,7 @@
>  #include <stdlib.h>
>  #include <sys/stat.h>
>  #include <errno.h>
> +#include <unistd.h>
>
>  #define SHIFT(x, y)  ((x)<<(y))
>  #define KILO SHIFT(1, 10)
> @@ -57,6 +58,7 @@
>  import (
>         "syscall"
>         "testing"
> +       "time"
>         "unsafe"
>  )
>
> @@ -124,6 +126,20 @@
>         C.free(unsafe.Pointer(p))
>  }
>
> +func testSetgid(t *testing.T) {
> +       // Issue 3871.
> +       c := make(chan bool)
> +       go func() {
> +               C.setgid(0)
> +               c <- true
> +       }()
> +       select {
> +       case <-c:
> +       case <-time.After(2 * time.Second):
> +               t.Error("setgid hung")
> +       }
> +}
> +
>  var (
>         cuint  = (C.uint)(0)
>         culong C.ulong
> Index: misc/cgo/test/cgo_test.go
> ==============================**==============================**=======
> --- a/misc/cgo/test/cgo_test.go
> +++ b/misc/cgo/test/cgo_test.go
> @@ -27,5 +27,6 @@
>  func TestParallelSleep(t *testing.T)       { testParallelSleep(t) }
>  func TestSetEnv(t *testing.T)              { testSetEnv(t) }
>  func TestHelpers(t *testing.T)             { testHelpers(t) }
> +func TestSetgid(t *testing.T)              { testSetgid(t) }
>
>  func BenchmarkCgoCall(b *testing.B) { benchCgoCall(b) }
>
>
>

[iant, 2012-07-27 06:21:45]

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

misc/cgo/test: add test for issue 3871: cgo setgid hang on GNU/Linux

R=golang-dev, bradfitz
CC=golang-dev
http://codereview.appspot.com/6445049

[iant2, 2012-07-27 06:21:52]

On Thu, Jul 26, 2012 at 11:11 PM, Brad Fitzpatrick <bradfitz@google.com> wrote:
> LGTM but I'd do more than 2 seconds.
>
> ARM builders always surprise me with their slowness.

Changed to 5 seconds and submitted.

Although I don't see how calling setgid could take even 1 second, even
on a terribly slow machine.  But, who knows.

Ian


> On Jul 26, 2012 10:49 PM, <iant@golang.org> wrote:
>>
>> Reviewers: golang-dev_googlegroups.com,
>>
>> Message:
>> Hello golang-dev@googlegroups.com,
>>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg/
>>
>>
>> Description:
>> misc/cgo/test: add test for issue 3871: cgo setgid hang on GNU/Linux
>>
>> Please review this at http://codereview.appspot.com/6445049/
>>
>> Affected files:
>>   M misc/cgo/test/basic.go
>>   M misc/cgo/test/cgo_test.go
>>
>>
>> Index: misc/cgo/test/basic.go
>> ===================================================================
>> --- a/misc/cgo/test/basic.go
>> +++ b/misc/cgo/test/basic.go
>> @@ -11,6 +11,7 @@
>>  #include <stdlib.h>
>>  #include <sys/stat.h>
>>  #include <errno.h>
>> +#include <unistd.h>
>>
>>  #define SHIFT(x, y)  ((x)<<(y))
>>  #define KILO SHIFT(1, 10)
>> @@ -57,6 +58,7 @@
>>  import (
>>         "syscall"
>>         "testing"
>> +       "time"
>>         "unsafe"
>>  )
>>
>> @@ -124,6 +126,20 @@
>>         C.free(unsafe.Pointer(p))
>>  }
>>
>> +func testSetgid(t *testing.T) {
>> +       // Issue 3871.
>> +       c := make(chan bool)
>> +       go func() {
>> +               C.setgid(0)
>> +               c <- true
>> +       }()
>> +       select {
>> +       case <-c:
>> +       case <-time.After(2 * time.Second):
>> +               t.Error("setgid hung")
>> +       }
>> +}
>> +
>>  var (
>>         cuint  = (C.uint)(0)
>>         culong C.ulong
>> Index: misc/cgo/test/cgo_test.go
>> ===================================================================
>> --- a/misc/cgo/test/cgo_test.go
>> +++ b/misc/cgo/test/cgo_test.go
>> @@ -27,5 +27,6 @@
>>  func TestParallelSleep(t *testing.T)       { testParallelSleep(t) }
>>  func TestSetEnv(t *testing.T)              { testSetEnv(t) }
>>  func TestHelpers(t *testing.T)             { testHelpers(t) }
>> +func TestSetgid(t *testing.T)              { testSetgid(t) }
>>
>>  func BenchmarkCgoCall(b *testing.B) { benchCgoCall(b) }
>>
>>
>

[minux1, 2012-07-28 04:02:58]

i think this test will fail on windows (although we currently don't enable
cgo tests on windows, but we would in the future, right?)

[iant2, 2012-07-28 06:44:55]

On Fri, Jul 27, 2012 at 9:02 PM,  <minux.ma@gmail.com> wrote:
> i think this test will fail on windows (although we currently don't
> enable
> cgo tests on windows, but we would in the future, right?)

Good point.  Does this look right?

http://codereview.appspot.com/6446060

Ian