From: Andre Noll <maan@tuebingen.mpg.de>
Date: Tue, 13 Jan 2015 22:53:28 +0000 (+0100)
Subject: audiod: Clean up fd closing logic in command handlers.
X-Git-Tag: v0.5.5~18
X-Git-Url: http://git.tuebingen.mpg.de/?p=paraslash.git;a=commitdiff_plain;h=c781b528c69927871c62cff33e94c87ce251bde9

audiod: Clean up fd closing logic in command handlers.

audiod_command.c contains this comment:

	/*
	 * command handlers don't close their fd on errors (ret < 0) so that
	 * its caller can send an error message. Otherwise (ret >= 0) it's up
	 * to each individual command to close the fd if necessary.
	 */

This is a somewhat weird rule and this commit gets rid of it. Instead,
from now on the command handlers must not close their file descriptor
and handle_connect() closes it unconditionally.

The grab and stat commands need special treatment, which was the reason
for imposing the above rule. They need to keep the file descriptor open
to send the status items or the grabbed stream to the client. This
patch makes these two handlers create a copy of the descriptor with
dup(2). The new approach is simpler and less error-prone.
---

diff --git a/audiod_command.c b/audiod_command.c
index 0fe2e5f0..2b18837b 100644
--- a/audiod_command.c
+++ b/audiod_command.c
@@ -110,15 +110,19 @@ static void dump_stat_client_list(void)
 static int stat_client_add(int fd, uint64_t mask, int parser_friendly)
 {
 	struct stat_client *new_client;
+	int ret;
 
 	if (num_clients >= MAX_STAT_CLIENTS) {
 		PARA_ERROR_LOG("maximal number of stat clients (%d) exceeded\n",
 			MAX_STAT_CLIENTS);
 		return -E_TOO_MANY_CLIENTS;
 	}
-	PARA_INFO_LOG("adding client on fd %d\n", fd);
-	new_client = para_calloc(sizeof(struct stat_client));
-	new_client->fd = fd;
+	ret = dup(fd);
+	if (ret < 0)
+		return -ERRNO_TO_PARA_ERROR(errno);
+	new_client = para_calloc(sizeof(*new_client));
+	new_client->fd = ret;
+	PARA_INFO_LOG("adding client on fd %d\n", new_client->fd);
 	new_client->item_mask = mask;
 	if (parser_friendly)
 		new_client->flags = SCF_PARSER_FRIENDLY;
@@ -245,22 +249,14 @@ static int dump_commands(int fd)
 	return ret;
 }
 
-/*
- * command handlers don't close their fd on errors (ret < 0) so that
- * its caller can send an error message. Otherwise (ret >= 0) it's up
- * to each individual command to close the fd if necessary.
- */
-
 static int com_help(int fd, int argc, char **argv)
 {
 	int i, ret;
 	char *buf;
 	const char *dflt = "No such command. Available commands:\n";
 
-	if (argc < 2) {
-		ret = dump_commands(fd);
-		goto out;
-	}
+	if (argc < 2)
+		return dump_commands(fd);
 	FOR_EACH_COMMAND(i) {
 		if (strcmp(audiod_cmds[i].name, argv[1]))
 			continue;
@@ -275,14 +271,11 @@ static int com_help(int fd, int argc, char **argv)
 		);
 		ret = client_write(fd, buf);
 		free(buf);
-		goto out;
+		return ret;
 	}
 	ret = client_write(fd, dflt);
 	if (ret > 0)
 		ret = dump_commands(fd);
-out:
-	if (ret >= 0)
-		close(fd);
 	return ret;
 }
 
@@ -294,8 +287,6 @@ static int com_tasks(int fd, __a_unused int argc, __a_unused char **argv)
 	if (tl)
 		ret = client_write(fd, tl);
 	free(tl);
-	if (ret > 0)
-		close(fd);
 	return ret;
 }
 
@@ -349,34 +340,30 @@ static int com_grab(int fd, int argc, char **argv)
 	return grab_client_new(fd, argc, argv, &sched);
 }
 
-static int com_term(int fd, __a_unused int argc, __a_unused char **argv)
+static int com_term(__a_unused int fd, __a_unused int argc, __a_unused char **argv)
 {
-	close(fd);
 	return -E_AUDIOD_TERM;
 }
 
-static int com_on(int fd, __a_unused int argc, __a_unused char **argv)
+static int com_on(__a_unused int fd, __a_unused int argc, __a_unused char **argv)
 {
 	audiod_status = AUDIOD_ON;
-	close(fd);
 	return 1;
 }
 
-static int com_off(int fd, __a_unused int argc, __a_unused char **argv)
+static int com_off(__a_unused int fd, __a_unused int argc, __a_unused char **argv)
 {
 	audiod_status = AUDIOD_OFF;
-	close(fd);
 	return 1;
 }
 
-static int com_sb(int fd, __a_unused int argc, __a_unused char **argv)
+static int com_sb(__a_unused int fd, __a_unused int argc, __a_unused char **argv)
 {
 	audiod_status = AUDIOD_STANDBY;
-	close(fd);
 	return 1;
 }
 
-static int com_cycle(int fd, int argc, char **argv)
+static int com_cycle(__a_unused int fd, int argc, char **argv)
 {
 	switch (audiod_status) {
 		case  AUDIOD_ON:
@@ -389,7 +376,6 @@ static int com_cycle(int fd, int argc, char **argv)
 			return com_off(fd, argc, argv);
 			break;
 	}
-	close(fd);
 	return 1;
 }
 
@@ -404,8 +390,6 @@ static int com_version(int fd, int argc, char **argv)
 		msg = make_message("%s\n", version_single_line("audiod"));
 	ret = client_write(fd, msg);
 	free(msg);
-	if (ret >= 0)
-		close(fd);
 	return ret;
 }
 
@@ -471,12 +455,12 @@ int handle_connect(int accept_fd, fd_set *rfds, uid_t *uid_whitelist)
 	ret = -E_INVALID_AUDIOD_CMD;
 out:
 	free_argv(argv);
-	if (clifd > 0 && ret < 0 && ret != -E_CLIENT_WRITE) {
+	if (ret < 0 && ret != -E_CLIENT_WRITE) {
 		char *tmp = make_message("%s\n", para_strerror(-ret));
 		client_write(clifd, tmp);
 		free(tmp);
-		close(clifd);
 	}
+	close(clifd);
 	return ret;
 }
 
diff --git a/grab_client.c b/grab_client.c
index 1a529176..0ef1c15f 100644
--- a/grab_client.c
+++ b/grab_client.c
@@ -284,7 +284,12 @@ int grab_client_new(int fd, int argc, char **argv, struct sched *s)
 	ret = gc_check_args(argc, argv, gc);
 	if (ret < 0)
 		goto err_out;
-	gc->fd = fd;
+	ret = dup(fd);
+	if (ret < 0) {
+		ret = -ERRNO_TO_PARA_ERROR(errno);
+		goto err_out;
+	}
+	gc->fd = ret;
 	para_list_add(&gc->node, &inactive_grab_client_list);
 	gc_activate(gc, s);
 	return 1;