From 905266256e0a564d8b5421fa9cd897901e583314 Mon Sep 17 00:00:00 2001
From: Thomas Jansen <mithi@mithi.net>
Date: Sat, 3 Oct 2009 15:32:06 +0200
Subject: [PATCH] plugin: handle stderr messages of lyrics plugins

Use a second pipe to capture error messages generated by lyrics plugins. If
one plugin succeeds to retrieve some lyrics, we display them and discard the
error messages. If no plugin succeeds, we show the error messages of all
plugins that failed with an exit code other than 69 (EX_UNAVAILABLE,
sysexits.h).

All plugins use exit code 0 to indicate success (i.e. they are able to provide
lyrics on stdout). Exit code 69 indicates failure to retrieve lyrics, but
otherwise normal operation of the plugin. A plugin returning exit code 69 is
not listed as a failed plugin. Other exit codes indicate an internal error in
the plugin. Those plugins are listed as failed along with the output they
provide on stderr.
---
 NEWS                    |   1 +
 lyrics/01-hd.sh         |   8 +-
 lyrics/02-lyricwiki.rb  |  13 ++-
 lyrics/03-leoslyrics.py |   2 +-
 src/plugin.c            | 177 +++++++++++++++++++++++++++-------------
 src/plugin.h            |   8 +-
 src/screen_lyrics.c     |  16 ++--
 7 files changed, 156 insertions(+), 69 deletions(-)

diff --git a/NEWS b/NEWS
index 571f7bf..bbbe18f 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,7 @@ ncmpc 0.16 - not yet released
 * allow multiple queued database updates
 * reactivate incremental playlist changes
 * optimize "add+play song" with addid/playid
+* handle stderr messages from lyrics plugins
 
 
 ncmpc 0.15 - 2009-09-24
diff --git a/lyrics/01-hd.sh b/lyrics/01-hd.sh
index 2480e5a..a0d7020 100755
--- a/lyrics/01-hd.sh
+++ b/lyrics/01-hd.sh
@@ -21,4 +21,10 @@
 # Load lyrics from the user's home directory
 #
 
-cat ~/.lyrics/"$1 - $2".txt 2>/dev/null
+FILENAME=~/.lyrics/"$1 - $2".txt
+
+if [ -e "$FILENAME" ] ; then
+	cat "$FILENAME" 2>/dev/null
+else
+	exit 69
+fi
diff --git a/lyrics/02-lyricwiki.rb b/lyrics/02-lyricwiki.rb
index 74c13b2..2e1aed7 100755
--- a/lyrics/02-lyricwiki.rb
+++ b/lyrics/02-lyricwiki.rb
@@ -28,11 +28,18 @@ url = "http://lyrics.wikia.com/api.php?action=lyrics&fmt=xml&func=getSong" + \
     "&artist=#{URI.escape(ARGV[0])}&song=#{URI.escape(ARGV[1])}"
 response = Net::HTTP.get(URI.parse(url))
 
-exit(2) unless response =~ /<url>\s*(.*?)\s*<\/url>/im
+if not response =~ /<url>\s*(.*?)\s*<\/url>/im
+	$stderr.puts "No URL in response!"
+	exit(1)
+end
+
 url = $1.gsub(/wikia.com/, "wikia.com/lyrics");
-exit(2) if $1 =~ /action=edit$/
+exit(69) if $1 =~ /action=edit$/
 
 response = Net::HTTP.get(URI.parse(url))
-exit(2) unless response =~ /<div class='lyricbox' >\s*(.*?)\s*<!--/im
+if not response =~ /<div class='lyricbox'>\s*(.*?)\s*<!--/im
+	$stderr.puts "No <div class='lyricbox'> in lyrics page!\n"
+	exit(1)
+end
 
 puts $1.gsub(/<br \/>/, "\n")
diff --git a/lyrics/03-leoslyrics.py b/lyrics/03-leoslyrics.py
index 5e4f8c8..b55f0ec 100755
--- a/lyrics/03-leoslyrics.py
+++ b/lyrics/03-leoslyrics.py
@@ -80,5 +80,5 @@ def lyrics(hid):
 
 hid = search(argv[1], argv[2])
 if hid is None:
-    exit(2)
+    exit(69)
 print lyrics(hid).encode('utf-8')
diff --git a/src/plugin.c b/src/plugin.c
index 88cc305..d784900 100644
--- a/src/plugin.c
+++ b/src/plugin.c
@@ -27,6 +27,18 @@
 #include <sys/signal.h>
 #include <sys/wait.h>
 
