gvfs r1784 - in branches/gnome-2-22: . client

[hansp svn gnome org]

Author: hansp
Date: Fri May 23 03:22:22 2008
New Revision: 1784
URL: http://svn.gnome.org/viewvc/gvfs?rev=1784&view=rev

Log:
2008-05-22  Hans Petter Jansson  <hpj novell com>

	Fix fuse daemon locking and file handle life-cycle issues that
	were causing frequent crashes.

	* client/gvfsfusedaemon.c (file_handle_new): Add a "path" field
	pointing to a string representing the path this file handle is
	associated with, for reverse mapping.
	(file_handle_unref): Is now responsible for decrementing the ref
	count and freeing the handle if it reaches 0. Note that we need
	to check the ref count again after obtaining the global mutex.
	(file_handle_free): The new path member is freed here.
	(get_file_handle_for_path): Ref the obtained handle.
	(get_or_create_file_handle_for_path): Ditto, and hold the lock the
	whole time.
	(reindex_file_handle_for_path): Steal the old entry to avoid
	buildup of stale handles.
	(free_file_handle_for_path): Remove.
	(vfs_getattr): Unref the handle when we're done with it.
	(vfs_rename): Ditto.
	(vfs_unlink): Ditto.
	(vfs_truncate): Ditto.
	(vfs_open): Don't ref the obtained handle; it's done in the helper.
	(vfs_create): Ditto.
	(vfs_release): Let file_handle_unref() free the handle if
	appropriate. Note that the old logic here was inverted, meaning we'd
	try to free the handle if the ref count was non-zero.
	(vfs_init): The hash table no longer owns the path key strings -
	the file handle does.



Modified:
   branches/gnome-2-22/ChangeLog
   branches/gnome-2-22/client/gvfsfusedaemon.c

Modified: branches/gnome-2-22/client/gvfsfusedaemon.c
==============================================================================
--- branches/gnome-2-22/client/gvfsfusedaemon.c	(original)
+++ branches/gnome-2-22/client/gvfsfusedaemon.c	Fri May 23 03:22:22 2008
@@ -74,6 +74,7 @@
   gint      refcount;
 
   GMutex   *mutex;
+  gchar    *path;
   FileOp    op;
   gpointer  stream;
   gint      length;
@@ -185,7 +186,7 @@
 }
 
 static FileHandle *
-file_handle_new (void)
+file_handle_new (const gchar *path)
 {
   FileHandle *file_handle;
 
@@ -193,6 +194,7 @@
   file_handle->refcount = 1;
   file_handle->mutex = g_mutex_new ();
   file_handle->op = FILE_OP_NONE;
+  file_handle->path = g_strdup (path);
 
   return file_handle;
 }
@@ -204,10 +206,22 @@
   return file_handle;
 }
 
-static gboolean
+static void
 file_handle_unref (FileHandle *file_handle)
 {
-  return g_atomic_int_dec_and_test (&file_handle->refcount);
+  if (g_atomic_int_dec_and_test (&file_handle->refcount))
+    {
+      g_static_mutex_lock (&global_mutex);
+
+      /* Test again, since e.g. get_file_handle_for_path() might have
+       * snatched the global mutex and revived the file handle between
+       * g_atomic_int_dec_and_test() and us obtaining the lock. */
+
+      if (g_atomic_int_get (&file_handle->refcount) == 0)
+        g_hash_table_remove (global_fh_table, file_handle->path);
+
+      g_static_mutex_unlock (&global_mutex);
+    }
 }
 
 static void
@@ -236,11 +250,13 @@
     }
 }
 
+/* Called on hash table removal */
 static void
 file_handle_free (FileHandle *file_handle)
 {
   file_handle_close_stream (file_handle);
   g_mutex_free (file_handle->mutex);
+  g_free (file_handle->path);
   g_free (file_handle);
 }
 
@@ -250,7 +266,11 @@
   FileHandle *fh;
 
   g_static_mutex_lock (&global_mutex);
+
   fh = g_hash_table_lookup (global_fh_table, path);
+  if (fh)
+    file_handle_ref (fh);
+
   g_static_mutex_unlock (&global_mutex);
 
   return fh;
