Re: [msysGit] [PATCH/RFC 09/11] daemon: use run-command api for async serving

On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:

 static void check_dead_children(void)
 {
-	int status;
-	pid_t pid;
-
-	while ((pid = waitpid(-1, &status, WNOHANG)) > 0) {
-		const char *dead = "";
-		remove_child(pid);
-		if (!WIFEXITED(status) || (WEXITSTATUS(status) > 0))
-			dead = " (with error)";
-		loginfo("[%"PRIuMAX"] Disconnected%s", (uintmax_t)pid, dead);
-	}
+	struct child **cradle, *blanket;
+	for (cradle = &firstborn; (blanket = *cradle);)
+		if (!is_async_alive(&blanket->async)) {

This would be the right place to call finish_async(). But since we cannot 
wait, you invented is_async_alive(). But actually we are not only interested 
in whether the process is alive, but also whether it completed successfully 
so that we can add "(with error)". Would it make sense to have a function 
finish_async_nowait() instead of is_async_alive() that (1) stresses the 
start/finish symmetry and (2) can return more than just Boolean?

+			*cradle = blanket->next;
+			loginfo("Disconnected\n");

Here you are losing information about the pid, which is important to have in 
the syslog. The \n should be dropped.

+	async.proc = async_execute;
+	async.data = ss;
+	async.out = incoming;

-	dup2(incoming, 0);
-	dup2(incoming, 1);
+	if (start_async(&async))
+		logerror("unable to fork");
+	else
+		add_child(&async, addr, addrlen);
 	close(incoming);
-
-	exit(execute(0, addr));

In start_command(), the convention is that fds that are provided by the caller 
are closed by start_command() (even if there are errors). The close(incoming) 
that you leave here indicates that you are not using the same convention with 
start_async(). It would be nice to switch to the same convention.

-- Hannes