* [Tarantool-patches] [PATCH 0/2] popen: provide more flags @ 2020-04-10 14:40 Cyrill Gorcunov 2020-04-10 14:40 ` [Tarantool-patches] [PATCH 1/2] popen: Allow to kill process group Cyrill Gorcunov 2020-04-10 14:40 ` [Tarantool-patches] [PATCH 2/2] popen: add ability to keep child on deletion Cyrill Gorcunov 0 siblings, 2 replies; 7+ messages in thread From: Cyrill Gorcunov @ 2020-04-10 14:40 UTC (permalink / raw) To: tml To be able to kill the group and keep child process running even if tarantool has exited. branch gorcunov/gh-4031-popen-tok-2 Cyrill Gorcunov (2): popen: Allow to kill process group popen: add ability to keep child on deletion src/lib/core/popen.c | 31 +++++++++++++++++++++++++++---- src/lib/core/popen.h | 12 ++++++++++++ 2 files changed, 39 insertions(+), 4 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Tarantool-patches] [PATCH 1/2] popen: Allow to kill process group 2020-04-10 14:40 [Tarantool-patches] [PATCH 0/2] popen: provide more flags Cyrill Gorcunov @ 2020-04-10 14:40 ` Cyrill Gorcunov 2020-04-10 16:41 ` Alexander Turenko 2020-04-12 1:27 ` Alexander Turenko 2020-04-10 14:40 ` [Tarantool-patches] [PATCH 2/2] popen: add ability to keep child on deletion Cyrill Gorcunov 1 sibling, 2 replies; 7+ messages in thread From: Cyrill Gorcunov @ 2020-04-10 14:40 UTC (permalink / raw) To: tml As Alexander pointed out this might be useful for running a pipe of programs inside shell (i.e. popen.shell('foo | bar | baz', 'r')). Reported-by: Alexander Turenko <alexander.turenko@tarantool.org> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com> --- src/lib/core/popen.c | 26 +++++++++++++++++++++++--- src/lib/core/popen.h | 6 ++++++ 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c index 6b6062215..200b31b21 100644 --- a/src/lib/core/popen.c +++ b/src/lib/core/popen.c @@ -86,6 +86,19 @@ handle_new(struct popen_opts *opts) assert(opts->argv != NULL && opts->nr_argv > 0); + /* + * Killing group of signals allowed for a new + * session only where it makes sense, otherwise + * child gonna inherit group and we will be killing + * ourself. + */ + if (opts->flags & POPEN_FLAG_GROUP_SIGNAL && + (opts->flags & POPEN_FLAG_SETSID) == 0) { + diag_set(IllegalParams, + "popen: group signal without setting sid"); + return NULL; + } + for (i = 0; i < opts->nr_argv; i++) { if (opts->argv[i] == NULL) continue; @@ -443,10 +456,17 @@ popen_send_signal(struct popen_handle *handle, int signo) if (!popen_may_pidop(handle)) return -1; - say_debug("popen: kill %d signo %d", handle->pid, signo); - ret = kill(handle->pid, signo); + say_debug("popen: %s %d signo %d", + handle->flags & POPEN_FLAG_GROUP_SIGNAL ? + "killpg" : "kill", handle->pid, signo); + if (handle->flags & POPEN_FLAG_GROUP_SIGNAL) + ret = killpg(handle->pid, signo); + else + ret = kill(handle->pid, signo); if (ret < 0) { - diag_set(SystemError, "Unable to kill %d signo %d", + diag_set(SystemError, "Unable to %s %d signo %d", + handle->flags & POPEN_FLAG_GROUP_SIGNAL ? + "killpg" : "kill", handle->pid, signo); } return ret; diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h index 9cbfed60c..f9dd0ff45 100644 --- a/src/lib/core/popen.h +++ b/src/lib/core/popen.h @@ -96,6 +96,12 @@ enum popen_flag_bits { */ POPEN_FLAG_RESTORE_SIGNALS_BIT = 15, POPEN_FLAG_RESTORE_SIGNALS = (1 << POPEN_FLAG_RESTORE_SIGNALS_BIT), + + /* + * Send signal to a process group. + */ + POPEN_FLAG_GROUP_SIGNAL_BIT = 16, + POPEN_FLAG_GROUP_SIGNAL = (1 << POPEN_FLAG_GROUP_SIGNAL_BIT), }; /** -- 2.20.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] popen: Allow to kill process group 2020-04-10 14:40 ` [Tarantool-patches] [PATCH 1/2] popen: Allow to kill process group Cyrill Gorcunov @ 2020-04-10 16:41 ` Alexander Turenko 2020-04-12 1:27 ` Alexander Turenko 1 sibling, 0 replies; 7+ messages in thread From: Alexander Turenko @ 2020-04-10 16:41 UTC (permalink / raw) To: Cyrill Gorcunov; +Cc: tml On Fri, Apr 10, 2020 at 05:40:20PM +0300, Cyrill Gorcunov wrote: > As Alexander pointed out this might be useful > for running a pipe of programs inside shell > (i.e. popen.shell('foo | bar | baz', 'r')). > > Reported-by: Alexander Turenko <alexander.turenko@tarantool.org> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com> Acked-by: Alexander Turenko <alexander.turenko@tarantool.org> > @@ -443,10 +456,17 @@ popen_send_signal(struct popen_handle *handle, int signo) > if (!popen_may_pidop(handle)) > return -1; > > - say_debug("popen: kill %d signo %d", handle->pid, signo); > - ret = kill(handle->pid, signo); > + say_debug("popen: %s %d signo %d", > + handle->flags & POPEN_FLAG_GROUP_SIGNAL ? > + "killpg" : "kill", handle->pid, signo); > + if (handle->flags & POPEN_FLAG_GROUP_SIGNAL) > + ret = killpg(handle->pid, signo); > + else > + ret = kill(handle->pid, signo); > if (ret < 0) { > - diag_set(SystemError, "Unable to kill %d signo %d", > + diag_set(SystemError, "Unable to %s %d signo %d", > + handle->flags & POPEN_FLAG_GROUP_SIGNAL ? > + "killpg" : "kill", > handle->pid, signo); > } Nit: I would define a string constant, it will be easier to read I guess. | const char *kill_func = handle->flags & POPEN_FLAG_GROUP_SIGNAL ? | "killpg" : "kill"; This function is changed in the 'Popen Lua API: preliminary patches' patchset, which is pushed to master now. Need to be rebased so. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] popen: Allow to kill process group 2020-04-10 14:40 ` [Tarantool-patches] [PATCH 1/2] popen: Allow to kill process group Cyrill Gorcunov 2020-04-10 16:41 ` Alexander Turenko @ 2020-04-12 1:27 ` Alexander Turenko 2020-04-12 1:40 ` Alexander Turenko 1 sibling, 1 reply; 7+ messages in thread From: Alexander Turenko @ 2020-04-12 1:27 UTC (permalink / raw) To: Cyrill Gorcunov; +Cc: tml > + /* > + * Killing group of signals allowed for a new > + * session only where it makes sense, otherwise > + * child gonna inherit group and we will be killing > + * ourself. > + */ > + if (opts->flags & POPEN_FLAG_GROUP_SIGNAL && > + (opts->flags & POPEN_FLAG_SETSID) == 0) { > + diag_set(IllegalParams, > + "popen: group signal without setting sid"); > + return NULL; > + } > + Since the patchset with the preliminary patches is land to master and it contains formal list of possible errors for popen_new(), I propose the following addition: diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c index 593e72d4d..fcc562734 100644 --- a/src/lib/core/popen.c +++ b/src/lib/core/popen.c @@ -812,6 +812,8 @@ signal_reset(void) * * Possible errors: * + * - IllegalParams: a parameter check fails: + * - group signal is set, while setsid is not. * - SystemError: dup(), fcntl(), pipe(), vfork() or close() fails * in the parent process. * - SystemError: (temporary restriction) one of std{in,out,err} ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] popen: Allow to kill process group 2020-04-12 1:27 ` Alexander Turenko @ 2020-04-12 1:40 ` Alexander Turenko 0 siblings, 0 replies; 7+ messages in thread From: Alexander Turenko @ 2020-04-12 1:40 UTC (permalink / raw) To: Cyrill Gorcunov; +Cc: tml On Sun, Apr 12, 2020 at 04:27:59AM +0300, Alexander Turenko wrote: > > + /* > > + * Killing group of signals allowed for a new > > + * session only where it makes sense, otherwise > > + * child gonna inherit group and we will be killing > > + * ourself. > > + */ > > + if (opts->flags & POPEN_FLAG_GROUP_SIGNAL && > > + (opts->flags & POPEN_FLAG_SETSID) == 0) { > > + diag_set(IllegalParams, > > + "popen: group signal without setting sid"); > > + return NULL; > > + } > > + > > Since the patchset with the preliminary patches is land to master and it > contains formal list of possible errors for popen_new(), I propose the > following addition: > > diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c > index 593e72d4d..fcc562734 100644 > --- a/src/lib/core/popen.c > +++ b/src/lib/core/popen.c > @@ -812,6 +812,8 @@ signal_reset(void) > * > * Possible errors: > * > + * - IllegalParams: a parameter check fails: > + * - group signal is set, while setsid is not. > * - SystemError: dup(), fcntl(), pipe(), vfork() or close() fails > * in the parent process. > * - SystemError: (temporary restriction) one of std{in,out,err} Ouch, no, ignore it. I added the list in the another patchset, which is under development for now. So I'll add the IllegalParams error to the list together with other errors. Sorry for noise. WBR, Alexander Turenko. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Tarantool-patches] [PATCH 2/2] popen: add ability to keep child on deletion 2020-04-10 14:40 [Tarantool-patches] [PATCH 0/2] popen: provide more flags Cyrill Gorcunov 2020-04-10 14:40 ` [Tarantool-patches] [PATCH 1/2] popen: Allow to kill process group Cyrill Gorcunov @ 2020-04-10 14:40 ` Cyrill Gorcunov 2020-04-10 16:45 ` Alexander Turenko 1 sibling, 1 reply; 7+ messages in thread From: Cyrill Gorcunov @ 2020-04-10 14:40 UTC (permalink / raw) To: tml Currently popen_delete kills all children process. Moreover we use popen_delete on tarantool exit. Alexander pointed out that keep children running even if tarantool is exited is still needed. Reported-by: Alexander Turenko <alexander.turenko@tarantool.org> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com> --- src/lib/core/popen.c | 5 ++++- src/lib/core/popen.h | 6 ++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c index 200b31b21..50b40b3a4 100644 --- a/src/lib/core/popen.c +++ b/src/lib/core/popen.c @@ -489,8 +489,11 @@ popen_delete(struct popen_handle *handle) return -1; } - if (popen_send_signal(handle, SIGKILL) && errno != ESRCH) + if ((handle->flags & POPEN_FLAG_KEEP_CHILD) == 0) { + if (popen_send_signal(handle, SIGKILL) && + errno != ESRCH) return -1; + } for (i = 0; i < lengthof(handle->ios); i++) { if (handle->ios[i].fd != -1) diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h index f9dd0ff45..2e87e11ff 100644 --- a/src/lib/core/popen.h +++ b/src/lib/core/popen.h @@ -102,6 +102,12 @@ enum popen_flag_bits { */ POPEN_FLAG_GROUP_SIGNAL_BIT = 16, POPEN_FLAG_GROUP_SIGNAL = (1 << POPEN_FLAG_GROUP_SIGNAL_BIT), + + /* + * Keep child rinning on delete. + */ + POPEN_FLAG_KEEP_CHILD_BIT = 17, + POPEN_FLAG_KEEP_CHILD = (1 << POPEN_FLAG_KEEP_CHILD_BIT), }; /** -- 2.20.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] popen: add ability to keep child on deletion 2020-04-10 14:40 ` [Tarantool-patches] [PATCH 2/2] popen: add ability to keep child on deletion Cyrill Gorcunov @ 2020-04-10 16:45 ` Alexander Turenko 0 siblings, 0 replies; 7+ messages in thread From: Alexander Turenko @ 2020-04-10 16:45 UTC (permalink / raw) To: Cyrill Gorcunov; +Cc: tml On Fri, Apr 10, 2020 at 05:40:21PM +0300, Cyrill Gorcunov wrote: > Currently popen_delete kills all children process. > Moreover we use popen_delete on tarantool exit. > > Alexander pointed out that keep children running > even if tarantool is exited is still needed. > > Reported-by: Alexander Turenko <alexander.turenko@tarantool.org> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com> Acked-by: Alexander Turenko <alexander.turenko@tarantool.org> > @@ -489,8 +489,11 @@ popen_delete(struct popen_handle *handle) > return -1; > } > > - if (popen_send_signal(handle, SIGKILL) && errno != ESRCH) > + if ((handle->flags & POPEN_FLAG_KEEP_CHILD) == 0) { > + if (popen_send_signal(handle, SIGKILL) && > + errno != ESRCH) > return -1; > + } Please, rebase after the patchset 'Popen Lua API: preliminary patches', which was pushed to master. > + /* > + * Keep child rinning on delete. > + */ Typo: rinning. > + POPEN_FLAG_KEEP_CHILD_BIT = 17, > + POPEN_FLAG_KEEP_CHILD = (1 << POPEN_FLAG_KEEP_CHILD_BIT), > }; ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-04-12 1:40 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-10 14:40 [Tarantool-patches] [PATCH 0/2] popen: provide more flags Cyrill Gorcunov 2020-04-10 14:40 ` [Tarantool-patches] [PATCH 1/2] popen: Allow to kill process group Cyrill Gorcunov 2020-04-10 16:41 ` Alexander Turenko 2020-04-12 1:27 ` Alexander Turenko 2020-04-12 1:40 ` Alexander Turenko 2020-04-10 14:40 ` [Tarantool-patches] [PATCH 2/2] popen: add ability to keep child on deletion Cyrill Gorcunov 2020-04-10 16:45 ` Alexander Turenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox