[libmediaart] cache: Improve _remove() function and fix crash with NULL passed

[Martyn James Russell <mr src gnome org>]

commit e258350a8abb694b2fcd201d72a1a5e2e8c8a6a1
Author: Martyn Russell <martyn lanedo com>
Date:   Fri Feb 7 17:16:31 2014 +0000

    cache: Improve _remove() function and fix crash with NULL passed

 libmediaart/cache.c  |   78 +++++++++++++++++++++++++++++---------------------
 tests/mediaarttest.c |    3 +-
 2 files changed, 46 insertions(+), 35 deletions(-)
---
diff --git a/libmediaart/cache.c b/libmediaart/cache.c
index 2e29bae..7249b14 100644
--- a/libmediaart/cache.c
+++ b/libmediaart/cache.c
@@ -394,6 +394,24 @@ media_art_get_path (const gchar  *artist,
        }
 }
 
+static void
+media_art_remove_foreach (gpointer data,
+                          gpointer user_data)
+{
+       gchar *filename = data;
+       gboolean total_success = * (gboolean *) user_data;
+       gboolean success;
+
+       success = g_unlink (filename) == 0;
+       total_success &= success;
+
+       if (!success) {
+               g_warning ("Could not delete file '%s'", filename);
+       }
+
+       g_free (filename);
+}
+
 /**
  * media_art_remove:
  * @artist: artist the media art belongs to
@@ -416,32 +434,28 @@ media_art_remove (const gchar *artist,
        gchar *dirname;
        GList *to_remove = NULL;
        gchar *target = NULL;
-       gchar *album_path = NULL;
+       gboolean success = TRUE;
 
        g_return_val_if_fail (artist != NULL && artist[0] != '\0', FALSE);
 
        dirname = g_build_filename (g_get_user_cache_dir (), "media-art", NULL);
 
-       if (!g_file_test (dirname, G_FILE_TEST_EXISTS)) {
-               /* Ignore this and just quit the function */
-               g_debug ("Nothing to do, media-art cache directory '%s' doesn't exist", dirname);
-               g_free (dirname);
-               return TRUE;
-       }
-
        dir = g_dir_open (dirname, 0, &error);
+       if (!dir || error) {
+               /* Nothing to do if there is no directory in the first place. */
+               g_debug ("Removing media-art for artist:'%s', album:'%s': directory could not be opened, %s",
+                        artist, album, error ? error->message : "no error given");
 
-       if (error) {
-               g_critical ("Call to g_dir_open() failed, %s", error->message);
-
-               g_error_free (error);
-               g_free (dirname);
-
+               g_clear_error (&error);
                if (dir) {
                        g_dir_close (dir);
                }
+               g_free (dirname);
 
-               return FALSE;
+               /* We wanted to remove media art, so if there is no
+                * media art, the caller has achieved what they wanted.
+                */
+               return TRUE;
        }
 
        table = g_hash_table_new_full (g_str_hash,
@@ -451,11 +465,15 @@ media_art_remove (const gchar *artist,
 
        /* The get_path API does stripping itself */
        media_art_get_path (artist, album, "album", NULL, &target, NULL);
-       g_hash_table_replace (table, target, target);
+       if (target) {
+               g_hash_table_replace (table, target, target);
+       }
 
-       /* Also add the file to which the symlinks are made */
-       media_art_get_path (NULL, album, "album", NULL, &album_path, NULL);
-       g_hash_table_replace (table, album_path, album_path);
+       /* Add the album path also (to which the symlinks are made) */
+       media_art_get_path (NULL, album, "album", NULL, &target, NULL);
+       if (target) {
+               g_hash_table_replace (table, target, target);
+       }
 
        /* Perhaps we should have an internal list of media art files that we made,
         * instead of going over all the media art (which could also have been made
@@ -465,30 +483,24 @@ media_art_remove (const gchar *artist,
                gchar *full;
 
                full = g_build_filename (dirname, name, NULL);
-
                value = g_hash_table_lookup (table, full);
 
                if (!value) {
-                       g_message ("Removing media-art file '%s': no album exists with songs for this 
media-art cache", name);
+                       g_message ("Removing media-art for artist:'%s', album:'%s': deleting file '%s'",
+                                  artist, album, name);
                        to_remove = g_list_prepend (to_remove, (gpointer) full);
                } else {
                        g_free (full);
                }
        }
 
-       if (dir) {
-               g_dir_close (dir);
-       }
-
-       g_free (dirname);
-
-       g_list_foreach (to_remove, (GFunc) g_unlink, NULL);
-       g_list_foreach (to_remove, (GFunc) g_free, NULL);
+       g_list_foreach (to_remove, media_art_remove_foreach, &success);
        g_list_free (to_remove);
 
-       if (table) {
-               g_hash_table_unref (table);
-       }
+       g_hash_table_unref (table);
+
+       g_dir_close (dir);
+       g_free (dirname);
 
-       return TRUE;
+       return success;
 }
diff --git a/tests/mediaarttest.c b/tests/mediaarttest.c
index 77a5a42..3690335 100644
--- a/tests/mediaarttest.c
+++ b/tests/mediaarttest.c
@@ -284,8 +284,7 @@ test_mediaart_png (void)
         retval = media_art_remove ("Lanedo", "");
         g_assert_true (retval);
 
-        /* FIXME: This breaks, passing NULL for second param */
-        /* retval = media_art_remove ("Lanedo", NULL); */
+        retval = media_art_remove ("Lanedo", NULL);
 
         g_object_unref (file);
         g_free (path);