Created attachment 135015 [details] [review]
Patch fixing the problem

I don't like how you are using __digital__ and __NetBSD__ as checks. 
It would be much better if it used generic autoconf checks like HAVE_STATFS_F_FSTYPENAME, GETMNTINFO_RETURNS_STATVFS, etc.

The code in this file is already one of the most horrid ifdef spagethi things in existance (which is required unfortunately), so lets not make it worse.

Created attachment 135307 [details] [review]
Patch using different ifdefs

Attached version gets rid of __digital__ and __NetBSD__ ifdefs. I didn't find a GETMNTINFO_RETURNS_STATVFS symbol, so I made the code depend on statvfs existence.

FYI: Previous patch was compile-tested on NetBSD.

I don't mean that these defined exists
We need to add the corresponding checks to configure.in to detect them, using e.g. AC_CHECK_MEMBERS for struct members.

The trivial configury bits are probably sorted out by Bug 617949, but what of the polling part and the extra mount points?

Just as an FYI this is also in the OpenBSD ports tree and would be required for DragonFly too.

Created attachment 259177 [details] [review]
mounted file system polling - without OS defines

Reworking of patch making use of Bug 617949 feature tests as per Comment 6.

Review of attachment 259177 [details] [review]:

Can you change the commit message to be something like this:

----
GUnixMounts: Fall back to polling on systems without mtab

This is necessary for many of the BSD family at least.

https://bugzilla.gnome.org/show_bug.cgi?id=583330
----

Basically we've added a prefix saying which code is being changed, and we have a very brief mention of what systems are affected in the message.  The details can be in this bug.

::: gio/gunixmounts.c
@@ +1413,3 @@
+      int i;
+
+      for (i = 0; i < g_list_length (current_mounts); i++)

A little ugly to have N^2 iteration here; hopefully the system has few enough mounts that we won't care.  It's not too hard to iterate two lists at once though...

@@ +1497,3 @@
+      monitor->mount_poller_mounts = _g_get_unix_mounts ();
+      mount_poller_time = (guint64)time (NULL);
+      monitor->mount_poller_source = g_timeout_add_seconds (3, (GSourceFunc)mount_change_poller, monitor);

This will create the source on the default context, whereas the normal GFileMonitor (as used by GUnixMountMonitor) will use the *thread default* context.  This distinction matters for applications which use GLib APIs on threads other than the main thread, and that's a use case that we want to support.

To fix this, do:

GSource *src = g_timeout_source_new_seconds (3);
g_source_set_callback (src, (GSourceFunc)mount_change_poller, monitor, NULL);
g_source_attach (src, g_main_context_get_thread_default ());
g_source_unref (src);

Created attachment 273879 [details] [review]
Updated patch to poll gunixmounts

As per Colin's detailed review:
- updated the commit message
- both lists are actually just iterated over together in one pass. The pass isn't stopped when a change is found so that the elements of one of the list are freed.
- I had no idea about the thread context issue - fixed as per suggestion!

Created attachment 283824 [details] [review]
GUnixMounts: Fall back to polling on systems without mtab

Working version of previous...

PASS: mainloop 20 /mainloop/unix-file-poll

Review of attachment 283824 [details] [review]:

looks good to me

Attachment 283824 [details] pushed as 4f775b7 - GUnixMounts: Fall back to polling on systems without mtab

Review of attachment 283824 [details] [review]:

this does not look really good to me, in fairness.

