code review 5418045: inotify: Fix readEntry hanging and Add/RemoveWatch fail...

inotify: Fix readEntry hanging and Add/RemoveWatch failing when watched directories deleted and re-created.

If a watched directory is deleted and recreated, and watched again, many of the events sent back are hosed.
- The event.Name is incomplete, missing the path of the watched directory (you get /a.txt or "")
- RemoveWatch thereafter fails with message: invalid argument
- Close() doesn't really close, and readEvents hangs forever on syscall.Read(...)

Note that the readEvents() hanging was always there, irrespective of the steps to produce issues with Add/RemoveWatch.

Fix involves:
- use Select to ensure there's data to read, else syscall.Read(...) will keep on waiting, even though intent to close was sent.
- Let Close() wait till completely closed (use sync channel with send/acknowledge receipt).
- AddWatch should always modify watch, in case watch description from OS changes (e.g. due to move, delete/re-add, etc)

Also, added a test for this: TestInotifyDeleteReAdd

Fixes issue 2483.

[ugorji, 2011-11-18 23:30:41]

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

I'd like you to review this change to
https://code.google.com/p/go/

[ugorji, 2011-11-19 02:28:43]

http://codereview.appspot.com/5418045/diff/2001/src/pkg/exp/inotify/inotify_l...
File src/pkg/exp/inotify/inotify_linux.go (right):

http://codereview.appspot.com/5418045/diff/2001/src/pkg/exp/inotify/inotify_l...
src/pkg/exp/inotify/inotify_linux.go:155: fdset.Bits[w.fd/4] |= 1 <<
uint(w.fd%4) //do fdset
Please look closely at the fd_set logic here. 
On further research, I'm thinking it should be:

	//create fdSet, taking into consideration that 
	//64-bit OS uses Bits: [16]int64, 32-bit OS==>[32]int32
	fdset := new(syscall.FdSet)
	fdiv := 32 * 32 / len(fdset.Bits)
	fdset.Bits[w.fd/fdiv] |= 1 << uint(w.fd%fdiv)

[ugorji, 2011-11-21 23:25:03]

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

Please take another look.

[ugorji, 2011-11-21 23:25:22]

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

Please take another look.

[leczb, 2011-12-05 15:15:36]

On 2011/11/21 23:25:22, ugorji wrote:
> Hello mailto:golang-dev@googlegroups.com (cc:
mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Hi,

Thanks for reporting and fixing this bug!
Please see my comments inline.

Note, that I don't have approval rights. My comments are just FYI.

Regards,
Balázs

[bradfitz, 2011-12-05 16:26:29]

http://codereview.appspot.com/5418045/diff/5002/src/pkg/exp/inotify/inotify_l...
File src/pkg/exp/inotify/inotify_linux.go (right):

http://codereview.appspot.com/5418045/diff/5002/src/pkg/exp/inotify/inotify_l...
src/pkg/exp/inotify/inotify_linux.go:76: done:    make(chan bool), //remove
buffer. make sync.
this comment only makes sense in the context of this diff.  it's confusing to
anybody reading this code in the future.  Also, space after //.

If anything, I'd make the comment like:
 
// needs to be unbuffered

http://codereview.appspot.com/5418045/diff/5002/src/pkg/exp/inotify/inotify_l...
src/pkg/exp/inotify/inotify_linux.go:117: //If a path being watched was deleted
and re-added,
Spaces after //

http://codereview.appspot.com/5418045/diff/5002/src/pkg/exp/inotify/inotify_l...
src/pkg/exp/inotify/inotify_linux.go:131: // If the path is not in the watched
file set, return a NoWatchExistError.
// If the path is not already being watched, the returned error
// is of type NoWatchExistError.

But I would actually remove this comment and NoWatchExistError.  It only exists
for your test, so just make it lowercase, or have your test match on the Error()
string here instead of the type of the error interface.  But it serves little
value having it be exported for other callers.

[ugorji, 2011-12-05 22:48:32]

Hello golang-dev@googlegroups.com, leczb@google.com, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[ugorji, 2011-12-05 22:54:51]

http://codereview.appspot.com/5418045/diff/5002/src/pkg/exp/inotify/inotify_l...
File src/pkg/exp/inotify/inotify_linux.go (right):

http://codereview.appspot.com/5418045/diff/5002/src/pkg/exp/inotify/inotify_l...
src/pkg/exp/inotify/inotify_linux.go:76: done:    make(chan bool), //remove
buffer. make sync.
On 2011/12/05 16:26:29, bradfitz wrote:
> this comment only makes sense in the context of this diff.  it's confusing to
> anybody reading this code in the future.  Also, space after //.
> 
> If anything, I'd make the comment like:
>  
> // needs to be unbuffered

Done

http://codereview.appspot.com/5418045/diff/5002/src/pkg/exp/inotify/inotify_l...
src/pkg/exp/inotify/inotify_linux.go:117: //If a path being watched was deleted
and re-added,
On 2011/12/05 16:26:29, bradfitz wrote:
> Spaces after //

Done.

http://codereview.appspot.com/5418045/diff/5002/src/pkg/exp/inotify/inotify_l...
src/pkg/exp/inotify/inotify_linux.go:131: // If the path is not in the watched
file set, return a NoWatchExistError.
On 2011/12/05 16:26:29, bradfitz wrote:
> // If the path is not already being watched, the returned error
> // is of type NoWatchExistError.
> 
> But I would actually remove this comment and NoWatchExistError.  It only
exists
> for your test, so just make it lowercase, or have your test match on the
Error()
> string here instead of the type of the error interface.  But it serves little
> value having it be exported for other callers.

Done. Removed the comment and made error type unexported i.e. noWatchExistError.

[bradfitz, 2012-01-17 21:41:11]

What's the status of this?


On Mon, Nov 21, 2011 at 3:25 PM, <ugorji@gmail.com> wrote:

> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**5418045/<http://codereview.appspot.com/5418045/>
>

[rsc, 2012-01-17 22:14:23]

I think the status is that inotify is in exp so we've been
leaving this on the back burner.  I do think we should get
back to this at some point.

[ugorji, 2013-03-20 16:12:14]

*** Abandoned ***