From: Erik Faye-Lund <kusmabite@googlemail.com>
Date: Wed, 02 Dec 2009 15:45:03 GMT
Subject: Re: [msysGit] [PATCH/RFC 09/11] daemon: use run-command api for async serving
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:
> 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