From c344fa6a506d117f1e4c9a6213c5c94937631ff9 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sun, 5 Oct 2008 20:13:50 -0700 Subject: Assert if we don't have song or song->url set song objects cannot exist without a path or URL --- src/directory.c | 4 ++-- src/song.c | 10 ++++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/directory.c b/src/directory.c index 5dbab5018..57a615cbd 100644 --- a/src/directory.c +++ b/src/directory.c @@ -296,8 +296,8 @@ removeDeletedFromDirectory(char *path_max_tmp, Directory * directory) 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() */ + assert(song); + assert(*song->url); if (dirname) sprintf(path_max_tmp, "%s/%s", dirname, song->url); diff --git a/src/song.c b/src/song.c index ab98963ad..27f48ad03 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); -- cgit v1.2.3 From a5f8bb82300d08568180482c93e8b9a067cc715d Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sun, 5 Oct 2008 20:54:11 -0700 Subject: song: replace printSong* with song_print_* This make argument order more consistent for iterators. Additionally, these now return ssize_t results for error checking. --- src/dbUtils.c | 15 +++++++-------- src/playlist.c | 4 ++-- src/song.c | 20 ++++++++++---------- src/song.h | 4 ++-- src/songvec.c | 4 ++-- 5 files changed, 23 insertions(+), 24 deletions(-) diff --git a/src/dbUtils.c b/src/dbUtils.c index 4cb3edcca..ff5f1883e 100644 --- a/src/dbUtils.c +++ b/src/dbUtils.c @@ -64,10 +64,9 @@ static int printDirectoryInDirectory(Directory * directory, void *data) return 0; } -static int printSongInDirectory(Song * song, mpd_unused void *data) +static int printSongInDirectory(Song * song, void *data) { - int fd = (int)(size_t)data; - printSongUrl(fd, song); + song_print_url(song, (int)(size_t)data); return 0; } @@ -83,7 +82,7 @@ static int searchInDirectory(Song * song, void *_data) LocateTagItemArray *array = &data->array; if (strstrSearchTags(song, array->numItems, array->items)) - printSongInfo(fd, song); + song_print_info(song, fd); return 0; } @@ -125,7 +124,7 @@ static int findInDirectory(Song * song, void *_data) LocateTagItemArray *array = &data->array; if (tagItemsFoundAndMatches(song, array->numItems, array->items)) - printSongInfo(fd, song); + song_print_info(song, fd); return 0; } @@ -220,9 +219,9 @@ int addAllInToStoredPlaylist(const char *name, const char *utf8file) static int directoryPrintSongInfo(Song * song, void *data) { - int fd = (int)(size_t)data; + song_print_info(song, (int)(size_t)data); - return printSongInfo(fd, song); + return 0; } static int sumSongTime(Song * song, void *data) @@ -285,7 +284,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/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 27f48ad03..61fe78a43 100644 --- a/src/song.c +++ b/src/song.c @@ -92,24 +92,24 @@ 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; } static void insertSongIntoList(struct songvec *sv, Song *newsong) diff --git a/src/song.h b/src/song.h index 377a78814..3419cd43e 100644 --- a/src/song.h +++ b/src/song.h @@ -46,13 +46,13 @@ void freeSong(Song *); void freeJustSong(Song *); -int printSongInfo(int fd, Song * song); +ssize_t song_print_info(Song * song, int fd); 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); /* * 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..969e67220 100644 --- a/src/songvec.c +++ b/src/songvec.c @@ -81,7 +81,7 @@ int songvec_write(struct songvec *sv, int fd, int extra) Song *song = *sp++; if (fdprintf(fd, SONG_KEY "%s\n", song->url) < 0) return -1; - if (printSongInfo(fd, song) < 0) + if (song_print_info(song, fd) < 0) return -1; if (fdprintf(fd, SONG_MTIME "%li\n", (long)song->mtime) < 0) @@ -92,7 +92,7 @@ int songvec_write(struct songvec *sv, int fd, int extra) return -1; } else { for (i = sv->nr; --i >= 0;) - if (printSongInfo(fd, *sp++) < 0) + if (song_print_info(*sp++, fd) < 0) return -1; } -- cgit v1.2.3 From 36c0254a850bf3b24cfd67175cd17bc6afa6035b Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sun, 5 Oct 2008 21:00:39 -0700 Subject: songvec: add songvec_for_each iterator This is so we can more consistently deal with locking needed for thread-safety in iterator functions. --- src/songvec.c | 13 +++++++++++++ src/songvec.h | 2 ++ 2 files changed, 15 insertions(+) diff --git a/src/songvec.c b/src/songvec.c index 969e67220..50694c68a 100644 --- a/src/songvec.c +++ b/src/songvec.c @@ -68,6 +68,19 @@ void songvec_destroy(struct songvec *sv) sv->nr = 0; } +int songvec_for_each(struct songvec *sv, int (*fn)(Song *, void *), void *arg) +{ + int i; + Song **sp = sv->base; + + for (i = sv->nr; --i >= 0; ) { + if (fn(*sp++, arg) < 0) + return -1; + } + + return 0; +} + int songvec_write(struct songvec *sv, int fd, int extra) { int i; diff --git a/src/songvec.h b/src/songvec.h index fb5c38c8b..adfdeccfb 100644 --- a/src/songvec.h +++ b/src/songvec.h @@ -19,6 +19,8 @@ void songvec_add(struct songvec *sv, Song *add); void songvec_destroy(struct songvec *sv); +int songvec_for_each(struct songvec *sv, int (*fn)(Song *, void *), void *arg); + int songvec_write(struct songvec *sv, int fd, int extra); #endif /* SONGVEC_H */ -- cgit v1.2.3 From 9bda68824ebd1b87d5ab28802ab4c310bc9d148a Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Mon, 6 Oct 2008 01:44:53 -0700 Subject: song: add print_song_info_x for iterators tha pass void * traverseAllIn will be modified to take < 0 as errors instead of non-zero... --- src/song.c | 5 +++++ src/song.h | 3 +++ 2 files changed, 8 insertions(+) diff --git a/src/song.c b/src/song.c index 61fe78a43..71191c149 100644 --- a/src/song.c +++ b/src/song.c @@ -112,6 +112,11 @@ ssize_t song_print_info(Song *song, int fd) return ret; } +int song_print_info_x(Song * song, void *data) +{ + return song_print_info(song, (int)(size_t)data); +} + static void insertSongIntoList(struct songvec *sv, Song *newsong) { Song *existing = songvec_find(sv, newsong->url); diff --git a/src/song.h b/src/song.h index 3419cd43e..e65f70f38 100644 --- a/src/song.h +++ b/src/song.h @@ -48,6 +48,9 @@ void freeJustSong(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); -- cgit v1.2.3 From 4602f4fd6e22c7ed7195d71b681214f3a9a53b02 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Tue, 7 Oct 2008 01:37:43 -0700 Subject: songvec: lock traversals for thread-safe updates/reads Only one lock is used for all songvec traversals since they're rarely changed. Also, minimize lock time and release it before calling iterator functions since they may block (updateSongInfo => stat/open/seek/read). This lock only protects songvecs (and all of them) during traversals; not the individual song structures themselves. --- src/songvec.c | 44 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/src/songvec.c b/src/songvec.c index 50694c68a..640eff0a8 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,37 +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_for_each(struct songvec *sv, int (*fn)(Song *, void *), void *arg) { - int i; - Song **sp = sv->base; + size_t i; - for (i = sv->nr; --i >= 0; ) { - if (fn(*sp++, arg) < 0) + pthread_mutex_lock(&nr_lock); + for (i = 0; i < sv->nr; ++i) { + Song *song = sv->base[i]; + + assert(song); + assert(*song->url); + + pthread_mutex_unlock(&nr_lock); /* fn() may block */ + if (fn(song, arg) < 0) return -1; + pthread_mutex_lock(&nr_lock); /* sv->nr may change in fn() */ } + pthread_mutex_unlock(&nr_lock); return 0; } -- cgit v1.2.3 From d271b5f13090595b67de6f2161dc309bb43e6fbe Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Tue, 7 Oct 2008 02:01:21 -0700 Subject: dbUtils/directory: traverseAllIn forEachSong returns -1 on error Being consistent with most UNIX functions... --- src/dbUtils.c | 15 +++++---------- src/directory.c | 21 +++++++++------------ 2 files changed, 14 insertions(+), 22 deletions(-) diff --git a/src/dbUtils.c b/src/dbUtils.c index ff5f1883e..a9d83eb41 100644 --- a/src/dbUtils.c +++ b/src/dbUtils.c @@ -82,7 +82,8 @@ static int searchInDirectory(Song * song, void *_data) LocateTagItemArray *array = &data->array; if (strstrSearchTags(song, array->numItems, array->items)) - song_print_info(song, fd); + if (song_print_info(song, fd) < 0) + return -1; return 0; } @@ -124,7 +125,8 @@ static int findInDirectory(Song * song, void *_data) LocateTagItemArray *array = &data->array; if (tagItemsFoundAndMatches(song, array->numItems, array->items)) - song_print_info(song, fd); + if (song_print_info(song, fd) < 0) + return -1; return 0; } @@ -217,13 +219,6 @@ int addAllInToStoredPlaylist(const char *name, const char *utf8file) &data); } -static int directoryPrintSongInfo(Song * song, void *data) -{ - song_print_info(song, (int)(size_t)data); - - return 0; -} - static int sumSongTime(Song * song, void *data) { unsigned long *sum_time = (unsigned long *)data; @@ -236,7 +231,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); } diff --git a/src/directory.c b/src/directory.c index 57a615cbd..04523ceea 100644 --- a/src/directory.c +++ b/src/directory.c @@ -960,14 +960,11 @@ 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; @@ -976,16 +973,16 @@ static int traverseAllInSubDirectory(Directory * directory, for (i = sv->nr; --i >= 0; ) { Song *song = *sp++; - if ((errFlag = forEachSong(song, data))) - return errFlag; + if ((err = forEachSong(song, data)) < 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, -- cgit v1.2.3 From a67facd051d817dd53bac1aebfc554b07420e8cd Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Tue, 7 Oct 2008 02:06:01 -0700 Subject: song: Add song_print_url_x It'll be handy for passing throug songvec_for_each like song_print_info_x. --- src/song.c | 5 +++++ src/song.h | 3 +++ 2 files changed, 8 insertions(+) diff --git a/src/song.c b/src/song.c index 71191c149..f967e3e17 100644 --- a/src/song.c +++ b/src/song.c @@ -117,6 +117,11 @@ 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) { Song *existing = songvec_find(sv, newsong->url); diff --git a/src/song.h b/src/song.h index e65f70f38..d8189d42c 100644 --- a/src/song.h +++ b/src/song.h @@ -57,6 +57,9 @@ int updateSongInfo(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 * path_max_tmp is the argument that the URL is written to, this -- cgit v1.2.3 From 99841dca7bdc7e85df88800df86e22bb70ed683d Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Tue, 7 Oct 2008 02:08:29 -0700 Subject: dbUtils: more cleanups Remove unnecessary wrapper function now that we have song_print_url_x and also return the results directly since we'll know the song_print_* functions will all return -1 on error. --- src/dbUtils.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/dbUtils.c b/src/dbUtils.c index a9d83eb41..adcb7e0da 100644 --- a/src/dbUtils.c +++ b/src/dbUtils.c @@ -64,12 +64,6 @@ static int printDirectoryInDirectory(Directory * directory, void *data) return 0; } -static int printSongInDirectory(Song * song, void *data) -{ - song_print_url(song, (int)(size_t)data); - return 0; -} - struct search_data { int fd; LocateTagItemArray array; @@ -82,8 +76,7 @@ static int searchInDirectory(Song * song, void *_data) LocateTagItemArray *array = &data->array; if (strstrSearchTags(song, array->numItems, array->items)) - if (song_print_info(song, fd) < 0) - return -1; + return (int)song_print_info(song, fd); return 0; } @@ -125,8 +118,7 @@ static int findInDirectory(Song * song, void *_data) LocateTagItemArray *array = &data->array; if (tagItemsFoundAndMatches(song, array->numItems, array->items)) - if (song_print_info(song, fd) < 0) - return -1; + return (int)song_print_info(song, fd); return 0; } @@ -182,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); } -- cgit v1.2.3 From a340bf0dee5bf134770f5d5f532efa90cdb08b3c Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Tue, 7 Oct 2008 02:25:27 -0700 Subject: directory: use songvec_for_each for iterators Get rid of songvec_write so we can enforce proper locking --- src/directory.c | 86 +++++++++++++++++++++++++++++++++++++-------------------- src/songvec.c | 31 --------------------- src/songvec.h | 2 -- 3 files changed, 56 insertions(+), 63 deletions(-) diff --git a/src/directory.c b/src/directory.c index 04523ceea..a11d01d25 100644 --- a/src/directory.c +++ b/src/directory.c @@ -276,15 +276,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 +313,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]; - assert(song); - assert(*song->url); - - if (dirname) - sprintf(path_max_tmp, "%s/%s", dirname, song->url); - else - strcpy(path_max_tmp, song->url); - - if (!isFile(path_max_tmp, NULL)) { - delete_song(directory, song); - ret = UPDATE_RETURN_UPDATED; - } - } + data.dir = directory; + data.tmp = path_max_tmp; + data.ret = UPDATE_RETURN_UPDATED; + songvec_for_each(&directory->songs, delete_song_if_removed, &data); - return ret; + return data.ret; } static Directory *addDirectoryPathToDB(const char *utf8path) @@ -684,7 +692,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 +732,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", @@ -967,15 +999,9 @@ static int traverseAllInSubDirectory(Directory * directory, 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 ((err = forEachSong(song, data)) < 0) - return err; - } + err = songvec_for_each(&directory->songs, forEachSong, data); + if (err < 0) + return err; } for (j = 0; err >= 0 && j < dv->nr; ++j) diff --git a/src/songvec.c b/src/songvec.c index 640eff0a8..cf0991029 100644 --- a/src/songvec.c +++ b/src/songvec.c @@ -104,34 +104,3 @@ int songvec_for_each(struct songvec *sv, int (*fn)(Song *, void *), void *arg) return 0; } - -int songvec_write(struct songvec *sv, int fd, int extra) -{ - int i; - Song **sp = sv->base; - - if (extra) { - if (fdprintf(fd, SONG_BEGIN "\n") < 0) - return -1; - - for (i = sv->nr; --i >= 0; ) { - Song *song = *sp++; - 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; - } - - if (fdprintf(fd, SONG_END "\n") < 0) - return -1; - } else { - for (i = sv->nr; --i >= 0;) - if (song_print_info(*sp++, fd) < 0) - return -1; - } - - return 0; -} diff --git a/src/songvec.h b/src/songvec.h index adfdeccfb..dbe6be508 100644 --- a/src/songvec.h +++ b/src/songvec.h @@ -21,6 +21,4 @@ void songvec_destroy(struct songvec *sv); int songvec_for_each(struct songvec *sv, int (*fn)(Song *, void *), void *arg); -int songvec_write(struct songvec *sv, int fd, int extra); - #endif /* SONGVEC_H */ -- cgit v1.2.3 From 19c6f1ee92cf6514e885def44f7deb9d225de4dc Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Tue, 7 Oct 2008 02:57:59 -0700 Subject: directory: serialize song deletes from playlist during update This makes the update code thread-safe and doesn't penalize the playlist code by complicating it with complicated and error-prone locks (and the associated overhead, not everybody has a thread-implementation as good as NPTL). The update task blocks during the delete; but the update task is a slow task anyways so we can block w/o people caring too much. This was also our only freeSong call site, so remove that function. Note that deleting entire directories is not fully thread-safe, yet; as their traversals are not yet locked. --- src/directory.c | 30 +++++++++++++++++++++++++++--- src/song.c | 6 ------ src/song.h | 2 -- 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/directory.c b/src/directory.c index a11d01d25..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) diff --git a/src/song.c b/src/song.c index f967e3e17..c9301386d 100644 --- a/src/song.c +++ b/src/song.c @@ -79,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) diff --git a/src/song.h b/src/song.h index d8189d42c..9aa9efa94 100644 --- a/src/song.h +++ b/src/song.h @@ -42,8 +42,6 @@ typedef struct _Song { Song *newSong(const char *url, struct _Directory *parentDir); -void freeSong(Song *); - void freeJustSong(Song *); ssize_t song_print_info(Song * song, int fd); -- cgit v1.2.3