+struct plugin_pipe {
+	/** the pipe to the plugin process, or -1 if none is currently
+	    open */
+	int fd;
+	/** the GLib channel of #fd */
+	GIOChannel *channel;
+	/** the GLib IO watch of #channel */
+	guint event_id;
+	/** the output of the current plugin */
+	GString *data;
+};
+
 struct plugin_cycle {
 	/** the plugin list; used for traversing to the next plugin */
 	struct plugin_list *list;
@@ -46,16 +58,14 @@ struct plugin_cycle {
 	/** the pid of the plugin process, or -1 if none is currently
 	    running */
 	pid_t pid;
-	/** the pipe to the plugin process, or -1 if none is currently
-	    open */
-	int fd;
-	/** the GLib channel of #fd */
-	GIOChannel *channel;
-	/** the GLib IO watch of #channel */
-	guint event_id;
 
-	/** the output of the current plugin */
-	GString *data;
+	/** the stdout pipe */
+	struct plugin_pipe pipe_stdout;
+	/** the stderr pipe */
+	struct plugin_pipe pipe_stderr;
+
+	/** list of all error messages from failed plugins */
+	GString *all_errors;
 };
 
 static bool
@@ -122,26 +132,43 @@ static void
 next_plugin(struct plugin_cycle *cycle);
 
 static void
-plugin_eof(struct plugin_cycle *cycle)
+plugin_eof(struct plugin_cycle *cycle, struct plugin_pipe *p)
 {
 	int ret, status;
 
-	g_io_channel_unref(cycle->channel);
-	close(cycle->fd);
-	cycle->fd = -1;
+	g_io_channel_unref(p->channel);
+	close(p->fd);
+	p->fd = -1;
+
+	/* Only if both pipes are have EOF status we are done */
+	if (cycle->pipe_stdout.fd != -1 || cycle->pipe_stderr.fd != -1)
+		return;
 
 	ret = waitpid(cycle->pid, &status, 0);
 	cycle->pid = -1;
 
 	if (ret < 0 || !WIFEXITED(status) || WEXITSTATUS(status) != 0) {
+		/* If we encountered an error other than service unavailable
+		 * (69), log it for later. If all plugins fail, we may get
+		 * some hints for debugging.*/
+		if (cycle->pipe_stderr.data->len > 0 &&
+		    WEXITSTATUS(status) != 69)
+			g_string_append_printf(cycle->all_errors,
+					       "*** %s ***\n\n%s\n",
+					       cycle->argv[0],
+					       cycle->pipe_stderr.data->str);
+
 		/* the plugin has failed */
-		g_string_free(cycle->data, TRUE);
-		cycle->data = NULL;
+		g_string_free(cycle->pipe_stdout.data, TRUE);
+		cycle->pipe_stdout.data = NULL;
+		g_string_free(cycle->pipe_stderr.data, TRUE);
+		cycle->pipe_stderr.data = NULL;
 
 		next_plugin(cycle);
 	} else {
 		/* success: invoke the callback */
-		cycle->callback(cycle->data, cycle->callback_data);
+		cycle->callback(cycle->pipe_stdout.data, true,
+				cycle->callback_data);
 	}
 }
 
@@ -150,23 +177,26 @@ plugin_data(G_GNUC_UNUSED GIOChannel *source,
 	    G_GNUC_UNUSED GIOCondition condition, gpointer data)
 {
 	struct plugin_cycle *cycle = data;
+	struct plugin_pipe *p = NULL;
 	char buffer[256];
 	ssize_t nbytes;
 
 	assert(cycle != NULL);
-	assert(cycle->fd >= 0);
 	assert(cycle->pid > 0);
-	assert(source == cycle->channel);
-
-	nbytes = condition & G_IO_IN
-		? read(cycle->fd, buffer, sizeof(buffer))
-		: 0;
+	if (source == cycle->pipe_stdout.channel)
+		p = &cycle->pipe_stdout;
+	else if (source == cycle->pipe_stderr.channel)
+		p = &cycle->pipe_stderr;
+	assert(p != NULL);
+	assert(p->fd >= 0);
+
+	nbytes = condition & G_IO_IN ? read(p->fd, buffer, sizeof(buffer)) : 0;
 	if (nbytes <= 0) {
-		plugin_eof(cycle);
+		plugin_eof(cycle, p);
 		return FALSE;
 	}
 
-	g_string_append_len(cycle->data, buffer, nbytes);
+	g_string_append_len(p->data, buffer, nbytes);
 	return TRUE;
 }
 
@@ -183,48 +213,70 @@ plugin_delayed_fail(gpointer data)
 	struct plugin_cycle *cycle = data;
 
 	assert(cycle != NULL);
-	assert(cycle->fd < 0);
+	assert(cycle->pipe_stdout.fd < 0);
+	assert(cycle->pipe_stderr.fd < 0);
 	assert(cycle->pid < 0);
-	assert(cycle->data == NULL);
 
-	cycle->callback(NULL, cycle->callback_data);
+	cycle->callback(cycle->all_errors, false, cycle->callback_data);
 
 	return FALSE;
 }
 
