Hey. I'd looked at this back when Dmitry sent it to gtk-devel-list last summer, and I remember that it mostly looked good. I'll try to find a chance to re-review it soon.

In the meantime, you may want to look at bug 532815 and add support for the new flag suggested there. (Assuming this backend has the same problem the inotify one does with hardlinks.)

(In reply to comment #0)
> the backend adds an optionnal configuration file:
> /etc/gio-kqueue.conf (and ~/.config/gio-kqueue.conf)

Gross.  This is an OS-level deficiency; there should be some mechanism for the kernel to force-close the monitors on umount.

Second, in reality I seriously doubt any GNOME-FreeBSD system administrator is going to actually find this file and edit it.  Think about it.  They'll run unmount, hit the "In use" error, then...what?  Somehow know that because the process uses GLib that they should edit the file?

(In reply to comment #1)
> Hey. I'd looked at this back when Dmitry sent it to gtk-devel-list last summer,
> and I remember that it mostly looked good. I'll try to find a chance to
> re-review it soon.

Cool, thank you.

> In the meantime, you may want to look at bug 532815 and add support for the new
> flag suggested there. (Assuming this backend has the same problem the inotify
> one does with hardlinks.)

Yeah I looked at it but kqueue(2) does watch the files as well; so we don't have any issue with hardlinks.
I made sure by trying the test case in the aforementioned ticket.

(In reply to comment #2)
> (In reply to comment #0)
> > the backend adds an optionnal configuration file:
> > /etc/gio-kqueue.conf (and ~/.config/gio-kqueue.conf)
> 
> Gross.  This is an OS-level deficiency; there should be some mechanism for the
> kernel to force-close the monitors on umount.

There is none. I think only Mac OS has some kind of a hack to mark file descriptors in a way that they are not busy when used by kqueue(2) or something like that (I have 0 knowledge about Mac OS).

> Second, in reality I seriously doubt any GNOME-FreeBSD system administrator is

Well, so far I have been talking about OpenBSD and NetBSD. I have no idea what the FreeBSD people are after.

> going to actually find this file and edit it.  Think about it.  They'll run

I have _ever_ run into _any_ issue with using that way of doing things. Why? Because the OpenBSD package adds a configuration file that includes the most obvious temporary mount points to the ignore list.
The thing is the people that use GNOME on OpenBSD are either:
* dummy users that don't want to know anything about what is going behind the scenes; this case is covered because hotpluging a USB key will automatically mount on a mount point that is in the ignore list
* power users who know how things are working and are able to edit whatever needs be to comply to their requirements

> unmount, hit the "In use" error, then...what?  Somehow know that because the
> process uses GLib that they should edit the file?

Look, there is no need to be rude here. I actually proposed another way of doing this in my first message: use regular polling for removable devices.
In any case, right-click/umount works without any issue because our hotplug scripts mounts removable devices under a poll-only directory.

I am opened to suggestions to modify the patch according to what GLib people want, I just don't need the OS bashing.

I would have to say I agree with Antoine's statements here. The way the OpenBSD glib package is setup and installed is done in a way that satisfies the dummies and the power users.

I for one have been using this patch for months, without any issues, as have all the other users of GLib on OpenBSD.

(In reply to comment #2)
> Gross.  This is an OS-level deficiency; there should be some mechanism for the
> kernel to force-close the monitors on umount.

Shrug. You could make an equally plausible argument that linux is broken for *not* considering an inotify watch to be an "open file" for purposes of mountpoint busy-ness.


But what is it that's setting up watches on removable devices anyway? Most watches I know of are either in system directories or in $HOME...

(In reply to comment #6)
> But what is it that's setting up watches on removable devices anyway? Most
> watches I know of are either in system directories or in $HOME...

Indeed, except when it comes to the file manager. e.g. you open /path/to/your/usbkey/mount with Nautilus.

Anyway, if the "only do poll for removable devices" proposal sounds ok to you guys, then I'll fix the patch accordingly.

(In reply to comment #6)

> Shrug. You could make an equally plausible argument that linux is broken for
> *not* considering an inotify watch to be an "open file" for purposes of
> mountpoint busy-ness.

I really struggle to think of a use case where if the system administrator says "unmount", you would want it to be blocked by an application just monitoring the directory.

Yeah, and I struggle to think of a use case where you would say "unmount" and want it to be blocked because you have a shell open in a subdirectory of that mountpoint. But that's what happens.

At any rate, if the problem is "you can't unmount a filesystem from within nautilus when you're looking at that filesystem in nautilus", well, that's easy enough to fix within nautilus; it should just drop its watches before trying to unmount.

(In reply to comment #8)
> I really struggle to think of a use case where if the system administrator says
> "unmount", you would want it to be blocked by an application just monitoring
> the directory.

No you are right in this case, there is no case. But it's how the kqueue(2) interface works, it requires an fd...
I'm not saying this is clever, it's just the way it is.

(In reply to comment #4)
>
> Look, there is no need to be rude here.

Before we go back into the technical discussion, let me say that I believe blunt criticism is an important part of the peer review process that helps Free Software thrive.   Please don't take it as a personal attack or whatever.  I expect exactly the same from other people when *I* submit patches.

(In reply to comment #11)
> Before we go back into the technical discussion, let me say that I believe
> blunt criticism is an important part of the peer review process that helps Free
> Software thrive.   Please don't take it as a personal attack or whatever.  I
> expect exactly the same from other people when *I* submit patches.

Fair enough. I have nothing against criticism, on the contrary. It's just that I get my fair amount of regular bashing from both sides (GNOME saying OpenBSD is badly designed or old or whatever; and OpenBSD saying GNOME is bloated and blah blah blah)... so sometimes I just get tired of it because I'm just trying to get things done as I like both projects ;-)
Anyway... back to technical stuff.

(In reply to comment #9)
> Yeah, and I struggle to think of a use case where you would say "unmount" and
> want it to be blocked because you have a shell open in a subdirectory of that
> mountpoint. But that's what happens.

Sure, but that doesn't change the fact that the Linux behavior for inotify is better.

> At any rate, if the problem is "you can't unmount a filesystem from within
> nautilus when you're looking at that filesystem in nautilus", well, that's easy
> enough to fix within nautilus; it should just drop its watches before trying to
> unmount.

Also, there's no reason GFileMonitor couldn't be aware of GDrive too; they both live in Gio.  That would presumably help the Nautilus-on-removable-media case, but admittedly wouldn't fix say a non-GUI app just using GFileMonitor.

Created attachment 224541 [details] [review]
Add kqueue(3) support to GIO, v2

Hi.

Very sorry about the latency but I have been extremely busy lately...
Anyway, here's a new revision of the gio-kqueue patch with the following
changes to workaround the fact that one cannot unmount a kqueue
monitored directory without using `-f':
* if the path we monitor in under /mnt, then we fallback to polling
* if the mount is g_mount_can_unmount(), then we fallback to polling

I think it is an acceptable solution...
I'd really like to move forward with it so I'd appreciate the input.
Thanks.

Hi guys.

I would really like some review on this patch so that we can move on it.
I already have a small enhancement I'd like to add for Mac OS X specific kqueue behavior but it doesn't really make sense to add more things right now.
Ideally this would be nice if I could push it somewhat soon and keep enhancing it in-tree.
Now that a new major GLib release was released, I think it may be a good time...

Also it would mean easier testing from BSD users.

Guys, is there any interest at all in getting this patch in or should BSD platforms keep it as a vendor patch? I am trying here, but the lack of answer kind of demotivates me :-/

Sorry for not getting this reviewed faster; I'll try to drum up some reviewers...

Review of attachment 224541 [details] [review]:

In the big picture, this should be OK to merge.  Since I don't know kqueue, this is mostly style stuff.  It's fine to import non-GLib code as long as it's cleanly separate files with a clearly defined coding style, but mixing GNU with the whatever the other one is (strstroup) is ugly.

I'm pretty curious how well this is tested and works.  Did you do some basic checks creating/destroying monitors to be sure we're not leaking memory?

Anyways, let's put a bit of effort into some style and just get this merged; anything more can be iterated on.

::: gio/giomodule.c
@@ +920,3 @@
+#if defined(HAVE_KQUEUE)
+      _g_kqueue_directory_monitor_get_type ();
+      _g_kqueue_file_monitor_get_type ();

Should use g_type_ensure () like the others.

::: gio/kqueue/kqueue-helper.c
@@ +49,3 @@
+G_GNUC_INTERNAL G_LOCK_DEFINE (hash_lock);
+
+int kqueue_descriptor = -1;

Should be static; if it's not, make it so, and add an accessor function for any other compilation units that need it.

::: gio/kqueue/kqueue-thread.c
@@ +49,3 @@
+
+/* TODO: Probably it would be better to pass it as a thread param? */
+extern int kqueue_descriptor;

Add an accessor please.

@@ +216,3 @@
+  waiting.kq_size = 1;
+
+  for (;;) {

Here you're just inconsistent in the same function . The earlier condtional is GNU style, this one should have brace on same line too.

@@ +231,3 @@
+      {
+        KT_W ("kevent failed: %d", errno);
+        if (errno == EINTR)

If KT_W is a printf call or something, it's highly likely it stomped on errno.  You should to save errno right after the kevent() call to be safe.  Or likely better, just move the KT_W to after the EINTR handling.

::: gio/kqueue/kqueue-utils.c
@@ +196,3 @@
+  memset (&st, 0, sizeof (struct stat));
+
+  if (fstat (fd, &st) == -1) {

Brace on new line.

@@ +198,3 @@
+  if (fstat (fd, &st) == -1) {
+      KU_W ("fstat failed, assuming it is just a file");
+      return;

Here we've left *is_dir untouched.  For out values, it's better to always assign them so the caller doesn't have to guess.

> I'm pretty curious how well this is tested and works.  Did you do some basic
> checks creating/destroying monitors to be sure we're not leaking memory?

As I mentioned in my former post, this patch has been incorporated in the GLib package in OpenBSD since last January. And I have been running with it even before that.

> Anyways, let's put a bit of effort into some style and just get this merged;
> anything more can be iterated on.

Thanks Colin, I'll work on those.

> ::: gio/giomodule.c
> @@ +920,3 @@
> +#if defined(HAVE_KQUEUE)
> +      _g_kqueue_directory_monitor_get_type ();
> +      _g_kqueue_file_monitor_get_type ();
> 
> Should use g_type_ensure () like the others.
> 
> ::: gio/kqueue/kqueue-helper.c
> @@ +49,3 @@
> +G_GNUC_INTERNAL G_LOCK_DEFINE (hash_lock);
> +
> +int kqueue_descriptor = -1;
> 
> Should be static; if it's not, make it so, and add an accessor function for any
> other compilation units that need it.
> 
> ::: gio/kqueue/kqueue-thread.c
> @@ +49,3 @@
> +
> +/* TODO: Probably it would be better to pass it as a thread param? */
> +extern int kqueue_descriptor;
> 
> Add an accessor please.
> 
> @@ +216,3 @@
> +  waiting.kq_size = 1;
> +
> +  for (;;) {
> 
> Here you're just inconsistent in the same function . The earlier condtional is
> GNU style, this one should have brace on same line too.
> 
> @@ +231,3 @@
> +      {
> +        KT_W ("kevent failed: %d", errno);
> +        if (errno == EINTR)
> 
> If KT_W is a printf call or something, it's highly likely it stomped on errno. 
> You should to save errno right after the kevent() call to be safe.  Or likely
> better, just move the KT_W to after the EINTR handling.
> 
> ::: gio/kqueue/kqueue-utils.c
> @@ +196,3 @@
> +  memset (&st, 0, sizeof (struct stat));
> +
> +  if (fstat (fd, &st) == -1) {
> 
> Brace on new line.
> 
> @@ +198,3 @@
> +  if (fstat (fd, &st) == -1) {
> +      KU_W ("fstat failed, assuming it is just a file");
> +      return;
> 
> Here we've left *is_dir untouched.  For out values, it's better to always
> assign them so the caller doesn't have to guess.

> Anyways, let's put a bit of effort into some style and just get this merged;
> anything more can be iterated on.

Hi Colin.

Here's an updated patch (successfully tested).
I do have a question:

> @@ +231,3 @@
> +      {
> +        KT_W ("kevent failed: %d", errno);
> +        if (errno == EINTR)
> 
> If KT_W is a printf call or something, it's highly likely it stomped on errno. 
> You should to save errno right after the kevent() call to be safe.  Or likely
> better, just move the KT_W to after the EINTR handling.

This part I'm not sure I understand what you meant. I blindly moved the KT_W after EINTR in the new patch _but_ that does not make much sense to me so I probably misunderstood what you meant in the first place...

Thanks!

Created attachment 228694 [details] [review]
 Add kqueue(3) support to GIO, v3

(In reply to comment #20)
> > If KT_W is a printf call or something, it's highly likely it stomped on errno. 
> > You should to save errno right after the kevent() call to be safe.  Or likely
> > better, just move the KT_W to after the EINTR handling.
> 
> This part I'm not sure I understand what you meant. I blindly moved the KT_W
> after EINTR in the new patch _but_ that does not make much sense to me so I
> probably misunderstood what you meant in the first place...

Whenever you make a libc call, do something else involving a function call, and then use errno, you run the risk that the "something else" has altered errno, so it no longer contains the error from the libc call. For instance:

  again:
    if (open ("foo", O_WRONLY) < 0)
      {
        g_debug ("%s", g_strerror (errno));

        if (errno == EINTR)        /* line 6 */
          goto again;
      }

If you see EINTR at line 6, it might be because open() failed with EINTR, or it might be because something called by g_debug (or even g_strerror if you're really unlucky) failed with EINTR. You can't tell. Similarly, if errno *isn't* EINTR at line 6, the open() call might still have failed with EINTR - one of the other library calls might have changed errno afterwards.

One way round that is to be careful about ordering, so you only call functions after the last time errno is evaluated:

  again:
    if (open ("foo", O_WRONLY) < 0)
      {
        if (errno == EINTR)   /* no library calls here, can't break errno */
          goto again;

        /* by the time g_strerror is invoked, we have the value of errno
         * on the stack/in a register as a parameter, so this counts as
         * "after" */
        g_debug ("%s", g_strerror (errno));
      }

Another is to save the interesting value of errno before you do anything that might modify it:

  again:
    if (open ("foo", O_WRONLY) < 0)
      {
        int save_errno = errno;

        g_debug ("%s", g_strerror (save_errno));

        if (save_errno == EINTR)
          goto again;
      }

> > > If KT_W is a printf call or something, it's highly likely it stomped on errno. 
> > > You should to save errno right after the kevent() call to be safe.  Or likely
> > > better, just move the KT_W to after the EINTR handling.
> > 
> > This part I'm not sure I understand what you meant. I blindly moved the KT_W
> > after EINTR in the new patch _but_ that does not make much sense to me so I
> > probably misunderstood what you meant in the first place...

Ah that makes perfect sense, of course. Thanks ;-)
New patch. Hopefully every request have been answered/fixed now...

Created attachment 228981 [details] [review]
Add kqueue(3) support to GIO, v4

Review of attachment 228981 [details] [review]:

Ok.  There are other miscellaneous bits that I noticed now (e.g. _ku_path_concat() checks for OOM, but it should just use g_build_filename() since all of GLib aborts on  OOM).  But we could bikeshed endlessly about to what degree the code should be "core BSD libc" versus "GLib", so like I said earlier, let's just get it in, and further code cleanups can happen later.

Colin, it's in as 2054cca.
Thank you again for helping on getting this is.
I'll keep the bug opened until all bikesheding is gone ;-)

looks like no more bikeshedding coming