Re: [PATCH v3 06/10] run-command: add clean_on_exit_handler

[Lars Schneider]

> On 02 Aug 2016, at 07:53, Johannes Sixt  wrote:
> 
> Am 01.08.2016 um 13:14 schrieb Lars Schneider:
> >> On 30 Jul 2016, at 11:50, Johannes Sixt  wrote:
> >> Am 30.07.2016 um 01:37 schrieb larsxschnei...@gmail.com:
> >>> static struct child_to_clean *children_to_clean;
> >>> @@ -30,6 +31,8 @@ static void cleanup_children(int sig, int in_signal)
> >>> {
> >>>   while (children_to_clean) {
> >>>   struct child_to_clean *p = children_to_clean;
> >>> + if (p->clean_on_exit_handler)
> >>> + p->clean_on_exit_handler(p->pid);
> >>
> >> This summons demons. cleanup_children() is invoked from a signal
> >> handler. In this case, it can call only async-signal-safe functions.
> >> It does not look like the handler that you are going to install
> >> later will take note of this caveat!
> >>
> >>>   children_to_clean = p->next;
> >>>   kill(p->pid, sig);
> >>>   if (!in_signal)
> >>
> >> The condition that we see here in the context protects free(p)
> >> (which is not async-signal-safe). Perhaps the invocation of the new
> >> callback should be skipped in the same manner when this is called
> >> from a signal handler? 507d7804 (pager: don't use unsafe functions
> >> in signal handlers) may be worth a look.
> >
> > Thanks a lot of pointing this out to me!
> >
> > Do I get it right that after the signal "SIGTERM" I can do a cleanup
> > and don't need to worry about any function calls but if I get any
> > other signal then I can only perform async-signal-safe calls?
> 
> No. SIGTERM is not special.
> 
> Perhaps you were misled by the SIGTERM mentioned in 
> cleanup_children_on_exit()? This function is invoked on regular exit (not 
> from a signal). SIGTERM is used in this case to terminate children that are 
> still lingering around.

Yes, that was my source of confusion. Thanks for the clarification!

> 
> > If this is correct, then the following solution would work great:
> >
> > if (!in_signal && p->clean_on_exit_handler)
> > p->clean_on_exit_handler(p->pid);
> 
> This should work nevertheless because in_signal is set when the function is 
> invoked from a signal handler (of any signal that is caught) via 
> cleanup_children_on_signal().

Right. Thank you!

- Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 06/10] run-command: add clean_on_exit_handler

[Johannes Sixt]

Am 01.08.2016 um 13:14 schrieb Lars Schneider:
>> On 30 Jul 2016, at 11:50, Johannes Sixt  wrote:
>> Am 30.07.2016 um 01:37 schrieb larsxschnei...@gmail.com:
>>> static struct child_to_clean *children_to_clean;
>>> @@ -30,6 +31,8 @@ static void cleanup_children(int sig, int in_signal)
>>> {
>>>while (children_to_clean) {
>>>struct child_to_clean *p = children_to_clean;
>>> +  if (p->clean_on_exit_handler)
>>> +  p->clean_on_exit_handler(p->pid);
>>
>> This summons demons. cleanup_children() is invoked from a signal
>> handler. In this case, it can call only async-signal-safe functions.
>> It does not look like the handler that you are going to install
>> later will take note of this caveat!
>>
>>>children_to_clean = p->next;
>>>kill(p->pid, sig);
>>>if (!in_signal)
>>
>> The condition that we see here in the context protects free(p)
>> (which is not async-signal-safe). Perhaps the invocation of the new
>> callback should be skipped in the same manner when this is called
>> from a signal handler? 507d7804 (pager: don't use unsafe functions
>> in signal handlers) may be worth a look.
>
> Thanks a lot of pointing this out to me!
>
> Do I get it right that after the signal "SIGTERM" I can do a cleanup
> and don't need to worry about any function calls but if I get any
> other signal then I can only perform async-signal-safe calls?

No. SIGTERM is not special.

Perhaps you were misled by the SIGTERM mentioned in 
cleanup_children_on_exit()? This function is invoked on regular exit 
(not from a signal). SIGTERM is used in this case to terminate children 
that are still lingering around.


> If this is correct, then the following solution would work great:
>
>if (!in_signal && p->clean_on_exit_handler)
>p->clean_on_exit_handler(p->pid);

This should work nevertheless because in_signal is set when the function 
is invoked from a signal handler (of any signal that is caught) via 
cleanup_children_on_signal().


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 06/10] run-command: add clean_on_exit_handler

[Lars Schneider]

> On 30 Jul 2016, at 11:50, Johannes Sixt  wrote:
> 
> Am 30.07.2016 um 01:37 schrieb larsxschnei...@gmail.com:
>> Some commands might need to perform cleanup tasks on exit. Let's give
>> them an interface for doing this.
>> 
>> Signed-off-by: Lars Schneider 
>> ---
>> run-command.c | 12 
>> run-command.h |  1 +
>> 2 files changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/run-command.c b/run-command.c
>> index 33bc63a..197b534 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);
>>  struct child_to_clean *next;
>> };
>> static struct child_to_clean *children_to_clean;
>> @@ -30,6 +31,8 @@ static void cleanup_children(int sig, int in_signal)
>> {
>>  while (children_to_clean) {
>>  struct child_to_clean *p = children_to_clean;
>> +if (p->clean_on_exit_handler)
>> +p->clean_on_exit_handler(p->pid);
> 
> This summons demons. cleanup_children() is invoked from a signal handler. In 
> this case, it can call only async-signal-safe functions. It does not look 
> like the handler that you are going to install later will take note of this 
> caveat!
> 
>>  children_to_clean = p->next;
>>  kill(p->pid, sig);
>>  if (!in_signal)
> 
> The condition that we see here in the context protects free(p) (which is not 
> async-signal-safe). Perhaps the invocation of the new callback should be 
> skipped in the same manner when this is called from a signal handler? 
> 507d7804 (pager: don't use unsafe functions in signal handlers) may be worth 
> a look.

Thanks a lot of pointing this out to me!

Do I get it right that after the signal "SIGTERM" I can do a cleanup and don't 
need to worry about any function calls but if I get any other signal then I can 
only perform async-signal-safe calls?

If this is correct, then the following solution would work great:

if (!in_signal && p->clean_on_exit_handler)
p->clean_on_exit_handler(p->pid);

Thanks,
Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 06/10] run-command: add clean_on_exit_handler

[Johannes Sixt]

Am 30.07.2016 um 01:37 schrieb larsxschnei...@gmail.com:

Some commands might need to perform cleanup tasks on exit. Let's give
them an interface for doing this.

Signed-off-by: Lars Schneider 
---
 run-command.c | 12 
 run-command.h |  1 +
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/run-command.c b/run-command.c
index 33bc63a..197b534 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);
struct child_to_clean *next;
 };
 static struct child_to_clean *children_to_clean;
