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

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Junio & Lars,
>
> On Wed, 5 Oct 2016, Junio C Hamano wrote:
>
>> Lars Schneider  writes:
>> 
>> > OK. Something like the patch below would work nicely.
>> 
>> Yeah, something along that line; it would eliminate the need to
>> worry about a field named "stdin" ;-)
>
> Not only a need to worry

Thanks, but I (and more importantly Lars, too) knew that stdin is
problematic when I sent the message you are responding to, as raised
by Ramsay in a separate thread:

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

[Johannes Schindelin]

Hi Junio & Lars,

On Wed, 5 Oct 2016, Junio C Hamano wrote:

> Lars Schneider  writes:
> 
> > OK. Something like the patch below would work nicely.
> 
> Yeah, something along that line; it would eliminate the need to
> worry about a field named "stdin" ;-)

Not only a need to worry. Git for Windows' SDK's headers define

#define stdin (&__iob_func()[0])

leading to the compile error

In file included from git-compat-util.h:159:0,
 from cache.h:4,
 from run-command.c:1:
run-command.c:25:6: error: expected identifier or '(' before '&' token
  int stdin;
  ^

I meant to investigate this build failure of `pu` earlier but only got
around to do it today.

Ciao,
Dscho

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

[Lars Schneider]

> On 06 Oct 2016, at 11:32, Johannes Schindelin  
> wrote:
> 
> Hi Junio & Lars,
> 
> On Wed, 5 Oct 2016, Junio C Hamano wrote:
> 
>> Lars Schneider  writes:
>> 
>>> OK. Something like the patch below would work nicely.
>> 
>> Yeah, something along that line; it would eliminate the need to
>> worry about a field named "stdin" ;-)
> 
> Not only a need to worry. Git for Windows' SDK's headers define
> 
>   #define stdin (&__iob_func()[0])
> 
> leading to the compile error
> 
>   In file included from git-compat-util.h:159:0,
> from cache.h:4,
> from run-command.c:1:
>   run-command.c:25:6: error: expected identifier or '(' before '&' token
> int stdin;
> ^
> 
> I meant to investigate this build failure of `pu` earlier but only got
> around to do it today.
> 
> Ciao,
> Dscho

Sorry for the trouble. The "stdin" will go away in the next round
as we agreed on a more generic solution:
http://public-inbox.org/git/1fd7fb64-0f40-47f0-a047-25b91b170...@gmail.com/

- Lars

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

[Junio C Hamano]

Lars Schneider  writes:

> OK. Something like the patch below would work nicely.

Yeah, something along that line; it would eliminate the need to
worry about a field named "stdin" ;-)

But ...

>   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);
> + }

... the application that used run_command() API would want to be
able to pass extra piece of data that is appliation-specific for the
child being killed, so it may make sense to extend the function
signature to take a pointer to "struct child_process" for the child
process being killed, together with a new field added to "struct
child_process" that is "void *exit_handler_cbdata;", perhaps?

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

[Lars Schneider]

> On 04 Oct 2016, at 21:30, Junio C Hamano  wrote:
> 
> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider 
>> 
>> 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 
>> ---
>> ...
>> 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 }
 

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

[Junio C Hamano]

larsxschnei...@gmail.com writes:

> From: Lars Schneider 
>
> 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 
> ---
>  run-command.c | 55 +++
>  run-command.h |  3 +++
>  2 files changed, 50 insertions(+), 8 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 3269362..96c54fe 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -21,6 +21,9 @@ void child_process_clear(struct child_process *child)
>
>  struct child_to_clean {
>   pid_t pid;
> + char *name;
> + int stdin;
> + int wait;
>   struct child_to_clean *next;
>  };
>  static struct child_to_clean *children_to_clean;
> @@ -28,12 +31,33 @@ static int installed_child_cleanup_handler;
>
>  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?

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

[larsxschneider]

From: Lars Schneider 

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 
---
 run-command.c | 55 +++
 run-command.h |  3 +++
 2 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/run-command.c b/run-command.c
index 3269362..96c54fe 100644
--- a/run-command.c
+++ b/run-command.c
@@ -21,6 +21,9 @@ void child_process_clear(struct child_process *child)

 struct child_to_clean {
pid_t pid;
+   char *name;
+   int stdin;
+   int wait;
struct child_to_clean *next;
 };
 static struct child_to_clean *children_to_clean;
@@ -28,12 +31,33 @@ static int installed_child_cleanup_handler;

 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;
+   }
+
while (children_to_clean) {
-   struct child_to_clean *p = children_to_clean;
+   p = children_to_clean;
children_to_clean = p->next;
+
+   if (p->wait) {
+   fprintf(stderr, _("Waiting for '%s' to finish..."), 
p->name);
+   while ((waitpid(p->pid, &status, 0)) < 0 && errno == 
EINTR)
+   ;   /* nothing */
+   fprintf(stderr, _("done!\n"));
+   }
+
kill(p->pid, sig);
-   if (!in_signal)
+   if (!in_signal) {
+   free(p->name);
free(p);
+   }
}
 }

@@ -49,10 +73,16 @@ 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, const char *name, int stdin, int 
wait)
 {
struct child_to_clean *p = xmalloc(sizeof(*p));
p->pid = pid;
+   p->wait = wait;
+   p->stdin = stdin;
+   if (name)
+   p->name = xstrdup(name);
+   else
+   p->name = "process";
p->next = children_to_clean;
children_to_clean = p;

@@ -63,6 +93,13 @@ static void mark_child_for_cleanup(pid_t pid)
}
 }

+#ifdef NO_PTHREADS
+static void mark_child_for_cleanup_no_wait(pid_t pid, const char *name, int 
timeout, int stdin)
+{
+   mark_child_for_cleanup(pid, NULL, 0, 0);
+}
+#endif
+
 static void clear_child_for_cleanup(pid_t pid)
 {
struct child_to_clean **pp;
@@ -421,8 +458,9 @@ 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->wait_on_exit)
+   mark_child_for_cleanup(
+   cmd->pid, cmd->argv[0], cmd->in, cmd->wait_on_exit);

/*
 * Wait for child's execvp. If the execvp succeeds (or if fork()
@@ -482,8 +520,9 @@ 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->wait_on_exit) && cmd->pid >= 0)
+   mark_child_for_cleanup(
+   cmd->pid, cmd->argv[0], cmd->in, 
cmd->clean_on_exit_timeout);

argv_array_clear(&nargv);
cmd->argv = sargv;
@@ -765,7 +804,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_no_wait(async->pid);

if (need_in)
close(fdin[0]);
diff --git a/run-command.h b/run-command.h
index cf29a31..f7b9907 100644
--- a/run-command.h
+++ b/run-command.h
@@ -42,7 +42,10 @@ struct child_process {
unsigned silent_exec_failure:1;
unsigned stdout_to_stderr:1;
unsigned use_shell:1;
+/* kill the child on Git exit */
unsigned clean_on_exit:1;
+   /* close the child's stdin on Git exit and wait until it terminates */
+   unsigned wait_on_exit:1;
 };

 #define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT, ARGV_ARRAY_INIT }
--
2.10.0