Merge branch 'ew/update-thrsafe'

* ew/update-thrsafe:
  directory: serialize song deletes from playlist during update
  directory: use songvec_for_each for iterators
  dbUtils: more cleanups
  song: Add song_print_url_x
  dbUtils/directory: traverseAllIn forEachSong returns -1 on error
  songvec: lock traversals for thread-safe updates/reads
  song: add print_song_info_x for iterators tha pass void *
  songvec: add songvec_for_each iterator
  song: replace printSong* with song_print_*
  Assert if we don't have song or song->url set

7 files changed, 167 insertions, 114 deletions

diff --git a/src/dbUtils.c b/src/dbUtils.c
index 246d27385..bd990e96d 100644
--- a/src/dbUtils.c
+++ b/src/dbUtils.c
@@ -64,13 +64,6 @@ static int printDirectoryInDirectory(Directory * directory, void *data)
 	return 0;
 }
 
-static int printSongInDirectory(Song * song, mpd_unused void *data)
-{
-	int fd = (int)(size_t)data;
-	printSongUrl(fd, song);
-	return 0;
-}
-
 struct search_data {
 	int fd;
 	LocateTagItemArray array;
@@ -83,7 +76,7 @@ static int searchInDirectory(Song * song, void *_data)
 	LocateTagItemArray *array = &data->array;
 
 	if (strstrSearchTags(song, array->numItems, array->items))
-		printSongInfo(fd, song);
+		return (int)song_print_info(song, fd);
 
 	return 0;
 }
@@ -125,7 +118,7 @@ static int findInDirectory(Song * song, void *_data)
 	LocateTagItemArray *array = &data->array;
 
 	if (tagItemsFoundAndMatches(song, array->numItems, array->items))
-		printSongInfo(fd, song);
+		return (int)song_print_info(song, fd);
 
 	return 0;
 }
@@ -181,7 +174,7 @@ int searchStatsForSongsIn(int fd, const char *name, int numItems,
 
 int printAllIn(int fd, const char *name)
 {
-	return traverseAllIn(name, printSongInDirectory,
+	return traverseAllIn(name, song_print_url_x,
 			     printDirectoryInDirectory, (void*)(size_t)fd);
 }
 
@@ -217,13 +210,6 @@ int addAllInToStoredPlaylist(const char *name, const char *utf8file)
 	                     &data);
 }
 
