Issue 58660044: code review 58660044: syscall: add fcntl test (Closed)
Descriptionsyscall: add fcntl test
Also updates documentation.
Patch Set 1 : diff -r e2dd08f26402 https://code.google.com/p/go #Patch Set 2 : diff -r e2dd08f26402 https://code.google.com/p/go #
Total comments: 2
Patch Set 3 : diff -r 18ecaf7688b2 https://code.google.com/p/go #
Total comments: 1
Patch Set 4 : diff -r a15f344a9efa https://code.google.com/p/go #Patch Set 5 : diff -r cf5dd8accd5e https://code.google.com/p/go #Patch Set 6 : diff -r cf5dd8accd5e https://code.google.com/p/go #
Total comments: 5
Patch Set 7 : diff -r cf5dd8accd5e https://code.google.com/p/go #
Total comments: 1
Patch Set 8 : diff -r cf5dd8accd5e https://code.google.com/p/go #Patch Set 9 : diff -r cf5dd8accd5e https://code.google.com/p/go #
Total comments: 2
Patch Set 10 : diff -r 3cf533be5e36 https://code.google.com/p/go #Patch Set 11 : diff -r 3cf533be5e36 https://code.google.com/p/go #MessagesTotal messages: 16
Hello iant@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
https://codereview.appspot.com/58660044/diff/40001/src/pkg/syscall/fcntl_unix... File src/pkg/syscall/fcntl_unix_test.go (right): https://codereview.appspot.com/58660044/diff/40001/src/pkg/syscall/fcntl_unix... src/pkg/syscall/fcntl_unix_test.go:27: name := "/tmp/TestFcntlFlock" make a tempdir first and put name in there. don't depend on being able to make this. there could be two builders running on the same machine as different users.
Sign in to reply to this message.
ptal https://codereview.appspot.com/58660044/diff/40001/src/pkg/syscall/fcntl_unix... File src/pkg/syscall/fcntl_unix_test.go (right): https://codereview.appspot.com/58660044/diff/40001/src/pkg/syscall/fcntl_unix... src/pkg/syscall/fcntl_unix_test.go:27: name := "/tmp/TestFcntlFlock" On 2014/02/12 16:37:17, bradfitz wrote: > make a tempdir first and put name in there. don't depend on being able to make > this. there could be two builders running on the same machine as different > users. Done.
Sign in to reply to this message.
On Thu, Feb 13, 2014 at 6:50 AM, <nightlyone@googlemail.com> wrote: > Just two grammar nits. thanks. but i changed my mind to leave consistency_unix_test.go as it is and drop the blank identifier function from this cl.
Sign in to reply to this message.
Hello iant@golang.org, bradfitz@golang.org, nightlyone@googlemail.com (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/58660044/diff/110001/src/pkg/syscall/syscall_u... File src/pkg/syscall/syscall_unix_test.go (right): https://codereview.appspot.com/58660044/diff/110001/src/pkg/syscall/syscall_u... src/pkg/syscall/syscall_unix_test.go:7: // This file tests that some basic syscalls are consistent across please update this comment while you're at it. https://codereview.appspot.com/58660044/diff/110001/src/pkg/syscall/syscall_u... src/pkg/syscall/syscall_unix_test.go:72: if err := syscall.FcntlFlock(uintptr(fd), syscall.F_GETLK, &flock); err != nil { do you want to first set and then get to verify, for example, the Flock_t structure definition is correct? I'm not sure if we want to do more tests than that.
Sign in to reply to this message.
ptal https://codereview.appspot.com/58660044/diff/110001/src/pkg/syscall/syscall_u... File src/pkg/syscall/syscall_unix_test.go (right): https://codereview.appspot.com/58660044/diff/110001/src/pkg/syscall/syscall_u... src/pkg/syscall/syscall_unix_test.go:7: // This file tests that some basic syscalls are consistent across On 2014/02/24 05:49:05, minux wrote: > please update this comment while you're at it. Done. https://codereview.appspot.com/58660044/diff/110001/src/pkg/syscall/syscall_u... src/pkg/syscall/syscall_unix_test.go:72: if err := syscall.FcntlFlock(uintptr(fd), syscall.F_GETLK, &flock); err != nil { On 2014/02/24 05:49:05, minux wrote: > do you want to first set and then get to verify, for example, > the Flock_t structure definition is correct? > > I'm not sure if we want to do more tests than that. i'm fine as it is.
Sign in to reply to this message.
https://codereview.appspot.com/58660044/diff/110001/src/pkg/syscall/syscall_u... File src/pkg/syscall/syscall_unix_test.go (right): https://codereview.appspot.com/58660044/diff/110001/src/pkg/syscall/syscall_u... src/pkg/syscall/syscall_unix_test.go:72: if err := syscall.FcntlFlock(uintptr(fd), syscall.F_GETLK, &flock); err != nil { On 2014/02/24 06:41:18, mikio wrote: > On 2014/02/24 05:49:05, minux wrote: > > do you want to first set and then get to verify, for example, > > the Flock_t structure definition is correct? > > > > I'm not sure if we want to do more tests than that. > > i'm fine as it is. Don't you want to test F_SETLK? If you don't want to add that test, at least add _ = syscall.F_SETLK (probably also F_SETLKW). https://codereview.appspot.com/58660044/diff/130001/src/pkg/syscall/syscall_u... File src/pkg/syscall/syscall_unix_test.go (right): https://codereview.appspot.com/58660044/diff/130001/src/pkg/syscall/syscall_u... src/pkg/syscall/syscall_unix_test.go:25: // Unix-like systems. i think these comments are too verbose though.
Sign in to reply to this message.
On Mon, Feb 24, 2014 at 3:41 PM, <mikioh.mikioh@gmail.com> wrote: > i'm fine as it is. i mean, on linux fcntl_getlk in fs/locks.c calls fancy flock_to_posix_lock, so it looks enough.
Sign in to reply to this message.
On Mon, Feb 24, 2014 at 3:46 PM, <minux.ma@gmail.com> wrote: > Don't you want to test F_SETLK? > > If you don't want to add that test, at least add _ = syscall.F_SETLK > (probably also F_SETLKW). Ah, do you want a functionality test? > src/pkg/syscall/syscall_unix_test.go:25: // Unix-like systems. > i think these comments are too verbose though. suggestions pls.
Sign in to reply to this message.
On Mon, Feb 24, 2014 at 1:52 AM, Mikio Hara <mikioh.mikioh@gmail.com> wrote: > On Mon, Feb 24, 2014 at 3:46 PM, <minux.ma@gmail.com> wrote: > > Don't you want to test F_SETLK? > > > > If you don't want to add that test, at least add _ = syscall.F_SETLK > > (probably also F_SETLKW). > > Ah, do you want a functionality test? > It's up to you. But I want to make sure we at least provide all required constants and structures for each Unix. (I was suggesting set and get a lock and compare if it's the same as a simple functionality test.) > > > src/pkg/syscall/syscall_unix_test.go:25: // Unix-like systems. > > i think these comments are too verbose though. > > suggestions pls. > Remove redundant "This blank identifier function tests that" (the reader should know that from code), and change "are consistent across all Unix-like systems." to "are consistent" (or similar). Actually, I suggest we remove both, and add a block comment before all those consistency test function, explaining what these do (test all these function/constants are consistent [and have the same type] on all unix-like systems. Then for each function, just say what's it's testing, e.g. simply "// termios constants", "// Flock_t structure", etc.
Sign in to reply to this message.
On Mon, Feb 24, 2014 at 4:06 PM, minux <minux.ma@gmail.com> wrote: >> Ah, do you want a functionality test? > > It's up to you. But I want to make sure we at least provide all required > constants and structures for each Unix. got it, i'll simply add F_GETLK/F_SETLK/F_SEKLKW constant tests in this cl. >> suggestions pls. > > Remove redundant ... (snip) thanks for the suggestions.
Sign in to reply to this message.
Hello iant@golang.org, bradfitz@golang.org, nightlyone@googlemail.com, minux.ma@gmail.com (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM. https://codereview.appspot.com/58660044/diff/170001/src/pkg/syscall/syscall_u... File src/pkg/syscall/syscall_unix_test.go (right): https://codereview.appspot.com/58660044/diff/170001/src/pkg/syscall/syscall_u... src/pkg/syscall/syscall_unix_test.go:69: defer func() { why not just defer syscall.Unlink(name) defer syscall.Close(fd)
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=aaeb25be6d7f *** syscall: add fcntl test Also updates documentation. LGTM=minux.ma R=iant, bradfitz, nightlyone, minux.ma CC=golang-codereviews https://codereview.appspot.com/58660044 https://codereview.appspot.com/58660044/diff/170001/src/pkg/syscall/syscall_u... File src/pkg/syscall/syscall_unix_test.go (right): https://codereview.appspot.com/58660044/diff/170001/src/pkg/syscall/syscall_u... src/pkg/syscall/syscall_unix_test.go:69: defer func() { On 2014/02/24 07:59:11, minux wrote: > why not just > defer syscall.Unlink(name) > defer syscall.Close(fd) Done.
Sign in to reply to this message.
|
