From 0b5c29fae8853bac16b91503df70b0e101dccca4 Mon Sep 17 00:00:00 2001
From: Andre Noll <maan@tuebingen.mpg.de>
Date: Tue, 30 Aug 2022 13:55:54 +0200
Subject: [PATCH] Introduce afs_error().

The callbacks of some afs commands employ the normal ->pbpout para
buffer to send an error message to the client on failure. These
messages are therefore tagged with the OUTPUT sideband designator
just as regular command output.

The receiving client writes such messages to stdout, so applications
which call para_client have no other way than parsing the output to
guess whether it is normal command output or an error message.

This commit improves on this by providing a public helper in afs.c
to format and send an error message that is tagged with the ERROR
sideband designator and thus gets written to stderr on the client
side. All afs callbacks which currently use ->pbout for error messages
are converted to call the new helper.
---
 afs.c       | 48 ++++++++++++++++++++++++++++++++++++++++++------
 afs.h       |  2 ++
 aft.c       | 14 +++++++-------
 attribute.c | 13 ++++++-------
 blob.c      | 11 +++++------
 mood.c      | 13 ++++++-------
 playlist.c  | 13 +++++++------
 7 files changed, 75 insertions(+), 39 deletions(-)

diff --git a/afs.c b/afs.c
index 6493586a..b15f8385 100644
--- a/afs.c
+++ b/afs.c
@@ -728,6 +728,43 @@ err:
 	return ret;
 }
 