+static void
+plugin_fd_add(struct plugin_cycle *cycle, struct plugin_pipe *p, int fd)
+{
+	p->fd = fd;
+	p->data = g_string_new(NULL);
+	p->channel = g_io_channel_unix_new(fd);
+	p->event_id = g_io_add_watch(p->channel, G_IO_IN|G_IO_HUP,
+				     plugin_data, cycle);
+}
+
 static int
 start_plugin(struct plugin_cycle *cycle, const char *plugin_path)
 {
-	int ret, fds[2];
+	int ret, fds_stdout[2], fds_stderr[2];
 	pid_t pid;
 
 	assert(cycle != NULL);
 	assert(cycle->pid < 0);
-	assert(cycle->fd < 0);
-	assert(cycle->data == NULL);
+	assert(cycle->pipe_stdout.fd < 0);
+	assert(cycle->pipe_stderr.fd < 0);
+	assert(cycle->pipe_stdout.data == NULL);
+	assert(cycle->pipe_stderr.data == NULL);
 
 	/* set new program name, but free the one from the previous
 	   plugin */
 	g_free(cycle->argv[0]);
 	cycle->argv[0] = g_path_get_basename(plugin_path);
 
-	ret = pipe(fds);
+	ret = pipe(fds_stdout);
 	if (ret < 0)
 		return -1;
+	ret = pipe(fds_stderr);
+	if (ret < 0) {
+		close(fds_stdout[0]);
+		close(fds_stdout[1]);
+		return -1;
+	}
 
 	pid = fork();
 
 	if (pid < 0) {
-		close(fds[0]);
-		close(fds[1]);
+		close(fds_stdout[0]);
+		close(fds_stdout[1]);
+		close(fds_stderr[0]);
+		close(fds_stderr[1]);
 		return -1;
 	}
 
 	if (pid == 0) {
-		dup2(fds[1], 1);
-		dup2(fds[1], 2);
-		close(fds[0]);
-		close(fds[1]);
+		dup2(fds_stdout[1], 1);
+		dup2(fds_stderr[1], 2);
+		close(fds_stdout[0]);
+		close(fds_stdout[1]);
+		close(fds_stderr[0]);
+		close(fds_stderr[1]);
 		close(0);
 		/* XXX close other fds? */
 
@@ -232,17 +284,15 @@ start_plugin(struct plugin_cycle *cycle, const char *plugin_path)
 		_exit(1);
 	}
 
-	close(fds[1]);
+	close(fds_stdout[1]);
+	close(fds_stderr[1]);
 
 	cycle->pid = pid;
-	cycle->fd = fds[0];
-	cycle->data = g_string_new(NULL);
 
 	/* XXX CLOEXEC? */
 
-	cycle->channel = g_io_channel_unix_new(cycle->fd);
-	cycle->event_id = g_io_add_watch(cycle->channel, G_IO_IN|G_IO_HUP,
-					 plugin_data, cycle);
+	plugin_fd_add(cycle, &cycle->pipe_stdout, fds_stdout[0]);
+	plugin_fd_add(cycle, &cycle->pipe_stderr, fds_stderr[0]);
 
 	return 0;
 }
@@ -254,8 +304,10 @@ next_plugin(struct plugin_cycle *cycle)
 	int ret = -1;
 
 	assert(cycle->pid < 0);
