From e28fb29349a811f0f306e9ac4bedf6013417d043 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sat, 11 Oct 2008 20:41:55 -0700 Subject: directory: make music_root global and avoid runtime initialization mpd can't function without music_root; so don't bother allocating it on the heap nor checking to see if it's initialized. Don't allow directory_new() to create a directory w/o a parent or with an empty path, either: root is root and there can be only one. --- src/update.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) (limited to 'src/update.c') diff --git a/src/update.c b/src/update.c index ca3640b73..3f6fa15ed 100644 --- a/src/update.c +++ b/src/update.c @@ -317,10 +317,9 @@ directory_make_child_checked(struct directory *parent, const char *path) return dir; } -static struct directory * -addParentPathToDB(const char *utf8path) +static struct directory * addParentPathToDB(const char *utf8path) { - struct directory *dir = db_get_root(); + struct directory *dir = &music_root; char *duplicated = xstrdup(utf8path); char *slash = duplicated; @@ -354,11 +353,10 @@ static void * update_task(void *_path) updatePath((char *)_path); free(_path); } else { - struct directory *dir = db_get_root(); struct stat st; - if (myStat(directory_get_path(dir), &st) == 0) - updateDirectory(dir, &st); + if (myStat(directory_get_path(&music_root), &st) == 0) + updateDirectory(&music_root, &st); } if (modified) -- cgit v1.2.3 From 8636b3185268e8de90c566d472c52b814111b8c3 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sat, 11 Oct 2008 21:09:08 -0700 Subject: Avoid calling isRootDirectory when we have a directory object There is only one music_root and we can just compare addresses. --- src/update.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/update.c') diff --git a/src/update.c b/src/update.c index 3f6fa15ed..7c2a7570a 100644 --- a/src/update.c +++ b/src/update.c @@ -278,7 +278,7 @@ static int updateDirectory(struct directory *dir, const struct stat *st) if (!utf8) continue; - if (!isRootDirectory(dir->path)) + if (dir != &music_root) utf8 = pfx_dir(path_max_tmp, utf8, strlen(utf8), dirname, strlen(dirname)); -- cgit v1.2.3 From 0fd45f50ec56bcfacae3f6f141ea804b9ac3493f Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sat, 11 Oct 2008 21:31:26 -0700 Subject: directory: rename isRootDirectory => path_is_music_root down with camelCase! --- src/update.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/update.c') diff --git a/src/update.c b/src/update.c index 7c2a7570a..72f4b943b 100644 --- a/src/update.c +++ b/src/update.c @@ -349,8 +349,8 @@ static void updatePath(const char *utf8path) static void * update_task(void *_path) { - if (_path != NULL && !isRootDirectory(_path)) { - updatePath((char *)_path); + if (_path && !path_is_music_root(_path)) { + updatePath(_path); free(_path); } else { struct stat st; -- cgit v1.2.3 From 3e652ee92d4bbe2a9e68cc80b6d58a7f0e6a6093 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sat, 11 Oct 2008 21:46:00 -0700 Subject: update: validate in command.c and fix small memory leak If update_task was called with "" as its path argument, that string would never get freed because it false positived. Instead, never pass "" to update_task and trip an assertion if we do. --- src/update.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) (limited to 'src/update.c') diff --git a/src/update.c b/src/update.c index 72f4b943b..6c8b6e7bc 100644 --- a/src/update.c +++ b/src/update.c @@ -349,9 +349,12 @@ static void updatePath(const char *utf8path) static void * update_task(void *_path) { - if (_path && !path_is_music_root(_path)) { - updatePath(_path); - free(_path); + char *utf8path = _path; + + if (utf8path) { + assert(*utf8path); + updatePath(utf8path); + free(utf8path); } else { struct stat st; @@ -386,6 +389,8 @@ unsigned directory_update_init(char *path) { assert(pthread_equal(pthread_self(), main_task)); + assert(!path || (path && *path)); + if (progress != UPDATE_PROGRESS_IDLE) { unsigned next_task_id; -- cgit v1.2.3 From afef3a6e68f4007f8e087b6ae359518ff59b04ef Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sat, 11 Oct 2008 21:49:29 -0700 Subject: update: allow music_root updates to be queued Previously only updates with subdirectories being specified could be queued. No harm in queueing full updates. --- src/update.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'src/update.c') diff --git a/src/update.c b/src/update.c index 6c8b6e7bc..35605ee51 100644 --- a/src/update.c +++ b/src/update.c @@ -394,10 +394,9 @@ unsigned directory_update_init(char *path) if (progress != UPDATE_PROGRESS_IDLE) { unsigned next_task_id; - if (!path) - return 0; if (update_paths_nr == ARRAY_SIZE(update_paths)) { - free(path); + if (path) + free(path); return 0; } -- cgit v1.2.3 From e018b35b0d99d24e1f3779fd51c07154b370eca9 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sat, 11 Oct 2008 23:04:36 -0700 Subject: dirvec: use dirvec_for_each where it makes sense This way we can introduce locking to allow safe traversals from the main thread while we're updating. --- src/update.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) (limited to 'src/update.c') diff --git a/src/update.c b/src/update.c index 35605ee51..99a58a06e 100644 --- a/src/update.c +++ b/src/update.c @@ -91,15 +91,12 @@ static int delete_each_song(struct mpd_song *song, mpd_unused void *data) * Recursively remove all sub directories and songs from a directory, * leaving an empty directory. */ -static void clear_directory(struct directory *dir) +static int clear_directory(struct directory *dir, mpd_unused void *arg) { - int i; - - for (i = dir->children.nr; --i >= 0;) - clear_directory(dir->children.base[i]); + dirvec_for_each(&dir->children, clear_directory, NULL); dirvec_clear(&dir->children); - songvec_for_each(&dir->songs, delete_each_song, dir); + return 0; } /** @@ -109,7 +106,7 @@ static void delete_directory(struct directory *dir) { assert(dir->parent != NULL); - clear_directory(dir); + clear_directory(dir, NULL); dirvec_delete(&dir->parent->children, dir); directory_free(dir); } @@ -149,23 +146,27 @@ static void delete_path(const char *path) } } -static void -removeDeletedFromDirectory(char *path_max_tmp, struct directory *dir) +/* passed to dirvec_for_each */ +static int delete_directory_if_removed(struct directory *dir, void *_data) { - int i; - struct dirvec *dv = &dir->children; - struct delete_data data; + if (!isDir(dir->path)) { + struct delete_data *data = _data; - for (i = dv->nr; --i >= 0; ) { - if (isDir(dv->base[i]->path)) - continue; - LOG("removing directory: %s\n", dv->base[i]->path); - dirvec_delete(dv, dv->base[i]); + LOG("removing directory: %s\n", directory_get_path(dir)); + dirvec_delete(&data->dir->children, dir); modified = 1; } + return 0; +} + +static void +removeDeletedFromDirectory(char *path_max_tmp, struct directory *dir) +{ + struct delete_data data; data.dir = dir; data.tmp = path_max_tmp; + dirvec_for_each(&dir->children, delete_directory_if_removed, &data); songvec_for_each(&dir->songs, delete_song_if_removed, &data); } -- cgit v1.2.3 From f58827951ff79539125dcb4b1bd08e549e347a62 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sat, 11 Oct 2008 23:59:45 -0700 Subject: update: serialize song_free in main thread It's actually possible to have a traverser accessing a song while we're freeing it in the main thread, as the songvec iterators don't lock the individual elements. --- src/update.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) (limited to 'src/update.c') diff --git a/src/update.c b/src/update.c index 99a58a06e..c3e5b9a7c 100644 --- a/src/update.c +++ b/src/update.c @@ -74,9 +74,6 @@ static void delete_song(struct directory *dir, struct mpd_song *del) wakeup_main_task(); do { cond_wait(&delete_cond); } while (delete); cond_leave(&delete_cond); - - /* finally, all possible references gone, free it */ - song_free(del); } static int delete_each_song(struct mpd_song *song, mpd_unused void *data) @@ -423,6 +420,7 @@ void reap_update_task(void) char tmp[MPD_PATH_MAX]; LOG("removing: %s\n", song_get_url(delete, tmp)); deleteASongFromPlaylist(delete); + song_free(delete); delete = NULL; cond_signal(&delete_cond); } -- cgit v1.2.3 From 6ad5891f2c57bca861d51160cbfa54f172fa1ea5 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sun, 12 Oct 2008 00:23:38 -0700 Subject: update: serialize directory deletions We only delete directory object in the main thread to prevent access to individual elements (mainly path). --- src/update.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-) (limited to 'src/update.c') diff --git a/src/update.c b/src/update.c index c3e5b9a7c..b244c3a3b 100644 --- a/src/update.c +++ b/src/update.c @@ -46,7 +46,11 @@ static const unsigned update_task_id_max = 1 << 15; static unsigned update_task_id; -static struct mpd_song *delete; +enum update_type { UPDATE_TYPE_SONG, UPDATE_TYPE_DIR }; + +static enum update_type delete_type; + +static void *delete; static struct condition delete_cond; @@ -62,20 +66,26 @@ static void directory_set_stat(struct directory *dir, const struct stat *st) dir->stat = 1; } -static void delete_song(struct directory *dir, struct mpd_song *del) +static void serialized_delete(void *del, enum update_type type) { - /* first, prevent traversers in main task from getting this */ - songvec_delete(&dir->songs, del); - - /* now take it out of the playlist (in the main_task) */ cond_enter(&delete_cond); assert(!delete); + delete_type = type; delete = del; wakeup_main_task(); do { cond_wait(&delete_cond); } while (delete); cond_leave(&delete_cond); } +static void delete_song(struct directory *dir, struct mpd_song *del) +{ + /* first, prevent traversers in main task from getting this */ + songvec_delete(&dir->songs, del); + + /* now take it out of the playlist (in the main_task) and free */ + serialized_delete(del, UPDATE_TYPE_SONG); +} + static int delete_each_song(struct mpd_song *song, mpd_unused void *data) { struct directory *dir = data; @@ -105,7 +115,7 @@ static void delete_directory(struct directory *dir) clear_directory(dir, NULL); dirvec_delete(&dir->parent->children, dir); - directory_free(dir); + serialized_delete(dir, UPDATE_TYPE_DIR); } struct delete_data { @@ -417,10 +427,17 @@ void reap_update_task(void) cond_enter(&delete_cond); if (delete) { - char tmp[MPD_PATH_MAX]; - LOG("removing: %s\n", song_get_url(delete, tmp)); - deleteASongFromPlaylist(delete); - song_free(delete); + switch (delete_type) { + case UPDATE_TYPE_SONG: { + char tmp[MPD_PATH_MAX]; + LOG("removing: %s\n", song_get_url(delete, tmp)); + deleteASongFromPlaylist(delete); + song_free(delete); + } + break; + case UPDATE_TYPE_DIR: + directory_free(delete); + } delete = NULL; cond_signal(&delete_cond); } -- cgit v1.2.3 From 085d139e21511109b0a38278baf20993c7edfa38 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sun, 12 Oct 2008 01:56:13 -0700 Subject: update: remove delete_each_song and clear_directory Our beefier directory_free takes care of it now in the main task. --- src/update.c | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-) (limited to 'src/update.c') diff --git a/src/update.c b/src/update.c index b244c3a3b..9c0113446 100644 --- a/src/update.c +++ b/src/update.c @@ -86,26 +86,6 @@ static void delete_song(struct directory *dir, struct mpd_song *del) serialized_delete(del, UPDATE_TYPE_SONG); } -static int delete_each_song(struct mpd_song *song, mpd_unused void *data) -{ - struct directory *dir = data; - assert(song->parent == dir); - delete_song(dir, song); - return 0; -} - -/** - * Recursively remove all sub directories and songs from a directory, - * leaving an empty directory. - */ -static int clear_directory(struct directory *dir, mpd_unused void *arg) -{ - dirvec_for_each(&dir->children, clear_directory, NULL); - dirvec_clear(&dir->children); - songvec_for_each(&dir->songs, delete_each_song, dir); - return 0; -} - /** * Recursively free a directory and all its contents. */ @@ -113,8 +93,10 @@ static void delete_directory(struct directory *dir) { assert(dir->parent != NULL); - clear_directory(dir, NULL); + /* first, prevent traversers in main task from getting this */ dirvec_delete(&dir->parent->children, dir); + + /* now we let the main task recursively delete everything under us */ serialized_delete(dir, UPDATE_TYPE_DIR); } -- cgit v1.2.3 From d60f11020f4901b806c822ef5ab2d87bbb9e4077 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sun, 12 Oct 2008 02:06:08 -0700 Subject: update: simplify the serialized_delete usage a bit Pass a callback and argument to it instead of requiring explicit type. --- src/update.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-) (limited to 'src/update.c') diff --git a/src/update.c b/src/update.c index 9c0113446..9ede72759 100644 --- a/src/update.c +++ b/src/update.c @@ -46,9 +46,7 @@ static const unsigned update_task_id_max = 1 << 15; static unsigned update_task_id; -enum update_type { UPDATE_TYPE_SONG, UPDATE_TYPE_DIR }; - -static enum update_type delete_type; +static void (*delete_fn)(void *); static void *delete; @@ -66,24 +64,33 @@ static void directory_set_stat(struct directory *dir, const struct stat *st) dir->stat = 1; } -static void serialized_delete(void *del, enum update_type type) +static void serialized_delete(void (*fn)(void *), void *del) { cond_enter(&delete_cond); assert(!delete); - delete_type = type; + assert(!delete_fn); + delete_fn = fn; delete = del; wakeup_main_task(); do { cond_wait(&delete_cond); } while (delete); cond_leave(&delete_cond); } +static void delete_song_safely(struct mpd_song *song) +{ + char tmp[MPD_PATH_MAX]; + LOG("removing: %s\n", song_get_url(song, tmp)); + deleteASongFromPlaylist(song); + song_free(song); +} + static void delete_song(struct directory *dir, struct mpd_song *del) { /* first, prevent traversers in main task from getting this */ songvec_delete(&dir->songs, del); /* now take it out of the playlist (in the main_task) and free */ - serialized_delete(del, UPDATE_TYPE_SONG); + serialized_delete((void (*)(void *))delete_song_safely, del); } /** @@ -97,7 +104,7 @@ static void delete_directory(struct directory *dir) dirvec_delete(&dir->parent->children, dir); /* now we let the main task recursively delete everything under us */ - serialized_delete(dir, UPDATE_TYPE_DIR); + serialized_delete((void (*)(void *))directory_free, dir); } struct delete_data { @@ -408,19 +415,9 @@ void reap_update_task(void) return; cond_enter(&delete_cond); - if (delete) { - switch (delete_type) { - case UPDATE_TYPE_SONG: { - char tmp[MPD_PATH_MAX]; - LOG("removing: %s\n", song_get_url(delete, tmp)); - deleteASongFromPlaylist(delete); - song_free(delete); - } - break; - case UPDATE_TYPE_DIR: - directory_free(delete); - } - delete = NULL; + if (delete && delete_fn) { + delete_fn(delete); + delete = delete_fn = NULL; cond_signal(&delete_cond); } cond_leave(&delete_cond); -- cgit v1.2.3