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.

diff --git a/afs.c b/afs.c
index b017a7afcc0f01030f70e9f3133b27003f530ee3..06b4132acde340d16500a321e756dac57221fc6c 100644 (file)
--- 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 0983692ec30abfb38608547a29f00f934f780e46..f12bf369cd430d1ec6c6be321bf7e5756daf1e1a 100644 (file)
--- 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 66024db6add623716b428edd0263847705bc80e9..7b22f72d003673454f56d759952424891e297307 100644 (file)
--- 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 171a6d26e3f77f6377c70d3ce560d35964ac2ca8..8f5c3d7d1089a7cc48352a70d6f1aaf6e8ee2aa0 100644 (file)
--- 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 = &current_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;
 }