GNOME Bugzilla – Bug 660511
Use /proc/mounts for monitoring mounts, not /etc/mtab
Last modified: 2011-09-29 19:51:37 UTC
On recent Linux distros /etc/mtab is just a symlink to /proc/mounts and GFileMonitor does not work there because of how the kernel conveys that the file changes. I will attach a patch for this.
Created attachment 197801 [details] [review] Patch Tested with 'gvfs-mount -oi' on F15
Committed on master: http://git.gnome.org/browse/glib/commit/?id=55065461bfc5ace53e736dfa8181909e918c161b
Matthias, Ryan: we probably want this on the glib-2-30 branch but I'm deferring to you to cherry pick it. From what I can tell, this has been broken for all of F15...
Review of attachment 197801 [details] [review]: ::: gio/gunixmounts.c @@ +1352,3 @@ +{ + GUnixMountMonitor *mount_monitor = G_UNIX_MOUNT_MONITOR (user_data); + if (cond & ~G_IO_ERR) This makes more sense to me as: if (!(cond & G_IO_ERR)) personally. @@ +1353,3 @@ + GUnixMountMonitor *mount_monitor = G_UNIX_MOUNT_MONITOR (user_data); + if (cond & ~G_IO_ERR) + goto out; Why the goto? Just seems weird in a 4 line function, you could just as easily put the g_signal_emit() under the if(). @@ +1378,3 @@ + + mtab_path = get_mtab_monitor_file (); + /* /proc/mounts monitoring is special - can't just use GFileMonitor */ You could say "man proc" here. @@ +1381,3 @@ + if (g_strcmp0 (mtab_path, "/proc/mounts") == 0) + { + monitor->proc_mounts_channel = g_io_channel_new_file ("/proc/mounts", "r", NULL); Do we need to keep around the GIOChannel? I don't see it being used after you've attached the source (which holds a reference itself). Eventually we'll get that g_source_new_pollfd() API and won't have to indirect through GIOChannel... @@ +1382,3 @@ + { + monitor->proc_mounts_channel = g_io_channel_new_file ("/proc/mounts", "r", NULL); + monitor->proc_mounts_watch_source = g_io_create_watch (monitor->proc_mounts_channel, G_IO_ERR); So if /proc isn't mounted here we segfault or g_return_if_fail spew (since monitor->proc_mounts_channel) will be NULL. Possibly at least guard this with if (!monitor->proc_mounts_channel) g_warning ("Failed to open /proc/mounts") ?
(In reply to comment #2) > Committed on master: > > > http://git.gnome.org/browse/glib/commit/?id=55065461bfc5ace53e736dfa8181909e918c161b Hmm...did you get review from anyone first?
(In reply to comment #4) > Review of attachment 197801 [details] [review]: > > ::: gio/gunixmounts.c > @@ +1352,3 @@ > +{ > + GUnixMountMonitor *mount_monitor = G_UNIX_MOUNT_MONITOR (user_data); > + if (cond & ~G_IO_ERR) > > This makes more sense to me as: if (!(cond & G_IO_ERR)) personally. > > @@ +1353,3 @@ > + GUnixMountMonitor *mount_monitor = G_UNIX_MOUNT_MONITOR (user_data); > + if (cond & ~G_IO_ERR) > + goto out; > > Why the goto? Just seems weird in a 4 line function, you could just as easily > put the g_signal_emit() under the if(). I prefer it the way I wrote it. > > @@ +1378,3 @@ > + > + mtab_path = get_mtab_monitor_file (); > + /* /proc/mounts monitoring is special - can't just use GFileMonitor */ > > You could say "man proc" here. That's pretty useless since that man page does not tell you why it is the way it is. > > @@ +1381,3 @@ > + if (g_strcmp0 (mtab_path, "/proc/mounts") == 0) > + { > + monitor->proc_mounts_channel = g_io_channel_new_file > ("/proc/mounts", "r", NULL); > > Do we need to keep around the GIOChannel? I don't see it being used after > you've attached the source (which holds a reference itself). Not really. Feel free to remove it if you want. > > Eventually we'll get that g_source_new_pollfd() API and won't have to indirect > through GIOChannel... > > @@ +1382,3 @@ > + { > + monitor->proc_mounts_channel = g_io_channel_new_file > ("/proc/mounts", "r", NULL); > + monitor->proc_mounts_watch_source = g_io_create_watch > (monitor->proc_mounts_channel, G_IO_ERR); > > So if /proc isn't mounted here we segfault or g_return_if_fail spew (since > monitor->proc_mounts_channel) will be NULL. > > Possibly at least guard this with if (!monitor->proc_mounts_channel) g_warning > ("Failed to open /proc/mounts") ? I don't think the rest of glib really works if /proc is not mounted. But, sure, feel free to do that.
(In reply to comment #5) > (In reply to comment #2) > > Committed on master: > > > > > > http://git.gnome.org/browse/glib/commit/?id=55065461bfc5ace53e736dfa8181909e918c161b > > Hmm...did you get review from anyone first? <davidz> mclasen: I'm committing this to master <davidz> from what I can tell it's been broken in F15 <davidz> all along <mclasen> davidz: looks good to me
Actually, the proc man page does have an reference to this. Let me prepare a patch.
OK, committed the cleanups that Colin requested: http://git.gnome.org/browse/glib/commit/?id=934e0a7470fbcfccfd7c4d3e589d59e8778fa973 Thanks for the review.