Re: [PATCH 1/5] filesystem: refactor search function



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]