+/**
+ * Format and send an error message to the command handler.
+ *
+ * To pass an error message from the callback of an afs command to the client,
+ * this function should be called. It formats the message into a buffer which
+ * is passed as a shared memory area to the command handler from where it
+ * propagates to the client.
+ *
+ * The message will be tagged with the ERROR_LOG sideband designator so that
+ * the client writes it to its stderr stream rather than to stdout as with
+ * aca->pbout. In analogy to the default Unix semantics of stderr, the message
+ * is sent without buffering.
+ *
+ * If sending the error message fails, an error is logged on the server side,
+ * but no other action is taken.
+ *
+ * \param aca Used to obtain the fd to send the shmid to.
+ * \param fmt Usual format string.
+ */
+__printf_2_3 void afs_error(const struct afs_callback_arg *aca,
+		const char *fmt,...)
+{
+	va_list argp;
+	char *msg;
+	unsigned n;
+	int ret;
+
+	va_start(argp, fmt);
+	n = xvasprintf(&msg, fmt, argp);
+	va_end(argp);
+	ret = pass_buffer_as_shm(aca->fd, SBD_ERROR_LOG, msg, n + 1);
+	if (ret < 0)
+		PARA_ERROR_LOG("Could not send %s: %s\n", msg,
+			para_strerror(-ret));
+	free(msg);
+}
+
 static int call_callback(int fd, int query_shmid)
 {
 	void *query_shm;
@@ -958,12 +995,12 @@ static int com_select_callback(struct afs_callback_arg *aca)
 	/* ignore subsequent errors (but log them) */
 	if (current_mop && strcmp(current_mop, arg) != 0) {
 		int ret2;
-		para_printf(&aca->pbout, "switching back to %s\n", current_mop);
+		afs_error(aca, "switching back to %s\n", current_mop);
 		ret2 = activate_mood_or_playlist(current_mop, &aca->pbout);
 		if (ret2 >= 0)
 			goto free_lpr;
-		para_printf(&aca->pbout, "could not reactivate %s: %s\n",
-			current_mop, para_strerror(-ret2));
+		afs_error(aca, "could not reactivate %s: %s\n", current_mop,
+			para_strerror(-ret2));
 	}
 	activate_mood_or_playlist(NULL, &aca->pbout);
 free_lpr:
@@ -1001,8 +1038,7 @@ static int com_init_callback(struct afs_callback_arg *aca)
 			continue;
 		ret = t->ops->create(database_dir);
 		if (ret < 0) {
-			para_printf(&aca->pbout, "cannot create table %s\n",
-				t->name);
+			afs_error(aca, "cannot create table %s\n", t->name);
 			goto out;
 		}
 		para_printf(&aca->pbout, "successfully created %s table\n",
@@ -1010,7 +1046,7 @@ static int com_init_callback(struct afs_callback_arg *aca)
 	}
 	ret = open_afs_tables();
 	if (ret < 0)
-		para_printf(&aca->pbout, "cannot open afs tables: %s\n",
+		afs_error(aca, "cannot open afs tables: %s\n",
 			para_strerror(-ret));
 out:
 	return ret;
diff --git a/afs.h b/afs.h
index a6394fa6..59c887a0 100644
--- a/afs.h
+++ b/afs.h
@@ -225,6 +225,8 @@ int send_callback_request(afs_callback *f, struct osl_object *query,
 int send_lls_callback_request(afs_callback *f,
 		const struct lls_command * const cmd,
 		struct lls_parse_result *lpr, void *private_result_data);
+__printf_2_3 void afs_error(const struct afs_callback_arg *aca,
+		const char *fmt,...);
 int string_compare(const struct osl_object *obj1, const struct osl_object *obj2);
 int for_each_matching_row(struct pattern_match_data *pmd);
 
diff --git a/aft.c b/aft.c
index b4fabbc8..0009a54f 100644
--- a/aft.c
+++ b/aft.c
@@ -1761,7 +1761,7 @@ static int com_add_callback(struct afs_callback_arg *aca)
 	ret = afs_event(AUDIO_FILE_ADD, &aca->pbout, aft_row);
 out:
 	if (ret < 0)
-		para_printf(&aca->pbout, "could not add %s\n", path);
+		afs_error(aca, "could not add %s\n", path);
 	lls_free_parse_result(aca->lpr, cmd);
 	return ret;
 }
@@ -1979,12 +1979,12 @@ static int touch_audio_file(__a_unused struct osl_table *table,
 
 	ret = get_afsi_object_of_row(row, &obj);
 	if (ret < 0) {
-		para_printf(&aca->pbout, "cannot touch %s\n", name);
+		afs_error(aca, "cannot touch %s\n", name);
 		return ret;
 	}
 	ret = load_afsi(&old_afsi, &obj);
 	if (ret < 0) {
-		para_printf(&aca->pbout, "cannot touch %s\n", name);
+		afs_error(aca, "cannot touch %s\n", name);
 		return ret;
 	}
 	new_afsi = old_afsi;
@@ -2038,7 +2038,7 @@ static int com_touch_callback(struct afs_callback_arg *aca)
 		uint32_t id = lls_uint32_val(0, r_i);
 		ret = img_get_name_by_id(id, NULL);
 		if (ret < 0) {
-			para_printf(&aca->pbout, "invalid image ID: %u\n", id);
+			afs_error(aca, "invalid image ID: %u\n", id);
 			return ret;
 		}
 	}
@@ -2047,7 +2047,7 @@ static int com_touch_callback(struct afs_callback_arg *aca)
 		uint32_t id = lls_uint32_val(0, r_y);
 		ret = lyr_get_name_by_id(id, NULL);
 		if (ret < 0) {
-			para_printf(&aca->pbout, "invalid lyrics ID: %u\n", id);
+			afs_error(aca, "invalid lyrics ID: %u\n", id);
 			return ret;
 		}
 	}
@@ -2090,7 +2090,7 @@ static int remove_audio_file(__a_unused struct osl_table *table,
 		return ret;
 	ret = osl(osl_del_row(audio_file_table, row));
 	if (ret < 0)
-		para_printf(&aca->pbout, "cannot remove %s\n", name);
+		afs_error(aca, "cannot remove %s\n", name);
 	return ret;
 }
 
@@ -2326,7 +2326,7 @@ static int com_setatt_callback(struct afs_callback_arg *aca)
 		ret = get_attribute_bitnum_by_name(p, &bitnum);
 		free(p);
 		if (ret < 0) {
-			para_printf(&aca->pbout, "invalid argument: %s\n", arg);
+			afs_error(aca, "invalid argument: %s\n", arg);
 			goto out;
 		}
 		if (c == '+')
diff --git a/attribute.c b/attribute.c
index 8c87fa30..51630b25 100644
--- a/attribute.c
+++ b/attribute.c
@@ -119,7 +119,7 @@ static int print_attribute(struct osl_table *table, struct osl_row *row,
 	}
 	ret = osl(osl_get_object(table, row, ATTCOL_BITNUM, &bitnum_obj));
 	if (ret < 0) {
-		para_printf(&aca->pbout, "%s: %s\n", name, para_strerror(-ret));
+		afs_error(aca, "%s: %s\n", name, para_strerror(-ret));
 		return ret;
 	}
 	para_printf(&aca->pbout, "%u\t%s\n", *(unsigned char*)bitnum_obj.data,
@@ -186,8 +186,7 @@ static int com_addatt_callback(struct afs_callback_arg *aca)
 
 		len = strlen(name);
 		if (len == 0 || name[len - 1] == '-' || name[len - 1] == '+') {
-			para_printf(&aca->pbout,
-				"invalid attribute name: %s\n", name);
+			afs_error(aca, "invalid attribute name: %s\n", name);
 			continue;
 		}
 		ret = get_attribute_bitnum_by_name(name, &bitnum);
@@ -226,7 +225,7 @@ static int com_addatt_callback(struct afs_callback_arg *aca)
 	}
 out:
 	if (ret < 0)
-		para_printf(&aca->pbout, "error while adding %s\n",
+		afs_error(aca, "error while adding %s\n",
 			lls_input(i, aca->lpr));
 	lls_free_parse_result(aca->lpr, cmd);
 	return ret;
@@ -269,7 +268,7 @@ static int com_mvatt_callback(struct afs_callback_arg *aca)
 	ret = osl(osl_update_object(attribute_table, row, ATTCOL_NAME, &obj));
 out:
 	if (ret < 0)
-		para_printf(&aca->pbout, "cannot rename %s to %s\n", old, new);
+		afs_error(aca, "cannot rename %s to %s\n", old, new);
 	else
 		ret = afs_event(ATTRIBUTE_RENAME, &aca->pbout, NULL);
 	lls_free_parse_result(aca->lpr, cmd);
@@ -298,13 +297,13 @@ static int remove_attribute(struct osl_table *table, struct osl_row *row,
 
 	ret = get_attribute_bitnum_by_name(name, &red.bitnum);
 	if (ret < 0) {
-		para_printf(&aca->pbout, "cannot remove %s\n", name);
+		afs_error(aca, "cannot remove %s\n", name);
 		return ret;
 	}
 	para_printf(&aca->pbout, "removing attribute %s\n", name);
 	ret = osl(osl_del_row(table, row));
 	if (ret < 0) {
-		para_printf(&aca->pbout, "cannot remove %s\n", name);
+		afs_error(aca, "cannot remove %s\n", name);
 		return ret;
 	}
 	return afs_event(ATTRIBUTE_REMOVE, &aca->pbout, &red);
diff --git a/blob.c b/blob.c
index 20d8cba8..1802de5d 100644
--- a/blob.c
+++ b/blob.c
@@ -100,7 +100,7 @@ static int print_blob(struct osl_table *table, struct osl_row *row,
 	}
 	ret = osl(osl_get_object(table, row, BLOBCOL_ID, &obj));
 	if (ret < 0) {
-		para_printf(&aca->pbout, "cannot list %s\n", name);
+		afs_error(aca, "cannot list %s\n", name);
 		return ret;
 	}
 	id = read_u32(obj.data);
@@ -210,7 +210,7 @@ static int remove_blob(struct osl_table *table, struct osl_row *row,
 	int ret = osl(osl_del_row(table, row));
 
 	if (ret < 0) {
-		para_printf(&aca->pbout, "cannot remove %s\n", name);
+		afs_error(aca, "cannot remove %s\n", name);
 		return ret;
 	}
 	return 1;
@@ -338,7 +338,7 @@ static int com_addblob_callback(__a_unused const struct lls_command * const cmd,
 	ret = afs_event(BLOB_ADD, NULL, table);
 out:
 	if (ret < 0)
-		para_printf(&aca->pbout, "cannot add %s\n", name);
+		afs_error(aca, "cannot add %s\n", name);
 	else
 		para_printf(&aca->pbout, "added %s as id %u\n", name, id);
 	return ret;
@@ -446,15 +446,14 @@ static int com_mvblob_callback(const struct lls_command * const cmd,
 	ret = osl(osl_get_row(table, BLOBCOL_NAME, &obj, &row));
 
 	if (ret < 0) {
-		para_printf(&aca->pbout, "cannot find source blob %s\n", src);
+		afs_error(aca, "cannot find source blob %s\n", src);
 		goto out;
 	}
 	obj.data = (char *)dest;
 	obj.size = strlen(dest) + 1;
 	ret = osl(osl_update_object(table, row, BLOBCOL_NAME, &obj));
 	if (ret < 0) {
-		para_printf(&aca->pbout, "cannot rename blob %s to %s\n",
-			src, dest);
+		afs_error(aca, "cannot rename blob %s to %s\n", src, dest);
 		goto out;
 	}
 	ret = afs_event(BLOB_RENAME, NULL, table);
diff --git a/mood.c b/mood.c
index 50a67793..15da76f2 100644
--- a/mood.c
+++ b/mood.c
@@ -174,14 +174,14 @@ static int init_mood_parser(const char *mood_name, struct mood_instance **m,
 
 static int check_mood(struct osl_row *mood_row, void *data)
 {
-	struct para_buffer *pb = data;
+	struct afs_callback_arg *aca = data;
 	char *mood_name, *errmsg;
 	struct osl_object mood_def;
 	struct mood_instance *m;
 	int ret = mood_get_name_and_def_by_row(mood_row, &mood_name, &mood_def);
 
 	if (ret < 0) {
-		para_printf(pb, "cannot read mood\n");
+		afs_error(aca, "cannot read mood\n");
 		return ret;
 	}
 	if (!*mood_name) /* ignore dummy row */
@@ -190,9 +190,9 @@ static int check_mood(struct osl_row *mood_row, void *data)
 	ret = mp_init(mood_def.data, mood_def.size, &m->parser_context,
 		&errmsg);
 	if (ret < 0) {
-		para_printf(pb, "%s: %s\n", mood_name, errmsg);
+		afs_error(aca, "%s: %s\n%s\n", mood_name, errmsg,
+			para_strerror(-ret));
 		free(errmsg);
-		para_printf(pb, "%s\n", para_strerror(-ret));
 	} else
 		destroy_mood(m);
 	ret = 1; /* don't fail the loop on invalid mood definitions */
@@ -204,7 +204,7 @@ out:
 /**
  * Check all moods for syntax errors.
  *
- * \param aca Only ->pbout is used for diagnostics.
+ * \param aca Output goes to ->pbout, errors to ->fd on the error band.
  *
  * \return Negative on fatal errors. Inconsistent mood definitions are not
  * considered an error.
@@ -212,8 +212,7 @@ out:
 int mood_check_callback(struct afs_callback_arg *aca)
 {
 	para_printf(&aca->pbout, "checking moods...\n");
-	return osl(osl_rbtree_loop(moods_table, BLOBCOL_ID, &aca->pbout,
-		check_mood));
+	return osl(osl_rbtree_loop(moods_table, BLOBCOL_ID, aca, check_mood));
 }
 
 /*
diff --git a/playlist.c b/playlist.c
index 9a6e4829..d02ade3b 100644
--- a/playlist.c
+++ b/playlist.c
@@ -57,31 +57,32 @@ static int add_playlist_entry(char *path, void *data)
 
 static int check_playlist_path(char *path, void *data)
 {
-	struct para_buffer *pb = data;
+	struct afs_callback_arg *aca = data;
 	struct osl_row *aft_row;
 	int ret = aft_get_row_of_path(path, &aft_row);
 
 	if (ret < 0)
-		para_printf(pb, "%s: %s\n", path, para_strerror(-ret));
+		afs_error(aca, "%s: %s\n", path, para_strerror(-ret));
 	return 1; /* do not fail the loop on bad paths */
 }
 
 static int check_playlist(struct osl_row *row, void *data)
 {
-	struct para_buffer *pb = data;
+	struct afs_callback_arg *aca = data;
+	struct para_buffer *pb = &aca->pbout;
 	struct osl_object playlist_def;
 	char *playlist_name;
 	int ret = pl_get_name_and_def_by_row(row, &playlist_name, &playlist_def);
 
 	if (ret < 0) { /* log error, but continue */
-		para_printf(pb, "failed to get playlist data: %s\n",
+		afs_error(aca, "failed to get playlist data: %s\n",
 			para_strerror(-ret));
 		return 1;
 	}
 	if (*playlist_name) { /* skip dummy row */
 		para_printf(pb, "checking playlist %s...\n", playlist_name);
 		for_each_line(FELF_READ_ONLY, playlist_def.data,
-			playlist_def.size, check_playlist_path, pb);
+			playlist_def.size, check_playlist_path, aca);
 	}
 	osl_close_disk_object(&playlist_def);
 	return 1;
@@ -98,7 +99,7 @@ static int check_playlist(struct osl_row *row, void *data)
 int playlist_check_callback(struct afs_callback_arg *aca)
 {
 	para_printf(&aca->pbout, "checking playlists...\n");
-	return osl(osl_rbtree_loop(playlists_table, BLOBCOL_ID, &aca->pbout,
+	return osl(osl_rbtree_loop(playlists_table, BLOBCOL_ID, aca,
 		check_playlist));
 }
 
-- 
2.39.2