@@ -261,16 +281,22 @@
 {
   FileHandle *fh;
 
-  fh = get_file_handle_for_path (path);
-  if (!fh)
-    {
-      fh = file_handle_new ();
+  g_static_mutex_lock (&global_mutex);
 
-      g_static_mutex_lock (&global_mutex);
-      g_hash_table_insert (global_fh_table, g_strdup (path), fh);
-      g_static_mutex_unlock (&global_mutex);
+  fh = g_hash_table_lookup (global_fh_table, path);
+
+  if (fh)
+    {
+      file_handle_ref (fh);
+    }
+  else
+    {
+      fh = file_handle_new (path);
+      g_hash_table_insert (global_fh_table, fh->path, fh);
     }
 
+  g_static_mutex_unlock (&global_mutex);
+
   return fh;
 }
 
@@ -287,28 +313,15 @@
                                      (gpointer *) &fh))
       goto out;
 
-  g_free (old_path_internal);
-  g_hash_table_insert (global_fh_table, g_strdup (new_path), fh);
+  g_free (fh->path);
+  fh->path = g_strdup (new_path);
+  g_hash_table_steal (global_fh_table, old_path);
+  g_hash_table_insert (global_fh_table, fh->path, fh);
 
  out:
   g_static_mutex_unlock (&global_mutex);
 }
 
-static void
-free_file_handle_for_path (const gchar *path)
-{
-  FileHandle *fh;
-
-  g_static_mutex_lock (&global_mutex);
-  fh = g_hash_table_lookup (global_fh_table, path);
-  if (fh)
-    {
-      if (file_handle_unref (fh))
-        g_hash_table_remove (global_fh_table, path);
-    }
-  g_static_mutex_unlock (&global_mutex);
-}
-
 static MountRecord *
 mount_record_new (GMount *mount)
 {
@@ -809,7 +822,10 @@
     }
 
   if (fh)
-    g_mutex_unlock (fh->mutex);
+    {
+      g_mutex_unlock (fh->mutex);
+      file_handle_unref (fh);
+    }
 
   debug_print ("vfs_getattr: -> %s\n", strerror (-result));
 
@@ -937,7 +953,6 @@
               
               /* File exists */
 
-              file_handle_ref (fh);
               SET_FILE_HANDLE (fi, fh);
 
               debug_print ("vfs_open: flags=%o\n", fi->flags);
@@ -948,6 +963,8 @@
                 result = setup_output_stream (file, fh);
               else
                 result = setup_input_stream (file, fh);
+
+              /* The added reference to the file handle is released in vfs_release() */
             }
           else if (file_type == G_FILE_TYPE_DIRECTORY)
             {
@@ -1028,13 +1045,14 @@
 
               /* Success */
 
-              file_handle_ref (fh);
               SET_FILE_HANDLE (fi, fh);
 
               g_assert (fh->stream == NULL);
 
               fh->stream = file_output_stream;
               fh->op = FILE_OP_WRITE;
+
+              /* The added reference to the file handle is released in vfs_release() */
             }
           else
             {
@@ -1063,10 +1081,7 @@
   debug_print ("vfs_release: %s\n", path);
 
   if (fh)
-    {
-      if (!file_handle_unref (fh))
-        free_file_handle_for_path (path);
-    }
+    file_handle_unref (fh);
 
   return 0;
 }
@@ -1498,6 +1513,7 @@
       if (fh)
         {
           g_mutex_unlock (fh->mutex);
+          file_handle_unref (fh);
         }
 
       if (result == -EISDIR)
@@ -1549,6 +1565,7 @@
       if (fh)
         {
           g_mutex_unlock (fh->mutex);
+          file_handle_unref (fh);
         }
 
       if (error)
@@ -1767,7 +1784,10 @@
         }
 
       if (fh)
-        g_mutex_unlock (fh->mutex);
+        {
+          g_mutex_unlock (fh->mutex);
+          file_handle_unref (fh);
+        }
 
       g_object_unref (file);
     }
@@ -2074,7 +2094,7 @@
 
   mount_list_mutex = g_mutex_new ();
   global_fh_table = g_hash_table_new_full (g_str_hash, g_str_equal,
-                                           g_free, (GDestroyNotify) file_handle_free);
+                                           NULL, (GDestroyNotify) file_handle_free);
 
 	dbus_error_init (&error);