directory: require the caller to lock the db_mutex

Reduce the number of lock/unlock cycles, and make database handling
safer.

src/command.c

@@ -45,6 +45,7 @@
 #include "db_error.h"
 #include "db_print.h"
 #include "db_selection.h"
+#include "db_lock.h"
 #include "tag.h"
 #include "client.h"
 #include "client_idle.h"
@@ -1892,8 +1893,10 @@ handle_sticker_song(struct client *client, int argc, char *argv[])
 			.name = argv[4],
 		};
 
+		db_lock();
 		directory = db_get_directory(argv[3]);
 		if (directory == NULL) {
+			db_unlock();
 			command_error(client, ACK_ERROR_NO_EXIST,
 				      "no such directory");
 			return COMMAND_RETURN_ERROR;
@@ -1901,6 +1904,7 @@ handle_sticker_song(struct client *client, int argc, char *argv[])
 
 		success = sticker_song_find(directory, data.name,
 					    sticker_song_find_print_cb, &data);
+		db_unlock();
 		if (!success) {
 			command_error(client, ACK_ERROR_SYSTEM,
 				      "failed to set search sticker database");

src/database.c

@@ -92,7 +92,9 @@ db_get_directory(const char *name)
 	if (name == NULL)
 		return music_root;
 
-	return directory_lookup_directory(music_root, name);
+	struct directory *directory =
+		directory_lookup_directory(music_root, name);
+	return directory;
 }
 
 struct song *

src/database.h

@@ -50,6 +50,9 @@ db_finish(void);
 struct directory *
 db_get_root(void);
 
+/**
+ * Caller must lock the #db_mutex.
+ */
 gcc_nonnull(1)
 struct directory *
 db_get_directory(const char *name);

src/db/simple_db_plugin.c

@@ -59,7 +59,11 @@ simple_db_lookup_directory(const struct simple_db *db, const char *uri)
 	assert(db->root != NULL);
 	assert(uri != NULL);
 
-	return directory_lookup_directory(db->root, uri);
+	db_lock();
+	struct directory *directory =
+		directory_lookup_directory(db->root, uri);
+	db_unlock();
+	return directory;
 }
 
 static struct db *
@@ -236,7 +240,9 @@ simple_db_get_song(struct db *_db, const char *uri, GError **error_r)
 
 	assert(db->root != NULL);
 
+	db_lock();
 	struct song *song = directory_lookup_song(db->root, uri);
+	db_unlock();
 	if (song == NULL)
 		g_set_error(error_r, db_quark(), DB_NOT_FOUND,
 			    "No such song: %s", uri);
@@ -301,13 +307,16 @@ simple_db_save(struct db *_db, GError **error_r)
 	struct simple_db *db = (struct simple_db *)_db;
 	struct directory *music_root = db->root;
 
+	db_lock();
+
 	g_debug("removing empty directories from DB");
 	directory_prune_empty(music_root);
 
 	g_debug("sorting DB");
-
 	directory_sort(music_root);
 
+	db_unlock();
+
 	g_debug("writing DB");
 
 	FILE *fp = fopen(db->path, "w");

src/db_save.c

@@ -19,6 +19,7 @@
 
 #include "config.h"
 #include "db_save.h"
+#include "db_lock.h"
 #include "directory.h"
 #include "directory_save.h"
 #include "song.h"
@@ -169,7 +170,9 @@ db_load_internal(FILE *fp, struct directory *music_root, GError **error)
 
 	g_debug("reading DB");
 
+	db_lock();
 	success = directory_load(fp, music_root, buffer, error);
+	db_unlock();
 	g_string_free(buffer, true);
 
 	return success;

src/directory.c

@@ -74,13 +74,11 @@ directory_free(struct directory *directory)
 void
 directory_delete(struct directory *directory)
 {
+	assert(holding_db_lock());
 	assert(directory != NULL);
 	assert(directory->parent != NULL);
 
-	db_lock();
 	list_del(&directory->siblings);
-	db_unlock();
-
 	directory_free(directory);
 }
 
