GNOME Bugzilla – Bug 520132
Move hal volume monitor out of process
Last modified: 2008-07-08 16:51:57 UTC
Some reasons are summarized here http://mail.gnome.org/archives/nautilus-list/2008-February/msg00186.html http://mail.gnome.org/archives/nautilus-list/2008-February/msg00195.html This is also needed to fix bug 520123.
Taking ownership
Created attachment 114134 [details] [review] patch to provide infrastructure for out-of-process volume monitors (will follow up in a bit with an explanation of how the patch works)
b/Makefile.am | 1 b/configure.ac | 2 Add new directory monitor and monitor/proxy. b/programs/gvfs-mount.c | 19 Minor enhancements to gvfs-mount needed for testing this. gvfs/monitor/Makefile.am | 2 gvfs/monitor/proxy/Makefile.am | 67 + gvfs/monitor/proxy/gproxydrive.c | 578 ++++++++++ gvfs/monitor/proxy/gproxydrive.h | 55 gvfs/monitor/proxy/gproxymount.c | 502 ++++++++ gvfs/monitor/proxy/gproxymount.h | 56 gvfs/monitor/proxy/gproxyvolume.c | 688 ++++++++++++ gvfs/monitor/proxy/gproxyvolume.h | 58 + gvfs/monitor/proxy/gproxyvolumemonitor.c | 1068 +++++++++++++++++++ gvfs/monitor/proxy/gproxyvolumemonitor.h | 73 + gvfs/monitor/proxy/remote-volume-monitor-module.c | 50 This is a standard gio module that provides one or more volume monitors. It reads the files /usr/share/gvfs/remote-volume-monitors/*.mount and based upon information in these create volume monitor types and registers them with the gio subsystem. A .monitor is simple, it looks like this [RemoteVolumeMonitor] Name=GProxyVolumeMonitorHal DBusName=org.gtk.Private.HalVolumeMonitor IsNative=true NativePriority=2 One only need to provide the NativePriority key in the event IsNative=true. In a nutshell, instances of the created types (which are all of the class GProxyVolumeMonitor) simply proxies information from the given D-Bus service. The D-Bus protocol is private to gvfs. gvfs/monitor/proxy/gvfsproxyvolumemonitordaemon.c | 914 ++++++++++++++++ gvfs/monitor/proxy/gvfsproxyvolumemonitordaemon.h | 35 Whereas the GProxyVolumeMonitor derived types all represent the D-Bus clients, the server side is provided via the libgvfsproxyvolumemonitordaemon-noin static library. This library provides two functions that are implemented in the following way int main (int argc, char *argv[]) { g_vfs_proxy_volume_monitor_daemon_init (); return g_vfs_proxy_volume_monitor_daemon_main ( argc, argv, "org.gtk.Private.HalVolumeMonitor", G_TYPE_HAL_VOLUME_MONITOR); } Keep in mind that this is not limited to the hal volume monitor; I plan to follow up with a patch to move the gphoto2 volume monitor into it's own process. So this is the generic infrastructure. What follows is the port of the hal volume monitor to use this infrastructure. gvfs/hal/hal-volume-monitor-daemon.c | 44 Basically the C snippet above. gvfs/hal/hal.monitor | 5 The .monitor file snippet above. gvfs/hal/org.gtk.Private.HalVolumeMonitor.service.in | 3 Needed for activation of the org.gtk.Private.HalVolumeMonitor D-Bus session service. b/hal/Makefile.am | 37 Build a daemon instead of a gio module. b/hal/ghaldrive.c | 31 b/hal/ghaldrive.h | 1 b/hal/ghalmount.c | 23 b/hal/ghalmount.h | 1 b/hal/ghalvolume.c | 33 b/hal/ghalvolume.h | 1 b/hal/ghalvolumemonitor.c | 23 b/hal/ghalvolumemonitor.h | 3 b/hal/hal-device.c | 13 b/hal/hal-device.h | 1 b/hal/hal-pool.c | 14 b/hal/hal-pool.h | 1 Make all types static since we'll only be instantiating them in the volume monitor daemon. Also implement GVolumeMonitor::drive-eject-button (bug 541794) and g_volume_get_activation_root (bug 541793).
> + g_warning ("TODO: oh no; can't remove match rule"); Oh no, I forgot to fix a few details. Lemme upload a new patch in a bit.
Created attachment 114158 [details] [review] updated patch implement remaining functionality and fix remaining TODO's g_volume_monitor_is_supported g_volume_monitor_get_mount_for_uuid g_volume_monitor_get_volume_for_uuid g_volume_monitor_get_mount_for_mount_path [1] g_drive_poll_for_media and clean up the code a bit more. [1] : we really need to deprecate that static method in gio - it's totally asking for trouble to have it. The implementation I've done here should work just fine though; we do return a GMount instance... But that's for another bug.
Some review of the previous patch: +/* Protects all fields of GProxyDrive that can change */ +G_LOCK_DEFINE_STATIC(proxy_mount); s/GProxyDrive/GProxyMount/ +static GFile * +g_proxy_mount_get_root (GMount *mount) +{ + GProxyMount *proxy_mount = G_PROXY_MOUNT (mount); + GFile *root; + + G_LOCK (proxy_mount); + root = proxy_mount->root != NULL ? g_object_ref (proxy_mount->root) : NULL; + G_UNLOCK (proxy_mount); + return root; +} We usually don't have reffing getters in GTK+. If this is different for gio, then the documentation of e.g. /** * g_mount_get_root: * @mount: a #GMount. * * Gets the root directory on @mount. * * Returns: a #GFile. **/ must be clarified to say so. + /* TODO: support mount_operation */ Will this be filled in before committing ? +/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */ +/* GIO - GLib Input, Output and Streaming Library + * + * Copyright (C) 2006-2007 Red Hat, Inc. Should probably say "gvfs" here, and 2008. + * We avoid locking since GUnionVolumeMonitor, the only user of us, + * does locking. + */ + +G_LOCK_DEFINE_STATIC(proxy_vm); You say you avoid locking, but you define the lock anyway ?! +static GVolume * +get_volume_for_uuid (GVolumeMonitor *volume_monitor, const char *uuid) +{ + return NULL; +#if 0 +static GMount * +get_mount_for_uuid (GVolumeMonitor *volume_monitor, const char *uuid) +{ + return NULL; +#if 0 Will those ifzeroed functions be fixed, or removed ?
Created attachment 114188 [details] [review] third strike is the charm
+ * Copyright (C) 2006-2008 Red Hat, Inc. For new files, this should probably just be 2008... +/* Protects all fields of GProxyDrive that can change */ +G_LOCK_DEFINE_STATIC(proxy_volume); Thats still wrong... Other than that, it looks good to me now.
Oh, mid-air collision.. anyway here are the comments I wrote up (In reply to comment #6) > +/* Protects all fields of GProxyDrive that can change */ > +G_LOCK_DEFINE_STATIC(proxy_mount); > > s/GProxyDrive/GProxyMount/ Fixed. > We usually don't have reffing getters in GTK+. > If this is different for gio, then the documentation of e.g. [...] > must be clarified to say so. Yes, that needs to be fixed in the docs in gio. > + /* TODO: support mount_operation */ > > Will this be filled in before committing ? No - the hal volume monitor wasn't using it so I haven't implemented it yet since it's not currently needed. There's a TODO in the source file about it though: * - implement support for GMountOperation * - not implemented in the HAL volume monitor and that's all * that is using it right now. Will implement at some point * when it's needed or someone has spare cycles. > +/* GIO - GLib Input, Output and Streaming Library > + * > + * Copyright (C) 2006-2007 Red Hat, Inc. > > Should probably say "gvfs" here, and 2008. Fixed. > + * We avoid locking since GUnionVolumeMonitor, the only user of us, > + * does locking. > + */ > + > +G_LOCK_DEFINE_STATIC(proxy_vm); > > You say you avoid locking, but you define the lock anyway ?! Removed the comment; it was a spillover from copy-paste. > +static GVolume * > +get_volume_for_uuid (GVolumeMonitor *volume_monitor, const char *uuid) > +{ > + return NULL; > +#if 0 > > +static GMount * > +get_mount_for_uuid (GVolumeMonitor *volume_monitor, const char *uuid) > +{ > + return NULL; > +#if 0 > > Will those ifzeroed functions be fixed, or removed ? These are implemented now. There's also a TODO saying this * - instead of calling IsSupported() we just call List() * - e.g. exactly the same IPC overhead * - neglible memory + cpu overhead * - will need to construct them at some point * - we can actually implement get_mount_for_mount_path() * correctly Again, this is not urgent to implement.
(In reply to comment #8) > + * Copyright (C) 2006-2008 Red Hat, Inc. > > For new files, this should probably just be 2008... It's pretty much a derivative work of the hal and unix volume monitors. > +/* Protects all fields of GProxyDrive that can change */ > +G_LOCK_DEFINE_STATIC(proxy_volume); > > Thats still wrong... Fixed that. > Other than that, it looks good to me now. Thanks for the review. I've committed the patch. I'll also move the hal/ directory into the monitor/ directory. 2008-07-08 David Zeuthen <davidz@redhat.com> Provide infrastructure for out of process volume monitors and port the hal volume monitor to use it (#520132). * Makefile.am: * configure.ac: Add the monitor and monitor/proxy directories. * hal/Makefile.am: Don't build a gio module for the hal volume monitor; instead build a volume monitor daemon. * hal/ghaldrive.[ch]: * hal/ghalmount.[ch]: * hal/ghalvolume.[ch]: * hal/ghalvolumemonitor.[ch]: * hal/hal-device.[ch]: * hal/hal-device.[ch]: * hal/hal-pool.[ch]: Make all types static and implement g_volume_get_activation_root() added to gio (#541793). Also emit the drive-eject-button signal (#541794). * hal/hal-module.c: Removed since the monitor is being moved out of process. * hal/hal-volume-monitor-daemon.c: * hal/hal.monitor: * hal/org.gtk.Private.HalVolumeMonitor.service.in: New files for remote volume monitor. * monitor/Makefile.am: New file. * monitor/proxy/*: Add proxy volume monitor gio module (the D-Bus client side of out-of-process volume monitors) and a static library for providing the D-Bus server side of out of process volume monitors. * programs/gvfs-mount.c: Print activation uri for a volumes and icons for drives. Also unref volume monitor when no longer in use.
(In reply to comment #10) > I'll also move the hal/ directory into the monitor/ directory. Done. And, god, how svn blows.