@@ -30,6 +31,8 @@ static void cleanup_children(int sig, int in_signal)
 {
while (children_to_clean) {
struct child_to_clean *p = children_to_clean;
+   if (p->clean_on_exit_handler)
+   p->clean_on_exit_handler(p->pid);


This summons demons. cleanup_children() is invoked from a signal 
handler. In this case, it can call only async-signal-safe functions. It 
does not look like the handler that you are going to install later will 
take note of this caveat!



children_to_clean = p->next;
kill(p->pid, sig);
if (!in_signal)


The condition that we see here in the context protects free(p) (which is 
not async-signal-safe). Perhaps the invocation of the new callback 
should be skipped in the same manner when this is called from a signal 
handler? 507d7804 (pager: don't use unsafe functions in signal handlers) 
may be worth a look.


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 06/10] run-command: add clean_on_exit_handler

[larsxschneider]

From: Lars Schneider 

Some commands might need to perform cleanup tasks on exit. Let's give
them an interface for doing this.

Signed-off-by: Lars Schneider 
---
 run-command.c | 12 
 run-command.h |  1 +
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/run-command.c b/run-command.c
index 33bc63a..197b534 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);
struct child_to_clean *next;
 };
 static struct child_to_clean *children_to_clean;
@@ -30,6 +31,8 @@ static void cleanup_children(int sig, int in_signal)
 {
while (children_to_clean) {
struct child_to_clean *p = children_to_clean;
+   if (p->clean_on_exit_handler)
+   p->clean_on_exit_handler(p->pid);
children_to_clean = p->next;
kill(p->pid, sig);
if (!in_signal)
@@ -49,10 +52,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))
 {
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;
 
@@ -422,7 +426,7 @@ 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);
+   mark_child_for_cleanup(cmd->pid, cmd->clean_on_exit_handler);
 
/*
 * Wait for child's execvp. If the execvp succeeds (or if fork()
@@ -483,7 +487,7 @@ int start_command(struct child_process *cmd)
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);
+   mark_child_for_cleanup(cmd->pid, cmd->clean_on_exit_handler);
 
argv_array_clear();
cmd->argv = sargv;
@@ -752,7 +756,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 5066649..59d21ea 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);
 };
 
 #define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT, ARGV_ARRAY_INIT }
-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html