Re: [PATCH v9 04/14] run-command: add wait_on_exit

[Lars Schneider]

> On 04 Oct 2016, at 21:30, Junio C Hamano <gits...@pobox.com> wrote:
> 
> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> The flag 'clean_on_exit' kills child processes spawned by Git on exit.
>> A hard kill like this might not be desired in all cases.
>> 
>> Add 'wait_on_exit' which closes the child's stdin on Git exit and waits
>> until the child process has terminated.
>> 
>> The flag is used in a subsequent patch.
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>> ...
>> static void cleanup_children(int sig, int in_signal)
>> {
>> +    int status;
>> +    struct child_to_clean *p = children_to_clean;
>> +
>> +    /* Close the the child's stdin as indicator that Git will exit soon */
>> +    while (p) {
>> +            if (p->wait)
>> +                    if (p->stdin > 0)
>> +                            close(p->stdin);
>> +            p = p->next;
>> +    }
> 
> This part and the "stdin" field feels a bit too specific to the
> caller you are adding.  Allowing the user of the API to specify what
> clean-up cation needs to be taken in the form of a callback function
> may not be that much more effort and would be more flexible and
> useful, I would imagine?

OK. Something like the patch below would work nicely.
Does this look acceptable?

Thanks,
Lars


diff --git a/run-command.c b/run-command.c
index 3269362..a0256a6 100644
--- a/run-command.c
+++ b/run-command.c
@@ -21,6 +21,7 @@ void child_process_clear(struct child_process *child)
 
 struct child_to_clean {
        pid_t pid;
+       void (*clean_on_exit_handler)(pid_t, int);
        struct child_to_clean *next;
 };
 static struct child_to_clean *children_to_clean;
@@ -31,6 +32,11 @@ static void cleanup_children(int sig, int in_signal)
        while (children_to_clean) {
                struct child_to_clean *p = children_to_clean;
                children_to_clean = p->next;
+
+               if (p->clean_on_exit_handler) {
+                       p->clean_on_exit_handler(p->pid, in_signal);
+               }
+
                kill(p->pid, sig);
                if (!in_signal)
                        free(p);
@@ -49,10 +55,11 @@ static void cleanup_children_on_exit(void)
        cleanup_children(SIGTERM, 0);
 }
 
-static void mark_child_for_cleanup(pid_t pid)
+static void mark_child_for_cleanup(pid_t pid, void 
(*clean_on_exit_handler)(pid_t, int))
 {
        struct child_to_clean *p = xmalloc(sizeof(*p));
        p->pid = pid;
+       p->clean_on_exit_handler = clean_on_exit_handler;
        p->next = children_to_clean;
        children_to_clean = p;
 
@@ -421,8 +428,8 @@ int start_command(struct child_process *cmd)
        }
        if (cmd->pid < 0)
                error_errno("cannot fork() for %s", cmd->argv[0]);
-       else if (cmd->clean_on_exit)
-               mark_child_for_cleanup(cmd->pid);
+       else if (cmd->clean_on_exit || cmd->clean_on_exit_handler)
+               mark_child_for_cleanup(cmd->pid, cmd->clean_on_exit_handler);
 
        /*
         * Wait for child's execvp. If the execvp succeeds (or if fork()
@@ -482,8 +489,8 @@ int start_command(struct child_process *cmd)
        failed_errno = errno;
        if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
                error_errno("cannot spawn %s", cmd->argv[0]);
-       if (cmd->clean_on_exit && cmd->pid >= 0)
-               mark_child_for_cleanup(cmd->pid);
+       if ((cmd->clean_on_exit || cmd->clean_on_exit_handler) && cmd->pid >= 0)
+               mark_child_for_cleanup(cmd->pid, cmd->clean_on_exit_handler);
 
        argv_array_clear(&nargv);
        cmd->argv = sargv;
@@ -765,7 +772,7 @@ int start_async(struct async *async)
                exit(!!async->proc(proc_in, proc_out, async->data));
        }
 
-       mark_child_for_cleanup(async->pid);
+       mark_child_for_cleanup(async->pid, NULL);
 
        if (need_in)
                close(fdin[0]);
diff --git a/run-command.h b/run-command.h
index cf29a31..3630733 100644
--- a/run-command.h
+++ b/run-command.h
@@ -43,6 +43,7 @@ struct child_process {
        unsigned stdout_to_stderr:1;
        unsigned use_shell:1;
        unsigned clean_on_exit:1;
+       void (*clean_on_exit_handler)(pid_t, int);
 };
 
 #define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT, ARGV_ARRAY_INIT }