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

On Fri, Nov 27, 2009 at 9:59 PM, Johannes Sixt [off-list ref] wrote:

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

quoted

 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?

Yes, it does.

quoted

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

Yeah... I removed the pid mostly because after moving to async, there
wasn't "just a pid" any more. But if we make finish_async_nowait()
return whatever we need to report, I guess we can add the information
back somehow.

I'm not entirely sure how to make the interface, though. Any good suggestions?

quoted

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

Yeah, I've fixed this for the next round.

-- 
Erik "kusma" Faye-Lund