|
|
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change.
Let's do this review in two steps. Could you drop the os files from this CL, so that it's just the syscall changes? Also, I have cleaned up some hand-editing drift in the syscall directory, so you will need to sync and rebuild the z files. Please do so on a standard Ubuntu Lucid box. Thanks.
On 2010年09月24日 19:39:07, rsc1 wrote: > Let's do this review in two steps. > > Could you drop the os files from this CL, > so that it's just the syscall changes? > > Also, I have cleaned up some hand-editing > drift in the syscall directory, so you will > need to sync and rebuild the z files. > Please do so on a standard Ubuntu Lucid box. > Thanks. I've moved the syscall changes into a separate CL: http://codereview.appspot.com/2241045 Also synced and regenerated. Thanks, Balazs
http://codereview.appspot.com/2049043/diff/18001/src/pkg/os/inotify/inotify_l... File src/pkg/os/inotify/inotify_linux.go (right): http://codereview.appspot.com/2049043/diff/18001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:77: go in.readEvents() Hello all I'm not sure what the current thinking is, but should API-type code be spawning goroutines? Maybe the choice to call in.readEvents() should be left to the user of the API. Some users might spawn goroutines, but others might want to write some kind of loop that is part of a larger function executed by a goroutine. Just a thought. Cheers Albert
http://codereview.appspot.com/2049043/diff/18001/src/pkg/os/inotify/inotify_l... File src/pkg/os/inotify/inotify_linux.go (right): http://codereview.appspot.com/2049043/diff/18001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:77: go in.readEvents() On 2010年09月29日 15:04:40, albert.strasheim wrote: > Hello all > > I'm not sure what the current thinking is, but should API-type code be spawning > goroutines? I think it's correct and idiomatic to do this. Run $ hg grep 'go .*\(' | grep ^src/pkg | less to see a list of examples. > > Maybe the choice to call in.readEvents() should be left to the user of the API. My idea was to make the API straightforward to use and only expose what's really necessary for the user. Do you have a concrete recommendation how to expose readEvents()? > Some users might spawn goroutines, but others might want to write some kind of > loop that is part of a larger function executed by a goroutine. > > Just a thought. > > Cheers > > Albert
Hello all
On 2010年09月23日 11:52:54, leczb wrote:
> Hello mailto:golang-dev@googlegroups.com (cc:
mailto:golang-dev@googlegroups.com),
>
> I'd like you to review this change.
I tried the following:
watcher, err := inotify.New()
watcher.Close()
watcher.Close()
The second Close hangs, which isn't what I would expect.
I also tried:
watcher, err := inotify.New()
watcher.Close()
err = watcher.Watch("/tmp")
fmt.Printf("%#v\n", err)
err comes back as nil. I would expect EBADF instead?
Regards
Albert
On 2010年09月30日 14:52:37, albert.strasheim wrote:
> Hello all
>
> On 2010年09月23日 11:52:54, leczb wrote:
> > Hello mailto:golang-dev@googlegroups.com (cc:
> mailto:golang-dev@googlegroups.com),
> >
> > I'd like you to review this change.
>
> I tried the following:
>
> watcher, err := inotify.New()
> watcher.Close()
> watcher.Close()
>
> The second Close hangs, which isn't what I would expect.
>
> I also tried:
>
> watcher, err := inotify.New()
> watcher.Close()
> err = watcher.Watch("/tmp")
> fmt.Printf("%#v\n", err)
>
> err comes back as nil. I would expect EBADF instead?
>
> Regards
>
> Albert
Thanks for testing it. I'll look into it.
Balázs
On 2010年09月30日 14:54:58, leczb wrote:
> On 2010年09月30日 14:52:37, albert.strasheim wrote:
> > Hello all
> >
> > On 2010年09月23日 11:52:54, leczb wrote:
> > > Hello mailto:golang-dev@googlegroups.com (cc:
> > mailto:golang-dev@googlegroups.com),
> > >
> > > I'd like you to review this change.
> >
> > I tried the following:
> >
> > watcher, err := inotify.New()
> > watcher.Close()
> > watcher.Close()
> >
> > The second Close hangs, which isn't what I would expect.
I found the problem and fixed it.
> >
> > I also tried:
> >
> > watcher, err := inotify.New()
> > watcher.Close()
> > err = watcher.Watch("/tmp")
> > fmt.Printf("%#v\n", err)
> >
> > err comes back as nil. I would expect EBADF instead?
I've added a guard that returns an error if the inotify instance is already
closed. It won't return EBADF though. It will return "inotify instance already
closed" instead.
Background:
The file descriptor is closed in readEvents(), which is running as a separate
goroutine. You are not supposed to execute the close() syscall on a file
descriptor that is in use, so it must be closed in readEvents().
When you call watcher.Close(), it sends a "done" message to readEvents(), but
it's usually blocking on the read() syscall and only gets the message when
read() returns. Then it duly closes the file descriptor and returns. This might
take forever, if there are no incoming inotify events.
Cheers,
Balázs
> >
> > Regards
> >
> > Albert
>
> Thanks for testing it. I'll look into it.
>
> Balázs
Hello all On 2010年09月30日 15:57:43, leczb wrote: > Background: > The file descriptor is closed in readEvents(), which is running as a separate > goroutine. You are not supposed to execute the close() syscall on a file > descriptor that is in use, so it must be closed in readEvents(). > When you call watcher.Close(), it sends a "done" message to readEvents(), but > it's usually blocking on the read() syscall and only gets the message when > read() returns. Then it duly closes the file descriptor and returns. This might > take forever, if there are no incoming inotify events. I'm sure this will happen with this API, as an application probably won't call inotify.New/Close too many times in its life, but I can imagine that with patterns like this, one could introduce slow "goroutine leaks"? This might be fine for a short-running application, but for a server process that runs for days or months, couldn't this become a problem? Regards Albert
On Fri, Oct 1, 2010 at 06:42, <fullung@gmail.com> wrote: > Hello all > > > On 2010年09月30日 15:57:43, leczb wrote: > >> Background: >> The file descriptor is closed in readEvents(), which is running as a >> > separate > >> goroutine. You are not supposed to execute the close() syscall on a >> > file > >> descriptor that is in use, so it must be closed in readEvents(). >> When you call watcher.Close(), it sends a "done" message to >> > readEvents(), but > >> it's usually blocking on the read() syscall and only gets the message >> > when > >> read() returns. Then it duly closes the file descriptor and returns. >> > This might > >> take forever, if there are no incoming inotify events. >> > > I'm sure this will happen with this API, as an application probably > won't call inotify.New/Close too many times in its life, but I can > imagine that with patterns like this, one could introduce slow > "goroutine leaks"? > > This might be fine for a short-running application, but for a server > process that runs for days or months, couldn't this become a problem? > In a long running server, you should only call Close() in the server tear-down part, so I wouldn't be worried about this. Also, the Close() call will most probably generate at least one IN_IGNORED event, so the reader goroutine will shut down cleanly. Do you have alternative suggestions for the design to avoid the possible "goroutine leak"? > Regards > > Albert > > > http://codereview.appspot.com/2049043/ >
http://codereview.appspot.com/2049043/diff/26001/src/pkg/os/inotify/inotify_l... File src/pkg/os/inotify/inotify_linux.go (right): http://codereview.appspot.com/2049043/diff/26001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:105: println("adding watch for " + path) Probably didn't mean to have this println in here.
http://codereview.appspot.com/2049043/diff/26001/src/pkg/os/inotify/inotify_l... File src/pkg/os/inotify/inotify_linux.go (right): http://codereview.appspot.com/2049043/diff/26001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:105: println("adding watch for " + path) On 2010年10月05日 14:35:39, albert.strasheim wrote: > Probably didn't mean to have this println in here. Of course not. Removed it.
Hi Russ, Could you please take a look at it again? /leczb On 2010年09月24日 19:39:07, rsc1 wrote: > Let's do this review in two steps. > > Could you drop the os files from this CL, > so that it's just the syscall changes? > > Also, I have cleaned up some hand-editing > drift in the syscall directory, so you will > need to sync and rebuild the z files. > Please do so on a standard Ubuntu Lucid box. > Thanks.
please change the first line of the cl desc to os/inotify: new package This interface is nice and clean, much more so than I expected from having to wrap a Linux API. Nice job! http://codereview.appspot.com/2049043/diff/36001/src/pkg/Makefile File src/pkg/Makefile (right): http://codereview.appspot.com/2049043/diff/36001/src/pkg/Makefile#newcode102 src/pkg/Makefile:102: os/inotify\ not sorted right but also will cause other builds to fail. instead i would add after this list ifeq ($(GOOS),linux) DIRS+=\ os/inotify\ endif http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... File src/pkg/os/inotify/inotify_linux.go (right): http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:5: // This package provides a wrapper for the Linux inotify system See http://golang.org/doc/effective_go.html for more on package comments. Should be one comment before package inotify, and the example should be intended. /* This package implements a wrapper for the Linux inotify system. Example: in, err := inotify.New() if err != nil { log.Exit(err) } in.Watch("/tmp") for { select { case ev := <-in.Event: log.Println("event:", ev) case err := <-in.Error: log.Println("error:", err) } } */ package inotify http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:57: errors chan os.Error // Errors are sent on this channel another possibility is to add public fields Error <-chan os.Error Event <-chan *Event to the struct and then delete the getters below. this matches the interface to, say, time.Ticker and others. http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:65: func New() (*Inotify, os.Error) { Will there be other types? If there is a good name for this one, you could rely on the package name to provide the "inotify" word, like inotify.Watcher or whatever. And then this would be func NewWatcher. See effective go for comment format. Comments should start with the noun they describe. // New creates and returns a new inotify instance using inotify_init(2). The part about the event reader goroutine is an implementation detail http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:83: // Close an inotify instance // Close closes an inotify instance. it should return os.Error to match io.Closer http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:99: // Add a new watch for path (see inotify_add_watch(2)) // AddWatch adds path to the watched file set. // The flags are interpreted as described in inotify_add_watch(2). http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:123: // A convenience wrapper for AddWatch() // Watch adds path to the watched file set, watching all events. http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:129: // Remove a watch (see inotify_rm_watch(2)) ... http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:217: // Format the event as a human-readable string can drop this comment, because String() is so common. or keep it but fix the phrasing: // String formats the event e in the form // "filename: 0xEventMask = IN_ACCESS|IN_ATTRIB_|..." http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:221: if e.Mask&IN_ACCESS != 0 { this is aching for a table http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:281: return fmt.Sprintf("\"%s\": 0x%x == %s", e.Name, e.Mask, events[1:]) use %q http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:289: // Options for AddWatch() s/()// http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:290: IN_DONT_FOLLOW uint32 = syscall.IN_DONT_FOLLOW The IN_ prefix is not really necessary here but it's fine if you want to keep it. http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... File src/pkg/os/inotify/inotify_linux_test.go (right): http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux_test.go:13: func TestInotifyEvents(t *testing.T) { Do all recent Linuxes support inotify? http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux_test.go:20: // Add a watch for "_obj" Does it matter if _obj is a network file system?
Thanks for the constructive review. I've changed the code as suggested, but there are still a couple of questions. Please see them inline. /leczb http://codereview.appspot.com/2049043/diff/36001/src/pkg/Makefile File src/pkg/Makefile (right): http://codereview.appspot.com/2049043/diff/36001/src/pkg/Makefile#newcode102 src/pkg/Makefile:102: os/inotify\ On 2010年12月07日 19:19:45, rsc wrote: > not sorted right > but also will cause other builds to fail. instead i would add after this list > > ifeq ($(GOOS),linux) > DIRS+=\ > os/inotify\ > > endif I fixed the sorting, but didn't remove it from the list. It should be a noop on non-Linux systems, as src/pkg/os/inotify/Makefile says: [...] GOFILES_linux=\ inotify_linux.go\ GOFILES+=$(GOFILES_$(GOOS)) [...] What do you think? http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... File src/pkg/os/inotify/inotify_linux.go (right): http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:5: // This package provides a wrapper for the Linux inotify system On 2010年12月07日 19:19:45, rsc wrote: > See http://golang.org/doc/effective_go.html for more on package comments. > > Should be one comment before package inotify, and the example should be > intended. > > /* > This package implements a wrapper for the Linux inotify system. > > Example: > in, err := inotify.New() > if err != nil { > log.Exit(err) > } > in.Watch("/tmp") > for { > select { > case ev := <-in.Event: > log.Println("event:", ev) > case err := <-in.Error: > log.Println("error:", err) > } > } > > */ > package inotify Fixed. http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:57: errors chan os.Error // Errors are sent on this channel On 2010年12月07日 19:19:45, rsc wrote: > another possibility is to add public fields > > Error <-chan os.Error > Event <-chan *Event > > to the struct and then delete the getters below. > this matches the interface to, say, time.Ticker > and others. Done. http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:65: func New() (*Inotify, os.Error) { On 2010年12月07日 19:19:45, rsc wrote: > Will there be other types? If there is a good name > for this one, you could rely on the package name > to provide the "inotify" word, like inotify.Watcher > or whatever. And then this would be func NewWatcher. > > See effective go for comment format. Comments > should start with the noun they describe. > > // New creates and returns a new inotify instance using inotify_init(2). > > The part about the event reader goroutine is an implementation detail > Done. http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:83: // Close an inotify instance On 2010年12月07日 19:19:45, rsc wrote: > // Close closes an inotify instance. > > it should return os.Error to match io.Closer > Done. http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:88: return Should we return an error if it's already closed? http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:99: // Add a new watch for path (see inotify_add_watch(2)) On 2010年12月07日 19:19:45, rsc wrote: > // AddWatch adds path to the watched file set. > // The flags are interpreted as described in inotify_add_watch(2). Done. http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:123: // A convenience wrapper for AddWatch() On 2010年12月07日 19:19:45, rsc wrote: > // Watch adds path to the watched file set, watching all events. Done. http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:129: // Remove a watch (see inotify_rm_watch(2)) On 2010年12月07日 19:19:45, rsc wrote: > ... Done. http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:217: // Format the event as a human-readable string On 2010年12月07日 19:19:45, rsc wrote: > can drop this comment, because String() is so common. > or keep it but fix the phrasing: > > // String formats the event e in the form > // "filename: 0xEventMask = IN_ACCESS|IN_ATTRIB_|..." Done. http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:221: if e.Mask&IN_ACCESS != 0 { On 2010年12月07日 19:19:45, rsc wrote: > this is aching for a table I've added a map. Not sure if this is what you had in mind. http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:281: return fmt.Sprintf("\"%s\": 0x%x == %s", e.Name, e.Mask, events[1:]) On 2010年12月07日 19:19:45, rsc wrote: > use %q Done. http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:289: // Options for AddWatch() On 2010年12月07日 19:19:45, rsc wrote: > s/()// Done. http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:290: IN_DONT_FOLLOW uint32 = syscall.IN_DONT_FOLLOW On 2010年12月07日 19:19:45, rsc wrote: > The IN_ prefix is not really necessary here but it's fine if you want to keep > it. I would keep it for better greppability. http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... File src/pkg/os/inotify/inotify_linux_test.go (right): http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux_test.go:13: func TestInotifyEvents(t *testing.T) { On 2010年12月07日 19:19:45, rsc wrote: > Do all recent Linuxes support inotify? It's been supported since 2.6.13 and it's turned on by default. Of course, you can compile a kernel without inotify support. Do we have to add guards for that - arguably very rare - case? http://codereview.appspot.com/2049043/diff/36001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux_test.go:20: // Add a watch for "_obj" On 2010年12月07日 19:19:45, rsc wrote: > Does it matter if _obj is a network file system? I don't think it matters. Not that it's a proof, but I've tested it on my NFS homedir and it worked.
> I fixed the sorting, but didn't remove it from the list.
> It should be a noop on non-Linux systems, as src/pkg/os/inotify/Makefile
> says:
> [...]
> GOFILES_linux=\
> inotify_linux.go\
>
> GOFILES+=$(GOFILES_$(GOOS))
> [...]
>
> What do you think?
It doesn't seem too useful to have a package
that you can import but not use, so I'd say don't
build it at all. (Also I think it is not allowed to
have a zero-file package, because then there
is no package statement.)
> src/pkg/os/inotify/inotify_linux.go:221: if e.Mask&IN_ACCESS != 0 {
> On 2010年12月07日 19:19:45, rsc wrote:
>>
>> this is aching for a table
>
> I've added a map. Not sure if this is what you had in mind.
Not quite; will reply via codereview.
>> Do all recent Linuxes support inotify?
>
> It's been supported since 2.6.13 and it's turned on by default. Of
> course, you can compile a kernel without inotify support. Do we have to
> add guards for that - arguably very rare - case?
No, that should be fine.
If not we can always make the test look at the
kernel version and just not run on old kernels.
But for now just leave it there always.
Russ
http://codereview.appspot.com/2049043/diff/49001/src/pkg/os/inotify/inotify_l... File src/pkg/os/inotify/inotify_linux.go (right): http://codereview.appspot.com/2049043/diff/49001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:209: for event_mask, event_name := range event_names { look for _ in yuor program; there shouldn't be any below, eventNames. here, can just drop event_ entirely since it's a local variable. with the change below for b := range eventBits { if e.Mask&b.Value != 0 { events += "|" + b.Name } } http://codereview.appspot.com/2049043/diff/49001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:261: var ( drop ( ) and indentation http://codereview.appspot.com/2049043/diff/49001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:262: event_names = map[uint32]string{ This is okay but kind of overkill since you never actually look up by value, which is what map adds over a plain slice. For an iterable-only list of key,value pairs it's more efficient to use a slice: var eventBits = []struct{ Value uint32 Name string }{ {IN_ACCESS, "IN_ACCESS"}, {IN_ATTRIB, "IN_ATTRIB"}, ..., }
http://codereview.appspot.com/2049043/diff/49001/src/pkg/os/inotify/inotify_l... File src/pkg/os/inotify/inotify_linux.go (right): http://codereview.appspot.com/2049043/diff/49001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:209: for event_mask, event_name := range event_names { On 2010年12月09日 14:55:02, rsc wrote: > look for _ in yuor program; there shouldn't be any > for b := range eventBits { for _, b := range eventBits {
http://codereview.appspot.com/2049043/diff/36001/src/pkg/Makefile File src/pkg/Makefile (right): http://codereview.appspot.com/2049043/diff/36001/src/pkg/Makefile#newcode102 src/pkg/Makefile:102: os/inotify\ On 2010年12月07日 19:19:45, rsc wrote: > not sorted right > but also will cause other builds to fail. instead i would add after this list > > ifeq ($(GOOS),linux) > DIRS+=\ > os/inotify\ > > endif This doesn't seem to work. all.bash fails with: [...] make[1]: Entering directory `/usr/local/google/go/src/pkg/os/inotify' 6g -o _go_.6 inotify_linux.go inotify_linux.go:27: can't find import: fmt make[1]: *** [_go_.6] Error 1 make[1]: Leaving directory `/usr/local/google/go/src/pkg/os/inotify' I'm pretty sure it's something trivial, but I don't know how to fix it. http://codereview.appspot.com/2049043/diff/49001/src/pkg/os/inotify/inotify_l... File src/pkg/os/inotify/inotify_linux.go (right): http://codereview.appspot.com/2049043/diff/49001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:209: for event_mask, event_name := range event_names { On 2010年12月09日 14:55:02, rsc wrote: > look for _ in yuor program; there shouldn't be any > > below, eventNames. > here, can just drop event_ entirely since it's > a local variable. > > with the change below > > for b := range eventBits { > if e.Mask&b.Value != 0 { > events += "|" + b.Name > } > } Got it. Fixed. http://codereview.appspot.com/2049043/diff/49001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:209: for event_mask, event_name := range event_names { On 2010年12月09日 15:10:22, rog wrote: > On 2010年12月09日 14:55:02, rsc wrote: > > look for _ in yuor program; there shouldn't be any > > for b := range eventBits { > > for _, b := range eventBits { > > Thanks! I figured it out too, just when you sent this comment ;) http://codereview.appspot.com/2049043/diff/49001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:261: var ( On 2010年12月09日 14:55:02, rsc wrote: > drop ( ) and indentation Done. http://codereview.appspot.com/2049043/diff/49001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:262: event_names = map[uint32]string{ On 2010年12月09日 14:55:02, rsc wrote: > This is okay but kind of overkill since you never > actually look up by value, which is what map adds > over a plain slice. For an iterable-only list > of key,value pairs it's more efficient to use a slice: > > var eventBits = []struct{ > Value uint32 > Name string > }{ > {IN_ACCESS, "IN_ACCESS"}, > {IN_ATTRIB, "IN_ATTRIB"}, > ..., > } > Done.
>> not sorted right >> but also will cause other builds to fail. instead i would add after > > this list > >> ifeq ($(GOOS),linux) >> DIRS+=\ >> os/inotify\ > >> endif > > This doesn't seem to work. > all.bash fails with: > [...] > make[1]: Entering directory `/usr/local/google/go/src/pkg/os/inotify' > 6g -o _go_.6 inotify_linux.go > inotify_linux.go:27: can't find import: fmt > make[1]: *** [_go_.6] Error 1 > make[1]: Leaving directory `/usr/local/google/go/src/pkg/os/inotify' > > I'm pretty sure it's something trivial, but I don't know how to fix it. The script src/pkg/deps.bash is not generating a line for os/inotify. I don't know why. I'll look later if you don't beat me to it. Russ
The deps.bash problem is probably that the script is too clever about pulling out the value of DIRS. It would be fine to add a rule echo-dirs: @echo $(DIRS) to the src/pkg/Makefile and change deps.bash to use that. Russ
http://codereview.appspot.com/2049043/diff/58001/src/pkg/Makefile File src/pkg/Makefile (right): http://codereview.appspot.com/2049043/diff/58001/src/pkg/Makefile#newcode103 src/pkg/Makefile:103: os/inotify\ Use tab instead of 8 spaces; they're visible in raw patch set.
On 2010年12月09日 15:29:54, rsc wrote: > The deps.bash problem is probably that the > script is too clever about pulling out the > value of DIRS. It would be fine to add a rule > > echo-dirs: > @echo $(DIRS) > > to the src/pkg/Makefile and change deps.bash > to use that. > > Russ Done.
http://codereview.appspot.com/2049043/diff/55002/src/pkg/os/inotify/inotify_l... File src/pkg/os/inotify/inotify_linux.go (right): http://codereview.appspot.com/2049043/diff/55002/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:217: events = " UNKOWN" s/UNKOWN/UNKNOWN/
http://codereview.appspot.com/2049043/diff/55002/src/pkg/os/inotify/inotify_l... File src/pkg/os/inotify/inotify_linux.go (right): http://codereview.appspot.com/2049043/diff/55002/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:217: events = " UNKOWN" On 2010年12月09日 16:14:09, jacekm wrote: > s/UNKOWN/UNKNOWN/ Well spotted! Fixed.
http://codereview.appspot.com/2049043/diff/72001/src/pkg/os/inotify/inotify_l... File src/pkg/os/inotify/inotify_linux.go (right): http://codereview.appspot.com/2049043/diff/72001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:13: watcher.Watch("/tmp") Examples should not contain bugs so please don't skip error checking.
http://codereview.appspot.com/2049043/diff/55002/src/pkg/os/inotify/inotify_l... File src/pkg/os/inotify/inotify_linux.go (right): http://codereview.appspot.com/2049043/diff/55002/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:217: events = " UNKOWN" On 2010年12月09日 16:25:22, leczb wrote: > On 2010年12月09日 16:14:09, jacekm wrote: > > s/UNKOWN/UNKNOWN/ > > Well spotted! Fixed. I think this handling of unknown bits is not general enough. It is far more likely to have one or two unknown bits than for all of them to be unknown. I suggest: m := e.Mask for _, b := ... { if ... { m &^= b.Value events += "|" + b.Name } } if m != 0 { events += fmt.Sprintf("|%#x", m) } if len(events) > 0 { events = " == " + events[1:] } return fmt.Sprintf("%q: %#x%s", e.Name, e.Mask, events)
http://codereview.appspot.com/2049043/diff/72001/src/pkg/os/inotify/inotify_l... File src/pkg/os/inotify/inotify_linux.go (right): http://codereview.appspot.com/2049043/diff/72001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:69: done: make(chan bool, 1)} Multi-line literals usually end with a closing brace on separate line.
On 2010年12月09日 17:12:13, rsc1 wrote: > http://codereview.appspot.com/2049043/diff/55002/src/pkg/os/inotify/inotify_l... > File src/pkg/os/inotify/inotify_linux.go (right): > > http://codereview.appspot.com/2049043/diff/55002/src/pkg/os/inotify/inotify_l... > src/pkg/os/inotify/inotify_linux.go:217: events = " UNKOWN" > On 2010年12月09日 16:25:22, leczb wrote: > > On 2010年12月09日 16:14:09, jacekm wrote: > > > s/UNKOWN/UNKNOWN/ > > > > Well spotted! Fixed. > > I think this handling of unknown bits is not > general enough. It is far more likely to have > one or two unknown bits than for all of them > to be unknown. I suggest: > > m := e.Mask > for _, b := ... { > if ... { > m &^= b.Value > events += "|" + b.Name > } > } > if m != 0 { > events += fmt.Sprintf("|%#x", m) > } > if len(events) > 0 { > events = " == " + events[1:] > } > > return fmt.Sprintf("%q: %#x%s", e.Name, e.Mask, events) SGTM. Implemented.
LGTM Please fix nit below and I will submit http://codereview.appspot.com/2049043/diff/63002/src/pkg/Makefile File src/pkg/Makefile (right): http://codereview.appspot.com/2049043/diff/63002/src/pkg/Makefile#newcode144 src/pkg/Makefile:144: DIRS+=\ drop leading spaces before DIRS please (we don't typically indent the bodies of ifeq)
Thanks Jacek for the review. Balazs http://codereview.appspot.com/2049043/diff/72001/src/pkg/os/inotify/inotify_l... File src/pkg/os/inotify/inotify_linux.go (right): http://codereview.appspot.com/2049043/diff/72001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:13: watcher.Watch("/tmp") On 2010年12月09日 16:47:07, jacekm wrote: > Examples should not contain bugs so please don't skip error checking. Done. http://codereview.appspot.com/2049043/diff/72001/src/pkg/os/inotify/inotify_l... src/pkg/os/inotify/inotify_linux.go:69: done: make(chan bool, 1)} On 2010年12月09日 17:15:03, jacekm wrote: > Multi-line literals usually end with a closing brace on separate line. Done.
On 2010年12月09日 17:28:00, rsc1 wrote: > LGTM Great! Thanks for the thorough and patient review again. I just ran an "hg sync", thus the extra patch version. /leczb > > Please fix nit below and I will submit > > http://codereview.appspot.com/2049043/diff/63002/src/pkg/Makefile > File src/pkg/Makefile (right): > > http://codereview.appspot.com/2049043/diff/63002/src/pkg/Makefile#newcode144 > src/pkg/Makefile:144: DIRS+=\ > drop leading spaces before DIRS please > (we don't typically indent the bodies of ifeq)
http://codereview.appspot.com/2049043/diff/63002/src/pkg/Makefile File src/pkg/Makefile (right): http://codereview.appspot.com/2049043/diff/63002/src/pkg/Makefile#newcode144 src/pkg/Makefile:144: DIRS+=\ On 2010年12月09日 17:28:00, rsc1 wrote: > drop leading spaces before DIRS please > (we don't typically indent the bodies of ifeq) Very thorough. I like it! ;) Done.
*** Submitted as 354e81112204 *** os/inotify: new package This patch adds a new package: os/inotify, which provides a Go wrapper to the Linux inotify system. R=rsc, albert.strasheim, rog, jacek.masiulaniec CC=golang-dev http://codereview.appspot.com/2049043 Committer: Russ Cox <rsc@golang.org>