@@ -93,6 +91,7 @@ directory_get_name(const struct directory *directory)
 struct directory *
 directory_new_child(struct directory *parent, const char *name_utf8)
 {
+	assert(holding_db_lock());
 	assert(parent != NULL);
 	assert(name_utf8 != NULL);
 	assert(*name_utf8 != 0);
@@ -111,32 +110,28 @@ directory_new_child(struct directory *parent, const char *name_utf8)
 	struct directory *directory = directory_new(path_utf8, parent);
 	g_free(allocated);
 
-	db_lock();
-	list_add_tail(&directory->siblings, &parent->children);
-	db_unlock();
+	list_add(&directory->siblings, &parent->children);
 	return directory;
 }
 
 struct directory *
 directory_get_child(const struct directory *directory, const char *name)
 {
-	db_lock();
+	assert(holding_db_lock());
 
 	struct directory *child;
-	directory_for_each_child(child, directory) {
-		if (strcmp(directory_get_name(child), name) == 0) {
-			db_unlock();
+	directory_for_each_child(child, directory)
+		if (strcmp(directory_get_name(child), name) == 0)
 			return child;
-		}
-	}
 
-	db_unlock();
 	return NULL;
 }
 
 void
 directory_prune_empty(struct directory *directory)
 {
+	assert(holding_db_lock());
+
 	struct directory *child, *n;
 	directory_for_each_child_safe(child, n, directory) {
 		directory_prune_empty(child);
@@ -149,12 +144,14 @@ directory_prune_empty(struct directory *directory)
 struct directory *
 directory_lookup_directory(struct directory *directory, const char *uri)
 {
+	assert(holding_db_lock());
 	assert(uri != NULL);
 
 	if (isRootDirectory(uri))
 		return directory;
 
 	char *duplicated = g_strdup(uri), *name = duplicated;
+
 	while (1) {
 		char *slash = strchr(name, '/');
 		if (slash == name) {
@@ -201,21 +198,18 @@ directory_remove_song(G_GNUC_UNUSED struct directory *directory,
 struct song *
 directory_get_song(const struct directory *directory, const char *name_utf8)
 {
+	assert(holding_db_lock());
 	assert(directory != NULL);
 	assert(name_utf8 != NULL);
 
-	db_lock();
 	struct song *song;
 	directory_for_each_song(song, directory) {
 		assert(song->parent == directory);
 
-		if (strcmp(song->uri, name_utf8) == 0) {
-			db_unlock();
+		if (strcmp(song->uri, name_utf8) == 0)
 			return song;
-		}
 	}
 
-	db_unlock();
 	return NULL;
 }
 
@@ -224,6 +218,7 @@ directory_lookup_song(struct directory *directory, const char *uri)
 {
 	char *duplicated, *base;
 
+	assert(holding_db_lock());
 	assert(directory != NULL);
 	assert(uri != NULL);
 
@@ -260,10 +255,10 @@ directory_cmp(G_GNUC_UNUSED void *priv,
 void
 directory_sort(struct directory *directory)
 {
-	db_lock();
+	assert(holding_db_lock());
+
 	list_sort(NULL, &directory->children, directory_cmp);
 	song_list_sort(&directory->songs);
-	db_unlock();
 
 	struct directory *child;
 	directory_for_each_child(child, directory)

src/directory.h

@@ -118,6 +118,8 @@ directory_free(struct directory *directory);
 /**
  * Remove this #directory object from its parent and free it.  This
  * must not be called with the root directory.
+ *
+ * Caller must lock the #db_mutex.
  */
 void
 directory_delete(struct directory *directory);
@@ -152,6 +154,9 @@ G_GNUC_PURE
 const char *
 directory_get_name(const struct directory *directory);
 
+/**
+ * Caller must lock the #db_mutex.
+ */
 G_GNUC_PURE
 struct directory *
 directory_get_child(const struct directory *directory, const char *name);
@@ -159,6 +164,8 @@ directory_get_child(const struct directory *directory, const char *name);
 /**
  * Create a new #directory object as a child of the given one.
  *
+ * Caller must lock the #db_mutex.
+ *
  * @param parent the parent directory the new one will be added to
  * @param name_utf8 the UTF-8 encoded name of the new sub directory
  */
@@ -169,6 +176,8 @@ directory_new_child(struct directory *parent, const char *name_utf8);
 /**
  * Look up a sub directory, and create the object if it does not
  * exist.
+ *
+ * Caller must lock the #db_mutex.
  */
 static inline struct directory *
 directory_make_child(struct directory *directory, const char *name_utf8)
@@ -179,6 +188,9 @@ directory_make_child(struct directory *directory, const char *name_utf8)
 	return child;
 }
 
+/**
+ * Caller must lock the #db_mutex.
+ */
 void
 directory_prune_empty(struct directory *directory);
 
@@ -209,6 +221,8 @@ directory_remove_song(struct directory *directory, struct song *song);
 
 /**
  * Look up a song in this directory by its name.
+ *
+ * Caller must lock the #db_mutex.
  */
 G_GNUC_PURE
 struct song *
@@ -217,6 +231,8 @@ directory_get_song(const struct directory *directory, const char *name_utf8);
 /**
  * Looks up a song by its relative URI.
  *
+ * Caller must lock the #db_mutex.
+ *
  * @param directory the parent (or grandparent, ...) directory
  * @param uri the relative URI
  * @return the song, or NULL if none was found
@@ -224,6 +240,11 @@ directory_get_song(const struct directory *directory, const char *name_utf8);
 struct song *
 directory_lookup_song(struct directory *directory, const char *uri);
 
+/**
+ * Sort all directory entries recursively.
+ *
+ * Caller must lock the #db_mutex.
+ */
 void
 directory_sort(struct directory *directory);
 

src/song_sticker.h

@@ -68,6 +68,8 @@ sticker_song_get(const struct song *song);
  * Finds stickers with the specified name below the specified
  * directory.
  *
+ * Caller must lock the #db_mutex.
+ *
  * @param directory the base directory to search in
  * @param name the name of the sticker
  * @return true on success (even if no sticker was found), false on

src/update_walk.c

@@ -20,6 +20,7 @@
 #include "config.h" /* must be first for large file support */
 #include "update_internal.h"
 #include "database.h"
+#include "db_lock.h"
 #include "exclude.h"
 #include "directory.h"
 #include "song.h"
@@ -89,6 +90,9 @@ directory_set_stat(struct directory *dir, const struct stat *st)
 	dir->have_stat = true;
 }
 
+/**
+ * Caller must lock the #db_mutex.
+ */
 static void
 delete_song(struct directory *dir, struct song *del)
 {
@@ -97,11 +101,15 @@ delete_song(struct directory *dir, struct song *del)
 	/* first, prevent traversers in main task from getting this */
 	directory_remove_song(dir, del);
 
+	db_unlock(); /* temporary unlock, because update_remove_song() blocks */
+
 	/* now take it out of the playlist (in the main_task) */
 	update_remove_song(del);
 
 	/* finally, all possible references gone, free it */
 	song_free(del);
+
+	db_lock();
 }
 
 static void
@@ -110,6 +118,8 @@ delete_directory(struct directory *directory);
 /**
  * Recursively remove all sub directories and songs from a directory,
  * leaving an empty directory.
+ *
+ * Caller must lock the #db_mutex.
  */
 static void
 clear_directory(struct directory *directory)
@@ -127,6 +137,8 @@ clear_directory(struct directory *directory)
 
 /**
  * Recursively free a directory and all its contents.
+ *
+ * Caller must lock the #db_mutex.
  */
 static void
 delete_directory(struct directory *directory)
@@ -134,12 +146,14 @@ delete_directory(struct directory *directory)
 	assert(directory->parent != NULL);
 
 	clear_directory(directory);
+
 	directory_delete(directory);
 }
 
 static void
 delete_name_in(struct directory *parent, const char *name)
 {
+	db_lock();
 	struct directory *directory = directory_get_child(parent, name);
 
 	if (directory != NULL) {
@@ -153,6 +167,8 @@ delete_name_in(struct directory *parent, const char *name)
 		modified = true;
 	}
 
+	db_unlock();
+
 	playlist_vector_remove(&parent->playlists, name);
 }
 
@@ -160,6 +176,8 @@ static void
 remove_excluded_from_directory(struct directory *directory,
 			       GSList *exclude_list)
 {
+	db_lock();
+
 	struct directory *child, *n;
 	directory_for_each_child_safe(child, n, directory) {
 		char *name_fs = utf8_to_fs_charset(directory_get_name(child));
@@ -184,6 +202,8 @@ remove_excluded_from_directory(struct directory *directory,
 
 		g_free(name_fs);
 	}
+
+	db_unlock();
 }
 
 static bool
@@ -232,7 +252,10 @@ removeDeletedFromDirectory(struct directory *directory)
 		if (directory_exists(child))
 			continue;
 
+		db_lock();
 		delete_directory(child);
+		db_unlock();
+
 		modified = true;
 	}
 
@@ -242,7 +265,10 @@ removeDeletedFromDirectory(struct directory *directory)
 		struct stat st;
 		if ((path = map_song_fs(song)) == NULL ||
 		    stat(path, &st) < 0 || !S_ISREG(st.st_mode)) {
+			db_lock();
 			delete_song(directory, song);
+			db_unlock();
+
 			modified = true;
 		}
 
@@ -344,8 +370,10 @@ update_archive_tree(struct directory *directory, char *name)
 	if (tmp) {
 		*tmp = 0;
 		//add dir is not there already
+		db_lock();
 		subdir = directory_make_child(directory, name);
 		subdir->device = DEVICE_INARCHIVE;
+		db_unlock();
 		//create directories first
 		update_archive_tree(subdir, tmp+1);
 	} else {
@@ -354,7 +382,9 @@ update_archive_tree(struct directory *directory, char *name)
 			return;
 		}
 		//add file
+		db_lock();
 		struct song *song = directory_get_song(directory, name);
+		db_unlock();
 		if (song == NULL) {
 			song = song_file_load(name, directory);
 			if (song != NULL) {
@@ -386,7 +416,9 @@ update_archive_file(struct directory *parent, const char *name,
 	struct directory *directory;
 	char *filepath;
 
+	db_lock();
 	directory = directory_get_child(parent, name);
+	db_unlock();
 	if (directory != NULL && directory->mtime == st->st_mtime &&
 	    !walk_discard)
 		/* MPD has already scanned the archive, and it hasn't
@@ -409,10 +441,12 @@ update_archive_file(struct directory *parent, const char *name,
 
 	if (directory == NULL) {
 		g_debug("creating archive directory: %s", name);
+		db_lock();
 		directory = directory_new_child(parent, name);
 		/* mark this directory as archive (we use device for
 		   this) */
 		directory->device = DEVICE_INARCHIVE;
+		db_unlock();
 	}
 
 	directory->mtime = st->st_mtime;
@@ -438,6 +472,8 @@ update_container_file(	struct directory* directory,
 	char* vtrack = NULL;
 	unsigned int tnum = 0;
 	char* pathname = map_directory_child_fs(directory, name);
+
+	db_lock();
 	struct directory *contdir = directory_get_child(directory, name);
 
 	// directory exists already
@@ -454,6 +490,7 @@ update_container_file(	struct directory* directory,
 			modified = true;
 		}
 		else {
+			db_unlock();
 			g_free(pathname);
 			return true;
 		}
@@ -462,6 +499,7 @@ update_container_file(	struct directory* directory,
 	contdir = directory_make_child(directory, name);
 	contdir->mtime = st->st_mtime;
 	contdir->device = DEVICE_CONTAINER;
+	db_unlock();
 
 	while ((vtrack = plugin->container_scan(pathname, ++tnum)) != NULL)
 	{
@@ -489,7 +527,9 @@ update_container_file(	struct directory* directory,
 
 	if (tnum == 1)
 	{
+		db_lock();
 		delete_directory(contdir);
+		db_unlock();
 		return false;
 	}
 	else
@@ -536,13 +576,19 @@ update_regular_file(struct directory *directory,
 
 	if ((plugin = decoder_plugin_from_suffix(suffix, false)) != NULL)
 	{
+		db_lock();
 		struct song *song = directory_get_song(directory, name);
+		db_unlock();
 
 		if (!directory_child_access(directory, name, R_OK)) {
 			g_warning("no read permissions on %s/%s",
 				  directory_get_path(directory), name);
-			if (song != NULL)
+			if (song != NULL) {
+				db_lock();
 				delete_song(directory, song);
+				db_unlock();
+			}
+
 			return;
 		}
 
@@ -552,8 +598,11 @@ update_regular_file(struct directory *directory,
 		{
 			if (update_container_file(directory, name, st, plugin))
 			{
-				if (song != NULL)
+				if (song != NULL) {
+					db_lock();
 					delete_song(directory, song);
+					db_unlock();
+				}
 
 				return;
 			}
@@ -579,7 +628,9 @@ update_regular_file(struct directory *directory,
 			if (!song_file_update(song)) {
 				g_debug("deleting unrecognized file %s/%s",
 					directory_get_path(directory), name);
+				db_lock();
 				delete_song(directory, song);
+				db_unlock();
 			}
 
 			modified = true;
@@ -614,12 +665,18 @@ updateInDirectory(struct directory *directory,
 		if (inodeFoundInParent(directory, st->st_ino, st->st_dev))
 			return;
 
+		db_lock();
 		subdir = directory_make_child(directory, name);
+		db_unlock();
+
 		assert(directory == subdir->parent);
 
 		ret = updateDirectory(subdir, st);
-		if (!ret)
+		if (!ret) {
+			db_lock();
 			delete_directory(subdir);
+			db_unlock();
+		}
 	} else {
 		g_debug("update: %s is not a directory, archive or music", name);
 	}
@@ -778,7 +835,9 @@ directory_make_child_checked(struct directory *parent, const char *name_utf8)
 	struct directory *directory;
 	struct stat st;
 
+	db_lock();
 	directory = directory_get_child(parent, name_utf8);
+	db_unlock();
 	if (directory != NULL)
 		return directory;
 
@@ -788,11 +847,14 @@ directory_make_child_checked(struct directory *parent, const char *name_utf8)
 
 	/* if we're adding directory paths, make sure to delete filenames
 	   with potentially the same name */
+	db_lock();
 	struct song *conflicting = directory_get_song(parent, name_utf8);
 	if (conflicting)
 		delete_song(parent, conflicting);
 
 	directory = directory_new_child(parent, name_utf8);
+	db_unlock();
+
 	directory_set_stat(directory, &st);
 	return directory;
 }