On Fri, Nov 27, 2009 at 9:59 PM, Johannes Sixt <j6t@xxxxxxxx> wrote:
> 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?
>
Yes, it does.
>> + *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?
>> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html