From bf402578ab5bd9bc35bb5539d381d5612d19d40b Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Thu, 2 Oct 2008 03:24:21 -0700 Subject: Revert "Start using song pointers in core data structures" This actually opened us up to making lock dependencies more difficult than they needed to be now that we have threaded updates. We would always use the memory anyways, just in the stack instead of bss. --- src/decode.c | 33 ++++++++++++++------------------- src/decode.h | 2 +- src/inputPlugins/wavpack_plugin.c | 6 ++---- src/outputBuffer.c | 1 - src/player_error.c | 36 +++++++++++++++++++----------------- src/player_error.h | 5 +---- src/playlist.c | 8 ++++++-- src/playlist.h | 2 +- 8 files changed, 44 insertions(+), 49 deletions(-) diff --git a/src/decode.c b/src/decode.c index e0685e0e9..dd1ea9396 100644 --- a/src/decode.c +++ b/src/decode.c @@ -173,12 +173,9 @@ static int decode_start(void) InputStream is; InputPlugin *plugin = NULL; char path_max_fs[MPD_PATH_MAX]; - char path_max_utf8[MPD_PATH_MAX]; assert(pthread_equal(pthread_self(), dc.thread)); assert(dc.state == DC_STATE_DECODE); - assert(dc.current_song); - get_song_url(path_max_utf8, dc.current_song); - assert(*path_max_utf8); + assert(*dc.utf8url); switch (dc.action) { case DC_ACTION_START: @@ -193,20 +190,20 @@ static int decode_start(void) default: assert("unknown action!" && 0); } - if (isRemoteUrl(path_max_utf8)) { - pathcpy_trunc(path_max_fs, path_max_utf8); + if (isRemoteUrl(dc.utf8url)) { + pathcpy_trunc(path_max_fs, dc.utf8url); } else { rmp2amp_r(path_max_fs, - utf8_to_fs_charset(path_max_fs, path_max_utf8)); + utf8_to_fs_charset(path_max_fs, dc.utf8url)); } if (openInputStream(&is, path_max_fs) < 0) { DEBUG("couldn't open song: %s\n", path_max_fs); - player_seterror(PLAYER_ERROR_FILENOTFOUND, dc.current_song); + player_seterror(PLAYER_ERROR_FILENOTFOUND, dc.utf8url); return err; } - if (isRemoteUrl(path_max_utf8)) { + if (isRemoteUrl(dc.utf8url)) { unsigned int next = 0; /* first we try mime types: */ @@ -224,7 +221,7 @@ static int decode_start(void) /* if that fails, try suffix matching the URL: */ if (plugin == NULL) { - const char *s = getSuffix(path_max_utf8); + const char *s = getSuffix(dc.utf8url); next = 0; while (err && (plugin = getInputPluginFromSuffix(s, next++))) { if (!plugin->streamDecodeFunc) @@ -250,7 +247,7 @@ static int decode_start(void) } } else { unsigned int next = 0; - const char *s = getSuffix(path_max_utf8); + const char *s = getSuffix(dc.utf8url); while (err && (plugin = getInputPluginFromSuffix(s, next++))) { if (!plugin->streamTypes & INPUT_PLUGIN_STREAM_FILE) continue; @@ -273,9 +270,9 @@ static int decode_start(void) if (err) { if (plugin) - player_seterror(PLAYER_ERROR_SYSTEM, dc.current_song); + player_seterror(PLAYER_ERROR_SYSTEM, dc.utf8url); else - player_seterror(PLAYER_ERROR_UNKTYPE, dc.current_song); + player_seterror(PLAYER_ERROR_UNKTYPE, dc.utf8url); } if (player_errno) ERROR("player_error: %s\n", player_strerror()); @@ -299,20 +296,18 @@ static void * decoder_task(mpd_unused void *arg) case DC_STATE_DECODE: /* DEBUG(__FILE__": %s %d\n", __func__, __LINE__); */ /* DEBUG("dc.action: %d\n", (int)dc.action); */ - if ((dc.current_song = playlist_queued_song())) { - char p[MPD_PATH_MAX]; + if (playlist_queued_url(dc.utf8url)) { int err; ob_advance_sequence(); - get_song_url(p, dc.current_song); - DEBUG("decoding song: %s\n", p); + DEBUG("decoding song: %s\n", dc.utf8url); err = decode_start(); - DEBUG("DONE decoding song: %s\n", p); + DEBUG("DONE decoding song: %s\n", dc.utf8url); if (err) ob_trigger_action(OB_ACTION_RESET); else ob_flush(); - dc.current_song = NULL; + dc.utf8url[0] = '\0'; } finalize_per_track_actions(); playlist_queue_next(); diff --git a/src/decode.h b/src/decode.h index 61eeee078..71e7a498e 100644 --- a/src/decode.h +++ b/src/decode.h @@ -43,7 +43,7 @@ enum dc_state { }; struct decoder_control { - Song * current_song; /* only needed for wavpack, remove? */ + char utf8url[MPD_PATH_MAX]; /* only needed for wavpack, remove? */ enum dc_state state; /* rw=dc.thread, r=main */ enum dc_action action; /* rw protected by action_cond */ float total_time; /* w=dc.thread, r=main */ diff --git a/src/inputPlugins/wavpack_plugin.c b/src/inputPlugins/wavpack_plugin.c index 8bba8ae2b..b6da51e5e 100644 --- a/src/inputPlugins/wavpack_plugin.c +++ b/src/inputPlugins/wavpack_plugin.c @@ -429,10 +429,8 @@ static int wavpack_open_wvc(InputStream *is_wvc) char wvc_url[MPD_PATH_MAX]; size_t len; - /* This is the only reader of dc.current_song */ - if (!get_song_url(wvc_url, dc.current_song)) - return 0; - + /* This is the only reader of dc.utf8url (in inputPlugins) */ + pathcpy_trunc(wvc_url, dc.utf8url); len = strlen(wvc_url); if ((len + 2) >= MPD_PATH_MAX) return 0; diff --git a/src/outputBuffer.c b/src/outputBuffer.c index 9a659db41..7216a9c9c 100644 --- a/src/outputBuffer.c +++ b/src/outputBuffer.c @@ -22,7 +22,6 @@ #include "normalize.h" #include "ringbuf.h" #include "condition.h" -#include "song.h" #include "main_notify.h" #include "player_error.h" #include "log.h" diff --git a/src/player_error.c b/src/player_error.c index 4c7f7b9de..38e76e2c4 100644 --- a/src/player_error.c +++ b/src/player_error.c @@ -4,50 +4,52 @@ #include "path.h" enum player_error player_errno; -Song *player_errsong; +static char errsong_url[MPD_PATH_MAX]; void player_clearerror(void) { player_errno = PLAYER_ERROR_NONE; - player_errsong = NULL; + *errsong_url = '\0'; } -void player_seterror(enum player_error err, Song *song) +void player_seterror(enum player_error err, const char *url) { if (player_errno) ERROR("Clobbering existing error: %s\n", player_strerror()); player_errno = err; - player_errsong = song; + pathcpy_trunc(errsong_url, url); } const char *player_strerror(void) { /* static OK here, only one user in main task */ static char error[MPD_PATH_MAX + 64]; /* still too much */ - char path_max_tmp[MPD_PATH_MAX]; - *error = '\0'; /* likely */ + const char *ret = NULL; switch (player_errno) { - case PLAYER_ERROR_NONE: break; + case PLAYER_ERROR_NONE: + ret = ""; + break; case PLAYER_ERROR_FILE: - snprintf(error, sizeof(error), "problems decoding \"%s\"", - get_song_url(path_max_tmp, player_errsong)); + snprintf(error, sizeof(error), + "problems decoding \"%s\"", errsong_url); break; case PLAYER_ERROR_AUDIO: - strcpy(error, "problems opening audio device"); + ret = "problems opening audio device"; break; case PLAYER_ERROR_SYSTEM: - strcpy(error, "system error occured"); + /* DONTFIX: misspelling "occurred" here is client-visible */ + ret = "system error occured"; break; case PLAYER_ERROR_UNKTYPE: - snprintf(error, sizeof(error), "file type of \"%s\" is unknown", - get_song_url(path_max_tmp, player_errsong)); - case PLAYER_ERROR_FILENOTFOUND: snprintf(error, sizeof(error), - "file \"%s\" does not exist or is inaccessible", - get_song_url(path_max_tmp, player_errsong)); + "file type of \"%s\" is unknown", errsong_url); break; + case PLAYER_ERROR_FILENOTFOUND: + snprintf(error, sizeof(error), + "file \"%s\" does not exist or is inaccessible", + errsong_url); } - return *error ? error : NULL; + return ret ? ret : error; } diff --git a/src/player_error.h b/src/player_error.h index c90c98420..997cb4604 100644 --- a/src/player_error.h +++ b/src/player_error.h @@ -19,8 +19,6 @@ #ifndef PLAYER_ERROR_H #define PLAYER_ERROR_H -#include "song.h" - enum player_error { PLAYER_ERROR_NONE = 0, PLAYER_ERROR_FILE, @@ -31,10 +29,9 @@ enum player_error { }; extern enum player_error player_errno; -extern Song *player_errsong; void player_clearerror(void); -void player_seterror(enum player_error err, Song *song); +void player_seterror(enum player_error err, const char *url); const char *player_strerror(void); #endif /* PLAYER_ERROR_H */ diff --git a/src/playlist.c b/src/playlist.c index ca903036d..2ab09ebff 100644 --- a/src/playlist.c +++ b/src/playlist.c @@ -554,11 +554,15 @@ void playlist_queue_next(void) wakeup_main_task(); } -Song *playlist_queued_song(void) +char *playlist_queued_url(char utf8url[MPD_PATH_MAX]) { + Song *song; + assert(pthread_equal(pthread_self(), dc.thread)); pthread_mutex_lock(&queue_lock); - return song_at(playlist.queued); + song = song_at(playlist.queued); + + return song ? get_song_url(utf8url, song) : NULL; } static void queue_song_locked(int order_num) diff --git a/src/playlist.h b/src/playlist.h index 265b58356..df1bd9e7b 100644 --- a/src/playlist.h +++ b/src/playlist.h @@ -68,7 +68,7 @@ enum playlist_result playlistInfo(int fd, int song); enum playlist_result playlistId(int fd, int song); -Song *playlist_queued_song(void); +char *playlist_queued_url(char utf8url[MPD_PATH_MAX]); void playlist_queue_next(void); -- cgit v1.2.3 From 1d9e9a408798358ea40012adc26eb6e670755337 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Thu, 2 Oct 2008 04:22:32 -0700 Subject: playlist: small lines of code reduction Hopefully this makes the code feel less claustrophobic... --- src/playlist.c | 143 +++++++++++++++++++-------------------------------------- 1 file changed, 47 insertions(+), 96 deletions(-) diff --git a/src/playlist.c b/src/playlist.c index 2ab09ebff..474e11f4b 100644 --- a/src/playlist.c +++ b/src/playlist.c @@ -100,14 +100,10 @@ static void randomizeOrder(int start, int end); static void incrPlaylistVersion(void) { static unsigned long max = ((uint32_t) 1 << 31) - 1; - playlist.version++; - if (playlist.version >= max) { - int i; - - for (i = 0; i < playlist.length; i++) { - playlist.songMod[i] = 0; - } + if (++playlist.version >= max) { + memset(playlist.songMod, 0, + playlist.length * sizeof(*playlist.songMod)); playlist.version = 1; } } @@ -116,9 +112,8 @@ void playlistVersionChange(void) { int i; - for (i = 0; i < playlist.length; i++) { + for (i = playlist.length; --i >= 0; ) playlist.songMod[i] = playlist.version; - } incrPlaylistVersion(); } @@ -128,12 +123,9 @@ static void incrPlaylistCurrent(void) if (playlist.current < 0) return; - if (playlist.current >= playlist.length - 1) { - if (playlist.repeat) - playlist.current = 0; - else - playlist.current = -1; - } else + if (playlist.current >= playlist.length - 1) + playlist.current = playlist.repeat ? 0 : -1; + else playlist.current++; } @@ -154,10 +146,9 @@ void initPlaylist(void) if (param) { playlist_max_length = strtol(param->value, &test, 10); - if (*test != '\0') { + if (*test != '\0') FATAL("max playlist length \"%s\" is not an integer, " "line %i\n", param->value, param->line); - } } playlist_saveAbsolutePaths = getBoolConfigParam( @@ -166,20 +157,17 @@ void initPlaylist(void) playlist_saveAbsolutePaths = DEFAULT_PLAYLIST_SAVE_ABSOLUTE_PATHS; - playlist.songs = xmalloc(sizeof(Song *) * playlist_max_length); + playlist.songs = xcalloc(playlist_max_length, sizeof(Song *)); playlist.songMod = xmalloc(sizeof(uint32_t) * playlist_max_length); playlist.order = xmalloc(sizeof(int) * playlist_max_length); playlist.idToPosition = xmalloc(sizeof(int) * playlist_max_length * PLAYLIST_HASH_MULT); playlist.positionToId = xmalloc(sizeof(int) * playlist_max_length); - memset(playlist.songs, 0, sizeof(char *) * playlist_max_length); - srandom(time(NULL)); - for (i = 0; i < playlist_max_length * PLAYLIST_HASH_MULT; i++) { + for (i = playlist_max_length * PLAYLIST_HASH_MULT; --i >= 0; ) playlist.idToPosition[i] = -1; - } } static int getNextId(void) @@ -187,10 +175,8 @@ static int getNextId(void) static int cur = -1; do { - cur++; - if (cur >= playlist_max_length * PLAYLIST_HASH_MULT) { + if (++cur >= playlist_max_length * PLAYLIST_HASH_MULT) cur = 0; - } } while (playlist.idToPosition[cur] != -1); return cur; @@ -199,10 +185,10 @@ static int getNextId(void) void finishPlaylist(void) { int i; - for (i = 0; i < playlist.length; i++) { - if (playlist.songs[i]->type == SONG_TYPE_URL) { + + for (i = playlist.length; --i >= 0; ) { + if (playlist.songs[i]->type == SONG_TYPE_URL) freeJustSong(playlist.songs[i]); - } } playlist.length = 0; @@ -225,10 +211,9 @@ void clearPlaylist(void) stopPlaylist(); - for (i = 0; i < playlist.length; i++) { - if (playlist.songs[i]->type == SONG_TYPE_URL) { + for (i = playlist.length; --i >= 0 ; ) { + if (playlist.songs[i]->type == SONG_TYPE_URL) freeJustSong(playlist.songs[i]); - } playlist.idToPosition[playlist.positionToId[i]] = -1; playlist.songs[i] = NULL; } @@ -248,10 +233,9 @@ void showPlaylist(int fd) int i; char path_max_tmp[MPD_PATH_MAX]; - for (i = 0; i < playlist.length; i++) { + for (i = 0; i < playlist.length; i++) fdprintf(fd, "%i:%s\n", i, get_song_url(path_max_tmp, playlist.songs[i])); - } } void savePlaylistState(int fd) @@ -305,10 +289,9 @@ static void loadPlaylistFromStateFile(FILE *fp, char *buffer, && current == song) { if (state == OB_STATE_PAUSE) ob_trigger_action(OB_ACTION_PAUSE_SET); - if (state != OB_STATE_STOP) { + if (state != OB_STATE_STOP) seekSongInPlaylist(playlist.length - 1, seek_time); - } } if (!myFgets(buffer, PLAYLIST_BUFFER_SIZE, fp)) state_file_fatal(); @@ -388,9 +371,8 @@ int playlistChanges(int fd, uint32_t version) for (i = 0; i < playlist.length; i++) { if (version > playlist.version || playlist.songMod[i] >= version || - playlist.songMod[i] == 0) { + playlist.songMod[i] == 0) printPlaylistSongInfo(fd, i); - } } return 0; @@ -403,10 +385,9 @@ int playlistChangesPosId(int fd, uint32_t version) for (i = 0; i < playlist.length; i++) { if (version > playlist.version || playlist.songMod[i] >= version || - playlist.songMod[i] == 0) { + playlist.songMod[i] == 0) fdprintf(fd, "cpos: %i\nId: %i\n", i, playlist.positionToId[i]); - } } return 0; @@ -522,7 +503,6 @@ static void queueNextSongInPlaylist(void) playlist.current = -1; } } else if (dc.state == DC_STATE_STOP) { - /* DEBUG("%s:%d (%d)\n", __func__, __LINE__, playlist.queued);*/ dc_trigger_action(DC_ACTION_START, 0); } } @@ -609,15 +589,13 @@ int addToStoredPlaylist(const char *url, const char *utf8file) DEBUG("add to stored playlist: %s\n", url); - song = getSongFromDB(url); - if (song) + if ((song = getSongFromDB(url))) return appendSongToStoredPlaylistByPath(utf8file, song); if (!isValidRemoteUtf8Url(url)) return ACK_ERROR_NO_EXIST; - song = newSong(url, SONG_TYPE_URL, NULL); - if (song) { + if ((song = newSong(url, SONG_TYPE_URL, NULL))) { int ret = appendSongToStoredPlaylistByPath(utf8file, song); freeJustSong(song); return ret; @@ -633,11 +611,10 @@ enum playlist_result addSongToPlaylist(Song * song, int *added_id) if (playlist.length == playlist_max_length) return PLAYLIST_RESULT_TOO_LARGE; - if (playlist_state == PLAYLIST_STATE_PLAY) { - if (playlist.queued >= 0 - && playlist.current == playlist.length - 1) - clear_queue(); - } + if (playlist_state == PLAYLIST_STATE_PLAY && + playlist.queued >= 0 && + playlist.current == playlist.length - 1) + clear_queue(); id = getNextId(); @@ -688,9 +665,8 @@ enum playlist_result swapSongsInPlaylist(int song1, int song2) return PLAYLIST_RESULT_BAD_RANGE; if (playlist_state == PLAYLIST_STATE_PLAY) { - if (playlist.queued >= 0) { + if (playlist.queued >= 0) queuedSong = playlist.order[playlist.queued]; - } assert(playlist.current >= 0 && playlist.current < playlist.length); currentSong = playlist.order[playlist.current]; @@ -759,21 +735,18 @@ enum playlist_result deleteFromPlaylist(int song) if (prev_queued >= 0 && (playlist.order[prev_queued] == song || playlist.order[playlist.current] == song)) { - /* DEBUG(__FILE__": %d (clearing)\n", __LINE__); */ clear_queue(); } } - if (playlist.songs[song]->type == SONG_TYPE_URL) { + if (playlist.songs[song]->type == SONG_TYPE_URL) freeJustSong(playlist.songs[song]); - } playlist.idToPosition[playlist.positionToId[song]] = -1; /* delete song from songs array */ - for (i = song; i < playlist.length - 1; i++) { + for (i = song; i < playlist.length - 1; i++) moveSongFromTo(i + 1, i); - } /* now find it in the order array */ for (i = 0; i < playlist.length - 1; i++) { if (playlist.order[i] == song) @@ -794,8 +767,6 @@ enum playlist_result deleteFromPlaylist(int song) incrPlaylistVersion(); - /* DEBUG("current: %d, songOrder: %d\n", playlist.current, songOrder); */ - /* DEBUG("playlist_state: %d\n", playlist_state); */ if (playlist_state != PLAYLIST_STATE_STOP && playlist.current == songOrder) stop_current = 1; @@ -808,13 +779,11 @@ enum playlist_result deleteFromPlaylist(int song) incrPlaylistCurrent(); } if (stop_current) { - /* DEBUG(__FILE__": %d\n", __LINE__); */ if (playlist.current >= 0 && songOrder > 0) play_order_num(playlist.current, 0); else stopPlaylist(); } else { - /* DEBUG(__FILE__": %d\n", __LINE__); */ queueNextSongInPlaylist(); } @@ -836,10 +805,9 @@ void deleteASongFromPlaylist(const Song * song) if (NULL == playlist.songs) return; - for (i = 0; i < playlist.length; i++) { - if (song == playlist.songs[i]) { + for (i = playlist.length; --i >= 0; ) { + if (mpd_unlikely(song == playlist.songs[i])) deleteFromPlaylist(i); - } } } @@ -888,8 +856,6 @@ enum playlist_result playPlaylist(int song, int stopOnError) { int i = song; - DEBUG("%s %d song(%d)\n", __func__, __LINE__, song); - player_clearerror(); if (song == -1) { @@ -900,11 +866,10 @@ enum playlist_result playPlaylist(int song, int stopOnError) ob_trigger_action(OB_ACTION_PAUSE_UNSET); return PLAYLIST_RESULT_SUCCESS; } - if (playlist.current >= 0 && playlist.current < playlist.length) { + if (playlist.current >= 0 && playlist.current < playlist.length) i = playlist.current; - } else { + else i = 0; - } } else if (song < 0 || song >= playlist.length) { return PLAYLIST_RESULT_BAD_RANGE; } @@ -933,9 +898,8 @@ enum playlist_result playPlaylist(int song, int stopOnError) enum playlist_result playPlaylistById(int id, int stopOnError) { - if (id == -1) { + if (id == -1) return playPlaylist(id, stopOnError); - } if (!song_id_exists(id)) return PLAYLIST_RESULT_NO_SUCH_SONG; @@ -961,8 +925,7 @@ static void sync_metadata(void) if (!(tag = metadata_pipe_current())) return; song = song_at(playlist.current); - if (!song || song->type != SONG_TYPE_URL || - tag_equal(song->tag, tag)) { + if (!song || song->type != SONG_TYPE_URL || tag_equal(song->tag, tag)) { tag_free(tag); return; } @@ -985,9 +948,8 @@ void syncPlayerAndPlaylist(void) playlist.queued != playlist.current && ob_synced() && dc.state == DC_STATE_STOP && - ob_get_state() != OB_STATE_PAUSE) { + ob_get_state() != OB_STATE_PAUSE) dc_trigger_action(DC_ACTION_START, 0); - } } void nextSongInPlaylist(void) @@ -1019,10 +981,9 @@ int getPlaylistRandomStatus(void) void setPlaylistRepeatStatus(int status) { - if (playlist_state == PLAYLIST_STATE_PLAY) { - if (playlist.repeat && !status && playlist.queued == 0) - clear_queue(); - } + if (playlist_state == PLAYLIST_STATE_PLAY && + playlist.repeat && !status && playlist.queued == 0) + clear_queue(); playlist.repeat = status; } @@ -1070,9 +1031,8 @@ enum playlist_result moveSongInPlaylist(int from, int to) tmpSong = playlist.songs[from]; tmpId = playlist.positionToId[from]; /* move songs to one less in from->to */ - for (i = from; i < to; i++) { + for (i = from; i < to; i++) moveSongFromTo(i + 1, i); - } /* move songs to one more in to->from */ for (i = from; i > to; i--) { moveSongFromTo(i - 1, i); @@ -1095,13 +1055,12 @@ enum playlist_result moveSongInPlaylist(int from, int to) } } } else { - if (playlist.current == from) { + if (playlist.current == from) playlist.current = to; - } else if (playlist.current > from && playlist.current <= to) { + else if (playlist.current > from && playlist.current <= to) playlist.current--; - } else if (playlist.current >= to && playlist.current < from) { + else if (playlist.current >= to && playlist.current < from) playlist.current++; - } if (queued_is_current) playlist.queued = playlist.current; } @@ -1133,7 +1092,7 @@ static void orderPlaylist(void) if (queued_is_current) playlist.queued = playlist.current; } - for (i = 0; i < playlist.length; i++) + for (i = playlist.length; --i >= 0; ) playlist.order[i] = i; } @@ -1158,7 +1117,6 @@ static void randomizeOrder(int start, int end) int queued_is_current = (playlist.queued == playlist.current); DEBUG("playlist: randomize from %i to %i\n", start, end); - DEBUG("%s:%d current: %d\n", __func__, __LINE__, playlist.current); if (!queued_is_current && playlist_state == PLAYLIST_STATE_PLAY && @@ -1176,7 +1134,6 @@ static void randomizeOrder(int start, int end) } if (queued_is_current) playlist.queued = playlist.current; - DEBUG("%s:%d current: %d\n", __func__, __LINE__, playlist.current); } void setPlaylistRandomStatus(int status) @@ -1190,11 +1147,8 @@ void setPlaylistRandomStatus(int status) randomizeOrder(0, playlist.length - 1); else orderPlaylist(); - if (playlist_state == PLAYLIST_STATE_PLAY) { + if (playlist_state == PLAYLIST_STATE_PLAY) queueNextSongInPlaylist(); - DEBUG("%s:%d queued: %d\n", - __func__,__LINE__,playlist.queued); - } } } @@ -1318,10 +1272,8 @@ enum playlist_result savePlaylist(const char *utf8file) int getPlaylistCurrentSong(void) { - DEBUG("%s:%d current: %d\n", __func__, __LINE__, playlist.current); - if (playlist.current >= 0 && playlist.current < playlist.length) { + if (playlist.current >= 0 && playlist.current < playlist.length) return playlist.order[playlist.current]; - } return -1; } @@ -1406,9 +1358,8 @@ int PlaylistInfo(int fd, const char *utf8file, int detail) } } - if (!wrote) { + if (!wrote) fdprintf(fd, SONG_FILE "%s\n", temp); - } node = node->nextNode; } -- cgit v1.2.3 From 662b6bc9d75d879e52456f680fb1c4fdd0053996 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Fri, 3 Oct 2008 02:19:54 -0700 Subject: song: start avoiding race in updateSongInfo This is still not SMP-safe yet, as it needs at least a barrier before calling tag_free(old_tag). Locks may be simpler to implement and the potential performance gain of avoiding locks may not be worth it on infrequently modified data structures... --- src/song.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/song.c b/src/song.c index 5c5024391..dc100899a 100644 --- a/src/song.c +++ b/src/song.c @@ -207,19 +207,22 @@ int updateSongInfo(Song * song) unsigned int next = 0; char path_max_tmp[MPD_PATH_MAX]; char abs_path[MPD_PATH_MAX]; + struct mpd_tag *old_tag = song->tag; + struct mpd_tag *new_tag = NULL; utf8_to_fs_charset(abs_path, get_song_url(path_max_tmp, song)); rmp2amp_r(abs_path, abs_path); - if (song->tag) - tag_free(song->tag); - - song->tag = NULL; - - while (!song->tag && (plugin = isMusic(abs_path, - &(song->mtime), - next++))) { - song->tag = plugin->tagDupFunc(abs_path); + while ((plugin = isMusic(abs_path, &song->mtime, next++))) { + if ((new_tag = plugin->tagDupFunc(abs_path))) + break; + } + if (new_tag && tag_equal(new_tag, old_tag)) { + tag_free(new_tag); + } else { + song->tag = new_tag; + if (old_tag) + tag_free(old_tag); } if (!song->tag || song->tag->time < 0) return -1; -- cgit v1.2.3 From ddc39977bc530954a2497c96cb78e7def0747908 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Mon, 29 Sep 2008 03:35:54 -0700 Subject: directory: streamline deletes Instead of relying on the shortname, just pass the song pointer to prevent redundant lookups during deletes. --- src/directory.c | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/src/directory.c b/src/directory.c index fc9c6b41d..0c34e2ea9 100644 --- a/src/directory.c +++ b/src/directory.c @@ -70,8 +70,7 @@ static enum update_return updateDirectory(Directory * directory); static void deleteEmptyDirectoriesInDirectory(Directory * directory); -static void removeSongFromDirectory(Directory * directory, - const char *shortname); +static void delete_song(Directory *dir, Song *del); static enum update_return addSubDirectoryToDirectory(Directory * directory, const char *name, struct stat *st); @@ -212,16 +211,12 @@ static void freeDirectory(Directory * directory) /*getDirectoryPath(NULL); */ } -static void removeSongFromDirectory(Directory * directory, const char *shortname) +static void delete_song(Directory *dir, Song *del) { - Song *song = songvec_find(&directory->songs, shortname); - - if (song) { - char path_max_tmp[MPD_PATH_MAX]; /* wasteful */ - LOG("removing: %s\n", get_song_url(path_max_tmp, song)); - songvec_delete(&directory->songs, song); - freeSong(song); /* FIXME racy */ - } + char path_max_tmp[MPD_PATH_MAX]; /* wasteful */ + LOG("removing: %s\n", get_song_url(path_max_tmp, del)); + songvec_delete(&dir->songs, del); + freeSong(del); /* FIXME racy */ } static void deleteEmptyDirectoriesInDirectory(Directory * directory) @@ -256,7 +251,7 @@ updateInDirectory(Directory * directory, const char *name) } else if (st.st_mtime != song->mtime) { LOG("updating %s\n", name); if (updateSongInfo(song) < 0) - removeSongFromDirectory(directory, shortname); + delete_song(directory, song); return UPDATE_RETURN_UPDATED; } } else if (S_ISDIR(st.st_mode)) { @@ -308,7 +303,7 @@ removeDeletedFromDirectory(char *path_max_tmp, Directory * directory) strcpy(path_max_tmp, song->url); if (!isFile(path_max_tmp, NULL)) { - removeSongFromDirectory(directory, song->url); + delete_song(directory, song); ret = UPDATE_RETURN_UPDATED; } } @@ -322,6 +317,7 @@ static Directory *addDirectoryPathToDB(const char *utf8path) char *parent; Directory *parentDirectory; Directory *directory; + Song *conflicting; parent = parent_path(path_max_tmp, utf8path); @@ -348,7 +344,10 @@ static Directory *addDirectoryPathToDB(const char *utf8path) /* if we're adding directory paths, make sure to delete filenames with potentially the same name */ - removeSongFromDirectory(parentDirectory, mpd_basename(directory->path)); + conflicting = songvec_find(&parentDirectory->songs, + mpd_basename(directory->path)); + if (conflicting) + delete_song(parentDirectory, conflicting); return directory; } @@ -419,14 +418,13 @@ static enum update_return updatePath(const char *utf8path) else if (updateSongInfo(song) == 0) return UPDATE_RETURN_UPDATED; else { - removeSongFromDirectory(parentDirectory, - song->url); + delete_song(parentDirectory, song); return UPDATE_RETURN_UPDATED; } } /* if updateDirectory fails, means we should delete it */ else { - removeSongFromDirectory(parentDirectory, song->url); + delete_song(parentDirectory, song); ret = UPDATE_RETURN_UPDATED; /* don't return, path maybe a directory now */ } -- cgit v1.2.3 From 41a44f79ee1e559257fb6b2c88e1eb140fe657bf Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Fri, 3 Oct 2008 15:16:33 -0700 Subject: main_notify: define main_task so we can use it for assertions It'll be easier to keep track of what code runs in what task/thread this way. --- src/main_notify.c | 3 +++ src/main_notify.h | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/src/main_notify.c b/src/main_notify.c index 2c546633d..04ceac62d 100644 --- a/src/main_notify.c +++ b/src/main_notify.c @@ -24,6 +24,8 @@ #include "gcc.h" #include "log.h" +pthread_t main_task; + static struct ioOps main_notify_IO; static int main_pipe[2]; @@ -55,6 +57,7 @@ static int ioops_consume(int fd_count, fd_set * rfds, void init_main_notify(void) { + main_task = pthread_self(); init_async_pipe(main_pipe); main_notify_IO.fdset = ioops_fdset; main_notify_IO.consume = ioops_consume; diff --git a/src/main_notify.h b/src/main_notify.h index db36042a7..dc743b833 100644 --- a/src/main_notify.h +++ b/src/main_notify.h @@ -21,6 +21,10 @@ #ifndef MAIN_NOTIFY_H #define MAIN_NOTIFY_H +#include + +extern pthread_t main_task; + void init_main_notify(void); void wakeup_main_task(void); -- cgit v1.2.3 From 8d9a77bfcb2e37728f1a683a74d266d145aff14c Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Fri, 3 Oct 2008 17:03:53 -0700 Subject: directory: simplify list update handling logic Now the "update" command can be issued multiple times regardless of whether the client is in list mode or not. We serialize the update tasks to prevent updates from trampling over each other and will spawn another update task once the current one is finished updating and reaped. Right now we cap the queue size to 32 which is probably enough (I bet most people usually run update with no argument anyways); but we can make it grow/shrink dynamically if needed. There'll still be a hard-coded limit to prevent DoS attacks, though. --- src/command.c | 44 ++++----------------- src/directory.c | 120 ++++++++++++++++++++++++++++---------------------------- src/directory.h | 7 ++-- 3 files changed, 71 insertions(+), 100 deletions(-) diff --git a/src/command.c b/src/command.c index 70bfa78b5..07f422720 100644 --- a/src/command.c +++ b/src/command.c @@ -813,47 +813,17 @@ static int print_update_result(int fd, int ret) return -1; } -static int listHandleUpdate(int fd, - mpd_unused int *permission, - mpd_unused int argc, - char *argv[], - struct strnode *cmdnode, CommandEntry * cmd) -{ - static char **pathv; - static int pathc; - CommandEntry *nextCmd = NULL; - struct strnode *next = cmdnode->next; - int last = pathc++; - - pathv = xrealloc(pathv, pathc * sizeof(char *)); - pathv[last] = sanitizePathDup(argc == 2 ? argv[1] : ""); - - if (next) - nextCmd = getCommandEntryFromString(next->data, permission); - - if (cmd != nextCmd) { - int ret = print_update_result(fd, updateInit(pathc, pathv)); - if (pathc) { - assert(pathv); - free(pathv); - pathv = NULL; - pathc = 0; - } - return ret; - } - - return 0; -} - static int handleUpdate(int fd, mpd_unused int *permission, mpd_unused int argc, char *argv[]) { - char *pathv[1]; + char *path = NULL; assert(argc <= 2); - if (argc == 2) - pathv[0] = sanitizePathDup(argv[1]); - return print_update_result(fd, updateInit(argc - 1, pathv)); + if (argc == 2 && !(path = sanitizePathDup(argv[1]))) { + commandError(fd, ACK_ERROR_ARG, "invalid path"); + return -1; + } + return print_update_result(fd, directory_update_init(path)); } static int handleNext(mpd_unused int fd, mpd_unused int *permission, @@ -1289,7 +1259,7 @@ void initCommands(void) addCommand(COMMAND_PLAYLISTINFO, PERMISSION_READ, 0, 1, handlePlaylistInfo, NULL); addCommand(COMMAND_FIND, PERMISSION_READ, 2, -1, handleFind, NULL); addCommand(COMMAND_SEARCH, PERMISSION_READ, 2, -1, handleSearch, NULL); - addCommand(COMMAND_UPDATE, PERMISSION_ADMIN, 0, 1, handleUpdate, listHandleUpdate); + addCommand(COMMAND_UPDATE, PERMISSION_ADMIN, 0, 1, handleUpdate, NULL); addCommand(COMMAND_NEXT, PERMISSION_CONTROL, 0, 0, handleNext, NULL); addCommand(COMMAND_PREVIOUS, PERMISSION_CONTROL, 0, 0, handlePrevious, NULL); addCommand(COMMAND_LISTALL, PERMISSION_READ, 0, 1, handleListAll, NULL); diff --git a/src/directory.c b/src/directory.c index 0c34e2ea9..1cd9da9eb 100644 --- a/src/directory.c +++ b/src/directory.c @@ -52,13 +52,18 @@ enum update_progress { UPDATE_PROGRESS_DONE = 2 } progress; +/* make this dynamic?, or maybe this is big enough... */ +static char *update_paths[32]; +static size_t update_paths_nr; + static Directory *music_root; static time_t directory_dbModTime; static pthread_t update_thr; -static int directory_updateJobId; +static const int update_task_id_max = 1 << 15; +static int update_task_id; static int addToDirectory(Directory * directory, const char *name); @@ -97,87 +102,84 @@ static char *getDbFile(void) int isUpdatingDB(void) { - return (progress != UPDATE_PROGRESS_IDLE) ? directory_updateJobId : 0; -} - -void reap_update_task(void) -{ - enum update_return ret; - - if (progress != UPDATE_PROGRESS_DONE) - return; - if (pthread_join(update_thr, (void **)&ret)) - FATAL("error joining update thread: %s\n", strerror(errno)); - if (ret == UPDATE_RETURN_UPDATED) - playlistVersionChange(); - progress = UPDATE_PROGRESS_IDLE; + return (progress != UPDATE_PROGRESS_IDLE) ? update_task_id : 0; } -/* @argv represents a null-terminated array of (null-terminated) strings */ -static void * update_task(void *argv) +static void * update_task(void *_path) { enum update_return ret = UPDATE_RETURN_NOUPDATE; - if (argv) { - char **pathv; - for (pathv = (char **)argv; *pathv; pathv++) { - switch (updatePath(*pathv)) { - case UPDATE_RETURN_ERROR: - ret = UPDATE_RETURN_ERROR; - goto out; - case UPDATE_RETURN_NOUPDATE: - break; - case UPDATE_RETURN_UPDATED: - ret = UPDATE_RETURN_UPDATED; - } - free(*pathv); - } - free(argv); + if (_path) { + ret = updatePath((char *)_path); + free(_path); } else { ret = updateDirectory(music_root); } if (ret == UPDATE_RETURN_UPDATED && writeDirectoryDB() < 0) ret = UPDATE_RETURN_ERROR; -out: progress = UPDATE_PROGRESS_DONE; wakeup_main_task(); return (void *)ret; } -int updateInit(int argc, char *argv[]) +static void spawn_update_task(char *path) { pthread_attr_t attr; - char **pathv = NULL; - int i; - - if (progress != UPDATE_PROGRESS_IDLE) { - for (i = argc; --i >= 0; ) - free(argv[i]); - return -1; - } - for (i = argc; --i >= 0; ) { - if (!argv[i]) - return -2; - } + assert(pthread_equal(pthread_self(), main_task)); progress = UPDATE_PROGRESS_RUNNING; - if (argc > 0) { - pathv = xmalloc((argc + 1) * sizeof(char *)); - memcpy(pathv, argv, argc * sizeof(char *)); - pathv[argc] = NULL; - } - pthread_attr_init(&attr); - if (pthread_create(&update_thr, &attr, update_task, pathv)) + if (pthread_create(&update_thr, &attr, update_task, path)) FATAL("Failed to spawn update task: %s\n", strerror(errno)); - directory_updateJobId++; - if (directory_updateJobId > 1 << 15) - directory_updateJobId = 1; - DEBUG("updateInit: spawned update thread for update job id %i\n", - (int)directory_updateJobId); - return directory_updateJobId; + if (++update_task_id > update_task_id_max) + update_task_id = 1; + DEBUG("spawned thread for update job id %i\n", update_task_id); +} + +void reap_update_task(void) +{ + enum update_return ret; + + assert(pthread_equal(pthread_self(), main_task)); + + if (progress != UPDATE_PROGRESS_DONE) + return; + if (pthread_join(update_thr, (void **)&ret)) + FATAL("error joining update thread: %s\n", strerror(errno)); + if (ret == UPDATE_RETURN_UPDATED) + playlistVersionChange(); + + if (update_paths_nr) { + char *path = update_paths[0]; + memmove(&update_paths[0], &update_paths[1], + --update_paths_nr * sizeof(char *)); + spawn_update_task(path); + } else { + progress = UPDATE_PROGRESS_IDLE; + } +} + +int directory_update_init(char *path) +{ + assert(pthread_equal(pthread_self(), main_task)); + + if (progress != UPDATE_PROGRESS_IDLE) { + int next_task_id; + + if (!path) + return -1; + if (update_paths_nr == ARRAY_SIZE(update_paths)) + return -1; + assert(update_paths_nr < ARRAY_SIZE(update_paths)); + update_paths[update_paths_nr++] = path; + next_task_id = update_task_id + update_paths_nr; + + return next_task_id > update_task_id_max ? 1 : next_task_id; + } + spawn_update_task(path); + return update_task_id; } static void directory_set_stat(Directory * dir, const struct stat *st) diff --git a/src/directory.h b/src/directory.h index eb1b35aa7..20b784166 100644 --- a/src/directory.h +++ b/src/directory.h @@ -43,11 +43,10 @@ int isUpdatingDB(void); /* * returns the non-negative update job ID on success, - * -1 if busy, -2 if invalid argument - * @argv itself is safe to free once updateInit returns, but the - * string values contained by @argv MUST NOT be freed manually + * returns -1 if busy + * @path will be freed by this function and should not be reused */ -int updateInit(int argc, char *argv[]); +int directory_update_init(char *path); void directory_init(void); -- cgit v1.2.3 From 18ba438d902c801c5f1a6a37d43c87a779e06a84 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Fri, 3 Oct 2008 17:18:58 -0700 Subject: command: get rid of specialized list handlers commands should really not behave differently if they're issued inside a command list or not; so stop having special handler functions to deal with them. "update" was the only command that used this functionality and I changed that in the last commit to serialize access. --- src/command.c | 173 +++++++++++++++++++++++----------------------------------- 1 file changed, 67 insertions(+), 106 deletions(-) diff --git a/src/command.c b/src/command.c index 07f422720..851e4f67d 100644 --- a/src/command.c +++ b/src/command.c @@ -127,8 +127,6 @@ typedef struct _CommandEntry CommandEntry; typedef int (*CommandHandlerFunction) (int, int *, int, char **); -typedef int (*CommandListHandlerFunction) - (int, int *, int, char **, struct strnode *, CommandEntry *); /* if min: -1 don't check args * * if max: -1 no max args */ @@ -138,7 +136,6 @@ struct _CommandEntry { int max; int reqPermission; CommandHandlerFunction handler; - CommandListHandlerFunction listHandler; }; /* this should really be "need a non-negative integer": */ @@ -152,23 +149,8 @@ static const char check_non_negative[] = "\"%s\" is not an integer >= 0"; static const char *current_command; static int command_listNum; - -static CommandEntry *getCommandEntryFromString(char *string, int *permission); - static List *commandList; -static CommandEntry *newCommandEntry(void) -{ - CommandEntry *cmd = xmalloc(sizeof(CommandEntry)); - cmd->cmd = NULL; - cmd->min = 0; - cmd->max = 0; - cmd->handler = NULL; - cmd->listHandler = NULL; - cmd->reqPermission = 0; - return cmd; -} - static void command_error_va(int fd, int error, const char *fmt, va_list args) { if (current_command && fd != STDERR_FILENO) { @@ -271,15 +253,13 @@ static void addCommand(const char *name, int reqPermission, int minargs, int maxargs, - CommandHandlerFunction handler_func, - CommandListHandlerFunction listHandler_func) + CommandHandlerFunction handler_func) { - CommandEntry *cmd = newCommandEntry(); + CommandEntry *cmd = xmalloc(sizeof(CommandEntry)); cmd->cmd = name; cmd->min = minargs; cmd->max = maxargs; cmd->handler = handler_func; - cmd->listHandler = listHandler_func; cmd->reqPermission = reqPermission; insertInList(commandList, cmd->cmd, cmd); @@ -1233,70 +1213,70 @@ void initCommands(void) { commandList = makeList(free, 1); - /* addCommand(name, permission, min, max, handler, list handler); */ - addCommand(COMMAND_PLAY, PERMISSION_CONTROL, 0, 1, handlePlay, NULL); - addCommand(COMMAND_PLAYID, PERMISSION_CONTROL, 0, 1, handlePlayId, NULL); - addCommand(COMMAND_STOP, PERMISSION_CONTROL, 0, 0, handleStop, NULL); - addCommand(COMMAND_CURRENTSONG, PERMISSION_READ, 0, 0, handleCurrentSong, NULL); - addCommand(COMMAND_PAUSE, PERMISSION_CONTROL, 0, 1, handlePause, NULL); - addCommand(COMMAND_STATUS, PERMISSION_READ, 0, 0, commandStatus, NULL); - addCommand(COMMAND_KILL, PERMISSION_ADMIN, -1, -1, handleKill, NULL); - addCommand(COMMAND_CLOSE, PERMISSION_NONE, -1, -1, handleClose, NULL); - addCommand(COMMAND_ADD, PERMISSION_ADD, 1, 1, handleAdd, NULL); - addCommand(COMMAND_ADDID, PERMISSION_ADD, 1, 2, handleAddId, NULL); - addCommand(COMMAND_DELETE, PERMISSION_CONTROL, 1, 1, handleDelete, NULL); - addCommand(COMMAND_DELETEID, PERMISSION_CONTROL, 1, 1, handleDeleteId, NULL); - addCommand(COMMAND_PLAYLIST, PERMISSION_READ, 0, 0, handlePlaylist, NULL); - addCommand(COMMAND_PLAYLISTID, PERMISSION_READ, 0, 1, handlePlaylistId, NULL); - addCommand(COMMAND_SHUFFLE, PERMISSION_CONTROL, 0, 0, handleShuffle, NULL); - addCommand(COMMAND_CLEAR, PERMISSION_CONTROL, 0, 0, handleClear, NULL); - addCommand(COMMAND_SAVE, PERMISSION_CONTROL, 1, 1, handleSave, NULL); - addCommand(COMMAND_LOAD, PERMISSION_ADD, 1, 1, handleLoad, NULL); - addCommand(COMMAND_LISTPLAYLIST, PERMISSION_READ, 1, 1, handleListPlaylist, NULL); - addCommand(COMMAND_LISTPLAYLISTINFO, PERMISSION_READ, 1, 1, handleListPlaylistInfo, NULL); - addCommand(COMMAND_LSINFO, PERMISSION_READ, 0, 1, handleLsInfo, NULL); - addCommand(COMMAND_RM, PERMISSION_CONTROL, 1, 1, handleRm, NULL); - addCommand(COMMAND_PLAYLISTINFO, PERMISSION_READ, 0, 1, handlePlaylistInfo, NULL); - addCommand(COMMAND_FIND, PERMISSION_READ, 2, -1, handleFind, NULL); - addCommand(COMMAND_SEARCH, PERMISSION_READ, 2, -1, handleSearch, NULL); - addCommand(COMMAND_UPDATE, PERMISSION_ADMIN, 0, 1, handleUpdate, NULL); - addCommand(COMMAND_NEXT, PERMISSION_CONTROL, 0, 0, handleNext, NULL); - addCommand(COMMAND_PREVIOUS, PERMISSION_CONTROL, 0, 0, handlePrevious, NULL); - addCommand(COMMAND_LISTALL, PERMISSION_READ, 0, 1, handleListAll, NULL); - addCommand(COMMAND_VOLUME, PERMISSION_CONTROL, 1, 1, handleVolume, NULL); - addCommand(COMMAND_REPEAT, PERMISSION_CONTROL, 1, 1, handleRepeat, NULL); - addCommand(COMMAND_RANDOM, PERMISSION_CONTROL, 1, 1, handleRandom, NULL); - addCommand(COMMAND_STATS, PERMISSION_READ, 0, 0, handleStats, NULL); - addCommand(COMMAND_CLEAR_ERROR, PERMISSION_CONTROL, 0, 0, handleClearError, NULL); - addCommand(COMMAND_LIST, PERMISSION_READ, 1, -1, handleList, NULL); - addCommand(COMMAND_MOVE, PERMISSION_CONTROL, 2, 2, handleMove, NULL); - addCommand(COMMAND_MOVEID, PERMISSION_CONTROL, 2, 2, handleMoveId, NULL); - addCommand(COMMAND_SWAP, PERMISSION_CONTROL, 2, 2, handleSwap, NULL); - addCommand(COMMAND_SWAPID, PERMISSION_CONTROL, 2, 2, handleSwapId, NULL); - addCommand(COMMAND_SEEK, PERMISSION_CONTROL, 2, 2, handleSeek, NULL); - addCommand(COMMAND_SEEKID, PERMISSION_CONTROL, 2, 2, handleSeekId, NULL); - addCommand(COMMAND_LISTALLINFO, PERMISSION_READ, 0, 1, handleListAllInfo, NULL); - addCommand(COMMAND_PING, PERMISSION_NONE, 0, 0, handlePing, NULL); - addCommand(COMMAND_SETVOL, PERMISSION_CONTROL, 1, 1, handleSetVol, NULL); - addCommand(COMMAND_PASSWORD, PERMISSION_NONE, 1, 1, handlePassword, NULL); - addCommand(COMMAND_CROSSFADE, PERMISSION_CONTROL, 1, 1, handleCrossfade, NULL); - addCommand(COMMAND_URL_HANDLERS, PERMISSION_READ, 0, 0, handleUrlHandlers, NULL); - addCommand(COMMAND_PLCHANGES, PERMISSION_READ, 1, 1, handlePlaylistChanges, NULL); - addCommand(COMMAND_PLCHANGESPOSID, PERMISSION_READ, 1, 1, handlePlaylistChangesPosId, NULL); - addCommand(COMMAND_ENABLE_DEV, PERMISSION_ADMIN, 1, 1, handleEnableDevice, NULL); - addCommand(COMMAND_DISABLE_DEV, PERMISSION_ADMIN, 1, 1, handleDisableDevice, NULL); - addCommand(COMMAND_DEVICES, PERMISSION_READ, 0, 0, handleDevices, NULL); - addCommand(COMMAND_COMMANDS, PERMISSION_NONE, 0, 0, handleCommands, NULL); - addCommand(COMMAND_NOTCOMMANDS, PERMISSION_NONE, 0, 0, handleNotcommands, NULL); - addCommand(COMMAND_PLAYLISTCLEAR, PERMISSION_CONTROL, 1, 1, handlePlaylistClear, NULL); - addCommand(COMMAND_PLAYLISTADD, PERMISSION_CONTROL, 2, 2, handlePlaylistAdd, NULL); - addCommand(COMMAND_PLAYLISTFIND, PERMISSION_READ, 2, -1, handlePlaylistFind, NULL); - addCommand(COMMAND_PLAYLISTSEARCH, PERMISSION_READ, 2, -1, handlePlaylistSearch, NULL); - addCommand(COMMAND_PLAYLISTMOVE, PERMISSION_CONTROL, 3, 3, handlePlaylistMove, NULL); - addCommand(COMMAND_PLAYLISTDELETE, PERMISSION_CONTROL, 2, 2, handlePlaylistDelete, NULL); - addCommand(COMMAND_TAGTYPES, PERMISSION_READ, 0, 0, handleTagTypes, NULL); - addCommand(COMMAND_COUNT, PERMISSION_READ, 2, -1, handleCount, NULL); - addCommand(COMMAND_RENAME, PERMISSION_CONTROL, 2, 2, handleRename, NULL); + /* addCommand(name, permission, min, max, handler); */ + addCommand(COMMAND_PLAY, PERMISSION_CONTROL, 0, 1, handlePlay); + addCommand(COMMAND_PLAYID, PERMISSION_CONTROL, 0, 1, handlePlayId); + addCommand(COMMAND_STOP, PERMISSION_CONTROL, 0, 0, handleStop); + addCommand(COMMAND_CURRENTSONG, PERMISSION_READ, 0, 0, handleCurrentSong); + addCommand(COMMAND_PAUSE, PERMISSION_CONTROL, 0, 1, handlePause); + addCommand(COMMAND_STATUS, PERMISSION_READ, 0, 0, commandStatus); + addCommand(COMMAND_KILL, PERMISSION_ADMIN, -1, -1, handleKill); + addCommand(COMMAND_CLOSE, PERMISSION_NONE, -1, -1, handleClose); + addCommand(COMMAND_ADD, PERMISSION_ADD, 1, 1, handleAdd); + addCommand(COMMAND_ADDID, PERMISSION_ADD, 1, 2, handleAddId); + addCommand(COMMAND_DELETE, PERMISSION_CONTROL, 1, 1, handleDelete); + addCommand(COMMAND_DELETEID, PERMISSION_CONTROL, 1, 1, handleDeleteId); + addCommand(COMMAND_PLAYLIST, PERMISSION_READ, 0, 0, handlePlaylist); + addCommand(COMMAND_PLAYLISTID, PERMISSION_READ, 0, 1, handlePlaylistId); + addCommand(COMMAND_SHUFFLE, PERMISSION_CONTROL, 0, 0, handleShuffle); + addCommand(COMMAND_CLEAR, PERMISSION_CONTROL, 0, 0, handleClear); + addCommand(COMMAND_SAVE, PERMISSION_CONTROL, 1, 1, handleSave); + addCommand(COMMAND_LOAD, PERMISSION_ADD, 1, 1, handleLoad); + addCommand(COMMAND_LISTPLAYLIST, PERMISSION_READ, 1, 1, handleListPlaylist); + addCommand(COMMAND_LISTPLAYLISTINFO, PERMISSION_READ, 1, 1, handleListPlaylistInfo); + addCommand(COMMAND_LSINFO, PERMISSION_READ, 0, 1, handleLsInfo); + addCommand(COMMAND_RM, PERMISSION_CONTROL, 1, 1, handleRm); + addCommand(COMMAND_PLAYLISTINFO, PERMISSION_READ, 0, 1, handlePlaylistInfo); + addCommand(COMMAND_FIND, PERMISSION_READ, 2, -1, handleFind); + addCommand(COMMAND_SEARCH, PERMISSION_READ, 2, -1, handleSearch); + addCommand(COMMAND_UPDATE, PERMISSION_ADMIN, 0, 1, handleUpdate); + addCommand(COMMAND_NEXT, PERMISSION_CONTROL, 0, 0, handleNext); + addCommand(COMMAND_PREVIOUS, PERMISSION_CONTROL, 0, 0, handlePrevious); + addCommand(COMMAND_LISTALL, PERMISSION_READ, 0, 1, handleListAll); + addCommand(COMMAND_VOLUME, PERMISSION_CONTROL, 1, 1, handleVolume); + addCommand(COMMAND_REPEAT, PERMISSION_CONTROL, 1, 1, handleRepeat); + addCommand(COMMAND_RANDOM, PERMISSION_CONTROL, 1, 1, handleRandom); + addCommand(COMMAND_STATS, PERMISSION_READ, 0, 0, handleStats); + addCommand(COMMAND_CLEAR_ERROR, PERMISSION_CONTROL, 0, 0, handleClearError); + addCommand(COMMAND_LIST, PERMISSION_READ, 1, -1, handleList); + addCommand(COMMAND_MOVE, PERMISSION_CONTROL, 2, 2, handleMove); + addCommand(COMMAND_MOVEID, PERMISSION_CONTROL, 2, 2, handleMoveId); + addCommand(COMMAND_SWAP, PERMISSION_CONTROL, 2, 2, handleSwap); + addCommand(COMMAND_SWAPID, PERMISSION_CONTROL, 2, 2, handleSwapId); + addCommand(COMMAND_SEEK, PERMISSION_CONTROL, 2, 2, handleSeek); + addCommand(COMMAND_SEEKID, PERMISSION_CONTROL, 2, 2, handleSeekId); + addCommand(COMMAND_LISTALLINFO, PERMISSION_READ, 0, 1, handleListAllInfo); + addCommand(COMMAND_PING, PERMISSION_NONE, 0, 0, handlePing); + addCommand(COMMAND_SETVOL, PERMISSION_CONTROL, 1, 1, handleSetVol); + addCommand(COMMAND_PASSWORD, PERMISSION_NONE, 1, 1, handlePassword); + addCommand(COMMAND_CROSSFADE, PERMISSION_CONTROL, 1, 1, handleCrossfade); + addCommand(COMMAND_URL_HANDLERS, PERMISSION_READ, 0, 0, handleUrlHandlers); + addCommand(COMMAND_PLCHANGES, PERMISSION_READ, 1, 1, handlePlaylistChanges); + addCommand(COMMAND_PLCHANGESPOSID, PERMISSION_READ, 1, 1, handlePlaylistChangesPosId); + addCommand(COMMAND_ENABLE_DEV, PERMISSION_ADMIN, 1, 1, handleEnableDevice); + addCommand(COMMAND_DISABLE_DEV, PERMISSION_ADMIN, 1, 1, handleDisableDevice); + addCommand(COMMAND_DEVICES, PERMISSION_READ, 0, 0, handleDevices); + addCommand(COMMAND_COMMANDS, PERMISSION_NONE, 0, 0, handleCommands); + addCommand(COMMAND_NOTCOMMANDS, PERMISSION_NONE, 0, 0, handleNotcommands); + addCommand(COMMAND_PLAYLISTCLEAR, PERMISSION_CONTROL, 1, 1, handlePlaylistClear); + addCommand(COMMAND_PLAYLISTADD, PERMISSION_CONTROL, 2, 2, handlePlaylistAdd); + addCommand(COMMAND_PLAYLISTFIND, PERMISSION_READ, 2, -1, handlePlaylistFind); + addCommand(COMMAND_PLAYLISTSEARCH, PERMISSION_READ, 2, -1, handlePlaylistSearch); + addCommand(COMMAND_PLAYLISTMOVE, PERMISSION_CONTROL, 3, 3, handlePlaylistMove); + addCommand(COMMAND_PLAYLISTDELETE, PERMISSION_CONTROL, 2, 2, handlePlaylistDelete); + addCommand(COMMAND_TAGTYPES, PERMISSION_READ, 0, 0, handleTagTypes); + addCommand(COMMAND_COUNT, PERMISSION_READ, 2, -1, handleCount); + addCommand(COMMAND_RENAME, PERMISSION_CONTROL, 2, 2, handleRename); sortList(commandList); } @@ -1377,21 +1357,6 @@ static CommandEntry *getCommandEntryAndCheckArgcAndPermission(int fd, return cmd; } -static CommandEntry *getCommandEntryFromString(char *string, int *permission) -{ - CommandEntry *cmd; - char *argv[COMMAND_ARGV_MAX] = { NULL }; - int argc = buffer2array(string, argv, COMMAND_ARGV_MAX); - - if (0 == argc) - return NULL; - - cmd = getCommandEntryAndCheckArgcAndPermission(0, permission, - argc, argv); - - return cmd; -} - static int processCommandInternal(int fd, mpd_unused int *permission, char *commandString, struct strnode *cmdnode) { @@ -1407,12 +1372,8 @@ static int processCommandInternal(int fd, mpd_unused int *permission, if ((cmd = getCommandEntryAndCheckArgcAndPermission(fd, permission, argc, argv))) { - if (!cmdnode || !cmd->listHandler) { + if (!cmdnode) ret = cmd->handler(fd, permission, argc, argv); - } else { - ret = cmd->listHandler(fd, permission, argc, argv, - cmdnode, cmd); - } } current_command = NULL; -- cgit v1.2.3 From c5a17794b0bb8112079dd100a334b0175aec4cfa Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Fri, 3 Oct 2008 02:19:54 -0700 Subject: song: start avoiding race in updateSongInfo This is still not SMP-safe yet, as it needs at least a barrier before calling tag_free(old_tag). --- src/song.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/song.c b/src/song.c index 5c5024391..dc100899a 100644 --- a/src/song.c +++ b/src/song.c @@ -207,19 +207,22 @@ int updateSongInfo(Song * song) unsigned int next = 0; char path_max_tmp[MPD_PATH_MAX]; char abs_path[MPD_PATH_MAX]; + struct mpd_tag *old_tag = song->tag; + struct mpd_tag *new_tag = NULL; utf8_to_fs_charset(abs_path, get_song_url(path_max_tmp, song)); rmp2amp_r(abs_path, abs_path); - if (song->tag) - tag_free(song->tag); - - song->tag = NULL; - - while (!song->tag && (plugin = isMusic(abs_path, - &(song->mtime), - next++))) { - song->tag = plugin->tagDupFunc(abs_path); + while ((plugin = isMusic(abs_path, &song->mtime, next++))) { + if ((new_tag = plugin->tagDupFunc(abs_path))) + break; + } + if (new_tag && tag_equal(new_tag, old_tag)) { + tag_free(new_tag); + } else { + song->tag = new_tag; + if (old_tag) + tag_free(old_tag); } if (!song->tag || song->tag->time < 0) return -1; -- cgit v1.2.3 From cd1e02c8a88dd3e55cbde3056906dfbd3207f30e Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Fri, 3 Oct 2008 18:11:47 -0700 Subject: tag: merge clearMpdTag into tag_free avoid some redundant clearing logic as well, since the tag is getting freed. --- src/tag.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/tag.c b/src/tag.c index 2dcaf4ef8..db446836c 100644 --- a/src/tag.c +++ b/src/tag.c @@ -296,12 +296,12 @@ void tag_clear_items_by_type(struct mpd_tag *tag, enum tag_type type) } } -static void clearMpdTag(struct mpd_tag *tag) +void tag_free(struct mpd_tag *tag) { int i; pthread_mutex_lock(&tag_pool_lock); - for (i = 0; i < tag->numOfItems; i++) + for (i = tag->numOfItems; --i >= 0; ) tag_pool_put_item(tag->items[i]); pthread_mutex_unlock(&tag_pool_lock); @@ -314,16 +314,6 @@ static void clearMpdTag(struct mpd_tag *tag) free(tag->items); } - tag->items = NULL; - - tag->numOfItems = 0; - - tag->time = -1; -} - -void tag_free(struct mpd_tag *tag) -{ - clearMpdTag(tag); free(tag); } -- cgit v1.2.3 From e46bbc95eae9bc75e6d809cdf157637b682765c2 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sat, 4 Oct 2008 02:44:21 -0700 Subject: song: better handling of existing songs when rereading DB Don't reallocate existing tags if they haven't changed. This isn't used as often anymore, but we still take HUP signals to reread the DB if other processes changed it. --- src/song.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/song.c b/src/song.c index dc100899a..4f7b2f8bb 100644 --- a/src/song.c +++ b/src/song.c @@ -123,11 +123,20 @@ static void insertSongIntoList(struct songvec *sv, Song *newsong) tag_end_add(newsong->tag); } else { /* prevent dupes, just update the existing song info */ if (existing->mtime != newsong->mtime) { - tag_free(existing->tag); - if (newsong->tag) - tag_end_add(newsong->tag); - existing->tag = newsong->tag; existing->mtime = newsong->mtime; + if (tag_equal(existing->tag, newsong->tag)) { + if (newsong->tag) + tag_free(newsong->tag); + } else { + struct mpd_tag *old_tag = existing->tag; + + if (newsong->tag) + tag_end_add(newsong->tag); + existing->tag = newsong->tag; + if (old_tag) + tag_free(old_tag); + } + /* prevent tag_free in freeJustSong */ newsong->tag = NULL; } freeJustSong(newsong); -- cgit v1.2.3 From b84bf082df6ec5a7223c86abb94d799569aed1c1 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sat, 4 Oct 2008 03:41:59 -0700 Subject: directory: reuse existing directory if found on update Instead of allocating a new one, just reuse an existing one if one is found when rereading the DB. This is a small makes the previous commit work on subdirectories of the root music directory. [1] "song: better handling of existing songs when rereading DB" --- src/directory.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/directory.c b/src/directory.c index fc9c6b41d..fb0ca0b07 100644 --- a/src/directory.c +++ b/src/directory.c @@ -723,12 +723,13 @@ static void readDirectoryInfo(FILE * fp, Directory * directory) char buffer[MPD_PATH_MAX * 2]; int bufferSize = MPD_PATH_MAX * 2; char key[MPD_PATH_MAX * 2]; - Directory *subDirectory; char *name; while (myFgets(buffer, bufferSize, fp) && prefixcmp(buffer, DIRECTORY_END)) { if (!prefixcmp(buffer, DIRECTORY_DIR)) { + Directory *subdir; + strcpy(key, &(buffer[strlen(DIRECTORY_DIR)])); if (!myFgets(buffer, bufferSize, fp)) FATAL("Error reading db, fgets\n"); @@ -740,9 +741,13 @@ static void readDirectoryInfo(FILE * fp, Directory * directory) if (prefixcmp(buffer, DIRECTORY_BEGIN)) FATAL("Error reading db at line: %s\n", buffer); name = &(buffer[strlen(DIRECTORY_BEGIN)]); - subDirectory = newDirectory(name, directory); - dirvec_add(&directory->children, subDirectory); - readDirectoryInfo(fp, subDirectory); + if ((subdir = getDirectory(name))) { + assert(subdir->parent == directory); + } else { + subdir = newDirectory(name, directory); + dirvec_add(&directory->children, subdir); + } + readDirectoryInfo(fp, subdir); } else if (!prefixcmp(buffer, SONG_BEGIN)) { readSongInfoIntoList(fp, directory); } else { -- cgit v1.2.3