::: gio/gunixmounts.c
@@ +1294,3 @@
+  if (monitor->mount_poller_mounts != NULL)
+    {
+      g_list_foreach (monitor->mount_poller_mounts, (GFunc)g_unix_mount_free, NULL);

foreach()+free() can be replaced by g_list_free_full().

@@ +1399,3 @@
+  current_mounts = _g_get_unix_mounts ();
+
+  if (g_list_length (current_mounts) != g_list_length (mount_monitor->mount_poller_mounts))

these should be placed on temporary variables, since the current_mounts list length is checked twice.

@@ +1408,3 @@
+      int i;
+
+      for (i = 0; i < g_list_length (current_mounts); i++)

ugh, this is *wildly* inefficient: you're iterating over the mounts list for each loop to get the length of the list...

@@ +1413,3 @@
+          GUnixMountEntry *m2;
+
+          m1 = (GUnixMountEntry *)g_list_nth_data (current_mounts, i);

...and then you're iterating over the list again to extract the data at the given index.

this is a linked list, not an array.

@@ +1427,3 @@
+  if (has_changed)
+    {
+      mount_poller_time = (guint64)time (NULL);

why are you using wallclock time, instead of a monotonic clock?

@@ +1492,3 @@
+      monitor->proc_mounts_watch_source = g_timeout_source_new_seconds (3);
+      monitor->mount_poller_mounts = _g_get_unix_mounts ();
+      mount_poller_time = (guint64)time (NULL);

same as above...

True, those are all things that could be better. 
Patrick, could you do a new patch fixing those up?

Created attachment 283871 [details] [review]
wip

Firstly thank you for the review! Colin had mentioned a double loop - I hadn't realised that getting the length of a list was implemented by iterating over it.

I think that this wip patch does what is wanted, but I get coredumps, and what looks like garbage in m2:

(gdb) frame 7

+ Trace 233973

$1 = {mount_path = 0x7f7fdd7f2dc0 "\370\342\266\334\177\177", 
  device_path = 0x7f7fee7c8e20 "\200\061\362\335\177\177", 
  filesystem_type = 0x0, is_read_only = 0, is_system_internal = 0}

I'm having a hard time spotting the difference...

The committed version seems stable. (I had to put that back to get a working webrowser to be able to upload this wip.diff)

As to monotonic time, it seems that mount_poller_time is being used as a replacement for st_mtime, which is a time in seconds since 1 Jan 1970, so time() seems a better fit. Comments?

The time returned from get_mounts_timestamp() should only be compared with g_unix_mounts_changed_since(), so using the monotonic clock should be ok.

Review of attachment 283871 [details] [review]:

::: gio/gunixmounts.c
@@ +1396,3 @@
   current_mounts = _g_get_unix_mounts ();
 
+  m1 = (GUnixMountEntry *)g_list_first (current_mounts);

g_list_first() returns a GList*, not the data inside the list. you'd notice that if you didn't cast. in general, you should not be casting from void*/gpointer to and from your data structures.

also, you're iterating on the list of current mounts but freeing elements on the old mounts — which is fine, but then you free the old mounts list. if the two list do not have the same number of elements, there's a potential leak.

you can replace this whole block using:

  GList *new_iter = current_mounts;
  GList *old_iter = mount_monitor->mount_poller_mounts;

  /* check if the lists are different */
  while (new_iter != NULL)
    {
      GUnixMountEntry *m1 = new_iter->data;
      GUnixMountEntry *m2 = old_iter->data;

      if (g_unix_mount_compare (m1, m2) != 0)
        {
          /* no point in continuing */
          has_changed = TRUE;
          break;
        }

      new_iter = new_iter->next;
      old_iter = old_iter->next;
    }

  /* free the old list and assign the new one */
  g_list_free_full (mount_monitor->mount_poller_mounts, (GDestroyNotify) g_unix_mount_free);
  mount_monitor->mount_poller_mounts = current_mounts;

Created attachment 283880 [details] [review]
example patch

You want something like this (untested)

Created attachment 283883 [details] [review]
Better algorithm

This is what I have been testing - I think it is similar enough to both of yours? The only real difference is using free_full would be more readable, but cause another loop, so we free while checking for changes in one go.

(In reply to comment #20)

> This is what I have been testing - I think it is similar enough to both of
> yours? The only real difference is using free_full would be more readable, but
> cause another loop, so we free while checking for changes in one go.

no, that's wrong as I explained it my review.

if the lists have different lengths, you're just going to free the list and leak some element.

iterating again to free the contents of the list is inconsequential.

Unlike in your loop, I am not stopping early - there should not be any leak!

(In reply to comment #22)
> Unlike in your loop, I am not stopping early - there should not be any leak!

you're iterating one list (current_mounts) until it's exhausted, but you're freeing the contents of the second list (mount_poller_mounts). if the first list is shorter than the second, the rest of the second list is left dangling; if the second list is shorter than the first, then you're going to check invalid data — which is something that I didn't handle in my code, so you'll have to fix it that as well. the only case where your code works is if the list of mounts is exactly the same length.

Selectively cut and pasting:

  m2 = g_list_first (mount_monitor->mount_poller_mounts);
  while (m2 != NULL)  
    {
      g_unix_mount_free (m2->data);
      m2 = g_list_next (m2);
    }
  g_list_free (mount_monitor->mount_poller_mounts);

(all I did was skip lines)

I believe that that is identical to

g_list_free_full (mount_monitor->mount_poller_mounts, (GDestroyNotify)g_unix_mount_free);)

it's just that we get to check for changes during the loop.

Patrick: 

Yes, but your code also does:
      m1 = g_list_next (m1);

which will keep stepping on the m1 list without checking if it ended.
If you protect against this by doing:
  while (m2 != NULL && m1 != NULL)  

Then you instead will stop early if m1 is null before m2, not freeing everything.

And anyway, its not like you save any work. g_list_free() iterates over the whole list anyway, g_list_free_full is just cleaner and easier to understand.

(In reply to comment #25)
> Patrick: 
> 
> Yes, but your code also does:
>       m1 = g_list_next (m1);
> 
> which will keep stepping on the m1 list without checking if it ended.

But we don't care about m1, do we. I see that I won't set has_changed in that case, so am about to attach a patch which does this.

> If you protect against this by doing:
>   while (m2 != NULL && m1 != NULL)  

But we don't want to protect against this. We are just looking at m1 (the current list) to see if it is different to m2 (the old list)

> Then you instead will stop early if m1 is null before m2, not freeing
> everything.

Precisely why I haven't been doing that.

> And anyway, its not like you save any work. g_list_free() iterates over the
> whole list anyway, g_list_free_full is just cleaner and easier to understand.

Oh not again - another iteration. OK.

Created attachment 283954 [details] [review]
Better algorithm (and deals with unmounting)

Created attachment 284317 [details] [review]
another version of previous patch - possibly slightly more readable

Review of attachment 284317 [details] [review]:

::: gio/gunixmounts.c
@@ +1396,3 @@
 
+  new_it = g_list_first (current_mounts);
+  old_it = g_list_first (mount_monitor->mount_poller_mounts);

You don't really need to call g_list_first() here. We're already guaranteed that the things we pass in are the list heads.
Just do new_it = current_mounts; etc.

@@ +1397,3 @@
+  new_it = g_list_first (current_mounts);
+  old_it = g_list_first (mount_monitor->mount_poller_mounts);
+  while (old_it != NULL)

This is still wrong. If the new list has an item added to the end we will never get to it as we only iterate the old list. Thus we will not notice that something changed.

In general the way you do list comparison like this is: iterated until either old or new list ends (while new != NULL && old != NULL), and bail early if the items were non-empty. Then at the end (if you didn't bail early), unless *both* new and old are NULL, then something changed.

Review of attachment 284317 [details] [review]:

::: gio/gunixmounts.c
@@ +1397,3 @@
+  new_it = g_list_first (current_mounts);
+  old_it = g_list_first (mount_monitor->mount_poller_mounts);
+  while (old_it != NULL)

Eh, bail early if the items were non-identical i mean...

Created attachment 284599 [details] [review]
iteration i+1

Review of attachment 284599 [details] [review]:

looks good