From 462a71176aa847494a1a26826768b5fa52994f54 Mon Sep 17 00:00:00 2001 From: Andre Noll Date: Sun, 13 Mar 2022 23:28:21 +0100 Subject: [PATCH] Simplify and improve activate_mood_or_playlist(). The logic of this function can be simplified to match the four possible cases for the argument. Each of the four conditional blocks now initializes ret, mode and the message pointer. The message is then printed into the para buffer, which is now passed to the function instead of a char ** because this simplifies the select callback a bit. The other callers (for init and SIGHUP handling) pass NULL and don't need to be adjusted. To make this work we have to make sure that the message pointer is properly initialized in all cases, not only in the error case as before. Thus, playlist_open() and mood_switch() are changed to return a suitable message also on success. For playlists, the message only contains the number of files in the playlist. For moods we also include the afs statistics of the mood and no longer write this information to the server log. We omit the correction factors and the normalization divisor, however, as these are not very interesting. The only purpose of the num_admissible parameter of activate_mood_or_playlist() was to let the caller log this information. This task is now performed by playlist_open() and mood_switch(), so the parameter can be dropped. --- afs.c | 67 ++++++++++++++++++------------------------------- afs.h | 4 +-- mood.c | 73 ++++++++++++++++++++++++++++-------------------------- playlist.c | 14 ++++------- 4 files changed, 69 insertions(+), 89 deletions(-) diff --git a/afs.c b/afs.c index b017a7af..06b4132a 100644 --- a/afs.c +++ b/afs.c @@ -431,37 +431,30 @@ no_admissible_files: return write_all(server_socket, buf, 8); } -static int activate_mood_or_playlist(const char *arg, int *num_admissible, - char **errmsg) +static int activate_mood_or_playlist(const char *arg, struct para_buffer *pb) { enum play_mode mode; int ret; + char *msg; if (!arg) { + ret = mood_switch(NULL, &msg); + mode = PLAY_MODE_MOOD; + } else if (!strncmp(arg, "p/", 2)) { + ret = playlist_open(arg + 2, &msg); + mode = PLAY_MODE_PLAYLIST; + } else if (!strncmp(arg, "m/", 2)) { + ret = mood_switch(arg + 2, &msg); mode = PLAY_MODE_MOOD; - ret = mood_switch(NULL, errmsg); - if (ret < 0) { - if (num_admissible) - *num_admissible = 0; - return ret; - } } else { - if (!strncmp(arg, "p/", 2)) { - ret = playlist_open(arg + 2, errmsg); - mode = PLAY_MODE_PLAYLIST; - } else if (!strncmp(arg, "m/", 2)) { - ret = mood_switch(arg + 2, errmsg); - mode = PLAY_MODE_MOOD; - } else { - if (errmsg) - *errmsg = make_message("%s: parse error", arg); - return -ERRNO_TO_PARA_ERROR(EINVAL); - } - if (ret < 0) - return ret; + ret = -ERRNO_TO_PARA_ERROR(EINVAL); + msg = make_message("%s: parse error", arg); } - if (num_admissible) - *num_admissible = ret; + if (pb) + para_printf(pb, "%s", msg); + free(msg); + if (ret < 0) + return ret; current_play_mode = mode; /* * We get called with arg == current_mop from the signal dispatcher @@ -536,8 +529,7 @@ static int com_select_callback(struct afs_callback_arg *aca) { const struct lls_command *cmd = SERVER_CMD_CMD_PTR(SELECT); const char *arg; - int num_admissible, ret; - char *errmsg; + int ret; ret = lls_deserialize_parse_result(aca->query.data, cmd, &aca->lpr); assert(ret >= 0); @@ -551,31 +543,20 @@ static int com_select_callback(struct afs_callback_arg *aca) close_current_mood(); else playlist_close(); - ret = activate_mood_or_playlist(arg, &num_admissible, &errmsg); + ret = activate_mood_or_playlist(arg, &aca->pbout); if (ret >= 0) - goto out; + goto free_lpr; /* ignore subsequent errors (but log them) */ - para_printf(&aca->pbout, "%s\n", errmsg); - free(errmsg); - para_printf(&aca->pbout, "could not activate %s\n", arg); if (current_mop && strcmp(current_mop, arg) != 0) { int ret2; para_printf(&aca->pbout, "switching back to %s\n", current_mop); - ret2 = activate_mood_or_playlist(current_mop, &num_admissible, - &errmsg); + ret2 = activate_mood_or_playlist(current_mop, &aca->pbout); if (ret2 >= 0) - goto out; - para_printf(&aca->pbout, "%s\n", errmsg); - free(errmsg); + goto free_lpr; para_printf(&aca->pbout, "could not reactivate %s: %s\n", current_mop, para_strerror(-ret2)); } - para_printf(&aca->pbout, "activating dummy mood\n"); - activate_mood_or_playlist(NULL, &num_admissible, NULL); -out: - para_printf(&aca->pbout, "activated %s (%d admissible file%s)\n", - current_mop? current_mop : "dummy mood", num_admissible, - num_admissible == 1? "" : "s"); + activate_mood_or_playlist(NULL, &aca->pbout); free_lpr: lls_free_parse_result(aca->lpr, cmd); return ret; @@ -597,12 +578,12 @@ EXPORT_SERVER_CMD_HANDLER(select); static void init_admissible_files(const char *arg) { - int ret = activate_mood_or_playlist(arg, NULL, NULL); + int ret = activate_mood_or_playlist(arg, NULL); if (ret < 0) { PARA_WARNING_LOG("could not activate %s: %s\n", arg, para_strerror(-ret)); if (arg) - activate_mood_or_playlist(NULL, NULL, NULL); + activate_mood_or_playlist(NULL, NULL); } } diff --git a/afs.h b/afs.h index 0983692e..f12bf369 100644 --- a/afs.h +++ b/afs.h @@ -260,12 +260,12 @@ int aft_check_callback(struct afs_callback_arg *aca); void free_status_items(void); /* mood */ -int mood_switch(const char *mood_name, char **errmsg); +int mood_switch(const char *mood_name, char **msg); void close_current_mood(void); int mood_check_callback(struct afs_callback_arg *aca); /* playlist */ -int playlist_open(const char *name, char **errmsg); +int playlist_open(const char *name, char **msg); void playlist_close(void); int playlist_check_callback(struct afs_callback_arg *aca); diff --git a/mood.c b/mood.c index 66024db6..7b22f72d 100644 --- a/mood.c +++ b/mood.c @@ -521,28 +521,24 @@ static int mood_update_audio_file(const struct osl_row *aft_row, } /* sse: seconds since epoch. */ -static void log_statistics(struct afs_statistics *stats, int64_t sse) +static char *get_statistics(struct mood *m, int64_t sse) { - unsigned n = stats->num; + unsigned n = m->stats.num; int mean_days, sigma_days; - if (!n) { - PARA_WARNING_LOG("no admissible files\n"); - return; - } - PARA_NOTICE_LOG("%u admissible files\n", stats->num); - mean_days = (sse - stats->last_played_sum / n) / 3600 / 24; - sigma_days = int_sqrt(stats->last_played_qd / n) / 3600 / 24; - PARA_NOTICE_LOG("last_played mean/sigma: %d/%d days\n", mean_days, sigma_days); - PARA_NOTICE_LOG("num_played mean/sigma: %" PRId64 "/%" PRIu64 "\n", - stats->num_played_sum / n, - int_sqrt(stats->num_played_qd / n)); - PARA_NOTICE_LOG("num_played correction factor: %" PRId64 "\n", - stats->num_played_correction); - PARA_NOTICE_LOG("last_played correction factor: %" PRId64 "\n", - stats->last_played_correction); - PARA_NOTICE_LOG("normalization divisor: %" PRId64 "\n", - stats->normalization_divisor); + mean_days = (sse - m->stats.last_played_sum / n) / 3600 / 24; + sigma_days = int_sqrt(m->stats.last_played_qd / n) / 3600 / 24; + return make_message( + "loaded mood %s (%u files)\n" + "last_played mean/sigma: %d/%d days\n" + "num_played mean/sigma: %" PRId64 "/%" PRIu64 "\n" + , + m->name? m->name : "(dummy)", + n, + mean_days, sigma_days, + m->stats.num_played_sum / n, + int_sqrt(m->stats.num_played_qd / n) + ); } /** @@ -575,22 +571,23 @@ static void compute_correction_factors(int64_t sse, struct afs_statistics *s) /** * Change the current mood. * + * If there is already an open mood, it will be closed first. + * * \param mood_name The name of the mood to open. - * \param errmsg Error description is returned here. + * \param msg Error message or mood info is returned here. * * If \a mood_name is \a NULL, load the dummy mood that accepts every audio file * and uses a scoring method based only on the \a last_played information. * - * The errmsg pointer may be NULL, in which case no error message will be - * returned. If a non-NULL pointer is given, the caller must free *errmsg. + * If the message pointer is not NULL, a suitable message is returned there in + * all cases. The caller must free this string. * - * If there is already an open mood, it will be closed first. - * - * \return Positive on success, negative on errors. + * \return The number of admissible files on success, negative on errors. It is + * not considered an error if no files are admissible. * * \sa struct \ref afs_info::last_played, \ref mp_eval_row(). */ -int mood_switch(const char *mood_name, char **errmsg) +int mood_switch(const char *mood_name, char **msg) { int i, ret; struct admissible_array aa = {.size = 0}; @@ -601,7 +598,7 @@ int mood_switch(const char *mood_name, char **errmsg) struct timeval rnow; if (mood_name) { - ret = init_mood_parser(mood_name, &aa.m, errmsg); + ret = init_mood_parser(mood_name, &aa.m, msg); if (ret < 0) return ret; } else /* load dummy mood */ @@ -609,27 +606,33 @@ int mood_switch(const char *mood_name, char **errmsg) PARA_NOTICE_LOG("computing statistics of admissible files\n"); ret = audio_file_loop(&aa, add_if_admissible); if (ret < 0) { - if (errmsg) - *errmsg = make_message("audio file loop failed"); + if (msg) /* false if we are called via the event handler */ + *msg = make_message("audio file loop failed\n"); goto out; } clock_get_realtime(&rnow); compute_correction_factors(rnow.tv_sec, &aa.m->stats); - log_statistics(&aa.m->stats, rnow.tv_sec); + if (aa.m->stats.num == 0) { + if (msg) + *msg = make_message("no admissible files\n"); + ret = 0; + goto out; + } for (i = 0; i < aa.m->stats.num; i++) { ret = add_to_score_table(aa.array[i], &aa.m->stats); if (ret < 0) { - if (errmsg) - *errmsg = make_message( - "could not add row to score table"); + if (msg) + *msg = make_message( + "could not add row to score table\n"); goto out; } } /* success */ + if (msg) + *msg = get_statistics(aa.m, rnow.tv_sec); + ret = aa.m->stats.num; close_current_mood(); current_mood = aa.m; - PARA_NOTICE_LOG("loaded mood %s\n", mood_name? mood_name : "(dummy)"); - ret = aa.m->stats.num; out: free(aa.array); if (ret < 0) diff --git a/playlist.c b/playlist.c index 171a6d26..8f5c3d7d 100644 --- a/playlist.c +++ b/playlist.c @@ -125,13 +125,13 @@ void playlist_close(void) * corresponding row of the audio file table is added to the score table. * * \param name The name of the playlist to open. - * \param errmsg To be sent to the client (if called via select command). + * \param msg Error message or playlist info is returned here. * * \return The length of the loaded playlist on success, negative error code * else. Files which are listed in the playlist, but are not contained in the * database are ignored. This is not considered an error. */ -int playlist_open(const char *name, char **errmsg) +int playlist_open(const char *name, char **msg) { int ret; struct playlist_info *playlist = ¤t_playlist; @@ -139,9 +139,7 @@ int playlist_open(const char *name, char **errmsg) ret = pl_get_def_by_name(name, &playlist_def); if (ret < 0) { - if (errmsg) - *errmsg = make_message("could not read playlist %s", - name); + *msg = make_message("could not read playlist %s\n", name); return ret; } playlist_close(); @@ -154,15 +152,13 @@ int playlist_open(const char *name, char **errmsg) if (!playlist->length) goto err; playlist->name = para_strdup(name); - PARA_NOTICE_LOG("loaded playlist %s (%u files)\n", playlist->name, + *msg = make_message("loaded playlist %s (%u files)\n", playlist->name, playlist->length); /* success */ return current_playlist.length; err: PARA_NOTICE_LOG("unable to load playlist %s\n", name); - if (errmsg) - *errmsg = make_message("unable to load playlist %s: %s\n", - name, para_strerror(-ret)); + *msg = make_message("unable to load playlist %s\n", name); return ret; } -- 2.39.2