-static int directoryPrintSongInfo(Song * song, void *data)
-{
-	int fd = (int)(size_t)data;
-
-	return printSongInfo(fd, song);
-}
-
 static int sumSongTime(Song * song, void *data)
 {
 	unsigned long *sum_time = (unsigned long *)data;
@@ -236,7 +222,7 @@ static int sumSongTime(Song * song, void *data)
 
 int printInfoForAllIn(int fd, const char *name)
 {
-	return traverseAllIn(name, directoryPrintSongInfo,
+	return traverseAllIn(name, song_print_info_x,
 			     printDirectoryInDirectory, (void*)(size_t)fd);
 }
 
@@ -284,7 +270,7 @@ static void visitTag(int fd, struct strset *set,
 	struct mpd_tag *tag = song->tag;
 
 	if (tagType == LOCATE_TAG_FILE_TYPE) {
-		printSongUrl(fd, song);
+		song_print_url(song, fd);
 		return;
 	}
 
diff --git a/src/directory.c b/src/directory.c
index 5dbab5018..9613bbfbd 100644
--- a/src/directory.c
+++ b/src/directory.c
@@ -63,8 +63,13 @@ static time_t directory_dbModTime;
 static pthread_t update_thr;
 
 static const int update_task_id_max = 1 << 15;
+
 static int update_task_id;
 
+static Song *delete;
+
+static struct condition delete_cond;
+
 static int addToDirectory(Directory * directory, const char *name);
 
 static void freeDirectory(Directory * directory);
@@ -144,6 +149,16 @@ void reap_update_task(void)
 
 	assert(pthread_equal(pthread_self(), main_task));
 
+	cond_enter(&delete_cond);
+	if (delete) {
+		char tmp[MPD_PATH_MAX];
+		LOG("removing: %s\n", get_song_url(tmp, delete));
+		deleteASongFromPlaylist(delete);
+		delete = NULL;
+		cond_signal(&delete_cond);
+	}
+	cond_leave(&delete_cond);
+
 	if (progress != UPDATE_PROGRESS_DONE)
 		return;
 	if (pthread_join(update_thr, (void **)&ret))
@@ -215,10 +230,19 @@ static void freeDirectory(Directory * directory)
 
 static void delete_song(Directory *dir, Song *del)
 {
-	char path_max_tmp[MPD_PATH_MAX]; /* wasteful */
-	LOG("removing: %s\n", get_song_url(path_max_tmp, del));
+	/* first, prevent traversers in main task from getting this */
 	songvec_delete(&dir->songs, del);
-	freeSong(del); /* FIXME racy */
+
+	/* now take it out of the playlist (in the main_task) */
+	cond_enter(&delete_cond);
+	assert(!delete);
+	delete = del;
+	wakeup_main_task();
+	do { cond_wait(&delete_cond); } while (delete);
+	cond_leave(&delete_cond);
+
+	/* finally, all possible references gone, free it */
+	freeJustSong(del);
 }
 
 static void deleteEmptyDirectoriesInDirectory(Directory * directory)
@@ -276,15 +300,34 @@ static int skip_path(const char *path)
 	return (path[0] == '.' || strchr(path, '\n')) ? 1 : 0;
 }
 
+struct delete_data {
+	char *tmp;
+	Directory *dir;
+	enum update_return ret;
+};
+
+/* passed to songvec_for_each */
+static int delete_song_if_removed(Song *song, void *_data)
+{
+	struct delete_data *data = _data;
+
+	data->tmp = get_song_url(data->tmp, song);
+	assert(data->tmp);
+
+	if (!isFile(data->tmp, NULL)) {
+		delete_song(data->dir, song);
+		data->ret = UPDATE_RETURN_UPDATED;
+	}
+	return 0;
+}
+
 static enum update_return
 removeDeletedFromDirectory(char *path_max_tmp, Directory * directory)
 {
-	const char *dirname = (directory && directory->path) ?
-	    directory->path : NULL;
 	enum update_return ret = UPDATE_RETURN_NOUPDATE;
 	int i;
-	struct songvec *sv = &directory->songs;
 	struct dirvec *dv = &directory->children;
+	struct delete_data data;
 
 	for (i = dv->nr; --i >= 0; ) {
 		if (isDir(dv->base[i]->path))
@@ -294,23 +337,12 @@ removeDeletedFromDirectory(char *path_max_tmp, Directory * directory)
 		ret = UPDATE_RETURN_UPDATED;
 	}
 
-	for (i = sv->nr; --i >= 0; ) { /* cleaner deletes if we go backwards */
-		Song *song = sv->base[i];
-		if (!song || !*song->url)
-			continue; /* does this happen?, perhaps assert() */
-
-		if (dirname)
-			sprintf(path_max_tmp, "%s/%s", dirname, song->url);
-		else
-			strcpy(path_max_tmp, song->url);
+	data.dir = directory;
+	data.tmp = path_max_tmp;
+	data.ret = UPDATE_RETURN_UPDATED;
+	songvec_for_each(&directory->songs, delete_song_if_removed, &data);
 
-		if (!isFile(path_max_tmp, NULL)) {
-			delete_song(directory, song);
-			ret = UPDATE_RETURN_UPDATED;
-		}
-	}
-
-	return ret;
+	return data.ret;
 }
 
 static Directory *addDirectoryPathToDB(const char *utf8path)
@@ -684,7 +716,22 @@ int printDirectoryInfo(int fd, const char *name)
 		return -1;
 
 	printDirectoryList(fd, &directory->children);
-	songvec_write(&directory->songs, fd, 0);
+	songvec_for_each(&directory->songs,
+	                 song_print_info_x, (void *)(size_t)fd);
+
+	return 0;
+}
+
+static int directory_song_write(Song *song, void *data)
+{
+	int fd = (int)(size_t)data;
+
+	if (fdprintf(fd, SONG_KEY "%s\n", song->url) < 0)
+		return -1;
+	if (song_print_info(song, fd) < 0)
+		return -1;
+	if (fdprintf(fd, SONG_MTIME "%li\n", (long)song->mtime) < 0)
+		return -1;
 
 	return 0;
 }
@@ -709,7 +756,16 @@ static int writeDirectoryInfo(int fd, Directory * directory)
 		if (writeDirectoryInfo(fd, cur) < 0)
 			return -1;
 	}
