code review 5304046: exp/fsnotify : A suggestion for how to implement a ...

exp/fsnotify : A suggestion for how to implement a filesystem notification package that is cross-platform.

    This was made in response to comments on os/kevent package here: http://codereview.appspot.com/5248062/
    I figured it would be best to provide a working proto-type for a yes/no discussion.

    The same test is used for both Linux and BSD systems.

Update per Dave's comments to use error.

[howeyc, 2011-10-20 02:16:01]

Here's an attempt at a cross-platform fsnotify package. 

I know I jumped way ahead, this is to start a discussion as to which way to go.

Some notes on the implementation:

I noticed the Linux os/inotify package was sending events for files that are
created within the directory that is being watched. The BSD kevent/kqueue system
requires you to have file descriptors added to the kqueue in order to get
events. I have made the BSD fsnotify automatically add created files to the
watch list. It also manufactures "Create" events as kevent provides events
against the directory and it's up to user-space to find out files that are
created.

The os/inotify package has some kind of OS_IGNORED event that gets triggered for
reasons I am unaware of. I stopped the package from passing those on the Event
channel. This was because of the change to use IsDelete(), IsModify(),... it
made no sense to pass an event that did not satisfy at least one of these
functions.

Also:
I noticed the winfsnotify test and it may be better to move the test to that
kind of format if this is the way we decide to go for a unified package.

[dave_cheney.net, 2011-10-20 20:11:33]

I haven't had a chance to apply this CL and run the tests but the code looks
good. Let's see what the others have to say.

