code review 2049043: Add Linux inotify support

os/inotify: new package

This patch adds a new package: os/inotify, which
provides a Go wrapper to the Linux inotify system.

[leczb, 2010-09-23 11:52:54]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc1, 2010-09-24 19:39:07]

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.

[leczb, 2010-09-27 12:32:14]

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

[albert.strasheim, 2010-09-29 15:04:40]

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

[leczb, 2010-09-29 15:26:30]

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

[albert.strasheim, 2010-09-30 14:52:37]

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

[leczb, 2010-09-30 14:54:58]

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

[leczb, 2010-09-30 15:57:43]

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

[albert.strasheim, 2010-10-01 05:42:18]

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

[leczb, 2010-10-01 16:04:07]

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/
>

[albert.strasheim, 2010-10-05 14:35:39]

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.

[leczb, 2010-10-05 14:41:10]

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.

[leczb, 2010-11-17 11:47:53]

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.

[rsc, 2010-12-07 19:19:45]

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?

[leczb, 2010-12-09 14:17:57]

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.

[rsc, 2010-12-09 14:55:00]

> 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

[rsc, 2010-12-09 14:55:02]

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"},
    ...,
}

[rog, 2010-12-09 15:10:21]

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 {

[leczb, 2010-12-09 15:26:04]

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.

[rsc, 2010-12-09 15:29:06]

>> 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

[rsc, 2010-12-09 15:29:54]

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

[jacekm, 2010-12-09 15:33:21]

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.

[leczb, 2010-12-09 15:41:50]

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.

[jacekm, 2010-12-09 16:14:09]

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/

[leczb, 2010-12-09 16:25:22]

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.

[jacekm, 2010-12-09 16:47:07]

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.

[rsc1, 2010-12-09 17:12:13]

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)

[jacekm, 2010-12-09 17:15:03]

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.

[leczb, 2010-12-09 17:22:04]

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.

[rsc1, 2010-12-09 17:28:00]

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)

[leczb, 2010-12-09 17:28:12]

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.

[leczb, 2010-12-09 17:37:16]

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)

[leczb, 2010-12-09 17:37:26]

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.

[rsc, 2010-12-09 18:11:41]

*** 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>