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

From: Johannes Sixt <j6t@kdbg.org>
Date: Nov 27, 2009, 20:59 UTC
Message-ID: <200911272159.38757.j6t@kdbg.org>
In-Reply-To: <1259196260-3064-10-git-send-email-kusmabite@gmail.com>

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

Show 15 quoted lines

>  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.

Show 13 quoted lines

> +	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