diff options
author | Hans Petter Jansson <hpj@novell.com> | 2008-05-23 03:21:17 +0000 |
---|---|---|
committer | Hans Petter <hansp@src.gnome.org> | 2008-05-23 03:21:17 +0000 |
commit | b12af08b665428134acf30839c5a363b104a2094 (patch) | |
tree | 270efc6ec03248d9704770c545ba2740cd6df4db | |
parent | 1569a35e7d8c3ad60ebfd92986aee0fa28747cf8 (diff) | |
download | gvfs-b12af08b665428134acf30839c5a363b104a2094.tar.gz |
Fix fuse daemon locking and file handle life-cycle issues that wereGVFS_0_2_4
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.
svn path=/trunk/; revision=1783
-rw-r--r-- | ChangeLog | 30 | ||||
-rw-r--r-- | client/gvfsfusedaemon.c | 92 |
2 files changed, 86 insertions, 36 deletions
@@ -1,5 +1,35 @@ 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. + +2008-05-22 Hans Petter Jansson <hpj@novell.com> + A more complete fix for GNOME bug #531516. * client/gvfsfusedaemon.c (subthread_main): Send SIGHUP to the diff --git a/client/gvfsfusedaemon.c b/client/gvfsfusedaemon.c index d59ed3cc..b59ab4ab 100644 --- a/client/gvfsfusedaemon.c +++ b/client/gvfsfusedaemon.c @@ -72,6 +72,7 @@ typedef struct { gint refcount; GMutex *mutex; + gchar *path; FileOp op; gpointer stream; gint length; @@ -183,7 +184,7 @@ errno_from_error (GError *error) } static FileHandle * -file_handle_new (void) +file_handle_new (const gchar *path) { FileHandle *file_handle; @@ -191,6 +192,7 @@ file_handle_new (void) 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; } @@ -202,10 +204,22 @@ file_handle_ref (FileHandle *file_handle) 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 @@ -234,11 +248,13 @@ file_handle_close_stream (FileHandle *file_handle) } } +/* 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); } @@ -248,7 +264,11 @@ get_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) + file_handle_ref (fh); + g_static_mutex_unlock (&global_mutex); return fh; @@ -259,16 +279,22 @@ get_or_create_file_handle_for_path (const gchar *path) { 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; } @@ -285,28 +311,15 @@ reindex_file_handle_for_path (const gchar *old_path, const gchar *new_path) (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) { @@ -807,7 +820,10 @@ vfs_getattr (const gchar *path, struct stat *sbuf) } if (fh) - g_mutex_unlock (fh->mutex); + { + g_mutex_unlock (fh->mutex); + file_handle_unref (fh); + } debug_print ("vfs_getattr: -> %s\n", strerror (-result)); @@ -935,7 +951,6 @@ vfs_open (const gchar *path, struct fuse_file_info *fi) /* File exists */ - file_handle_ref (fh); SET_FILE_HANDLE (fi, fh); debug_print ("vfs_open: flags=%o\n", fi->flags); @@ -946,6 +961,8 @@ vfs_open (const gchar *path, struct fuse_file_info *fi) 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) { @@ -1026,13 +1043,14 @@ vfs_create (const gchar *path, mode_t mode, struct fuse_file_info *fi) /* 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 { @@ -1061,10 +1079,7 @@ vfs_release (const gchar *path, struct fuse_file_info *fi) 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; } @@ -1496,6 +1511,7 @@ vfs_rename (const gchar *old_path, const gchar *new_path) if (fh) { g_mutex_unlock (fh->mutex); + file_handle_unref (fh); } if (result == -EISDIR) @@ -1547,6 +1563,7 @@ vfs_unlink (const gchar *path) if (fh) { g_mutex_unlock (fh->mutex); + file_handle_unref (fh); } if (error) @@ -1765,7 +1782,10 @@ vfs_truncate (const gchar *path, off_t size) } if (fh) - g_mutex_unlock (fh->mutex); + { + g_mutex_unlock (fh->mutex); + file_handle_unref (fh); + } g_object_unref (file); } @@ -2072,7 +2092,7 @@ vfs_init (struct fuse_conn_info *conn) 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); |