-	assert(cycle->fd < 0);
-	assert(cycle->data == NULL);
+	assert(cycle->pipe_stdout.fd < 0);
+	assert(cycle->pipe_stderr.fd < 0);
+	assert(cycle->pipe_stdout.data == NULL);
+	assert(cycle->pipe_stderr.data == NULL);
 
 	if (cycle->next_plugin >= cycle->list->plugins->len) {
 		/* no plugins left */
@@ -312,23 +364,32 @@ plugin_run(struct plugin_list *list, const char *const*args,
 	cycle->callback_data = callback_data;
 	cycle->next_plugin = 0;
 	cycle->pid = -1;
-	cycle->fd = -1;
-	cycle->data = NULL;
+	cycle->pipe_stdout.fd = -1;
+	cycle->pipe_stderr.fd = -1;
+	cycle->pipe_stdout.data = NULL;
+	cycle->pipe_stderr.data = NULL;
 
+	cycle->all_errors = g_string_new(NULL);
 	next_plugin(cycle);
 
 	return cycle;
 }
 
+static void
+plugin_fd_remove(struct plugin_pipe *p)
+{
+	if (p->fd >= 0) {
+		g_source_remove(p->event_id);
+		g_io_channel_unref(p->channel);
+		close(p->fd);
+	}
+}
+
 void
 plugin_stop(struct plugin_cycle *cycle)
 {
-	if (cycle->fd >= 0) {
-		/* close the pipe to the plugin */
-		g_source_remove(cycle->event_id);
-		g_io_channel_unref(cycle->channel);
-		close(cycle->fd);
-	}
+	plugin_fd_remove(&cycle->pipe_stdout);
+	plugin_fd_remove(&cycle->pipe_stderr);
 
 	if (cycle->pid > 0) {
 		/* kill the plugin process */
@@ -338,9 +399,13 @@ plugin_stop(struct plugin_cycle *cycle)
 		waitpid(cycle->pid, &status, 0);
 	}
 
-	if (cycle->data != NULL)
-		/* free data that has been received */
-		g_string_free(cycle->data, TRUE);
+	/* free data that has been received */
+	if (cycle->pipe_stdout.data != NULL)
+		g_string_free(cycle->pipe_stdout.data, TRUE);
+	if (cycle->pipe_stderr.data != NULL)
+		g_string_free(cycle->pipe_stderr.data, TRUE);
+	if (cycle->all_errors != NULL)
+		g_string_free(cycle->all_errors, TRUE);
 
 	/* free argument list */
 	for (guint i = 0; i == 0 || cycle->argv[i] != NULL; ++i)
diff --git a/src/plugin.h b/src/plugin.h
index 6681efd..e633154 100644
--- a/src/plugin.h
+++ b/src/plugin.h
@@ -34,10 +34,14 @@ struct plugin_list {
  * When a plugin cycle is finished, this function is called.  In any
  * case, plugin_stop() has to be called to free all memory.
  *
- * @param result the plugin's output (stdout) on success; NULL on failure
+ * @param result the plugin's output (stdout) on success; summary of all error
+ * messages on failure as determined by success
+ * @param success result of the plugin cycle; true if result is meaningful
+ * output, false if result contains error messages
  * @param data the caller defined pointer passed to plugin_run()
  */
-typedef void (*plugin_callback_t)(const GString *result, void *data);
+typedef void (*plugin_callback_t)(const GString *result, const bool success,
+				  void *data);
 
 /**
  * This object represents a cycle through all available plugins, until
diff --git a/src/screen_lyrics.c b/src/screen_lyrics.c
index 7c3d637..7b9d1df 100644
--- a/src/screen_lyrics.c
+++ b/src/screen_lyrics.c
@@ -140,22 +140,26 @@ screen_lyrics_set(const GString *str)
 	/* paint new data */
 
 	lyrics_repaint_if_active();
-
-	if (options.lyrics_autosave &&
-	    !exists_lyr_file(current.artist, current.title))
-		store_lyr_hd();
 }
 
 static void
-screen_lyrics_callback(const GString *result, G_GNUC_UNUSED void *data)
+screen_lyrics_callback(const GString *result, const bool success,
+		       G_GNUC_UNUSED void *data)
 {
 	assert(current.loader != NULL);
 
+	/* Display result, which may be lyrics or error messages */
 	if (result != NULL)
 		screen_lyrics_set(result);
-	else
+
+	if (success == true) {
+		if (options.lyrics_autosave &&
+		    !exists_lyr_file(current.artist, current.title))
+			store_lyr_hd();
+	} else {
 		/* translators: no lyrics were found for the song */
 		screen_status_message (_("No lyrics"));
+	}
 
 	plugin_stop(current.loader);
 	current.loader = NULL;
-- 
2.30.2