-	songvec_write(&directory->songs, fd, 1);
+
+	if (fdprintf(fd, SONG_BEGIN "\n") < 0)
+		return -1;
+
+	if (songvec_for_each(&directory->songs,
+	                     directory_song_write, (void *)(size_t)fd) < 0)
+		return -1;
+
+	if (fdprintf(fd, SONG_END "\n") < 0)
+		return -1;
 
 	if (directory->path &&
 	    fdprintf(fd, DIRECTORY_END "%s\n",
@@ -960,32 +1016,23 @@ static int traverseAllInSubDirectory(Directory * directory,
 				     void *data)
 {
 	struct dirvec *dv = &directory->children;
-	int errFlag = 0;
+	int err = 0;
 	size_t j;
 
-	if (forEachDir) {
-		errFlag = forEachDir(directory, data);
-		if (errFlag)
-			return errFlag;
-	}
+	if (forEachDir && (err = forEachDir(directory, data)) < 0)
+		return err;
 
 	if (forEachSong) {
-		int i;
-		struct songvec *sv = &directory->songs;
-		Song **sp = sv->base;
-
-		for (i = sv->nr; --i >= 0; ) {
-			Song *song = *sp++;
-			if ((errFlag = forEachSong(song, data)))
-				return errFlag;
-		}
+		err = songvec_for_each(&directory->songs, forEachSong, data);
+		if (err < 0)
+			return err;
 	}
 
-	for (j = 0; !errFlag && j < dv->nr; ++j)
-		errFlag = traverseAllInSubDirectory(dv->base[j], forEachSong,
-						    forEachDir, data);
+	for (j = 0; err >= 0 && j < dv->nr; ++j)
+		err = traverseAllInSubDirectory(dv->base[j], forEachSong,
+						forEachDir, data);
 
-	return errFlag;
+	return err;
 }
 
 int traverseAllIn(const char *name,
diff --git a/src/playlist.c b/src/playlist.c
index 6207ca985..696636993 100644
--- a/src/playlist.c
+++ b/src/playlist.c
@@ -360,7 +360,7 @@ void readPlaylistState(FILE *fp)
 
 static void printPlaylistSongInfo(int fd, int song)
 {
-	printSongInfo(fd, playlist.songs[song]);
+	song_print_info(playlist.songs[song], fd);
 	fdprintf(fd, "Pos: %i\nId: %i\n", song, playlist.positionToId[song]);
 }
 
@@ -1363,7 +1363,7 @@ int PlaylistInfo(int fd, const char *utf8file, int detail)
 		if (detail) {
 			Song *song = getSongFromDB(temp);
 			if (song) {
-				printSongInfo(fd, song);
+				song_print_info(song, fd);
 				wrote = 1;
 			}
 		}
diff --git a/src/song.c b/src/song.c
index ab98963ad..c9301386d 100644
--- a/src/song.c
+++ b/src/song.c
@@ -31,8 +31,13 @@
 
 static Song * song_alloc(const char *url, Directory *parent)
 {
-	size_t urllen = strlen(url);
-	Song *song = xmalloc(sizeof(Song) + urllen);
+	size_t urllen;
+	Song *song;
+
+	assert(url);
+	urllen = strlen(url);
+	assert(urllen);
+	song = xmalloc(sizeof(Song) + urllen);
 
 	song->tag = NULL;
 	memcpy(song->url, url, urllen + 1);
@@ -44,6 +49,7 @@ static Song * song_alloc(const char *url, Directory *parent)
 Song *newSong(const char *url, Directory * parentDir)
 {
 	Song *song;
+	assert(*url);
 
 	if (strchr(url, '\n')) {
 		DEBUG("newSong: '%s' is not a valid uri\n", url);
@@ -73,12 +79,6 @@ Song *newSong(const char *url, Directory * parentDir)
 	return song;
 }
 
-void freeSong(Song * song)
-{
-	deleteASongFromPlaylist(song);
-	freeJustSong(song);
-}
-
 void freeJustSong(Song * song)
 {
 	if (song->tag)
@@ -86,24 +86,34 @@ void freeJustSong(Song * song)
 	free(song);
 }
 
-void printSongUrl(int fd, Song * song)
+ssize_t song_print_url(Song *song, int fd)
 {
-	if (song->parentDir && song->parentDir->path) {
-		fdprintf(fd, "%s%s/%s\n", SONG_FILE,
-			  getDirectoryPath(song->parentDir), song->url);
-	} else {
-		fdprintf(fd, "%s%s\n", SONG_FILE, song->url);
-	}
+	if (song->parentDir && song->parentDir->path)
+		return fdprintf(fd, "%s%s/%s\n", SONG_FILE,
+			        getDirectoryPath(song->parentDir), song->url);
+	return fdprintf(fd, "%s%s\n", SONG_FILE, song->url);
 }
 
-int printSongInfo(int fd, Song * song)
+ssize_t song_print_info(Song *song, int fd)
 {
-	printSongUrl(fd, song);
+	ssize_t ret = song_print_url(song, fd);
 
+	if (ret < 0)
+		return ret;
 	if (song->tag)
 		tag_print(fd, song->tag);
 
-	return 0;
+	return ret;
+}
+
+int song_print_info_x(Song * song, void *data)
+{
+	return song_print_info(song, (int)(size_t)data);
+}
+
+int song_print_url_x(Song * song, void *data)
+{
+	return song_print_url(song, (int)(size_t)data);
 }
 
 static void insertSongIntoList(struct songvec *sv, Song *newsong)
diff --git a/src/song.h b/src/song.h
index 377a78814..9aa9efa94 100644
--- a/src/song.h
+++ b/src/song.h
@@ -42,17 +42,21 @@ typedef struct _Song {
 
 Song *newSong(const char *url, struct _Directory *parentDir);
 
-void freeSong(Song *);
-
 void freeJustSong(Song *);
 
-int printSongInfo(int fd, Song * song);
+ssize_t song_print_info(Song * song, int fd);
+
+/* like song_print_info, but casts data into an fd first */
+int song_print_info_x(Song * song, void *data);
 
 void readSongInfoIntoList(FILE * fp, struct _Directory *parent);
 
 int updateSongInfo(Song * song);
 
-void printSongUrl(int fd, Song * song);
+ssize_t song_print_url(Song * song, int fd);
+
+/* like song_print_url_x, but casts data into an fd first */
+int song_print_url_x(Song * song, void *data);
 
 /*
  * get_song_url - Returns a path of a song in UTF8-encoded form
diff --git a/src/songvec.c b/src/songvec.c
index 579ac7033..cf0991029 100644
--- a/src/songvec.c
+++ b/src/songvec.c
@@ -2,6 +2,8 @@
 #include "myfprintf.h"
 #include "utils.h"
 
+static pthread_mutex_t nr_lock = PTHREAD_MUTEX_INITIALIZER;
+
 /* Only used for sorting/searchin a songvec, not general purpose compares */
 static int songvec_cmp(const void *s1, const void *s2)
 {
@@ -17,23 +19,32 @@ static size_t sv_size(struct songvec *sv)
 
 void songvec_sort(struct songvec *sv)
 {
+	pthread_mutex_lock(&nr_lock);
 	qsort(sv->base, sv->nr, sizeof(Song *), songvec_cmp);
+	pthread_mutex_unlock(&nr_lock);
 }
 
 Song *songvec_find(struct songvec *sv, const char *url)
 {
 	int i;
+	Song *ret = NULL;
 
-	for (i = sv->nr; --i >= 0; )
-		if (!strcmp(sv->base[i]->url, url))
-			return sv->base[i];
-	return NULL;
+	pthread_mutex_lock(&nr_lock);
+	for (i = sv->nr; --i >= 0; ) {
+		if (strcmp(sv->base[i]->url, url))
+			continue;
+		ret = sv->base[i];
+		break;
+	}
+	pthread_mutex_unlock(&nr_lock);
+	return ret;
 }
 
 int songvec_delete(struct songvec *sv, const Song *del)
 {
 	int i;
 
+	pthread_mutex_lock(&nr_lock);
 	for (i = sv->nr; --i >= 0; ) {
 		if (sv->base[i] != del)
 			continue;
@@ -46,55 +57,50 @@ int songvec_delete(struct songvec *sv, const Song *del)
 				(sv->nr - i + 1) * sizeof(Song *));
 			sv->base = xrealloc(sv->base, sv_size(sv));
 		}
-		return i;
+		break;
 	}
+	pthread_mutex_unlock(&nr_lock);
 
-	return -1; /* not found */
+	return i;
 }
 
 void songvec_add(struct songvec *sv, Song *add)
 {
+	pthread_mutex_lock(&nr_lock);
 	++sv->nr;
 	sv->base = xrealloc(sv->base, sv_size(sv));
 	sv->base[sv->nr - 1] = add;
+	pthread_mutex_unlock(&nr_lock);
 }
 
 void songvec_destroy(struct songvec *sv)
 {
+	pthread_mutex_lock(&nr_lock);
 	if (sv->base) {
 		free(sv->base);
 		sv->base = NULL;
 	}
 	sv->nr = 0;
+	pthread_mutex_unlock(&nr_lock);
 }
 
-int songvec_write(struct songvec *sv, int fd, int extra)
+int songvec_for_each(struct songvec *sv, int (*fn)(Song *, void *), void *arg)
 {
-	int i;
-	Song **sp = sv->base;
+	size_t i;
 
-	if (extra) {
-		if (fdprintf(fd, SONG_BEGIN "\n") < 0)
-			return -1;
+	pthread_mutex_lock(&nr_lock);
+	for (i = 0; i < sv->nr; ++i) {
+		Song *song = sv->base[i];
 
-		for (i = sv->nr; --i >= 0; ) {
-			Song *song = *sp++;
-			if (fdprintf(fd, SONG_KEY "%s\n", song->url) < 0)
-				return -1;
-			if (printSongInfo(fd, song) < 0)
-				return -1;
-			if (fdprintf(fd,
-			            SONG_MTIME "%li\n", (long)song->mtime) < 0)
-				return -1;
-		}
+		assert(song);
+		assert(*song->url);
 
-		if (fdprintf(fd, SONG_END "\n") < 0)
+		pthread_mutex_unlock(&nr_lock); /* fn() may block */
+		if (fn(song, arg) < 0)
 			return -1;
-	} else {
-		for (i = sv->nr; --i >= 0;)
-			if (printSongInfo(fd, *sp++) < 0)
-				return -1;
+		pthread_mutex_lock(&nr_lock); /* sv->nr may change in fn() */
 	}
+	pthread_mutex_unlock(&nr_lock);
 
 	return 0;
 }
diff --git a/src/songvec.h b/src/songvec.h
index fb5c38c8b..dbe6be508 100644
--- a/src/songvec.h
+++ b/src/songvec.h
@@ -19,6 +19,6 @@ void songvec_add(struct songvec *sv, Song *add);
 
 void songvec_destroy(struct songvec *sv);
 
-int songvec_write(struct songvec *sv, int fd, int extra);
+int songvec_for_each(struct songvec *sv, int (*fn)(Song *, void *), void *arg);
 
 #endif /* SONGVEC_H */