Re: [msysGit] [PATCH/RFC 09/11] daemon: use run-command api for async serving
- From
Erik Faye-Lund <kusmabite@googlemail.com>
- Date
- Dec 2, 2009, 15:45 UTC
- Message-ID
- <40aa078e0912020745o4b72342fm722a944621cfda5@mail.gmail.com>
- In-Reply-To
- <200911272159.38757.j6t@kdbg.org>
On Fri, Nov 27, 2009 at 9:59 PM, Johannes Sixt <j6t@kdbg.org> wrote:
Show 24 quoted lines
> 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.
Show 6 quoted lines
>> + *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?
Show 19 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.
>Yeah, I've fixed this for the next round.
-- Erik "kusma" Faye-Lund