* [Tarantool-patches] [PATCH 01/12] popen: allow to kill process group
2020-04-14 11:38 [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
@ 2020-04-14 11:38 ` Alexander Turenko
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 02/12] popen: add ability to keep child on deletion Alexander Turenko
` (14 subsequent siblings)
15 siblings, 0 replies; 38+ messages in thread
From: Alexander Turenko @ 2020-04-14 11:38 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
From: Cyrill Gorcunov <gorcunov@gmail.com>
As Alexander pointed out this might be useful
for running a pipe of programs inside shell
(i.e. popen.shell('foo | bar | baz', 'r')).
Part of #4031
Reported-by: Alexander Turenko <alexander.turenko@tarantool.org>
Acked-by: Alexander Turenko <alexander.turenko@tarantool.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
src/lib/core/popen.c | 28 ++++++++++++++++++++++++----
src/lib/core/popen.h | 6 ++++++
2 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index df7f797b9..a5a305013 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -138,6 +138,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;
@@ -526,6 +539,9 @@ popen_state_str(unsigned int state)
int
popen_send_signal(struct popen_handle *handle, int signo)
{
+ static const char *killops[] = { "kill", "killpg" };
+ const char *killop = handle->flags & POPEN_FLAG_GROUP_SIGNAL ?
+ killops[1] : killops[0];
int ret;
assert(handle != NULL);
@@ -535,17 +551,21 @@ popen_send_signal(struct popen_handle *handle, int signo)
*/
ret = popen_may_pidop(handle);
if (ret == 0) {
- say_debug("popen: kill %d signo %d", handle->pid, signo);
+ say_debug("popen: %s %d signo %d", killop,
+ handle->pid, signo);
assert(handle->pid != -1);
- ret = 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 && errno == ESRCH) {
diag_set(SystemError, "Attempt to send a signal %d to a "
"process that does not exist anymore", signo);
return -1;
} else if (ret < 0) {
- diag_set(SystemError, "Unable to kill %d signo %d",
- handle->pid, signo);
+ diag_set(SystemError, "Unable to %s %d signo %d",
+ killop, handle->pid, signo);
return -1;
}
return 0;
diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h
index 97c581c13..623d826b9 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.25.0
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Tarantool-patches] [PATCH 02/12] popen: add ability to keep child on deletion
2020-04-14 11:38 [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 01/12] popen: allow to kill process group Alexander Turenko
@ 2020-04-14 11:38 ` Alexander Turenko
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 03/12] popen: log a reason of close inherited fds failure Alexander Turenko
` (13 subsequent siblings)
15 siblings, 0 replies; 38+ messages in thread
From: Alexander Turenko @ 2020-04-14 11:38 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
From: Cyrill Gorcunov <gorcunov@gmail.com>
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.
Part of #4031
Reported-by: Alexander Turenko <alexander.turenko@tarantool.org>
Acked-by: Alexander Turenko <alexander.turenko@tarantool.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
src/lib/core/popen.c | 15 +++++++++------
src/lib/core/popen.h | 6 ++++++
2 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index a5a305013..30be74a5f 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -585,12 +585,15 @@ popen_delete(struct popen_handle *handle)
assert(handle != NULL);
- /*
- * Unable to kill the process -- give an error.
- * The process is not exist already -- pass over.
- */
- if (popen_send_signal(handle, SIGKILL) != 0 && errno != ESRCH)
- return -1;
+ if ((handle->flags & POPEN_FLAG_KEEP_CHILD) == 0) {
+ /*
+ * Unable to kill the process -- give an error.
+ * The process is not exist already -- pass over.
+ */
+ if (popen_send_signal(handle, SIGKILL) != 0 &&
+ 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 623d826b9..570376d33 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 running on delete.
+ */
+ POPEN_FLAG_KEEP_CHILD_BIT = 17,
+ POPEN_FLAG_KEEP_CHILD = (1 << POPEN_FLAG_KEEP_CHILD_BIT),
};
/**
--
2.25.0
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Tarantool-patches] [PATCH 03/12] popen: log a reason of close inherited fds failure
2020-04-14 11:38 [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 01/12] popen: allow to kill process group Alexander Turenko
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 02/12] popen: add ability to keep child on deletion Alexander Turenko
@ 2020-04-14 11:38 ` Alexander Turenko
2020-04-14 11:52 ` Cyrill Gorcunov
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 04/12] popen: add missed diag_set() in popen_new() Alexander Turenko
` (12 subsequent siblings)
15 siblings, 1 reply; 38+ messages in thread
From: Alexander Turenko @ 2020-04-14 11:38 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
This information may be useful for debuggging.
Part of #4031
---
src/lib/core/popen.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 30be74a5f..fddcbae8f 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -652,6 +652,8 @@ make_pipe(int pfd[2])
*
* @a skip_fds is an array of @a nr_skip_fds elements
* with descriptors which should be kept opened.
+ *
+ * Returns 0 at success, otherwise -1 and set a diag.
*/
static int
close_inherited_fds(int *skip_fds, size_t nr_skip_fds)
@@ -1030,7 +1032,8 @@ popen_new(struct popen_opts *opts)
}
if (opts->flags & POPEN_FLAG_CLOSE_FDS) {
- if (close_inherited_fds(skip_fds, nr_skip_fds)) {
+ if (close_inherited_fds(skip_fds, nr_skip_fds) != 0) {
+ diag_log();
say_syserror("child: close inherited fds");
goto exit_child;
}
--
2.25.0
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Tarantool-patches] [PATCH 04/12] popen: add missed diag_set() in popen_new()
2020-04-14 11:38 [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
` (2 preceding siblings ...)
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 03/12] popen: log a reason of close inherited fds failure Alexander Turenko
@ 2020-04-14 11:38 ` Alexander Turenko
2020-04-14 11:54 ` Cyrill Gorcunov
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 05/12] popen: remove retval from popen_stat() Alexander Turenko
` (11 subsequent siblings)
15 siblings, 1 reply; 38+ messages in thread
From: Alexander Turenko @ 2020-04-14 11:38 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
See the previous similar commits:
* popen: add missed diag_set() in popen IO functions
* popen: add missed diag_set in popen_signal/delete
Part of #4031
---
src/lib/core/popen.c | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index fddcbae8f..c74ac17c7 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -803,7 +803,18 @@ signal_reset(void)
* process will run with all files inherited from a parent.
*
* Returns pointer to a new popen handle on success,
- * otherwise NULL returned setting @a errno.
+ * otherwise NULL returned and an error is set to the
+ * diagnostics area.
+ *
+ * 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}
+ * is closed in the parent process.
+ * - OutOfMemory: unable to allocate handle.
*/
struct popen_handle *
popen_new(struct popen_opts *opts)
@@ -972,6 +983,7 @@ popen_new(struct popen_opts *opts)
*/
handle->pid = vfork();
if (handle->pid < 0) {
+ diag_set(SystemError, "vfork() fails");
goto out_err;
} else if (handle->pid == 0) {
/*
@@ -1180,6 +1192,16 @@ exit_child:
out_err:
diag_log();
saved_errno = errno;
+
+ /*
+ * Save a reason of failure, because popen_delete() may
+ * clobber the diagnostics area.
+ */
+ struct diag *diag = diag_get();
+ struct error *e = diag_last_error(diag);
+ assert(e != NULL);
+ error_ref(e);
+
popen_delete(handle);
for (i = 0; i < lengthof(pfd); i++) {
if (pfd[i][0] != -1)
@@ -1189,6 +1211,11 @@ out_err:
}
if (log_fd >= 0)
close(log_fd);
+
+ /* Restore the diagnostics area entry. */
+ diag_set_error(diag, e);
+ error_unref(e);
+
errno = saved_errno;
return NULL;
}
--
2.25.0
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Tarantool-patches] [PATCH 05/12] popen: remove retval from popen_stat()
2020-04-14 11:38 [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
` (3 preceding siblings ...)
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 04/12] popen: add missed diag_set() in popen_new() Alexander Turenko
@ 2020-04-14 11:38 ` Alexander Turenko
2020-04-14 11:54 ` Cyrill Gorcunov
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 06/12] popen: quote multiword command arguments Alexander Turenko
` (10 subsequent siblings)
15 siblings, 1 reply; 38+ messages in thread
From: Alexander Turenko @ 2020-04-14 11:38 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
The change 'popen: require popen handle to be non-NULL' makes
popen_stat() function always successful. There is no reason to return a
success / failure result.
See the previous similar patch: 'popen: remove retval from
popen_state()'.
Part of #4031
---
src/lib/core/popen.c | 3 +--
src/lib/core/popen.h | 2 +-
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index c74ac17c7..3ba1e2a48 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -239,7 +239,7 @@ popen_may_pidop(struct popen_handle *handle)
/**
* Fill popen object statistics.
*/
-int
+void
popen_stat(struct popen_handle *handle, struct popen_stat *st)
{
assert(handle != NULL);
@@ -252,7 +252,6 @@ popen_stat(struct popen_handle *handle, struct popen_stat *st)
for (size_t i = 0; i < lengthof(handle->ios); i++)
st->fds[i] = handle->ios[i].fd;
- return 0;
}
/**
diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h
index 570376d33..4cdd95175 100644
--- a/src/lib/core/popen.h
+++ b/src/lib/core/popen.h
@@ -159,7 +159,7 @@ struct popen_stat {
int fds[POPEN_FLAG_FD_STDEND_BIT];
};
-extern int
+extern void
popen_stat(struct popen_handle *handle, struct popen_stat *st);
extern const char *
--
2.25.0
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Tarantool-patches] [PATCH 06/12] popen: quote multiword command arguments
2020-04-14 11:38 [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
` (4 preceding siblings ...)
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 05/12] popen: remove retval from popen_stat() Alexander Turenko
@ 2020-04-14 11:38 ` Alexander Turenko
2020-04-14 11:58 ` Cyrill Gorcunov
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 07/12] popen: add logging of duplicated logger fd Alexander Turenko
` (9 subsequent siblings)
15 siblings, 1 reply; 38+ messages in thread
From: Alexander Turenko @ 2020-04-14 11:38 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
Of course it is still not fair shell-style quoting: at least we should
also escape quotes inside arguments. But it gives correct output for
most of typical commands and has straightforward implementation.
Part of #4031
---
src/lib/core/popen.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 3ba1e2a48..089c84830 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -154,7 +154,7 @@ handle_new(struct popen_opts *opts)
for (i = 0; i < opts->nr_argv; i++) {
if (opts->argv[i] == NULL)
continue;
- size += strlen(opts->argv[i]) + 1;
+ size += strlen(opts->argv[i]) + 3;
}
handle = malloc(sizeof(*handle) + size);
@@ -168,8 +168,13 @@ handle_new(struct popen_opts *opts)
for (i = 0; i < opts->nr_argv-1; i++) {
if (opts->argv[i] == NULL)
continue;
+ bool is_multiword = strchr(opts->argv[i], ' ') != NULL;
+ if (is_multiword)
+ *pos++ = '\'';
strcpy(pos, opts->argv[i]);
pos += strlen(opts->argv[i]);
+ if (is_multiword)
+ *pos++ = '\'';
*pos++ = ' ';
}
pos[-1] = '\0';
--
2.25.0
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Tarantool-patches] [PATCH 07/12] popen: add logging of duplicated logger fd
2020-04-14 11:38 [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
` (5 preceding siblings ...)
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 06/12] popen: quote multiword command arguments Alexander Turenko
@ 2020-04-14 11:38 ` Alexander Turenko
2020-04-14 11:58 ` Cyrill Gorcunov
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 08/12] popen: fix close-on-exec flag setting Alexander Turenko
` (8 subsequent siblings)
15 siblings, 1 reply; 38+ messages in thread
From: Alexander Turenko @ 2020-04-14 11:38 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
For debugging purposes.
Part of #4031
---
src/lib/core/popen.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 089c84830..4d57cd41e 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -896,6 +896,7 @@ popen_new(struct popen_opts *opts)
int old_log_fd = log_get_fd();
if (old_log_fd >= 0) {
log_fd = dup_not_std_streams(old_log_fd);
+ say_debug("popen: duplicate logfd: %d", log_fd);
if (log_fd < 0)
return NULL;
if (fcntl(log_fd, F_SETFL, O_CLOEXEC) != 0) {
--
2.25.0
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Tarantool-patches] [PATCH 08/12] popen: fix close-on-exec flag setting
2020-04-14 11:38 [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
` (6 preceding siblings ...)
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 07/12] popen: add logging of duplicated logger fd Alexander Turenko
@ 2020-04-14 11:38 ` Alexander Turenko
2020-04-14 12:03 ` Cyrill Gorcunov
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 09/12] popen: clarify popen_{signal, delete} contract Alexander Turenko
` (7 subsequent siblings)
15 siblings, 1 reply; 38+ messages in thread
From: Alexander Turenko @ 2020-04-14 11:38 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
fcntl(2) lists flags that can be set using F_SETFL: O_CLOEXEC is not
included there. F_SETFD should be used to set close-on-exec.
Parent's end of pipes are closed explicitly in a child process anyway.
However this change fixes closing of the copy of a logger fd. See commit
07a07b3cc7b85375d20b3fc6ca1e5060304f337b ('popen: decouple logger fd
from stderr') for more information why this file descriptor was
introduced.
Part of #4031.
---
src/lib/core/popen.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 4d57cd41e..1733e5131 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -638,8 +638,8 @@ make_pipe(int pfd[2])
diag_set(SystemError, "Can't create pipe");
return -1;
}
- if (fcntl(pfd[0], F_SETFL, O_CLOEXEC) ||
- fcntl(pfd[1], F_SETFL, O_CLOEXEC)) {
+ if (fcntl(pfd[0], F_SETFD, FD_CLOEXEC) ||
+ fcntl(pfd[1], F_SETFD, FD_CLOEXEC)) {
int saved_errno = errno;
diag_set(SystemError, "Can't unblock pipe");
close(pfd[0]), pfd[0] = -1;
@@ -899,9 +899,9 @@ popen_new(struct popen_opts *opts)
say_debug("popen: duplicate logfd: %d", log_fd);
if (log_fd < 0)
return NULL;
- if (fcntl(log_fd, F_SETFL, O_CLOEXEC) != 0) {
+ if (fcntl(log_fd, F_SETFD, FD_CLOEXEC) != 0) {
diag_set(SystemError,
- "Unable to set O_CLOEXEC on temporary logfd");
+ "Unable to set FD_CLOEXEC on temporary logfd");
close(log_fd);
return NULL;
}
--
2.25.0
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Tarantool-patches] [PATCH 08/12] popen: fix close-on-exec flag setting
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 08/12] popen: fix close-on-exec flag setting Alexander Turenko
@ 2020-04-14 12:03 ` Cyrill Gorcunov
0 siblings, 0 replies; 38+ messages in thread
From: Cyrill Gorcunov @ 2020-04-14 12:03 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
On Tue, Apr 14, 2020 at 02:38:17PM +0300, Alexander Turenko wrote:
> fcntl(2) lists flags that can be set using F_SETFL: O_CLOEXEC is not
> included there. F_SETFD should be used to set close-on-exec.
>
> Parent's end of pipes are closed explicitly in a child process anyway.
> However this change fixes closing of the copy of a logger fd. See commit
> 07a07b3cc7b85375d20b3fc6ca1e5060304f337b ('popen: decouple logger fd
> from stderr') for more information why this file descriptor was
> introduced.
>
> Part of #4031.
Good catch!
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Tarantool-patches] [PATCH 09/12] popen: clarify popen_{signal, delete} contract
2020-04-14 11:38 [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
` (7 preceding siblings ...)
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 08/12] popen: fix close-on-exec flag setting Alexander Turenko
@ 2020-04-14 11:38 ` Alexander Turenko
2020-04-14 12:29 ` Cyrill Gorcunov
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 10/12] popen: add FIXME re group signal flaw Alexander Turenko
` (6 subsequent siblings)
15 siblings, 1 reply; 38+ messages in thread
From: Alexander Turenko @ 2020-04-14 11:38 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
It is convenient to have a formal description of an API during
development and when writing a documentation. I plan to use those
contracts when I will write an API description for the future popen Lua
module.
Part of #4031
---
src/lib/core/popen.c | 30 ++++++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 1733e5131..411aad03b 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -360,7 +360,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
*
* - IllegalParams: a parameter check fails:
* - count: buffer is too big.
- * - flags: POPEN_FLAG_FD_STD{OUT,ERR} are unset both.
+ * - flags: POPEN_FLAG_FD_STD{OUT,ERR} are set or unset both.
* - handle: handle does not support the requested IO operation.
* - SocketError: an IO error occurs at read().
* - TimedOut: @a timeout quota is exceeded.
@@ -536,8 +536,18 @@ popen_state_str(unsigned int state)
/**
* Send a signal to a child process.
*
+ * When POPEN_FLAG_GROUP_SIGNAL is set the function sends
+ * a signal to a process group rather than a process.
+ *
* Return 0 at success or -1 at failure (and set a diag).
*
+ * Possible errors:
+ *
+ * - SystemError: a process does not exists anymore
+ * - SystemError: invalid signal number
+ * - SystemError: no permission to send a signal to
+ * a process or a process group
+ *
* Set errno to ESRCH when a process does not exist.
*/
int
@@ -578,9 +588,21 @@ popen_send_signal(struct popen_handle *handle, int signo)
/**
* Delete a popen handle.
*
- * The function kills a child process and
- * close all fds and remove the handle from
- * a living list and finally frees the handle.
+ * The function performs the following actions:
+ *
+ * - Kill a child process or a process group depending of
+ * POPEN_FLAG_GROUP_SIGNAL (if the process is alive and
+ * POPEN_FLAG_KEEP_CHILD flag is not set).
+ * - Close all fds occupied by the handle.
+ * - Remove the handle from a living list.
+ * - Free all occupied memory.
+ *
+ * Return 0 at success and -1 at failure (and set a diag).
+ *
+ * Possible errors:
+ *
+ * - SystemError: no permission to send a signal to
+ * a process or a process group
*/
int
popen_delete(struct popen_handle *handle)
--
2.25.0
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Tarantool-patches] [PATCH 10/12] popen: add FIXME re group signal flaw
2020-04-14 11:38 [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
` (8 preceding siblings ...)
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 09/12] popen: clarify popen_{signal, delete} contract Alexander Turenko
@ 2020-04-14 11:38 ` Alexander Turenko
2020-04-14 13:19 ` Cyrill Gorcunov
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 11/12] popen: clarify popen_read_timeout error message Alexander Turenko
` (5 subsequent siblings)
15 siblings, 1 reply; 38+ messages in thread
From: Alexander Turenko @ 2020-04-14 11:38 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
It is convenient to have such anchors for known problems.
Part of #4031
---
src/lib/core/popen.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 411aad03b..36cd7b07d 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -562,6 +562,11 @@ popen_send_signal(struct popen_handle *handle, int signo)
/*
* A child may be killed or exited already.
+ *
+ * FIXME: A process may be died at the time and
+ * the function will not send a signal to other
+ * processes in the group even when
+ * POPEN_FLAG_GROUP_SIGNAL is set.
*/
ret = popen_may_pidop(handle);
if (ret == 0) {
--
2.25.0
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Tarantool-patches] [PATCH 10/12] popen: add FIXME re group signal flaw
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 10/12] popen: add FIXME re group signal flaw Alexander Turenko
@ 2020-04-14 13:19 ` Cyrill Gorcunov
2020-04-15 4:21 ` Alexander Turenko
0 siblings, 1 reply; 38+ messages in thread
From: Cyrill Gorcunov @ 2020-04-14 13:19 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
On Tue, Apr 14, 2020 at 02:38:19PM +0300, Alexander Turenko wrote:
> It is convenient to have such anchors for known problems.
>
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
Actually I need to think about all this more. But I think
it is safe to merge now.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Tarantool-patches] [PATCH 10/12] popen: add FIXME re group signal flaw
2020-04-14 13:19 ` Cyrill Gorcunov
@ 2020-04-15 4:21 ` Alexander Turenko
2020-04-15 7:27 ` Cyrill Gorcunov
0 siblings, 1 reply; 38+ messages in thread
From: Alexander Turenko @ 2020-04-15 4:21 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
On Tue, Apr 14, 2020 at 04:19:10PM +0300, Cyrill Gorcunov wrote:
> On Tue, Apr 14, 2020 at 02:38:19PM +0300, Alexander Turenko wrote:
> > It is convenient to have such anchors for known problems.
> >
> > Part of #4031
> Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
>
> Actually I need to think about all this more. But I think
> it is safe to merge now.
I think about it more and also briefly discussed with Cyrill.
It seems this problem is a kind of fundamental and we should explain it
in the documentation comments rather than mark to fix later.
So I rewrote the commit in this way:
commit 234b184ffdd9dce234520643f12fec12bbc9fa1a
Author: Alexander Turenko <alexander.turenko@tarantool.org>
Date: Mon Apr 13 13:37:55 2020 +0300
popen: clarify group signaling details
Even when ..._SETSID and ..._GROUP_SIGNAL are set, we unable to safely
kill a process group after the child process we spawned becomes died. So
we don't do that.
The behaviour seems to be indefeasible part of Unix process group
design. The best that we can do here is describe those details in the
documentation comment.
NB: It seems that pid namespaces allow to overcome this problem, however
it is the Linux specific feature, so we unlikely will use them.
Part of #4031
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 411aad03b..5cd7926d1 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -539,6 +539,13 @@ popen_state_str(unsigned int state)
* When POPEN_FLAG_GROUP_SIGNAL is set the function sends
* a signal to a process group rather than a process.
*
+ * A signal will not be sent if the child process is already
+ * dead: otherwise we might kill another process that occupies
+ * the same PID later. This means that if the child process
+ * dies before its own childs, the function will not send a
+ * signal to the process group even when ..._SETSID and
+ * ..._GROUP_SIGNAL are set.
+ *
* Return 0 at success or -1 at failure (and set a diag).
*
* Possible errors:
@@ -597,6 +604,8 @@ popen_send_signal(struct popen_handle *handle, int signo)
* - Remove the handle from a living list.
* - Free all occupied memory.
*
+ * @see popen_send_signal() for note about ..._GROUP_SIGNAL.
+ *
* Return 0 at success and -1 at failure (and set a diag).
*
* Possible errors:
diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h
index 4cdd95175..8cb71e28d 100644
--- a/src/lib/core/popen.h
+++ b/src/lib/core/popen.h
@@ -99,6 +99,8 @@ enum popen_flag_bits {
/*
* Send signal to a process group.
+ *
+ * @see popen_send_signal() for details.
*/
POPEN_FLAG_GROUP_SIGNAL_BIT = 16,
POPEN_FLAG_GROUP_SIGNAL = (1 << POPEN_FLAG_GROUP_SIGNAL_BIT),
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Tarantool-patches] [PATCH 10/12] popen: add FIXME re group signal flaw
2020-04-15 4:21 ` Alexander Turenko
@ 2020-04-15 7:27 ` Cyrill Gorcunov
0 siblings, 0 replies; 38+ messages in thread
From: Cyrill Gorcunov @ 2020-04-15 7:27 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
On Wed, Apr 15, 2020 at 07:21:23AM +0300, Alexander Turenko wrote:
...
> @@ -597,6 +604,8 @@ popen_send_signal(struct popen_handle *handle, int signo)
> * - Remove the handle from a living list.
> * - Free all occupied memory.
> *
> + * @see popen_send_signal() for note about ..._GROUP_SIGNAL.
> + *
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Tarantool-patches] [PATCH 11/12] popen: clarify popen_read_timeout error message
2020-04-14 11:38 [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
` (9 preceding siblings ...)
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 10/12] popen: add FIXME re group signal flaw Alexander Turenko
@ 2020-04-14 11:38 ` Alexander Turenko
2020-04-14 12:32 ` Cyrill Gorcunov
2020-04-15 4:21 ` Alexander Turenko
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds Alexander Turenko
` (4 subsequent siblings)
15 siblings, 2 replies; 38+ messages in thread
From: Alexander Turenko @ 2020-04-14 11:38 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
Popen backend errors should be meaningful for a user of the popen Lua
API, because otherwise we'll need to map backend errors into Lua API
errors. This particular failure can't appear when the function is called
from the Lua API, but it is good to keep all error messages in one
style.
Part of #4031
---
src/lib/core/popen.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 36cd7b07d..640dffc2b 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -360,7 +360,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
*
* - IllegalParams: a parameter check fails:
* - count: buffer is too big.
- * - flags: POPEN_FLAG_FD_STD{OUT,ERR} are set or unset both.
+ * - flags: stdout and stdrr are both choosen or both missed
* - handle: handle does not support the requested IO operation.
* - SocketError: an IO error occurs at read().
* - TimedOut: @a timeout quota is exceeded.
@@ -379,8 +379,8 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
}
if (!(flags & (POPEN_FLAG_FD_STDOUT | POPEN_FLAG_FD_STDERR))) {
- diag_set(IllegalParams, "popen: POPEN_FLAG_FD_STD{OUT,ERR} are "
- "unset both");
+ diag_set(IllegalParams,
+ "popen: neither stdout nor stderr is choosen");
return -1;
}
--
2.25.0
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Tarantool-patches] [PATCH 11/12] popen: clarify popen_read_timeout error message
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 11/12] popen: clarify popen_read_timeout error message Alexander Turenko
@ 2020-04-14 12:32 ` Cyrill Gorcunov
2020-04-15 4:21 ` Alexander Turenko
1 sibling, 0 replies; 38+ messages in thread
From: Cyrill Gorcunov @ 2020-04-14 12:32 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
On Tue, Apr 14, 2020 at 02:38:20PM +0300, Alexander Turenko wrote:
> Popen backend errors should be meaningful for a user of the popen Lua
> API, because otherwise we'll need to map backend errors into Lua API
> errors. This particular failure can't appear when the function is called
> from the Lua API, but it is good to keep all error messages in one
> style.
>
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Tarantool-patches] [PATCH 11/12] popen: clarify popen_read_timeout error message
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 11/12] popen: clarify popen_read_timeout error message Alexander Turenko
2020-04-14 12:32 ` Cyrill Gorcunov
@ 2020-04-15 4:21 ` Alexander Turenko
2020-04-15 7:39 ` Cyrill Gorcunov
1 sibling, 1 reply; 38+ messages in thread
From: Alexander Turenko @ 2020-04-15 4:21 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
On Tue, Apr 14, 2020 at 02:38:20PM +0300, Alexander Turenko wrote:
> Popen backend errors should be meaningful for a user of the popen Lua
> API, because otherwise we'll need to map backend errors into Lua API
> errors. This particular failure can't appear when the function is called
> from the Lua API, but it is good to keep all error messages in one
> style.
>
> Part of #4031
> ---
> src/lib/core/popen.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
> index 36cd7b07d..640dffc2b 100644
> --- a/src/lib/core/popen.c
> +++ b/src/lib/core/popen.c
> @@ -360,7 +360,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
> *
> * - IllegalParams: a parameter check fails:
> * - count: buffer is too big.
> - * - flags: POPEN_FLAG_FD_STD{OUT,ERR} are set or unset both.
> + * - flags: stdout and stdrr are both choosen or both missed
> * - handle: handle does not support the requested IO operation.
> * - SocketError: an IO error occurs at read().
> * - TimedOut: @a timeout quota is exceeded.
> @@ -379,8 +379,8 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
> }
>
> if (!(flags & (POPEN_FLAG_FD_STDOUT | POPEN_FLAG_FD_STDERR))) {
> - diag_set(IllegalParams, "popen: POPEN_FLAG_FD_STD{OUT,ERR} are "
> - "unset both");
> + diag_set(IllegalParams,
> + "popen: neither stdout nor stderr is choosen");
> return -1;
> }
I was inattentive: there is an error message of the same style in
popen_read_timeout(). Those functions should be changed both together.
Incremental diff:
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 640dffc2b..64711d737 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -297,7 +297,7 @@ stdX_str(unsigned int index)
*
* - IllegalParams: a parameter check fails:
* - count: data is too big.
- * - flags: POPEN_FLAG_FD_STDIN bit is unset.
+ * - flags: stdin is not choosen.
* - handle: handle does not support the requested IO operation.
* - SocketError: an IO error occurs at write().
* - TimedOut: @a timeout quota is exceeded.
@@ -322,8 +322,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
}
if (!(flags & POPEN_FLAG_FD_STDIN)) {
- diag_set(IllegalParams,
- "popen: POPEN_FLAG_FD_STDIN bit is unset");
+ diag_set(IllegalParams, "popen: stdin is not choosen");
return -1;
}
The new commit message:
| popen: refine popen_{read,write}_timeout errors
|
| Popen backend errors should be meaningful for a user of the popen Lua
| API, because otherwise we'll need to map backend errors into Lua API
| errors. Those particular failures can't appear when the functions are
| called from the Lua API, but it is good to keep all error messages in
| one style.
|
| Part of #4031
Don't sure, however, that 'choosen' is the right word in this context.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Tarantool-patches] [PATCH 11/12] popen: clarify popen_read_timeout error message
2020-04-15 4:21 ` Alexander Turenko
@ 2020-04-15 7:39 ` Cyrill Gorcunov
2020-04-15 21:45 ` Alexander Turenko
0 siblings, 1 reply; 38+ messages in thread
From: Cyrill Gorcunov @ 2020-04-15 7:39 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
On Wed, Apr 15, 2020 at 07:21:36AM +0300, Alexander Turenko wrote:
> >
> > if (!(flags & (POPEN_FLAG_FD_STDOUT | POPEN_FLAG_FD_STDERR))) {
> > - diag_set(IllegalParams, "popen: POPEN_FLAG_FD_STD{OUT,ERR} are "
> > - "unset both");
> > + diag_set(IllegalParams,
> > + "popen: neither stdout nor stderr is choosen");
> > return -1;
> > }
>
> I was inattentive: there is an error message of the same style in
> popen_read_timeout(). Those functions should be changed both together.
I think better "popen: neither stdout nor stderr is set", but no
strong preferences here (except there is no such word as 'choosen')
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Tarantool-patches] [PATCH 11/12] popen: clarify popen_read_timeout error message
2020-04-15 7:39 ` Cyrill Gorcunov
@ 2020-04-15 21:45 ` Alexander Turenko
0 siblings, 0 replies; 38+ messages in thread
From: Alexander Turenko @ 2020-04-15 21:45 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
On Wed, Apr 15, 2020 at 10:39:30AM +0300, Cyrill Gorcunov wrote:
> On Wed, Apr 15, 2020 at 07:21:36AM +0300, Alexander Turenko wrote:
> > >
> > > if (!(flags & (POPEN_FLAG_FD_STDOUT | POPEN_FLAG_FD_STDERR))) {
> > > - diag_set(IllegalParams, "popen: POPEN_FLAG_FD_STD{OUT,ERR} are "
> > > - "unset both");
> > > + diag_set(IllegalParams,
> > > + "popen: neither stdout nor stderr is choosen");
> > > return -1;
> > > }
> >
> > I was inattentive: there is an error message of the same style in
> > popen_read_timeout(). Those functions should be changed both together.
>
> I think better "popen: neither stdout nor stderr is set", but no
> strong preferences here (except there is no such word as 'choosen')
Strictly speaking ...FD_STD{OUT,ERR} can be set, they are flags. But
stdout / stderr are not flags, it is a stream on which a user want to do
an operaton. It is requested, askec, chosen.
However options in the Lua API names exactly as 'stdin', 'stdout',
'stderr': so 'set' is meaningful for the Lua API.
Anyway, I guess 'stdout is set' is handy shortcut for formal 'asked to
operate on stdout'. So I'll just use 'set' here.
Thanks for the advice!
----
The incremental diff:
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index d33efbb18..ee16da28b 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -297,7 +297,7 @@ stdX_str(unsigned int index)
*
* - IllegalParams: a parameter check fails:
* - count: data is too big.
- * - flags: stdin is not choosen.
+ * - flags: stdin is not set.
* - handle: handle does not support the requested IO operation.
* - SocketError: an IO error occurs at write().
* - TimedOut: @a timeout quota is exceeded.
@@ -322,7 +322,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
}
if (!(flags & POPEN_FLAG_FD_STDIN)) {
- diag_set(IllegalParams, "popen: stdin is not choosen");
+ diag_set(IllegalParams, "popen: stdin is not set");
return -1;
}
@@ -359,7 +359,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
*
* - IllegalParams: a parameter check fails:
* - count: buffer is too big.
- * - flags: stdout and stdrr are both choosen or both missed
+ * - flags: stdout and stdrr are both set or both missed.
* - handle: handle does not support the requested IO operation.
* - SocketError: an IO error occurs at read().
* - TimedOut: @a timeout quota is exceeded.
@@ -379,7 +379,7 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
if (!(flags & (POPEN_FLAG_FD_STDOUT | POPEN_FLAG_FD_STDERR))) {
diag_set(IllegalParams,
- "popen: neither stdout nor stderr is choosen");
+ "popen: neither stdout nor stderr is set");
return -1;
}
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds
2020-04-14 11:38 [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
` (10 preceding siblings ...)
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 11/12] popen: clarify popen_read_timeout error message Alexander Turenko
@ 2020-04-14 11:38 ` Alexander Turenko
2020-04-14 13:05 ` Cyrill Gorcunov
2020-04-15 4:25 ` [Tarantool-patches] [PATCH 13/13] popen: add caution comment for popen_may_io() Alexander Turenko
` (3 subsequent siblings)
15 siblings, 1 reply; 38+ messages in thread
From: Alexander Turenko @ 2020-04-14 11:38 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
The function popen_shutdown() checks whether std{in,out,err} was piped
and closes the parent's end. A user should have ability to send EOF for
child's stdin for stream programs like `grep`. It is better when there
is a function that encapsulates proper checks, error messages and the
actual actions.
This commit in particular reverts
1ef95b99f6553b246729e7bb5bdc19038043db74 ('popen: remove redundant fd
check before perform IO'), because now the check is meaningful: an fd
may become closed before the whole popen handle will be deleted.
Part of #4031
---
src/lib/core/popen.c | 170 ++++++++++++++++++++++++++++++++-----------
src/lib/core/popen.h | 3 +
2 files changed, 130 insertions(+), 43 deletions(-)
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 640dffc2b..8760429c2 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -34,6 +34,43 @@ static RLIST_HEAD(popen_head);
static int dev_null_fd_ro = -1;
static int dev_null_fd_wr = -1;
+static const struct {
+ unsigned int mask;
+ unsigned int mask_devnull;
+ unsigned int mask_close;
+ int fileno;
+ int *dev_null_fd;
+ int parent_idx;
+ int child_idx;
+ bool nonblock;
+} pfd_map[POPEN_FLAG_FD_STDEND_BIT] = {
+ {
+ .mask = POPEN_FLAG_FD_STDIN,
+ .mask_devnull = POPEN_FLAG_FD_STDIN_DEVNULL,
+ .mask_close = POPEN_FLAG_FD_STDIN_CLOSE,
+ .fileno = STDIN_FILENO,
+ .dev_null_fd = &dev_null_fd_ro,
+ .parent_idx = 1,
+ .child_idx = 0,
+ }, {
+ .mask = POPEN_FLAG_FD_STDOUT,
+ .mask_devnull = POPEN_FLAG_FD_STDOUT_DEVNULL,
+ .mask_close = POPEN_FLAG_FD_STDOUT_CLOSE,
+ .fileno = STDOUT_FILENO,
+ .dev_null_fd = &dev_null_fd_wr,
+ .parent_idx = 0,
+ .child_idx = 1,
+ }, {
+ .mask = POPEN_FLAG_FD_STDERR,
+ .mask_devnull = POPEN_FLAG_FD_STDERR_DEVNULL,
+ .mask_close = POPEN_FLAG_FD_STDERR_CLOSE,
+ .fileno = STDERR_FILENO,
+ .dev_null_fd = &dev_null_fd_wr,
+ .parent_idx = 0,
+ .child_idx = 1,
+ },
+};
+
/**
* Register popen handle in a pids map.
*/
@@ -213,7 +250,8 @@ handle_free(struct popen_handle *handle)
* Returns 0 if so and -1 otherwise (and set a diag).
*/
static inline int
-popen_may_io(struct popen_handle *handle, unsigned int io_flags)
+popen_may_io(struct popen_handle *handle, unsigned int idx,
+ unsigned int io_flags, bool allow_closed)
{
if (!(io_flags & handle->flags)) {
diag_set(IllegalParams, "popen: handle does not support the "
@@ -221,6 +259,12 @@ popen_may_io(struct popen_handle *handle, unsigned int io_flags)
return -1;
}
+ if (! allow_closed && handle->ios[idx].fd < 0) {
+ diag_set(IllegalParams, "popen: attempt to operate on a closed "
+ "file descriptor");
+ return -1;
+ }
+
return 0;
}
@@ -299,6 +343,7 @@ stdX_str(unsigned int index)
* - count: data is too big.
* - flags: POPEN_FLAG_FD_STDIN bit is unset.
* - handle: handle does not support the requested IO operation.
+ * - handle: attempt to operate on a closed fd.
* - SocketError: an IO error occurs at write().
* - TimedOut: @a timeout quota is exceeded.
* - FiberIsCancelled: cancelled by an outside code.
@@ -327,11 +372,11 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
return -1;
}
- if (popen_may_io(handle, flags) != 0)
- return -1;
-
int idx = STDIN_FILENO;
+ if (popen_may_io(handle, idx, flags, false) != 0)
+ return -1;
+
say_debug("popen: %d: write idx [%s:%d] buf %p count %zu "
"fds %d timeout %.9g",
handle->pid, stdX_str(idx), idx, buf, count,
@@ -362,6 +407,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
* - count: buffer is too big.
* - flags: stdout and stdrr are both choosen or both missed
* - handle: handle does not support the requested IO operation.
+ * - handle: attempt to operate on a closed fd.
* - SocketError: an IO error occurs at read().
* - TimedOut: @a timeout quota is exceeded.
* - FiberIsCancelled: cancelled by an outside code.
@@ -390,12 +436,12 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
return -1;
}
- if (popen_may_io(handle, flags) != 0)
- return -1;
-
int idx = flags & POPEN_FLAG_FD_STDOUT ?
STDOUT_FILENO : STDERR_FILENO;
+ if (popen_may_io(handle, idx, flags, false) != 0)
+ return -1;
+
say_debug("popen: %d: read idx [%s:%d] buf %p count %zu "
"fds %d timeout %.9g",
handle->pid, stdX_str(idx), idx, buf, count,
@@ -405,6 +451,80 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
timeout);
}
+/**
+ * Close parent's ends of std* fds.
+ *
+ * The following @a flags controls which fds should be closed:
+ *
+ * POPEN_FLAG_FD_STDIN close parent's end of child's stdin
+ * POPEN_FLAG_FD_STDOUT close parent's end of child's stdout
+ * POPEN_FLAG_FD_STDERR close parent's end of child's stderr
+ *
+ * The main reason to use this function is to send EOF to
+ * child's stdin. However parent's end of stdout / stderr
+ * may be closed too.
+ *
+ * The function does not fail on already closed fds (idempotence).
+ * However it fails on attempt to close the end of a pipe that was
+ * never exist. In other words, a subset of ..._FD_STD{IN,OUT,ERR}
+ * flags used at a handle creation may be used here.
+ *
+ * The function does not close any fds on a failure: either all
+ * requested fds are closed or neither of them.
+ *
+ * Returns 0 at success, otherwise -1 and set a diag.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: a parameter check fails:
+ * - flags: neither stdid, stdout nor stderr is choosen.
+ * - handle: handle does not support the requested IO operation
+ * (one of fds is not piped).
+ */
+int
+popen_shutdown(struct popen_handle *handle, unsigned int flags)
+{
+ assert(handle != NULL);
+
+ if ((flags & (POPEN_FLAG_FD_STDIN |
+ POPEN_FLAG_FD_STDOUT |
+ POPEN_FLAG_FD_STDERR)) == 0) {
+ diag_set(IllegalParams,
+ "popen: neither stdin, stdout nor stderr is choosen");
+ return -1;
+ }
+
+ /* Verify the operation. */
+ for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
+ /* Operate only on asked fds. */
+ unsigned int op_mask = pfd_map[idx].mask;
+ if ((flags & op_mask) == 0)
+ continue;
+
+ if (popen_may_io(handle, idx, op_mask, true) != 0)
+ return -1;
+ }
+
+ /* Perform the operation. */
+ for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
+ /* Operate only on asked fds. */
+ unsigned int op_mask = pfd_map[idx].mask;
+ if ((flags & op_mask) == 0)
+ continue;
+
+ /* Skip already closed fds. */
+ if (handle->ios[idx].fd < 0)
+ continue;
+
+ say_debug("popen: %d: shutdown idx [%s:%d] fd %s",
+ handle->pid, stdX_str(idx), idx,
+ handle->ios[idx].fd);
+ coio_close_io(loop(), &handle->ios[idx]);
+ }
+
+ return 0;
+}
+
/**
* Encode signal status into a human readable form.
*
@@ -865,42 +985,6 @@ popen_new(struct popen_opts *opts)
int saved_errno;
size_t i;
- static const struct {
- unsigned int mask;
- unsigned int mask_devnull;
- unsigned int mask_close;
- int fileno;
- int *dev_null_fd;
- int parent_idx;
- int child_idx;
- bool nonblock;
- } pfd_map[POPEN_FLAG_FD_STDEND_BIT] = {
- {
- .mask = POPEN_FLAG_FD_STDIN,
- .mask_devnull = POPEN_FLAG_FD_STDIN_DEVNULL,
- .mask_close = POPEN_FLAG_FD_STDIN_CLOSE,
- .fileno = STDIN_FILENO,
- .dev_null_fd = &dev_null_fd_ro,
- .parent_idx = 1,
- .child_idx = 0,
- }, {
- .mask = POPEN_FLAG_FD_STDOUT,
- .mask_devnull = POPEN_FLAG_FD_STDOUT_DEVNULL,
- .mask_close = POPEN_FLAG_FD_STDOUT_CLOSE,
- .fileno = STDOUT_FILENO,
- .dev_null_fd = &dev_null_fd_wr,
- .parent_idx = 0,
- .child_idx = 1,
- }, {
- .mask = POPEN_FLAG_FD_STDERR,
- .mask_devnull = POPEN_FLAG_FD_STDERR_DEVNULL,
- .mask_close = POPEN_FLAG_FD_STDERR_CLOSE,
- .fileno = STDERR_FILENO,
- .dev_null_fd = &dev_null_fd_wr,
- .parent_idx = 0,
- .child_idx = 1,
- },
- };
/*
* At max we could be skipping each pipe end
* plus dev/null variants and logfd
diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h
index 4cdd95175..c068d5028 100644
--- a/src/lib/core/popen.h
+++ b/src/lib/core/popen.h
@@ -175,6 +175,9 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
size_t count, unsigned int flags,
ev_tstamp timeout);
+extern int
+popen_shutdown(struct popen_handle *handle, unsigned int flags);
+
extern void
popen_state(struct popen_handle *handle, int *state, int *exit_code);
--
2.25.0
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds Alexander Turenko
@ 2020-04-14 13:05 ` Cyrill Gorcunov
2020-04-15 4:21 ` Alexander Turenko
0 siblings, 1 reply; 38+ messages in thread
From: Cyrill Gorcunov @ 2020-04-14 13:05 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
On Tue, Apr 14, 2020 at 02:38:21PM +0300, Alexander Turenko wrote:
> The function popen_shutdown() checks whether std{in,out,err} was piped
> and closes the parent's end. A user should have ability to send EOF for
> child's stdin for stream programs like `grep`. It is better when there
> is a function that encapsulates proper checks, error messages and the
> actual actions.
>
> This commit in particular reverts
> 1ef95b99f6553b246729e7bb5bdc19038043db74 ('popen: remove redundant fd
> check before perform IO'), because now the check is meaningful: an fd
> may become closed before the whole popen handle will be deleted.
>
> Part of #4031
> ---
> src/lib/core/popen.c | 170 ++++++++++++++++++++++++++++++++-----------
> src/lib/core/popen.h | 3 +
> 2 files changed, 130 insertions(+), 43 deletions(-)
>
> diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
> index 640dffc2b..8760429c2 100644
> --- a/src/lib/core/popen.c
> +++ b/src/lib/core/popen.c
> @@ -34,6 +34,43 @@ static RLIST_HEAD(popen_head);
> static int dev_null_fd_ro = -1;
> static int dev_null_fd_wr = -1;
>
> +static const struct {
> + unsigned int mask;
> + unsigned int mask_devnull;
> + unsigned int mask_close;
> + int fileno;
> + int *dev_null_fd;
> + int parent_idx;
> + int child_idx;
> + bool nonblock;
> +} pfd_map[POPEN_FLAG_FD_STDEND_BIT] = {
> + {
> + .mask = POPEN_FLAG_FD_STDIN,
> + .mask_devnull = POPEN_FLAG_FD_STDIN_DEVNULL,
> + .mask_close = POPEN_FLAG_FD_STDIN_CLOSE,
> + .fileno = STDIN_FILENO,
> + .dev_null_fd = &dev_null_fd_ro,
> + .parent_idx = 1,
> + .child_idx = 0,
> + }, {
> + .mask = POPEN_FLAG_FD_STDOUT,
> + .mask_devnull = POPEN_FLAG_FD_STDOUT_DEVNULL,
> + .mask_close = POPEN_FLAG_FD_STDOUT_CLOSE,
> + .fileno = STDOUT_FILENO,
> + .dev_null_fd = &dev_null_fd_wr,
> + .parent_idx = 0,
> + .child_idx = 1,
> + }, {
> + .mask = POPEN_FLAG_FD_STDERR,
> + .mask_devnull = POPEN_FLAG_FD_STDERR_DEVNULL,
> + .mask_close = POPEN_FLAG_FD_STDERR_CLOSE,
> + .fileno = STDERR_FILENO,
> + .dev_null_fd = &dev_null_fd_wr,
> + .parent_idx = 0,
> + .child_idx = 1,
> + },
> +};
> +
> /**
> * Register popen handle in a pids map.
> */
> @@ -213,7 +250,8 @@ handle_free(struct popen_handle *handle)
> * Returns 0 if so and -1 otherwise (and set a diag).
> */
> static inline int
> -popen_may_io(struct popen_handle *handle, unsigned int io_flags)
> +popen_may_io(struct popen_handle *handle, unsigned int idx,
> + unsigned int io_flags, bool allow_closed)
> {
> if (!(io_flags & handle->flags)) {
> diag_set(IllegalParams, "popen: handle does not support the "
> @@ -221,6 +259,12 @@ popen_may_io(struct popen_handle *handle, unsigned int io_flags)
> return -1;
> }
>
> + if (! allow_closed && handle->ios[idx].fd < 0) {
> + diag_set(IllegalParams, "popen: attempt to operate on a closed "
> + "file descriptor");
> + return -1;
> + }
> +
> return 0;
> }
>
> @@ -299,6 +343,7 @@ stdX_str(unsigned int index)
> * - count: data is too big.
> * - flags: POPEN_FLAG_FD_STDIN bit is unset.
> * - handle: handle does not support the requested IO operation.
> + * - handle: attempt to operate on a closed fd.
> * - SocketError: an IO error occurs at write().
> * - TimedOut: @a timeout quota is exceeded.
> * - FiberIsCancelled: cancelled by an outside code.
> @@ -327,11 +372,11 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
> return -1;
> }
>
> - if (popen_may_io(handle, flags) != 0)
> - return -1;
> -
> int idx = STDIN_FILENO;
>
> + if (popen_may_io(handle, idx, flags, false) != 0)
> + return -1;
> +
> say_debug("popen: %d: write idx [%s:%d] buf %p count %zu "
> "fds %d timeout %.9g",
> handle->pid, stdX_str(idx), idx, buf, count,
> @@ -362,6 +407,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
> * - count: buffer is too big.
> * - flags: stdout and stdrr are both choosen or both missed
> * - handle: handle does not support the requested IO operation.
> + * - handle: attempt to operate on a closed fd.
> * - SocketError: an IO error occurs at read().
> * - TimedOut: @a timeout quota is exceeded.
> * - FiberIsCancelled: cancelled by an outside code.
> @@ -390,12 +436,12 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
> return -1;
> }
>
> - if (popen_may_io(handle, flags) != 0)
> - return -1;
> -
> int idx = flags & POPEN_FLAG_FD_STDOUT ?
> STDOUT_FILENO : STDERR_FILENO;
>
> + if (popen_may_io(handle, idx, flags, false) != 0)
> + return -1;
> +
> say_debug("popen: %d: read idx [%s:%d] buf %p count %zu "
> "fds %d timeout %.9g",
> handle->pid, stdX_str(idx), idx, buf, count,
> @@ -405,6 +451,80 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
> timeout);
> }
...
> +int
> +popen_shutdown(struct popen_handle *handle, unsigned int flags)
> +{
> + assert(handle != NULL);
> +
> + if ((flags & (POPEN_FLAG_FD_STDIN |
> + POPEN_FLAG_FD_STDOUT |
> + POPEN_FLAG_FD_STDERR)) == 0) {
> + diag_set(IllegalParams,
> + "popen: neither stdin, stdout nor stderr is choosen");
> + return -1;
> + }
> +
> + /* Verify the operation. */
> + for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
for (size_t i = 0; i < lengthof(pfd_map); i++)
We already do a build time check for STDIN_x proper mapping to numbers,
lets make it shorter.
> + /* Operate only on asked fds. */
> + unsigned int op_mask = pfd_map[idx].mask;
> + if ((flags & op_mask) == 0)
> + continue;
> +
> + if (popen_may_io(handle, idx, op_mask, true) != 0)
> + return -1;
> + }
> +
> + /* Perform the operation. */
> + for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
> + /* Operate only on asked fds. */
> + unsigned int op_mask = pfd_map[idx].mask;
> + if ((flags & op_mask) == 0)
> + continue;
> +
> + /* Skip already closed fds. */
> + if (handle->ios[idx].fd < 0)
> + continue;
> +
> + say_debug("popen: %d: shutdown idx [%s:%d] fd %s",
> + handle->pid, stdX_str(idx), idx,
> + handle->ios[idx].fd);
> + coio_close_io(loop(), &handle->ios[idx]);
> + }
I don't get why we need two for() cycles? Also, I don't like that we
mangle popen_may_io(). The shutdown is special. Why not do something like
for (size_t idx = 0; i < lengthof(pfd_map); i++) {
unsigned int op_mask = pfd_map[idx].mask;
if ((flags & op_mask) == 0)
continue;
if (handle->ios[idx].fd < 0)
continue;
...
}
Can't we do something like that?
Cyrill
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds
2020-04-14 13:05 ` Cyrill Gorcunov
@ 2020-04-15 4:21 ` Alexander Turenko
2020-04-15 7:43 ` Cyrill Gorcunov
0 siblings, 1 reply; 38+ messages in thread
From: Alexander Turenko @ 2020-04-15 4:21 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
> > +int
> > +popen_shutdown(struct popen_handle *handle, unsigned int flags)
> > +{
> > + assert(handle != NULL);
> > +
> > + if ((flags & (POPEN_FLAG_FD_STDIN |
> > + POPEN_FLAG_FD_STDOUT |
> > + POPEN_FLAG_FD_STDERR)) == 0) {
> > + diag_set(IllegalParams,
> > + "popen: neither stdin, stdout nor stderr is choosen");
> > + return -1;
> > + }
> > +
> > + /* Verify the operation. */
> > + for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
>
> for (size_t i = 0; i < lengthof(pfd_map); i++)
>
> We already do a build time check for STDIN_x proper mapping to numbers,
> lets make it shorter.
Sure, changed.
The diff at bottom of the email.
>
>
> > + /* Operate only on asked fds. */
> > + unsigned int op_mask = pfd_map[idx].mask;
> > + if ((flags & op_mask) == 0)
> > + continue;
> > +
> > + if (popen_may_io(handle, idx, op_mask, true) != 0)
> > + return -1;
> > + }
> > +
> > + /* Perform the operation. */
> > + for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
> > + /* Operate only on asked fds. */
> > + unsigned int op_mask = pfd_map[idx].mask;
> > + if ((flags & op_mask) == 0)
> > + continue;
> > +
> > + /* Skip already closed fds. */
> > + if (handle->ios[idx].fd < 0)
> > + continue;
> > +
> > + say_debug("popen: %d: shutdown idx [%s:%d] fd %s",
> > + handle->pid, stdX_str(idx), idx,
> > + handle->ios[idx].fd);
> > + coio_close_io(loop(), &handle->ios[idx]);
> > + }
>
> I don't get why we need two for() cycles?
The first loop is to check that we able to close all fds: all checks are
made first, then all close()'s are called. But the first loop actually
may be replaced with the following check:
| flags &= POPEN_FLAG_FD_STDIN |
| POPEN_FLAG_FD_STDOUT |
| POPEN_FLAG_FD_STDERR;
| if ((handle->flags & flags) != flags)
| /* Give an error here. */
It looks more clean, so I'll use this way. Thanks for catching this!
> Also, I don't like that we
> mangle popen_may_io(). The shutdown is special. Why not do something like
We should check for closed fd in popen_may_io(), because read / write
should give proper error in the case. I guess that here we agreed.
But I made this check conditional in popen_may_io() and I guess it is
what you complain about.
I agree that it looks a bit artificial and so I extracted setting of the
'not supported IO operation' error into a helper function and use it in
popen_shutdown() directly. This really looks better.
See the diff below.
>
> for (size_t idx = 0; i < lengthof(pfd_map); i++) {
> unsigned int op_mask = pfd_map[idx].mask;
> if ((flags & op_mask) == 0)
> continue;
>
> if (handle->ios[idx].fd < 0)
> continue;
>
> ...
> }
>
> Can't we do something like that?
Not exactly, but a kind of this. See below.
WBR, Alexander Turenko.
----
Incremental diff:
--- a/src/lib/core/popen.c 2020-04-15 01:44:51.387489580 +0300
+++ b/src/lib/core/popen.c 2020-04-15 01:44:35.118490605 +0300
@@ -245,21 +245,30 @@
}
/**
+ * Generate an error about IO operation that is not supported by
+ * a popen handle.
+ */
+static inline int
+popen_set_unsupported_io_error(void)
+{
+ diag_set(IllegalParams, "popen: handle does not support the "
+ "requested IO operation");
+ return -1;
+}
+
+/**
* Test if the handle can run a requested IO operation.
*
* Returns 0 if so and -1 otherwise (and set a diag).
*/
static inline int
popen_may_io(struct popen_handle *handle, unsigned int idx,
- unsigned int io_flags, bool allow_closed)
+ unsigned int io_flags)
{
- if (!(io_flags & handle->flags)) {
- diag_set(IllegalParams, "popen: handle does not support the "
- "requested IO operation");
- return -1;
- }
+ if (!(io_flags & handle->flags))
+ return popen_set_unsupported_io_error();
- if (! allow_closed && handle->ios[idx].fd < 0) {
+ if (handle->ios[idx].fd < 0) {
diag_set(IllegalParams, "popen: attempt to operate "
"on a closed file descriptor");
return -1;
@@ -374,7 +383,7 @@
int idx = STDIN_FILENO;
- if (popen_may_io(handle, idx, flags, false) != 0)
+ if (popen_may_io(handle, idx, flags) != 0)
return -1;
say_debug("popen: %d: write idx [%s:%d] buf %p count %zu "
@@ -439,7 +448,7 @@
int idx = flags & POPEN_FLAG_FD_STDOUT ?
STDOUT_FILENO : STDERR_FILENO;
- if (popen_may_io(handle, idx, flags, false) != 0)
+ if (popen_may_io(handle, idx, flags) != 0)
return -1;
say_debug("popen: %d: read idx [%s:%d] buf %p count %zu "
@@ -486,27 +495,29 @@
{
assert(handle != NULL);
- if ((flags & (POPEN_FLAG_FD_STDIN |
- POPEN_FLAG_FD_STDOUT |
- POPEN_FLAG_FD_STDERR)) == 0) {
+ /* Ignore irrelevant flags. */
+ flags &= POPEN_FLAG_FD_STDIN |
+ POPEN_FLAG_FD_STDOUT |
+ POPEN_FLAG_FD_STDERR;
+
+ /* At least one ..._FD_STDx flag should be set. */
+ if (flags == 0) {
diag_set(IllegalParams,
"popen: neither stdin, stdout nor stderr is choosen");
return -1;
}
- /* Verify the operation. */
- for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
- /* Operate only on asked fds. */
- unsigned int op_mask = pfd_map[idx].mask;
- if ((flags & op_mask) == 0)
- continue;
-
- if (popen_may_io(handle, idx, op_mask, true) != 0)
- return -1;
- }
+ /*
+ * The handle should have all std*, which are asked to
+ * close, be piped.
+ *
+ * Otherwise the operation has no sense: we should close
+ * parent's end of a pipe, but it was not created at all.
+ */
+ if ((handle->flags & flags) != flags)
+ return popen_set_unsupported_io_error();
- /* Perform the operation. */
- for (int idx = STDIN_FILENO; idx < POPEN_FLAG_FD_STDEND_BIT; ++idx) {
+ for (size_t idx = 0; idx < lengthof(pfd_map); ++idx) {
/* Operate only on asked fds. */
unsigned int op_mask = pfd_map[idx].mask;
if ((flags & op_mask) == 0)
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds
2020-04-15 4:21 ` Alexander Turenko
@ 2020-04-15 7:43 ` Cyrill Gorcunov
2020-04-15 21:45 ` Alexander Turenko
0 siblings, 1 reply; 38+ messages in thread
From: Cyrill Gorcunov @ 2020-04-15 7:43 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
On Wed, Apr 15, 2020 at 07:21:12AM +0300, Alexander Turenko wrote:
>
> The diff at bottom of the email.
Ack
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds
2020-04-15 7:43 ` Cyrill Gorcunov
@ 2020-04-15 21:45 ` Alexander Turenko
0 siblings, 0 replies; 38+ messages in thread
From: Alexander Turenko @ 2020-04-15 21:45 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
On Wed, Apr 15, 2020 at 10:43:27AM +0300, Cyrill Gorcunov wrote:
> On Wed, Apr 15, 2020 at 07:21:12AM +0300, Alexander Turenko wrote:
> >
> > The diff at bottom of the email.
>
> Ack
Changed after last wording fixes in 'refine popen_{read,write}_timeout
errors' (incremental diff):
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 638826fb9..3a12a439b 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -492,7 +492,7 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
* Possible errors:
*
* - IllegalParams: a parameter check fails:
- * - flags: neither stdin, stdout nor stderr is choosen.
+ * - flags: neither stdin, stdout nor stderr is set.
* - handle: handle does not support the requested IO operation
* (one of fds is not piped).
*/
@@ -509,7 +509,7 @@ popen_shutdown(struct popen_handle *handle, unsigned int flags)
/* At least one ..._FD_STDx flag should be set. */
if (flags == 0) {
diag_set(IllegalParams,
- "popen: neither stdin, stdout nor stderr is choosen");
+ "popen: neither stdin, stdout nor stderr is set");
return -1;
}
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Tarantool-patches] [PATCH 13/13] popen: add caution comment for popen_may_io()
2020-04-14 11:38 [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
` (11 preceding siblings ...)
2020-04-14 11:38 ` [Tarantool-patches] [PATCH 12/12] popen: allow to close parent's end of std* fds Alexander Turenko
@ 2020-04-15 4:25 ` Alexander Turenko
2020-04-15 7:44 ` Cyrill Gorcunov
2020-04-15 4:52 ` [Tarantool-patches] [PATCH 14/14] popen: fix popen_write_timeout retval type Alexander Turenko
` (2 subsequent siblings)
15 siblings, 1 reply; 38+ messages in thread
From: Alexander Turenko @ 2020-04-15 4:25 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
It was easy to misinterpret popen_may_io() contract. In fact, I made
this mistake recently and want to clarify how the function should be
called.
Part of #4031
---
src/lib/core/popen.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 1b564d0c2..cbf2fd690 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -259,12 +259,19 @@ popen_set_unsupported_io_error(void)
/**
* Test if the handle can run a requested IO operation.
*
+ * NB: Expects @a io_flags to be a ..._FD_STDx flag rather
+ * then a mask with several flags: otherwise it'll check
+ * that one (any) of @a io_flags is set.
+ *
* Returns 0 if so and -1 otherwise (and set a diag).
*/
static inline int
popen_may_io(struct popen_handle *handle, unsigned int idx,
unsigned int io_flags)
{
+ assert(io_flags == POPEN_FLAG_FD_STDIN ||
+ io_flags == POPEN_FLAG_FD_STDOUT ||
+ io_flags == POPEN_FLAG_FD_STDERR);
if (!(io_flags & handle->flags))
return popen_set_unsupported_io_error();
--
2.25.0
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Tarantool-patches] [PATCH 14/14] popen: fix popen_write_timeout retval type
2020-04-14 11:38 [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
` (12 preceding siblings ...)
2020-04-15 4:25 ` [Tarantool-patches] [PATCH 13/13] popen: add caution comment for popen_may_io() Alexander Turenko
@ 2020-04-15 4:52 ` Alexander Turenko
2020-04-15 23:57 ` [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
2020-04-17 6:58 ` Kirill Yukhin
15 siblings, 0 replies; 38+ messages in thread
From: Alexander Turenko @ 2020-04-15 4:52 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
On Linux x86_64 `ssize_t` is 64 bit, while `int` is 32 bit wide (at
least typically). Let's return `ssize_t` from popen_write_timeout() to
prevent data loss.
Part of #4031
Reported-by: Cyrill Gorcunov <gorcunov@gmail.com>
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
src/lib/core/popen.c | 6 +++---
src/lib/core/popen.h | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index cbf2fd690..bb55a80e7 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -370,7 +370,7 @@ stdX_str(unsigned int index)
* FIXME: Provide an info re amount written bytes in the case.
* Say, return -(written) in the case.
*/
-int
+ssize_t
popen_write_timeout(struct popen_handle *handle, const void *buf,
size_t count, unsigned int flags,
ev_tstamp timeout)
@@ -397,8 +397,8 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
handle->pid, stdX_str(idx), idx, buf, count,
handle->ios[idx].fd, timeout);
- int rc = coio_write_timeout_noxc(&handle->ios[idx], buf, count,
- timeout);
+ ssize_t rc = coio_write_timeout_noxc(&handle->ios[idx], buf,
+ count, timeout);
assert(rc < 0 || rc == (ssize_t)count);
return rc;
}
diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h
index cffb53933..4f4a6d99d 100644
--- a/src/lib/core/popen.h
+++ b/src/lib/core/popen.h
@@ -167,7 +167,7 @@ popen_stat(struct popen_handle *handle, struct popen_stat *st);
extern const char *
popen_command(struct popen_handle *handle);
-extern int
+extern ssize_t
popen_write_timeout(struct popen_handle *handle, const void *buf,
size_t count, unsigned int flags,
ev_tstamp timeout);
--
2.25.0
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2
2020-04-14 11:38 [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
` (13 preceding siblings ...)
2020-04-15 4:52 ` [Tarantool-patches] [PATCH 14/14] popen: fix popen_write_timeout retval type Alexander Turenko
@ 2020-04-15 23:57 ` Alexander Turenko
2020-04-16 0:00 ` Alexander Turenko
2020-04-16 11:52 ` Cyrill Gorcunov
2020-04-17 6:58 ` Kirill Yukhin
15 siblings, 2 replies; 38+ messages in thread
From: Alexander Turenko @ 2020-04-15 23:57 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tarantool-patches
On Tue, Apr 14, 2020 at 02:38:09PM +0300, Alexander Turenko wrote:
> It is the second patchset of preparation patches that are needed to
> implement Lua API for popen.
>
> See the previous series here (already pushed to master):
> https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015609.html
>
> https://github.com/tarantool/tarantool/issues/4031
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4031-popen-lua-api-preparation-2
>
> I also pushed the same commit to *-full-ci branch to verify it on all
> platforms:
>
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4031-popen-lua-api-preparation-2-full-ci
CCed Kirill.
Added two patches upward (now the patchset contains 14 patches). Got
ACKs for all patches from Cyrill. Updated the both branches (see above).
GitLab CI looks okay (Travis CI jobs are pending).
Kirill, the patchset seems to be ready to final review and push.
> Alexander Turenko (10):
> popen: log a reason of close inherited fds failure
> popen: add missed diag_set() in popen_new()
> popen: remove retval from popen_stat()
> popen: quote multiword command arguments
> popen: add logging of duplicated logger fd
> popen: fix close-on-exec flag setting
> popen: clarify popen_{signal,delete} contract
> popen: add FIXME re group signal flaw
> popen: clarify popen_read_timeout error message
> popen: allow to close parent's end of std* fds
Added:
Alexander Turenko (2):
popen: fix popen_write_timeout retval type
popen: add caution comment for popen_may_io()
>
> Cyrill Gorcunov (2):
> popen: allow to kill process group
> popen: add ability to keep child on deletion
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2
2020-04-15 23:57 ` [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
@ 2020-04-16 0:00 ` Alexander Turenko
2020-04-16 11:52 ` Cyrill Gorcunov
1 sibling, 0 replies; 38+ messages in thread
From: Alexander Turenko @ 2020-04-16 0:00 UTC (permalink / raw)
To: Kirill Yukhin; +Cc: tarantool-patches, Cyrill Gorcunov
Misclicked. Resend it to add Kirill into the recipients list.
----
On Tue, Apr 14, 2020 at 02:38:09PM +0300, Alexander Turenko wrote:
> It is the second patchset of preparation patches that are needed to
> implement Lua API for popen.
>
> See the previous series here (already pushed to master):
> https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015609.html
>
> https://github.com/tarantool/tarantool/issues/4031
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4031-popen-lua-api-preparation-2
>
> I also pushed the same commit to *-full-ci branch to verify it on all
> platforms:
>
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4031-popen-lua-api-preparation-2-full-ci
CCed Kirill.
Added two patches upward (now the patchset contains 14 patches). Got
ACKs for all patches from Cyrill. Updated the both branches (see above).
GitLab CI looks okay (Travis CI jobs are pending).
Kirill, the patchset seems to be ready to final review and push.
> Alexander Turenko (10):
> popen: log a reason of close inherited fds failure
> popen: add missed diag_set() in popen_new()
> popen: remove retval from popen_stat()
> popen: quote multiword command arguments
> popen: add logging of duplicated logger fd
> popen: fix close-on-exec flag setting
> popen: clarify popen_{signal,delete} contract
> popen: add FIXME re group signal flaw
> popen: clarify popen_read_timeout error message
> popen: allow to close parent's end of std* fds
Added:
Alexander Turenko (2):
popen: fix popen_write_timeout retval type
popen: add caution comment for popen_may_io()
>
> Cyrill Gorcunov (2):
> popen: allow to kill process group
> popen: add ability to keep child on deletion
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2
2020-04-15 23:57 ` [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
2020-04-16 0:00 ` Alexander Turenko
@ 2020-04-16 11:52 ` Cyrill Gorcunov
1 sibling, 0 replies; 38+ messages in thread
From: Cyrill Gorcunov @ 2020-04-16 11:52 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
On Thu, Apr 16, 2020 at 02:57:13AM +0300, Alexander Turenko wrote:
> CCed Kirill.
>
> Added two patches upward (now the patchset contains 14 patches). Got
> ACKs for all patches from Cyrill. Updated the both branches (see above).
> GitLab CI looks okay (Travis CI jobs are pending).
>
> Kirill, the patchset seems to be ready to final review and push.
Yes, I think it is about a time to merge it up.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2
2020-04-14 11:38 [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
` (14 preceding siblings ...)
2020-04-15 23:57 ` [Tarantool-patches] [PATCH 00/12] Popen Lua API: preliminary patches 2 Alexander Turenko
@ 2020-04-17 6:58 ` Kirill Yukhin
15 siblings, 0 replies; 38+ messages in thread
From: Kirill Yukhin @ 2020-04-17 6:58 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
Hello,
On 14 апр 14:38, Alexander Turenko wrote:
> It is the second patchset of preparation patches that are needed to
> implement Lua API for popen.
>
> See the previous series here (already pushed to master):
> https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015609.html
>
> https://github.com/tarantool/tarantool/issues/4031
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4031-popen-lua-api-preparation-2
>
> I also pushed the same commit to *-full-ci branch to verify it on all
> platforms:
>
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4031-popen-lua-api-preparation-2-full-ci
>
> Alexander Turenko (10):
> popen: log a reason of close inherited fds failure
> popen: add missed diag_set() in popen_new()
> popen: remove retval from popen_stat()
> popen: quote multiword command arguments
> popen: add logging of duplicated logger fd
> popen: fix close-on-exec flag setting
> popen: clarify popen_{signal,delete} contract
> popen: add FIXME re group signal flaw
> popen: clarify popen_read_timeout error message
> popen: allow to close parent's end of std* fds
>
> Cyrill Gorcunov (2):
> popen: allow to kill process group
> popen: add ability to keep child on deletion
>
> src/lib/core/popen.c | 305 +++++++++++++++++++++++++++++++++----------
> src/lib/core/popen.h | 17 ++-
> 2 files changed, 253 insertions(+), 69 deletions(-)
I've checked your patch into master.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 38+ messages in thread