Re: [Nautilus-list] verify_current_mount_state

[Owen Taylor <otaylor redhat com>]

Darin Adler <darin bentspoon com> writes:

> on 9/5/01 5:11 PM, Owen Taylor at otaylor redhat com wrote:
> 
> > The code in nautilus-volume-monitor.c/verify_current_mount_state() is
> > confusing me a quite a bit:
> 
> Sadly, the original author is the only one who would know why the code is
> written the way it is -- this code was not carefully reviewed as it went in.
> I'll try to figure it out with you.

And every time I send mail to nautilus-list I get a bounce from 
gzr ix netcom com   
 
> > We actually need the full name and so forth for the old_mounts list,
> > fm-desktop-icon-view.c needs it to know what to remove, so what
> > we have to do is:
> > 
> > a) Call load_additional_mount_list_info() on current_mounts before
> >   we save it in monitor->details->mounts.
> > 
> > b) Not call load_additional_mount_list_info() on old_mounts
> >   or saved_mount_list since that will wipe out what we stored.
> 
> That sounds right to me.
> 
> I'm not even sure why load_additional_mount_list_info has to be done in a
> separate pass in the first place instead of being done as the mount list is
> created. I suspect the code in this file could be greatly simplified without
> changing its behavior for the worse.

The reason is that the code wants to be able to check changes cheaply,
so it checks to see if the old and new lists are identical before it
reads the volume names, which is a fairly expensive operation.

> > I don't understand this either ... old_mounts and saved_mount list
> > come from the same place (the previous list of mounts that was stored
> > in mount->details->mounts), so saved_mount_list won't have any information
> > that is not in old_mounts.
> > 
> > I think calling update_modified_volume_name doesn't actually do anything
> > useful, and it seems to be some attempt to solve the problem that
> > a) and b) are supposed to fix above.
> 
> I think you're right. This looks like an attempt to hack around a specific
> problem caused by getting additional volume information at a late date.

The following patch I think should fix the race condition and simplifies
the code. I haven't tested it in exactly this form, but a version
with all the changes except for removing the calls to 
update_modifed_volume_name() fixes the problems that we were seeing.

(And I'm pretty positive update_modifed_volume_name() is a no-op here
since the "to" volume is just a copy of the "from" volume structure.")

An optimization that could be made here would be to replace:

	/* Process list results to check for a properties that require opening files on disk. */
	load_additional_mount_list_info (current_mounts);

With:

 load_additional_mount_list_info (new_mounts);

 foreach mount in current_mounts:
   if (mount is in new_mounts):
      copy information from new mounts
   else:
      copy information from monitor->details->new_mounts

If you had a big bunch of CD-ROM drives on a system, it might be a 
considerable win to avoid opening them all and reading the labels
from all of them when a new disk was inserted in one. It won't
make any difference for the common case of one CD-ROM.

Regards,
                                        Owen

Index: ChangeLog
===================================================================
RCS file: /cvs/gnome/nautilus/ChangeLog,v
retrieving revision 1.4752
diff -u -r1.4752 ChangeLog
--- ChangeLog	2001/09/05 20:10:30	1.4752
+++ ChangeLog	2001/09/06 14:04:03
@@ -1,3 +1,16 @@
+2001-09-06  Owen Taylor  <otaylor redhat com>
+
+	* libnautilus-private/nautilus-volume-monitor.c (verify_current_mount_state): 
+	Fix bug where the mount list was stored without full mount
+	names. Don't call mount_volume_make_name() on volumes that
+	might not by on the system any more. Remove code that 
+	(unsuccesfully) tried to fix the old volume names up, since
+	we now have the correct old volume names.
+
+	Effect of these changes is to fix a race condition where
+	'unmount /mnt/cdrom; eject /dev/cdrom' would leave a left-over
+	icon on the deskop.
+
 2001-09-05  Maciej Stachowiak  <mjs noisehavoc org>
 
 	* libnautilus-private/nautilus-authn-manager.c:
Index: libnautilus-private/nautilus-volume-monitor.c
===================================================================
RCS file: /cvs/gnome/nautilus/libnautilus-private/nautilus-volume-monitor.c,v
retrieving revision 1.98
diff -u -r1.98 nautilus-volume-monitor.c
--- libnautilus-private/nautilus-volume-monitor.c	2001/08/16 22:46:04	1.98
+++ libnautilus-private/nautilus-volume-monitor.c	2001/09/06 14:04:03
@@ -966,21 +966,6 @@
 }
 
 
-static void
-update_modifed_volume_name (GList *mount_list, NautilusVolume *volume)
-{
-	GList *node;
-	NautilusVolume *found_volume;
-	
-	for (node = mount_list; node != NULL; node = node->next) {
-		found_volume = node->data;
-		if (strcmp (volume->device_path, found_volume->device_path) == 0) {
-			g_free (volume->volume_name);
-			volume->volume_name = g_strdup (found_volume->volume_name);
-		}
-	}
-}
-
 static gboolean
 mount_lists_are_identical (GList *list_a, GList *list_b)
 {
@@ -1016,24 +1001,17 @@
 		return;
 	}
 		
+	/* Process list results to check for a properties that require opening files on disk. */
+	load_additional_mount_list_info (current_mounts);
+	
 	/* Create list of new and old mounts */
 	new_mounts = build_volume_list_delta (current_mounts, monitor->details->mounts);
 	old_mounts = build_volume_list_delta (monitor->details->mounts, current_mounts);  		
 		
-	/* Stash a copy of the current mount list for updating mount names later. */
-	saved_mount_list = monitor->details->mounts;
-		
 	/* Free previous mount list and replace with new */
+	free_mount_list (monitor->details->mounts);
 	monitor->details->mounts = current_mounts;
 
-	/* Process list results to check for a properties that require opening files on disk. */
-	load_additional_mount_list_info (new_mounts);
-	
-	if (old_mounts != NULL) {
-		load_additional_mount_list_info (old_mounts);
-		load_additional_mount_list_info (saved_mount_list);
-	}
-	
 	/* Check and see if we have new mounts to add */
 	for (node = new_mounts; node != NULL; node = node->next) {
 		mount_volume_activate (monitor, node->data);
@@ -1041,20 +1019,11 @@
 	
 	/* Check and see if we have old mounts to remove */
 	for (node = old_mounts; node != NULL; node = node->next) {
-		/* First we need to update the volume names in this list with modified names in the old list. Names in
-		 * the old list may have been modifed due to icon name conflicts.  The list of old mounts is unable
-		 * take this into account when it is created
-		 */			
-		update_modifed_volume_name (saved_mount_list, node->data);
-		
-		/* Deactivate the volume. */
 		mount_volume_deactivate (monitor, node->data);
 	}
 	
 	free_mount_list (old_mounts);
 	free_mount_list (new_mounts);
-	
-	free_mount_list (saved_mount_list);
 }
 
 static int