Re: [PATCH 1/5] filesystem: refactor search function
- From: Iago Toral Quiroga <itoral igalia com>
- To: grilo-list gnome org
- Subject: Re: [PATCH 1/5] filesystem: refactor search function
- Date: Thu, 10 Feb 2011 12:28:51 +0100
I am ok with the overall idea, but I found a crash while testing. The
crash only shows up with this patch. Please look into it before pushing
this change.
FWIW, I can reproduce the crash in my system 100% of the times with this
patch applied when I use the search string "a" or "ab" (for example) but
with other search strings it worked ok, I guess the bugs depends on the
specific files that pass the search filter.
For reference, here is the backtrace:
Program received signal SIGSEGV, Segmentation fault.
(gdb) bt
#0 0xb78cc111 in IA__g_utf8_casefold (str=0x706f5f65 <Address
0x706f5f65 out of bounds>, len=-1)
at /build/buildd/glib2.0-2.20.1/glib/guniprop.c:1176
#1 0xb5a52cfc in file_cb (file_info=0x9352560, operation=0x934ef68) at
grl-filesystem.c:866
#2 0xb5a52838 in recursive_operation_got_file (enumerator=0x9385410,
res=0x934d030, operation=0x934ef68) at grl-filesystem.c:727
#3 0xb798586e in next_async_callback_wrapper (source_object=0x9385410,
res=0x934d030, user_data=0x934ef68)
at /build/buildd/glib2.0-2.20.1/gio/gfileenumerator.c:299
#4 0xb799a0d2 in IA__g_simple_async_result_complete (simple=0x934d030)
at /build/buildd/glib2.0-2.20.1/gio/gsimpleasyncresult.c:567
#5 0xb799a3ce in complete_in_idle_cb_for_thread (_data=0x9350398)
at /build/buildd/glib2.0-2.20.1/gio/gsimpleasyncresult.c:629
#6 0xb789ac81 in g_idle_dispatch (source=0x9350650,
callback=0xffffffff, user_data=0x9350398)
at /build/buildd/glib2.0-2.20.1/glib/gmain.c:3922
#7 0xb789cb88 in IA__g_main_context_dispatch (context=0x9196518)
at /build/buildd/glib2.0-2.20.1/glib/gmain.c:1814
#8 0xb78a00eb in g_main_context_iterate (context=0x9196518, block=1,
dispatch=1, self=0x917bf88)
at /build/buildd/glib2.0-2.20.1/glib/gmain.c:2448
#9 0xb78a05ba in IA__g_main_loop_run (loop=0x9329f50)
at /build/buildd/glib2.0-2.20.1/glib/gmain.c:2656
#10 0xb7de47d9 in IA__gtk_main () at /build/buildd/gtk
+2.0-2.16.1/gtk/gtkmain.c:1205
#11 0x080506ec in main (argc=1, argv=0xbfeba024) at main.c:1800
Iago
El mié, 09-02-2011 a las 17:57 +0100, Juan A. Suarez Romero escribió:
> Split specific code from searching (checking if files match the search term and
> send them) from the general process of crawling directories and files
> recursively.
>
> This way we can reuse the crawling code to do other operations on files an
> directories.
> ---
> src/filesystem/grl-filesystem.c | 375 +++++++++++++++++++++------------------
> 1 files changed, 203 insertions(+), 172 deletions(-)
>
> diff --git a/src/filesystem/grl-filesystem.c b/src/filesystem/grl-filesystem.c
> index eb3df03..a482ef6 100644
> --- a/src/filesystem/grl-filesystem.c
> +++ b/src/filesystem/grl-filesystem.c
> @@ -83,6 +83,11 @@ struct _GrlFilesystemSourcePrivate {
>
> /* --- Data types --- */
>
> +typedef struct _RecursiveOperation RecursiveOperation;
> +
> +typedef gboolean (*RecursiveOperationCb) (GFileInfo *file_info,
> + RecursiveOperation *operation);
> +
> typedef struct {
> GrlMediaSourceBrowseSpec *spec;
> GList *entries;
> @@ -93,23 +98,23 @@ typedef struct {
> guint id;
> } BrowseIdleData;
>
> -/* probably not thread-safe */
> -typedef struct {
> - GrlMediaSource *source;
> - GrlMediaSourceSearchSpec *ss;
> - guint found_count;
> - guint skipped;
> - guint max_depth;
> - GQueue *trees;
> +struct _RecursiveOperation {
> + RecursiveOperationCb on_cancel;
> + RecursiveOperationCb on_finish;
> + RecursiveOperationCb on_dir;
> + RecursiveOperationCb on_file;
> + gpointer on_dir_data;
> + gpointer on_file_data;
> GCancellable *cancellable;
> - guint id;
> -} SearchOperation;
> + GQueue *directories;
> + guint max_depth;
> +};
>
> typedef struct {
> - SearchOperation *operation;
> guint depth;
> GFile *directory;
> -} SearchTree;
> +} RecursiveEntry;
> +
>
> static GrlFilesystemSource *grl_filesystem_source_new (void);
>
> @@ -235,6 +240,8 @@ grl_filesystem_source_finalize (GObject *object)
>
> /* ======================= Utilities ==================== */
>
> +static void recursive_operation_next_entry (RecursiveOperation *operation);
> +
> static gboolean
> mime_is_video (const gchar *mime)
> {
> @@ -620,174 +627,107 @@ produce_from_path (GrlMediaSourceBrowseSpec *bs, const gchar *path)
> g_dir_close (dir);
> }
>
> -/*** search stuff ***/
> -
> -static void search_next_directory (SearchOperation *operation);
> -
> -static SearchTree *
> -search_tree_new (SearchOperation *operation, guint depth, GFile *directory)
> +static RecursiveEntry *
> +recursive_entry_new (guint depth, GFile *directory)
> {
> - SearchTree *tree = g_new (SearchTree, 1);
> - tree->operation = operation;
> - tree->depth = depth;
> - tree->directory = g_object_ref (directory);
> + RecursiveEntry *entry;
>
> - return tree;
> + entry = g_slice_new(RecursiveEntry);
> + entry->depth = depth;
> + entry->directory = g_object_ref (directory);
> +
> + return entry;
> }
>
> static void
> -search_tree_free (SearchTree *tree)
> +recursive_entry_free (RecursiveEntry *entry)
> {
> - g_object_unref (tree->directory);
> - g_free (tree);
> + g_object_unref (entry->directory);
> + g_slice_free (RecursiveEntry, entry);
> }
>
> -static SearchOperation *
> -search_operation_new (GrlMediaSource *source, GrlMediaSourceSearchSpec *ss, guint max_depth)
> +static RecursiveOperation *
> +recursive_operation_new ()
> {
> - SearchOperation *operation;
> -
> - operation = g_new (SearchOperation, 1);
> - operation->source = source;
> - operation->id = ss->search_id;
> - operation->ss = ss;
> - operation->found_count = 0;
> - operation->skipped = 0;
> - operation->max_depth = max_depth;
> - operation->trees = g_queue_new ();
> - operation->cancellable = g_cancellable_new ();
> + RecursiveOperation *operation;
>
> - g_hash_table_insert (GRL_FILESYSTEM_SOURCE (source)->priv->cancellables,
> - GUINT_TO_POINTER (operation->id),
> - operation->cancellable);
> + operation = g_slice_new0 (RecursiveOperation);
> + operation->directories = g_queue_new ();
> + operation->cancellable = g_cancellable_new ();
>
> return operation;
> }
>
> static void
> -search_operation_free (SearchOperation *operation)
> +recursive_operation_free (RecursiveOperation *operation)
> {
> - GrlFilesystemSource *source;
> - source = GRL_FILESYSTEM_SOURCE (operation->source);
> -
> - g_queue_foreach (operation->trees, (GFunc)search_tree_free, NULL);
> - g_queue_free (operation->trees);
> - g_hash_table_remove (source->priv->cancellables,
> - GUINT_TO_POINTER (operation->id));
> + g_queue_foreach (operation->directories, (GFunc) recursive_entry_free, NULL);
> + g_queue_free (operation->directories);
> g_object_unref (operation->cancellable);
> - g_free (operation);
> -}
> -
> -/* return TRUE if more files need to be returned, FALSE if we sent the count */
> -static gboolean
> -compare_and_return_file (SearchOperation *operation,
> - GFileEnumerator *enumerator,
> - GFileInfo *file_info)
> -{
> - gchar *needle = NULL;
> - gchar *haystack = NULL;
> - gchar *normalized_needle = NULL;
> - gchar *normalized_haystack = NULL;
> - GrlMediaSourceSearchSpec *ss = operation->ss;
> -
> - GRL_DEBUG ("compare_and_return_file");
> -
> - if (ss == NULL)
> - return FALSE;
> -
> - if (ss->text) {
> - haystack = g_utf8_casefold (g_file_info_get_display_name (file_info), -1);
> - normalized_haystack = g_utf8_normalize (haystack, -1, G_NORMALIZE_ALL);
> -
> - needle = g_utf8_casefold (ss->text, -1);
> - normalized_needle = g_utf8_normalize (needle, -1, G_NORMALIZE_ALL);
> - }
> -
> - if (!ss->text ||
> - strstr (normalized_haystack, normalized_needle)) {
> - GrlMedia *media = NULL;
> - GFile *dir, *file;
> - gchar *path;
> - gint remaining = -1;
> -
> - dir = g_file_enumerator_get_container (enumerator);
> - file = g_file_get_child (dir, g_file_info_get_name (file_info));
> - path = g_file_get_path (file);
> -
> - /* FIXME: both file_is_valid_content() and create_content() are likely to block */
> - if (file_is_valid_content (path, FALSE)) {
> - if (operation->skipped < ss->skip)
> - operation->skipped++;
> - else
> - media = create_content (NULL, path, ss->flags & GRL_RESOLVE_FAST_ONLY, FALSE);
> - }
> -
> - g_free (path);
> - g_object_unref (file);
> -
> - if (media) {
> - operation->found_count++;
> - if (operation->found_count == ss->count)
> - remaining = 0;
> - ss->callback (ss->source, ss->search_id, media, remaining, ss->user_data, NULL);
> - if (!remaining)
> - /* after a call to the callback with remaining=0, the core will free
> - * the search spec */
> - operation->ss = NULL;
> - }
> - }
> -
> - g_free (haystack);
> - g_free (normalized_haystack);
> - g_free (needle);
> - g_free (normalized_needle);
> - return operation->ss != NULL;
> + g_slice_free (RecursiveOperation, operation);
> }
>
> static void
> -got_file (GFileEnumerator *enumerator, GAsyncResult *res, SearchTree *tree)
> +recursive_operation_got_file (GFileEnumerator *enumerator, GAsyncResult *res, RecursiveOperation *operation)
> {
> GList *files;
> GError *error = NULL;
>
> - GRL_DEBUG ("got_file");
> + GRL_DEBUG (__func__);
>
> files = g_file_enumerator_next_files_finish (enumerator, res, &error);
> if (error) {
> if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
> - GRL_WARNING ("Got error while searching: %s", error->message);
> + GRL_WARNING ("Got error: %s", error->message);
> g_error_free (error);
> goto finished;
> }
>
> if (files) {
> GFileInfo *file_info;
> + RecursiveEntry *entry;
> + gboolean continue_operation;
> +
> /* we assume there is only one GFileInfo in the list since that's what we ask
> * for when calling g_file_enumerator_next_files_async() */
> file_info = (GFileInfo *)files->data;
> g_list_free (files);
> + /* Get the entry we are running now */
> + entry = g_queue_peek_head (operation->directories);
> switch (g_file_info_get_file_type (file_info)) {
> case G_FILE_TYPE_SYMBOLIC_LINK:
> /* we're too afraid of infinite recursion to touch this for now */
> break;
> case G_FILE_TYPE_DIRECTORY:
> {
> - if (tree->depth < tree->operation->max_depth) {
> + if (entry->depth < operation->max_depth) {
> GFile *subdir;
> - SearchTree *subtree;
> -
> - subdir = g_file_get_child (tree->directory,
> - g_file_info_get_name (file_info));
> - subtree = search_tree_new (tree->operation, tree->depth + 1, subdir);
> - g_queue_push_tail (tree->operation->trees, subtree);
> - g_object_unref (subdir);
> + RecursiveEntry *subentry;
> +
> + if (operation->on_dir) {
> + continue_operation = operation->on_dir(file_info, operation);
> + } else {
> + continue_operation = TRUE;
> + }
> +
> + if (continue_operation) {
> + subdir = g_file_get_child (entry->directory,
> + g_file_info_get_name (file_info));
> + subentry = recursive_entry_new (entry->depth + 1, subdir);
> + g_queue_push_tail (operation->directories, subentry);
> + g_object_unref (subdir);
> + } else {
> + g_object_unref (file_info);
> + goto finished;
> + }
> }
> }
> break;
> case G_FILE_TYPE_REGULAR:
> - if (!compare_and_return_file (tree->operation, enumerator, file_info)){
> - g_object_unref (file_info);
> - goto finished;
> + if (operation->on_file &&
> + !operation->on_file(file_info, operation)) {
> + g_object_unref (file_info);
> + goto finished;
> }
> break;
> default:
> @@ -800,87 +740,172 @@ got_file (GFileEnumerator *enumerator, GAsyncResult *res, SearchTree *tree)
> }
>
> g_file_enumerator_next_files_async (enumerator, 1, G_PRIORITY_DEFAULT,
> - tree->operation->cancellable,
> - (GAsyncReadyCallback)got_file, tree);
> + operation->cancellable,
> + (GAsyncReadyCallback)recursive_operation_got_file,
> + operation);
>
> return;
>
> finished:
> /* we're done with this dir/enumerator, let's treat the next one */
> g_object_unref (enumerator);
> - search_next_directory (tree->operation);
> - search_tree_free (tree);
> + recursive_entry_free (g_queue_pop_head (operation->directories));
> + recursive_operation_next_entry (operation);
> }
>
> static void
> -got_children (GFile *directory, GAsyncResult *res, SearchTree *tree)
> +recursive_operation_got_entry (GFile *directory, GAsyncResult *res, RecursiveOperation *operation)
> {
> GError *error = NULL;
> GFileEnumerator *enumerator;
>
> - GRL_DEBUG ("got_children");
> + GRL_DEBUG (__func__);
>
> enumerator = g_file_enumerate_children_finish (directory, res, &error);
> if (error) {
> - GRL_WARNING ("Got error while searching: %s", error->message);
> + GRL_WARNING ("Got error: %s", error->message);
> g_error_free (error);
> g_object_unref (enumerator);
> /* we couldn't get the children of this directory, but we probably have
> * other directories to try */
> - search_next_directory (tree->operation);
> - search_tree_free (tree);
> + recursive_entry_free (g_queue_pop_head (operation->directories));
> + recursive_operation_next_entry (operation);
> return;
> }
>
> g_file_enumerator_next_files_async (enumerator, 1, G_PRIORITY_DEFAULT,
> - tree->operation->cancellable,
> - (GAsyncReadyCallback)got_file, tree);
> + operation->cancellable,
> + (GAsyncReadyCallback)recursive_operation_got_file,
> + operation);
> }
>
> static void
> -search_next_directory (SearchOperation *operation)
> +recursive_operation_next_entry (RecursiveOperation *operation)
> {
> - SearchTree *tree;
> - GrlMediaSourceSearchSpec *ss = operation->ss;
> -
> - GRL_DEBUG ("search_next_directory");
> + RecursiveEntry *entry;
>
> - if (!ss) { /* count reached, last callback call done */
> - goto finished;
> - }
> + GRL_DEBUG (__func__);
>
> if (g_cancellable_is_cancelled (operation->cancellable)) {
> /* We've been cancelled! */
> - /* FIXME: set here a cancelled error when we have one */
> - GRL_DEBUG ("Search operation %d (\"%s\") has been cancelled",
> - operation->id, ss->text);
> - ss->callback (ss->source, ss->search_id, NULL, 0, ss->user_data, NULL);
> - operation->ss = NULL;
> + GRL_DEBUG ("Operation has been cancelled");
> + if (operation->on_cancel) {
> + operation->on_cancel(NULL, operation);
> + }
> goto finished;
> }
>
> - tree = g_queue_pop_head (operation->trees);
> - if (!tree) { /* We've crawled everything, before reaching count */
> - ss->callback (ss->source, ss->search_id, NULL, 0, ss->user_data, NULL);
> - operation->ss = NULL;
> + entry = g_queue_peek_head (operation->directories);
> + if (!entry) { /* We've crawled everything, before reaching count */
> + if (operation->on_finish) {
> + operation->on_finish (NULL, operation);
> + }
> goto finished;
> }
>
> - g_file_enumerate_children_async (tree->directory, G_FILE_ATTRIBUTE_STANDARD_TYPE ","
> + g_file_enumerate_children_async (entry->directory, G_FILE_ATTRIBUTE_STANDARD_TYPE ","
> G_FILE_ATTRIBUTE_STANDARD_NAME ","
> G_FILE_ATTRIBUTE_STANDARD_DISPLAY_NAME,
> G_FILE_QUERY_INFO_NOFOLLOW_SYMLINKS,
> G_PRIORITY_DEFAULT,
> operation->cancellable,
> - (GAsyncReadyCallback)got_children,
> - tree);
> + (GAsyncReadyCallback)recursive_operation_got_entry,
> + operation);
>
> return;
>
> finished:
> - search_operation_free (operation);
> + recursive_operation_free (operation);
> }
>
> +static gboolean
> +cancel_cb (GFileInfo *file_info, RecursiveOperation *operation)
> +{
> + if (operation->on_file_data) {
> + GrlMediaSourceSearchSpec *ss =
> + (GrlMediaSourceSearchSpec *) operation->on_file_data;
> + g_hash_table_remove (GRL_FILESYSTEM_SOURCE (ss->source)->priv->cancellables,
> + GUINT_TO_POINTER (ss->search_id));
> + ss->callback (ss->source, ss->search_id, NULL, 0, ss->user_data, NULL);
> + }
> +
> + return FALSE;
> +}
> +
> +static gboolean
> +finish_cb (GFileInfo *file_info, RecursiveOperation *operation)
> +{
> + if (operation->on_file_data) {
> + GrlMediaSourceSearchSpec *ss =
> + (GrlMediaSourceSearchSpec *) operation->on_file_data;
> + g_hash_table_remove (GRL_FILESYSTEM_SOURCE (ss->source)->priv->cancellables,
> + GUINT_TO_POINTER (ss->search_id));
> + ss->callback (ss->source, ss->search_id, NULL, 0, ss->user_data, NULL);
> + }
> +
> + return FALSE;
> +}
> +
> +/* return TRUE if more files need to be returned, FALSE if we sent the count */
> +static gboolean
> +file_cb (GFileInfo *file_info, RecursiveOperation *operation)
> +{
> + gchar *needle, *haystack, *normalized_needle, *normalized_haystack;
> + GrlMediaSourceSearchSpec *ss = operation->on_file_data;
> + gint remaining = -1;
> +
> + GRL_DEBUG (__func__);
> +
> + if (ss == NULL)
> + return FALSE;
> +
> + if (ss->text) {
> + haystack = g_utf8_casefold (g_file_info_get_display_name (file_info), -1);
> + normalized_haystack = g_utf8_normalize (haystack, -1, G_NORMALIZE_ALL);
> +
> + needle = g_utf8_casefold (ss->text, -1);
> + normalized_needle = g_utf8_normalize (needle, -1, G_NORMALIZE_ALL);
> + }
> +
> + if (!ss->text ||
> + strstr (normalized_haystack, normalized_needle)) {
> + GrlMedia *media = NULL;
> + RecursiveEntry *entry;
> + GFile *file;
> + gchar *path;
> +
> + entry = g_queue_peek_head (operation->directories);
> + file = g_file_get_child (entry->directory,
> + g_file_info_get_name (file_info));
> + path = g_file_get_path (file);
> +
> + /* FIXME: both file_is_valid_content() and create_content() are likely to block */
> + if (file_is_valid_content (path, FALSE)) {
> + if (ss->skip) {
> + ss->skip--;
> + } else {
> + media = create_content (NULL, path, ss->flags & GRL_RESOLVE_FAST_ONLY, FALSE);
> + }
> + }
> +
> + g_free (path);
> + g_object_unref (file);
> +
> + if (media) {
> + ss->count--;
> + if (ss->count == 0) {
> + remaining = 0;
> + }
> + ss->callback (ss->source, ss->search_id, media, remaining, ss->user_data, NULL);
> + }
> + }
> +
> + g_free (haystack);
> + g_free (normalized_haystack);
> + g_free (needle);
> + g_free (normalized_needle);
> + return remaining == -1;
> +}
>
> /* ================== API Implementation ================ */
>
> @@ -939,24 +964,30 @@ grl_filesystem_source_browse (GrlMediaSource *source,
> static void grl_filesystem_source_search (GrlMediaSource *source,
> GrlMediaSourceSearchSpec *ss)
> {
> - SearchOperation *operation;
> + RecursiveOperation *operation;
> GList *chosen_paths, *path;
> - guint max_search_depth;
> GrlFilesystemSource *fs_source;
>
> GRL_DEBUG ("grl_filesystem_source_search");
>
> fs_source = GRL_FILESYSTEM_SOURCE (source);
>
> - max_search_depth = fs_source->priv->max_search_depth;
> - operation = search_operation_new (source, ss, max_search_depth);
> + operation = recursive_operation_new ();
> + operation->on_cancel = cancel_cb;
> + operation->on_finish = finish_cb;
> + operation->on_file = file_cb;
> + operation->on_file_data = ss;
> + operation->max_depth = fs_source->priv->max_search_depth;
> + g_hash_table_insert (GRL_FILESYSTEM_SOURCE (source)->priv->cancellables,
> + GUINT_TO_POINTER (ss->search_id),
> + operation->cancellable);
>
> chosen_paths = fs_source->priv->chosen_paths;
> if (chosen_paths) {
> for (path = chosen_paths; path; path = g_list_next (path)) {
> GFile *directory = g_file_new_for_path (path->data);
> - g_queue_push_tail (operation->trees,
> - search_tree_new (operation, 0, directory));
> + g_queue_push_tail (operation->directories,
> + recursive_entry_new (0, directory));
> g_object_unref (directory);
> }
> } else {
> @@ -965,12 +996,12 @@ static void grl_filesystem_source_search (GrlMediaSource *source,
>
> home = g_getenv ("HOME");
> directory = g_file_new_for_path (home);
> - g_queue_push_tail (operation->trees,
> - search_tree_new (operation, 0, directory));
> + g_queue_push_tail (operation->directories,
> + recursive_entry_new (0, directory));
> g_object_unref (directory);
> }
>
> - search_next_directory (operation);
> + recursive_operation_next_entry (operation);
> }
>
> static void
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
