Re: New gnome-vfs hal patch

[Alexander Larsson <alexl redhat com>]

On Sat, 2004-09-25 at 12:20 +0200, David Zeuthen wrote:
> 
> Here's a new GNOME VFS hal patch. It's basically a complete rewrite as
> the old patch disabled all the /etc/fstab and /etc/mtab reading. This
> was bad since a) some volumes, such as network drives or RAID/LVM
> volumes in /etc/fstab are out of the reach of hal; and b) GNOME VFS
> needs this otherwise some of the API might fail.

This looks pretty good to me. I don't think its gnome 2.8 material, but
when fixed up it should be fine to get in as soon as we branch 2.8
(which I'd like to wait with a bit more).

Some comments:
Comment about the general parts:

diff -u -p -r1.9 GNOME_VFS_Daemon.idl
--- libgnomevfs/GNOME_VFS_Daemon.idl	15 Jul 2004 13:21:15 -0000	1.9
+++ libgnomevfs/GNOME_VFS_Daemon.idl	24 Sep 2004 22:50:33 -0000
@@ -63,6 +63,7 @@ module GNOME {
 			boolean is_user_visible;
 			boolean is_connected;
 		        string hal_udi;
+			boolean should_eject;
 		};
 		
 		typedef sequence<Drive> DriveList;


Can this be must_eject_at_unmount instead?


+_gnome_vfs_volume_monitor_find_volume_by_device_path (GnomeVFSVolumeMonitor *volume_monitor,
+						      const char *device_path)
..
+		if (vol->priv != NULL && vol->priv->hal_udi != NULL && 
+		    vol->priv->device_path != NULL && /* Hmm */

Why do these only work if hal_udi is set. Seems strange.

+_gnome_vfs_volume_monitor_find_drive_by_device_path (GnomeVFSVolumeMonitor *volume_monitor,
+						     const char *device_path)

Same here. Also, are these really needed? See below.

> 2. On Linux, the device file name is used for mount/umount as this is
>    required to eject blank/audio discs.
+#ifdef __linux__
+	name = device_path;
 #else
+# ifdef USE_VOLRMMOUNT

Why is this needed? We already always use the device for
eject. Shouldn't it instead do something like not unmount if
mountpoint is not set? 

+	if (drive->priv->should_eject) {
+		gnome_vfs_drive_eject(drive, callback, user_data);
+		return;
+	}

Space before paranthesis



gnome-vfs-hal-mounts.c:


	if (drive != NULL) {
		_gnome_vfs_volume_monitor_disconnected (GNOME_VFS_VOLUME_MONITOR (volume_monitor_daemon), drive);
	}

	if (volume != NULL) {
		_gnome_vfs_volume_monitor_unmounted (GNOME_VFS_VOLUME_MONITOR (volume_monitor_daemon), volume);
	}

Are these ever non-null at the same time? If so, we should unmount the
volume before we disconnect the drive.

_gnome_vfs_hal_mounts_modify_drive:

	    hal_drive_get_type (hal_drive)==HAL_DRIVE_TYPE_CDROM &&

Missing whitespace	    

		/* see if we already got a volume */
		volume = _gnome_vfs_volume_monitor_find_volume_by_device_path (
			GNOME_VFS_VOLUME_MONITOR (volume_monitor_daemon),
			hal_drive_get_device_file (hal_drive));
		if (volume == NULL) {


Why search by device path? Can't you use
gnome_vfs_drive_get_mounted_volumes(drive) instead?

			if (hal_drive_get_dedicated_icon_volume (hal_drive) != NULL)
				volume_icon = g_strdup (hal_drive_get_dedicated_icon_volume (hal_drive));
			else
				volume_icon = hal_volume_policy_compute_icon_name (
					hal_drive, hal_volume, hal_storage_policy);

You're mixing g_malloc and malloc memory here.

			volume->priv->volume_type = GNOME_VFS_VOLUME_TYPE_MOUNTPOINT;

These types of objects aren't real mountpoints (that means unix-style
mounts). Use GNOME_VFS_VOLUME_TYPE_VFS_MOUNT instead.



	} else if (hal_volume == NULL && hal_drive_get_type (hal_drive)==HAL_DRIVE_TYPE_CDROM) {

Missing spaces.
	
		/* Remove GnomeVFSVolume with same device file */
		
		volume = _gnome_vfs_volume_monitor_find_volume_by_device_path (
			GNOME_VFS_VOLUME_MONITOR (volume_monitor_daemon), hal_drive_get_device_file (hal_drive));
		if (volume != NULL) {
			_gnome_vfs_volume_monitor_unmounted (GNOME_VFS_VOLUME_MONITOR (volume_monitor_daemon), volume);
		}

Use gnome_vfs_drive_get_mounted_volumes(drive) instead perhaps?


	unique_drive_name = _gnome_vfs_volume_monitor_uniquify_drive_name (
		GNOME_VFS_VOLUME_MONITOR (volume_monitor_daemon), drive_name);
	if (drive->priv->display_name != NULL)
		free (drive->priv->display_name);
	drive->priv->display_name = unique_drive_name;

Need to use g_free instead.


		/* set icon name; try dedicated icon name first... */
	if (hal_drive_get_dedicated_icon_drive (hal_drive) != NULL)
		drive_icon = g_strdup (hal_drive_get_dedicated_icon_drive (hal_drive));
	else
		drive_icon = hal_drive_policy_compute_icon_name (hal_drive, hal_volume, hal_storage_policy);
	if (drive->priv->icon != NULL)
		free (drive->priv->icon);
	drive->priv->icon = g_strdup (drive_icon);
	free (drive_icon);

mixing allocators for drive_icon, using free instead of g_free on old
icon.


	/* otherwise lookup fstab */
	if (target_mount_point == NULL && setfsent () == 1) {
		struct fstab *fst;
		fst = getfsspec (drive->priv->device_path);
		if (fst != NULL )
			target_mount_point = g_strdup (fst->fs_file);
		endfsent ();
	}

This totally won't work. getfsspec etc are not portable like
that. Check out gnome-vfs-unix-mounts.c.

	
_gnome_vfs_hal_mounts_modify_volume:

	unique_volume_name = _gnome_vfs_volume_monitor_uniquify_volume_name (
		GNOME_VFS_VOLUME_MONITOR (volume_monitor_daemon), volume_name);
	if (volume->priv->display_name != NULL)
		free (volume->priv->display_name);

mixing allocators

	/* set icon name; try dedicated icon name first... */
	if (hal_drive_get_dedicated_icon_volume (hal_drive) != NULL)
		volume_icon = g_strdup (hal_drive_get_dedicated_icon_volume (hal_drive));
	else
		volume_icon = hal_volume_policy_compute_icon_name (hal_drive, hal_volume, hal_storage_policy);
	if (volume->priv->icon != NULL)
		free (volume->priv->icon);
	volume->priv->
		icon = g_strdup (volume_icon);

mixing allocators

		
	/* otherwise lookup fstab */
	if (target_mount_point == NULL && setfsent () == 1) {
		struct fstab *fst;
		fst = getfsspec (volume->priv->device_path);
		if (fst != NULL )
			target_mount_point = g_strdup (fst->fs_file);
		endfsent ();
	}

not portable
	

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's an otherworldly native American firefighter with a winning smile and a 
way with the ladies. She's an elegant out-of-work wrestler on her way to 
prison for a murder she didn't commit. They fight crime!