GNOME Bugzilla – Bug 314854
gnome-menus should have inotify support
Last modified: 2006-11-02 04:06:58 UTC
The attached patch implements this. I haven't actually run it yet, it's just a direct port. Their are two things left open: should the callbacks be using monitor_uri or info_uri? and gnome-vfs-monitor doesn't have a non existant file watch type.
Created attachment 51565 [details] [review] gnome-menu-gnome-vfs.patch
I don't think the lack of a non existant file watch type is going to matter at all. The only thing left to clear up is the uri usage.
I only changed gnome-menus to use FAM directly a few months ago: 2005-04-22 Mark McLoughlin <mark@skynet.ie> Use FAM directly instead of gnome-vfs and ensure that we only ever add a single FAM monitor any given path. Should fix bug #160194. Oh, also use the FAMNoExists() extension from gamin if available - should cut down on a whole heap of FAM traffic. I don't think its sane to pull in gnome-vfs only for file monitoring Happy to accept a patch to make it use inotify, though
In fact, having file monitoring support in glib might not be insane
(Well, apart from the fact that making it portable wouldn't be sane)
Created attachment 51569 [details] [review] gnome-menu-gnome-vfs-take-2.patch
New patch uses info_uri, and translates between uri and paths where appropriate.
Nautilus and the panel both use gnome-vfs, so you wouldn't be pulling anything into memory that isn't already there. As for porting gnome-menus to inotify, I have already ported gnome-vfs to use inotify instead of fam #314281. Porting specific applications is a wasteful path to go down.
Created attachment 51578 [details] [review] gnome-menu-gnome-vfs-take-3.patch call gnome_vfs_init
Created attachment 51580 [details] [review] gnome-menu-gnome-vfs-take-4.patch Don't call gnome_vfs_init (). Don't add a monitor to the list if gnome_vfs_monitor_add fails.
Created attachment 51587 [details] [review] gnome-menu-gnome-vfs-take-5.patch Fixed a couple bugs. Both gmenu-simple-editor, and gnome-menu-spec-test work with this patch.
Created attachment 51618 [details] [review] gnome-menus-gnome-vfs-take-6.patch Based on reversed diffs from gnome-vfs to FAM.
Mark, in gnome 2.14 gnome-vfs is going to use inotify instead of FAM. This is going to be the only left over requiring FAM on Linux. Could you reconsider your position?
John, you don't seem to be using pkgconfig to determine the cflags required for gnome-vfs. This makes compilation fail on my system.
Disregard this note. My rpm script didn't run autoconf, of course.
I don't think it's sane for an application to have to link to gnome-vfs purely for file monitoring. Moving gnome-menus from monitoring using gnome-vfs to FAM was a pretty trivial task. I would expect it to be even more trivial to add inotify support alongside the FAM support.
Mark, Adding proper inotify support to gnome-menus is not trivial and more importantly a duplication of effort and code in gnome-vfs. Anyways, you keep insisting on not wanting to bring in a dependency to gnome-vfs but you have failed to give any reasons other than your opinion. I can only assume you feel like this would bloat gnome-menus somehow. But I don't think that is a valid concern. It's a safe conclusion that gnome-menus is only being used in a gnome environment. As proof I ran a reverse dependency check in debian unstable, and the only applications that use gnome-menus are: gnome-panel and gnome-control-center both of which also depend on gnome-vfs. Even if there is a counter example, gnome-panel and gnome-control-center are going to account for 99% of gnome-menu's usage currently. So your users are already depending on gnome-vfs anyways, and the environment gnome-menus is being used it will be using gnome-vfs as well. So I can't see how depending on gnome-vfs is going to add to bloat. To make things even worse in gnome 2.14 FAM will no longer be used by default on Linux. So you are going to force every desktop machine to have gamin loaded just for watching the handful of paths that gnome-menus watches. So in gnome 2.14 your effort to reduce bloat is going to do just the opposite. Finally, Ubuntu and Suse ship with my attached patch for the reasons I have been stressing. Could you please reconsider applying this patch?
Mark, please don't make gnome-menus depend on gnome-vfs.
Just committed this to HEAD: --- 2006-01-29 Mark McLoughlin <mark@skynet.ie> Begin adding inotify support - bug #314854 * libmenu/menu-monitor-backend.h: define a generic backend interface for file monitor implementations * libmenu/menu-monitor.c: split out all the FAM stuff and use the generic backend interface * libmenu/menu-monitor-fam.c: both the FAM and "null" implementations * libmenu/Makefile.am: build menu-monitor-fam.c * libmenu/menu-monitor-inotify.c, libmenu/inotify-syscalls.h: libmenu/inotify.h: first cut at the inotify implementation; not finished yet and not built either. --- menu-monitor-inotify.c needs more work - see the FIXMEs. To build it, just modify the Makefile to built menu-monitor-inotify.c instead of menu-monitor-fam.c. When menu-monitor-inotify.c is ready, we can add some code to make it try inotify first and then fall back to FAM.
After a quick review of your code there are a couple problems, 1) It will fail if it runs in to symbolic links 2) Your read function is going to trick the kernel into thinking this is an interactive application. The kernel is going to schedule apps using gnome-menus way more than it should and the in kernel event compression will not be able to take place Why not a optional gnome-vfs backend instead?
Created attachment 58418 [details] [review] gnome-menus-configure-gnomevfs.patch
Created attachment 58419 [details] menu-monitor-gnomevfs.c
The patch still needs some integration work, mainly in configure/makefile space.
Hey Mark, why did you reject my patch?
Mark? Anyone else?
John: as I said, I don't think it's sane to add a gnome-vfs dependancy purely for the sake of file monitoring. Especially now, given alexl's work on gvfs. The way to avoid this kind of duplication is to get file monitoring support in glib. Anyway, inotify support should be fairly usable on HEAD now. Appreciate any testing, bug reports, patches etc. 2006-11-01 Mark McLoughlin <mark@skynet.ie> Flesh out the inotify support. Not enabled by default until it gets a tad more testing. Closes bug #314854 * configure.in: add --enable-inotify * libmenu/Makefile.am: build menu-monitor-inotify.c if inotify is enabled. * libmenu/menu-monitor-inotify.c: implement lots more of this. See TODO list at the top of the file.
Mark, my patch implemented the dependency on gnome-vfs in a similar fashion as the inotify dependency; Optional, specified at compile time. The future of gnome-vfs is irrelevant, a gvfs backend could easily be added once gvfs is ready to be used. This bug is a farce anyways, since almost every major distribution (suse,debian,ubuntu,fedora?) patches gnome-menus to depend on gnome-vfs. If that doesn't tell you something, then I don't know what will. As for your inotify backend. It has at least one fairly serious bug on top of those I listed originally (all still unfixed). Watching a file's inode with inotify will not give you expected results. You need to watch the file's parent directory's inode instead (and aggregate the events with the inotify event name field). I suggest you take a look at the inotify code in gamin/gnome-vfs. It handles every issue I've raised and only depends on glib (the version in gamin that is).