watchers leak inotify file descriptor when process fork/execs a child

[msolo-dropbox]

inotify.go needs a tiny patch in NewWatcher.

  // Set IN_CLOEXEC by default. The majority of use cases should not
  // pass this descriptor down.
  fd, errno := syscall.InotifyInit1(syscall.IN_CLOEXEC)

[nathany]

IN_CLOEXEC
Set the close-on-exec (FD_CLOEXEC) flag on the new file descriptor. See the description of the O_CLOEXEC flag...

O_CLOEXEC (Since Linux 2.6.23)
Enable the close-on-exec flag for the new file descriptor. Specifying this flag permits a program to avoid additional fcntl(2) F_SETFD operations to set the FD_CLOEXEC flag. Additionally, use of this flag is essential in some multithreaded programs since using a separate fcntl(2) F_SETFD operation to set the FD_CLOEXEC flag does not suffice to avoid race conditions where one thread opens a file descriptor at the same time as another thread does a fork(2) plus execve(2).

fsnotify currently uses InotifyInit (not InotifyInit1).

inotify_init() first appeared in Linux 2.6.13; library support was added to glibc in version 2.4.
inotify_init1() was added in Linux 2.6.27; library support was added to glibc in version 2.9.

This is fine (IMO).

What about support on ARM and PPC64? /cc @suihkulokki @TheTincho @clnperez @laboger @tophj-ibm?

[NightTsarina]

@nathany You mean support for inotify_init1? I can test building this on those arches if it is just that.

[nathany]

@TheTincho That would be very helpful. Thanks. inotify_init1 and the flags it takes. The code change is in #178.

I wonder if we should be using IN_NONBLOCK as well? (#5) https://github.com/fsnotify/fsnotify/blob/master/inotify_poller.go#L48

[nathany]

Is there any reason why someone would want the child process to have the inotify file descriptor carried over?

Presumably a new one would be created in the child process instead if needed.

[pattyshack]

If you're share the inotify fd between between processes, the event
notifications may end up in either process. In general, this behavior does
not seem particularly useful.

kqueue behaves the same way (i.e., child do not inherit kqueue) for the
same reason.

On Wed, Oct 5, 2016 at 2:43 PM, Nathan Youngman notifications@github.com
wrote:

Is there any reason why someone would want the child process to have the
inotify file descriptor carried over?

Presumably a new one would be created in the child process instead if
needed.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#155 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGflbD7qr18W4RbK8ERw-k69CL1igOaGks5qxBnzgaJpZM4JGirW
.

[NightTsarina]

I have just tried PPC64 and ARMel, and it compiles and passes the tests correctly with this patch applied. Do you need me to test somewhere else too?