Re: [patch] improved hal support for gnome-vfs

[Alexander Larsson <alexl redhat com>]

On Wed, 2005-06-29 at 12:00 -0400, David Zeuthen wrote:

> There's a few things left to do but the patch is mostly done.
> Specifically, one of the things I want to do is test with with a wider
> range of hardware, specifically legacy floppy drives and IDE zip drives.
> I'm submitting the patch now as it got a number of strings for i18n and
> there's a deadline on July 1st.

I'm commiting this to get all the new strings in. There are some issues
with the patch though. I didn't review all the code in the smallest
detail (to much code!), but looking at the structure I have a few
comments:

* Does this work with any version of libhal, or should we update the
libhal version requirement?

+static char *
+_hal_size_as_string (gint64 size)
+{
....
+	char* sizes_str[] = {"K", "M", "G", "T", NULL};
+	gint64 cur = 1000L;
+	gint64 base = 10L;
+	gint64 step = 10L*10L*10L;
+	int cur_str = 0;

Gnome in general, and gnome-vfs in particular uses 1024 for disk sizes.
Actually, this code should probably just use the function:
gchar* gnome_vfs_format_file_size_for_display (GnomeVFSFileSize size)


* _hal_drive_policy_check calls gconf calls directly. This can be a
problem in the daemon, because gconf calls are Corba calls. In this
particular instance this might be a problem because this code can be
reached by _gnome_vfs_hal_mounts_force_reprobe, which can be called in a
corba method handler. This can lead to strange reentrancy problem.

To solve this you need to make sure to always get the gconf value from a
well known non-reentrant place in the mainloop. I'd recommend reading
all the gconf values to static variables in a function, then calling
this function in _init and _hal_settings_changed.


+	g_debug ("entering _hal_add_drive_without_volumes for\n  drive udi '%s'\n",
+		 libhal_drive_get_udi (hal_drive));

I'm not sure we want all these debug calls compiled into the code.
Whenever you enable debug logging you'll get them all, which might mess
up things for an app developer using g_debug. You need to either delete
these or wrap them in some #ifdef action.

+ #define HAL_MOUNTS_INVALID_URI "/gnome/vfs2/internal/no/uri/available"

This is sort of ugly. Doesn't an empty uri work? maybe not...

+#if defined(USE_HAL) && defined(HAL_MOUNT) && defined(HAL_UMOUNT)
+				  /* do pass our environment when using hal mount progams */
+				  ((strcmp (info->argv[0], HAL_MOUNT) == 0) ||
+				   (strcmp (info->argv[0], HAL_UMOUNT) == 0)) ? NULL : envp,
+#else

Not using envp means we're not adding "LC_ALL=C" to the environment. Is
this really right? It means we'll get translated error message from the
app, which we don't understand. Isn't this a problem?


+	/* if we had a drive from here but where we weren't mounted, just use that drive and update
+	 * activation_uri as we now have a definite mount point...
+	 */
+	drive = _gnome_vfs_volume_monitor_find_drive_by_hal_udi (volume_monitor, libhal_volume_get_udi (hal_volume));
+	if (drive != NULL) {
+		if (libhal_volume_disc_has_audio (hal_volume)) {
+			drive->priv->activation_uri = g_strdup_printf ("cdda://%s", 
+								       libhal_volume_get_device_file (hal_volume));
+		} else if (libhal_volume_disc_is_blank (hal_volume)) {
+			drive->priv->activation_uri = g_strdup ("burn:///");
+		} else if (libhal_volume_is_mounted (hal_volume)) {
+			drive->priv->activation_uri = gnome_vfs_get_uri_from_local_path (
+				libhal_volume_get_mount_point (hal_volume));
+		} else {
+			/* This sucks but it doesn't make sense to talk about the activation_uri if we're not mounted!
+			 * So fake it...
+			 */
+			drive->priv->activation_uri = gnome_vfs_get_uri_from_local_path (HAL_MOUNTS_INVALID_URI); 
+		}
+

This doesn't work. First of all, drives and volumes, once initialized
are generally read-only (except the volume-drive relationships). This is
partly a threading issue, as all thread protection on these datatypes is
based on them being read-only. It also lacks any code to push such
changes from the daemon to the clients, so nobody will actually see this
change.

It would be nice to eventually change this and support changes to
drives/volumes, as that would allow nice things like renaming of
connected servers. However, right now there is no such support.

Also, the code above doesn't free old activation_uri.

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's a deeply religious drug-addicted dwarf from the 'hood. She's a cynical 
junkie college professor who believes she is the reincarnation of an ancient 
Egyptian queen. They fight crime!