Re: gvfs hal volume monitoring backend

[Alexander Larsson <alexl redhat com>]

On Fri, 2007-12-14 at 19:21 -0500, David Zeuthen wrote: 
> On Fri, 2007-12-14 at 03:22 -0500, David Zeuthen wrote:
> > Hi again,
> > 
> > Here we go with the patches for the hal volume monitoring backend:
> > 
> >  http://people.freedesktop.org/~david/gio.patch
> >  http://people.freedesktop.org/~david/nautilus.patch
> >  http://people.freedesktop.org/~david/gvfs.patch
> 
> Here's some updated patches
> 
>  http://people.freedesktop.org/~david/gio-2.patch
>  http://people.freedesktop.org/~david/gvfs-2.patch
> 
> against trunk. No changes to the nautilus patch; it should still apply.
> Notable changes

I commited all of these as a base to work from

Comments on the patches:

It seems this uses dbus-glib-1, which is an unstable API/ABI. Is this
really a good idea? It means gvfs could break at any point.

Also, it calls dbus_connection_setup_with_g_main() on the main shared
dbus connection. This means that if the application previously
initialized the mainloop with some other kind of mainloop integration
call (for instance the talked about lowlevel dbus-glib integration
library) then things will break in weird ways. For the gio integration
I've used a custom dbus connection until we have a sane lowlevel
mainloop integration setup (as discussed on the dbus list).

You have an empty g_daemon_mount_eject, it should be NULL so we get
proper G_IO_ERROR_NOT_SUPPORTED support from the GMount baseclass.

All the volume manager code just uses a single name for the GIcon. Why
not use a set of icons

+get_mount_for_mount_path (const char *mount_path)
...
+  if (the_volume_monitor == NULL)
+    {
+      /* Dammit, no monitor is set up.. so we have to create one, find
+       * what the user asks for and throw it away again. 
+       *
+       * What a waste - especially considering that there's IO
+       * involved in doing this: connect to the system message bus;
+       * IPC to hald...
+       */
+      volume_monitor = G_HAL_VOLUME_MONITOR (_g_hal_volume_monitor_new
());
+    }

get_mount_for_mount_path is used in the implementation of
g_file_find_enclosing_mount() for local files. This implementation stats
up the filesystem looking for the toplevel of the mountpoint. This is
then passed into get_mount_for_mount_path to get a GVolume object for
the volume that is mounted at that particular place.

This call was added to implement things like displaying where a file is
stored (with volume icon + name) in the properties dialog, and to e.g.
allow unmounting the current filesystem when displaying the toplevel of
a volume. 

There is no requirement for the returned GMount object to be part of a
full volume monitor. Instead the idea is that we can just create one for
the particular mountpoint. This should be far more efficient than
creating a full volume monitor.

I see that you're building the hal stuff in a separate module. It might
be better to add it to the general gvfs module. This will save 4k of
writable memory per process that uses gio. 

Also, is there a risk of the hal_ symbols in that module leaking out? We
load the file with G_MODULE_BIND_LOCAL which should mean this is not a
problem, but maybe there are system where this is not supported? Does
anyone know of something like that?

  /* TODO: Since dynamic types need some fixing we want to make ourselves resident; I couldn't
   *       figure out how to get a GModule from GIOModule so do this hack until then
   */
  g_type_module_use (G_TYPE_MODULE (module));

A GTypeModule (and thus a GIOModule which is a subclass of it) is not
actually a GModule. It is something that defines a set of GTypes that
are dynamically loaded (from e.g. a GModule or somewhere else). It knows
what types are contained in the module and only loads the actual code
when these types are in use (by calling GTypeModuleClass->load which in
GIOModule is implemented by loading the .so). 

Thus, you really should handle dynamic types correctly. Otherwise the
GModule won't be unloaded after the inital scan of the GIOModules even
if the application doesn't use the types in question.

> One fix is to make it handle when the constructor for a
> GNativeVolumeMonitor fails - this is important to handle as the hal
> volume monitor will fail if hald isn't running. I'm not happy about
> how I've implemented it; suggestions welcome.

The way this is handled by g_vfs_get_default() is that the instantiated
GVfs object is checked with g_vfs_is_active() to make sure that
construction was successful. This is the general pattern used for this
in GObject. As there is no exception handling in GObject it is not
generally allowed for construction to fail.

I'll start working on these details in svn.