Is it possible to make the Watcher type shared across bsd and linux in the same
way syscall.ProcAttr embeds OS specific attributes>

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_bs...
File src/pkg/exp/fsnotify/fsnotify_bsd.go (right):

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_bs...
src/pkg/exp/fsnotify/fsnotify_bsd.go:37: type FileEvent struct {
// A FileEvent represents a single notification event.

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_bs...
src/pkg/exp/fsnotify/fsnotify_bsd.go:43: // IsCreate reports whether the
FileEvent was triggerd by a creation
Periods at the end of comments, ditto for other IsDelete, IsModify, IsAttribute,
IsRename, etc

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_bs...
src/pkg/exp/fsnotify/fsnotify_bsd.go:69: 
s/ and returns//

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_bs...
src/pkg/exp/fsnotify/fsnotify_bsd.go:91: // It sends a message to the reader
goroutine to quit and removes all watches
s/It/Close

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_bs...
src/pkg/exp/fsnotify/fsnotify_bsd.go:153: return os.NewError(fmt.Sprintf("can't
remove non-existent kevent watch for: %s", path))
You can use fmt.Errorf here.

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_bs...
src/pkg/exp/fsnotify/fsnotify_bsd.go:180: *twait =
syscall.NsecToTimespec(keventWaitTime)
Can't this just be var twait *syscall.Timespec =
syscall.NsecToTimespec(keventWaitTime) ? I'm not sure what I'm missing here.

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_bs...
src/pkg/exp/fsnotify/fsnotify_bsd.go:190: case done = <-w.done:
Why not move the done logic below to here ?

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_bs...
src/pkg/exp/fsnotify/fsnotify_bsd.go:191: default:
And the n < 0 or n = 0 logic here ?

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_bs...
src/pkg/exp/fsnotify/fsnotify_bsd.go:215: for len(events) > 0 {
for i, _ := range events, then use &events[i] below ?

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_bs...
src/pkg/exp/fsnotify/fsnotify_bsd.go:248: if fileInfo.IsRegular() == true {
s/== true//

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_bs...
src/pkg/exp/fsnotify/fsnotify_bsd.go:258: fileEvent := new(FileEvent)
Consider using the compact literal format here.

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_bs...
src/pkg/exp/fsnotify/fsnotify_bsd.go:268: // Flags (from <sys/event.h>)
Maybe submit another CL that adds these constants to syscall?

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_li...
File src/pkg/exp/fsnotify/fsnotify_linux.go (right):

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_li...
src/pkg/exp/fsnotify/fsnotify_linux.go:1: // Copyright 2010 The Go Authors. All
rights reserved.
2011, check all the new files submitted in this package

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_li...
src/pkg/exp/fsnotify/fsnotify_linux.go:36: 
// A FileEvent represents ...

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_li...
src/pkg/exp/fsnotify/fsnotify_linux.go:43: // IsCreate reports whether the
FileEvent was triggerd by a creation
Periods at the end of comments.

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_li...
src/pkg/exp/fsnotify/fsnotify_linux.go:65: type Watcher struct {
// A watcher (represents|encapuslates) ...

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_li...
src/pkg/exp/fsnotify/fsnotify_linux.go:95: // It sends a message to the reader
goroutine to quit and removes all watches
s/It/Close

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_li...
src/pkg/exp/fsnotify/fsnotify_linux.go:145: return
os.NewError(fmt.Sprintf("can't remove non-existent inotify watch for: %s",
path))
fmt.Errorf

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_li...
src/pkg/exp/fsnotify/fsnotify_linux.go:169: case done = <-w.done:
Can the done logic move here, and the error and eof checks to default:

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_li...
src/pkg/exp/fsnotify/fsnotify_linux.go:198: event := new(FileEvent)
&FileEvent{ } ?

http://codereview.appspot.com/5304046/diff/3/src/pkg/exp/fsnotify/fsnotify_li...
src/pkg/exp/fsnotify/fsnotify_linux.go:206: event.Name = w.paths[int(raw.Wd)]
Do you need a mutex to protect w.path from concurrent modification?

[howeyc, 2011-11-08 23:31:38]

I figured I'd bump this for individuals that were interested in the initial BSD
kevent/kqueue change.

[dave_cheney.net, 2011-11-09 08:45:22]

This patch doesn't apply cleanly anymore after the errors fix. Can you
please gofix your tree and hg upload it.

Cheers

Dave

On Wed, Nov 9, 2011 at 10:31 AM,  <howeyc@gmail.com> wrote:
> I figured I'd bump this for individuals that were interested in the
> initial BSD kevent/kqueue change.
>
> http://codereview.appspot.com/5304046/
>

[howeyc, 2011-11-09 14:30:17]

Hello golang-dev@googlegroups.com, dave@cheney.net (cc: bradfitz@golang.org,
golang-dev@googlegroups.com, hectorchu@gmail.com, rsc@golang.org),

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

[howeyc, 2011-11-09 14:31:57]

Hello golang-dev@googlegroups.com, dave@cheney.net (cc: bradfitz@golang.org,
golang-dev@googlegroups.com, hectorchu@gmail.com, rsc@golang.org),

Please take another look.

[dave_cheney.net, 2012-01-17 10:32:26]

Hello, are you keen to continue this CL ?

[robfig, 2012-01-26 02:53:58]

On 2012/01/17 10:32:26, dfc wrote:
> Hello, are you keen to continue this CL ?

I am eager to use this code in my own projects.  Would it be appropriate for me
to push it down the home stretch?

[howeyc, 2012-01-27 01:20:37]

On 2012/01/26 02:53:58, robfig wrote:
> On 2012/01/17 10:32:26, dfc wrote:
> > Hello, are you keen to continue this CL ?
> 
> I am eager to use this code in my own projects.  Would it be appropriate for
me
> to push it down the home stretch?

I'd be happy to collaborate in any way to get this in a state for others to use.

[dave_cheney.net, 2012-01-27 01:31:32]

Could you please remerge the CL and do a hg mail, currently it does not apply
cleanly.

codereview issue 5304046 is out of date: patch and recent changes conflict
(afa4ccab6746->24e6b9d71966)

[howeyc, 2012-01-27 15:05:11]

I was unable to change this patch:
"cannot change non-local CL"

New CL: http://codereview.appspot.com/5585043