Re: gvfs hal volume monitoring backend

[David Zeuthen <david fubar dk>]

On Mon, 2007-12-17 at 11:37 +0100, Alexander Larsson wrote:
> I commited all of these as a base to work from

Thanks. FYI I'll keep on committing fixes / feature to the hal backend
in the mean time (one thing I want is to generate GVolume objects for
gphoto2 cameras pointing to a camera:// URI; then do a gvfs backend for
those URI's at some point).

> 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

OK, I've fixed this; dbus-glib-1 is no longer required for the hal
volume monitor. Instead an extra roundtrip (a few actually) to the bus
daemon will be made since I have to use dbus_bus_get_private(). 

>  until we have a sane lowlevel
> mainloop integration setup (as discussed on the dbus list).

We just should get this done and in a dbus release we can depend on.
It's really bad to use private D-Bus connections.

> 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.

OK, I see that you've fixed this already; (I just committed a small fix
to set the can_eject vtable instead of the can_unmount twice.)

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

I actually thought the gtk+ code using this would use the newly
introduced GTK_ICON_LOOKUP_GENERIC_FALLBACK that is available as of
2.12... 

> +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.

That's my understanding when I read the code too (it would probably be
good to add this to the docs too). However, if we want the information
to be accurate (and I think we do) we do need to connect to hal. 

Since I just introduced a fast path for getting info from hal [1] it's
not really more expensive, IO wise anyway, to construct a full volume
monitor.

In the majority of cases (nautilus, file chooser) the caller will
already have a volume monitor so there's no penalty for them. I can't
think of any apps without a volume monitor that would need to call it.

[1] : gvfs uses libhal_get_all_devices_with_properties() if it's
available (both build- and runtime checks are in place). This reduced
the time to get info from hal from 54ms to 15ms by doing a single IPC
call instead of N+1 (with N being the number of devices). 

(The hal bits will land in hal git later today - needs a little
tweaking).

> 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. 

Sure, I'll look into doing this when...

> > 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.

(cont'd from above paragraph) 

...this is done (sounds like you are doing it? otherwise I can look into
it).

> 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?

Hmm.. I thought

module_flags = -export_dynamic -avoid-version -module -no-undefined
-export-symbols-regex '^g_io_module_(load|unload)'

took care of that; I mean, we really only need to export the two
functions g_io_module_(load|unload) right? My knowledge of symbol
visibility is a bit rusty, sorry.

>   /* 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.

I saw you did this already. Thanks.

      David