* [Tarantool-patches] [PATCH v2 0/5] popen: Add ability to run external process
@ 2019-12-10 9:48 Cyrill Gorcunov
2019-12-10 9:48 ` [Tarantool-patches] [PATCH v2 1/5] popen: Introduce a backend engine Cyrill Gorcunov
` (5 more replies)
0 siblings, 6 replies; 10+ messages in thread
From: Cyrill Gorcunov @ 2019-12-10 9:48 UTC (permalink / raw)
To: tml
In this series we provide a way to execute external binaries
from inside of Lua scripts, control children process and
communicate with their stdin/out/err streams.
In v2 I tried to stabilize api a bit still i don't like
current Lua api with a passion, thus please take a look
if we can improve here something. I'm far from being
Lua code profi so help would be really appreciated.
Probably the best way to start read this series is
the last patch where real examples of usage are
present.
All patches are sitting in branch gorcunov/gh-4031-popen-2
Cyrill Gorcunov (5):
popen: Introduce a backend engine
lua/fio: Add lbox_fio_push_error as a separate helper
popen/fio: Merge popen engine into fio internal module
popen/fio: Add ability to run external programs
test: Add app/popen test
src/lib/core/CMakeLists.txt | 1 +
src/lib/core/coio_file.c | 115 ++++
src/lib/core/coio_file.h | 8 +
src/lib/core/popen.c | 1168 +++++++++++++++++++++++++++++++++++
src/lib/core/popen.h | 187 ++++++
src/lua/fio.c | 346 ++++++++++-
src/lua/fio.lua | 578 +++++++++++++++++
src/main.cc | 4 +
test/app/popen.result | 179 ++++++
test/app/popen.test.lua | 68 ++
10 files changed, 2650 insertions(+), 4 deletions(-)
create mode 100644 src/lib/core/popen.c
create mode 100644 src/lib/core/popen.h
create mode 100644 test/app/popen.result
create mode 100644 test/app/popen.test.lua
--
2.20.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Tarantool-patches] [PATCH v2 1/5] popen: Introduce a backend engine
2019-12-10 9:48 [Tarantool-patches] [PATCH v2 0/5] popen: Add ability to run external process Cyrill Gorcunov
@ 2019-12-10 9:48 ` Cyrill Gorcunov
2019-12-26 4:33 ` Konstantin Osipov
2019-12-10 9:48 ` [Tarantool-patches] [PATCH v2 2/5] lua/fio: Add lbox_fio_push_error as a separate helper Cyrill Gorcunov
` (4 subsequent siblings)
5 siblings, 1 reply; 10+ messages in thread
From: Cyrill Gorcunov @ 2019-12-10 9:48 UTC (permalink / raw)
To: tml
In the patch we introduce popen backend engine which provides
a way to execute external programs and communicate with their
stdin/stdout/stderr streams.
It is possible to run a child process with:
a) completely closed stdX descriptors
b) provide /dev/null descriptors to appropritate stdX
c) pass new transport into a child (currently we use
pipes for this sake, but may extend to tty/sockets)
d) inherit stdX from a parent, iow do nothing
On tarantool start we create @popen_pids_map hash which maps
created processes PIDs to popen_handle structure, this structure
keeps everything needed to control and communicate with the children.
The hash will allow us to find a hild process quickly from inside
of a signal handler.
Each handle links into @popen_head list, which is need to be able
to destory children processes on exit procedure (ie when we exit
tarantool and need to cleanup the resources used).
Every new process is born by vfork() call - we can't use fork()
because of at_fork() handlers in libeio which cause deadlocking
in internal mutex usage. Thus the caller waits until vfork()
finishes its work and runs exec (or exit with error).
Because children processes are running without any limitations
they can exit by self or can be killed by some other third side
(say user of a hw node), we need to watch their state which is
done by setting a hook with ev_child_start() helper. This helper
allows us to catch SIGCHLD when a child get exited/signaled
and unregister it from a pool or currently running children.
Note the libev wait() reaps child zomby by self. Another
interesting detail is that libev catches signal in async way
but our SIGCHLD hook is called in sync way before child reap.
This engine provides the following API:
- popen_init
to initialize engine
- popen_free
to finalize engine and free all reasources
allocated so far
- popen_new
to create a new child process and start it
- popen_delete
to release resources occupied and
terminate a child process
- popen_stat
to fetch statistics about a child process
- popen_command
to fetch command line string formerly used
on popen creation
- popen_write
to write data into child's stdin
- popen_read_timeout
to read data from child's stdout/stderr
with timeout
- popen_state
to fetch state (alive, exited or killed) and
exit code of a child process
- popen_state_str
to get state of a child process in string
form, for Lua usage mostly
- popen_send_signal
to send signal to a child process (for
example to kill it)
Known issues to fix in next series:
- environment variables for non-linux systems do not support
inheritance for now due to lack of testing on my side;
- for linux base systems we use popen2 system call passing
O_CLOEXEC flag so that two concurrent popen_create calls
would not affect each other with pipes inheritance (while
currently we don't have a case where concurrent calls could
be done as far as I know, still better to be on a safe side
from the beginning);
- there are some files (such as xlog) which tarantool opens
for own needs without setting O_CLOEXEC flag and it get
propagated to a children process; for linux based systems
we use close_inherited_fds helper which walks over opened
files of a process and close them. But for other targets
like MachO or FreeBSD this helper just zapped simply because
I don't have such machines to experimant with; we should
investigate this moment in more details later once base
code is merged in;
- need to consider a case where we will be using piping for
descriptors (for example we might be writting into stdin
of a child from another pipe, for this sake we could use
splice() syscall which gonna be a way faster than copying
data inside kernel between process). Still the question
is -- do we really need it? Since we use interanal flags
in popen handle this should not be a big problem to extend
this interfaces;
this particular feature is considered to have a very low
priority but I left it here just to not forget.
Part of #4031
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
src/lib/core/CMakeLists.txt | 1 +
src/lib/core/popen.c | 1168 +++++++++++++++++++++++++++++++++++
src/lib/core/popen.h | 187 ++++++
src/main.cc | 4 +
4 files changed, 1360 insertions(+)
create mode 100644 src/lib/core/popen.c
create mode 100644 src/lib/core/popen.h
diff --git a/src/lib/core/CMakeLists.txt b/src/lib/core/CMakeLists.txt
index 8623aa0de..3f13ff904 100644
--- a/src/lib/core/CMakeLists.txt
+++ b/src/lib/core/CMakeLists.txt
@@ -15,6 +15,7 @@ set(core_sources
coio.cc
coio_task.c
coio_file.c
+ popen.c
coio_buf.cc
fio.c
exception.cc
diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
new file mode 100644
index 000000000..f3e5543cd
--- /dev/null
+++ b/src/lib/core/popen.c
@@ -0,0 +1,1168 @@
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <paths.h>
+#include <signal.h>
+#include <poll.h>
+
+#include <sys/types.h>
+#include <dirent.h>
+#include <sys/wait.h>
+
+#include "popen.h"
+#include "fiber.h"
+#include "assoc.h"
+#include "say.h"
+
+/* Children pids map popen_handle map */
+static struct mh_i32ptr_t *popen_pids_map = NULL;
+
+/* All popen handles to be able to cleanup them on exit */
+static RLIST_HEAD(popen_head);
+
+/* /dev/null to be used inside children if requested */
+static int dev_null_fd_ro = -1;
+static int dev_null_fd_wr = -1;
+
+/**
+ * popen_register - register popen handle in a pids map
+ * @handle: handle to register
+ */
+static void
+popen_register(struct popen_handle *handle)
+{
+ struct mh_i32ptr_node_t node = {
+ .key = handle->pid,
+ .val = handle,
+ };
+ say_debug("popen: register %d", handle->pid);
+ mh_i32ptr_put(popen_pids_map, &node, NULL, NULL);
+}
+
+/**
+ * popen_find - find popen handler by its pid
+ * @pid: pid of a handler
+ *
+ * Returns a handle if found or NULL otherwise.
+ */
+static struct popen_handle *
+popen_find(pid_t pid)
+{
+ mh_int_t k = mh_i32ptr_find(popen_pids_map, pid, NULL);
+ if (k == mh_end(popen_pids_map))
+ return NULL;
+ return mh_i32ptr_node(popen_pids_map, k)->val;
+}
+
+/**
+ * popen_unregister - remove popen handler from a pids map
+ * @handle: handle to remove
+ */
+static void
+popen_unregister(struct popen_handle *handle)
+{
+ struct mh_i32ptr_node_t node = {
+ .key = handle->pid,
+ .val = NULL,
+ };
+ say_debug("popen: unregister %d", handle->pid);
+ mh_i32ptr_remove(popen_pids_map, &node, NULL);
+}
+
+/**
+ * command_new - allocates a command string from argv array
+ * @argv: an array with pointers to argv strings
+ * @nr_argv: number of elements in the @argv
+ *
+ * Returns a new string or NULL on error.
+ */
+static inline char *
+command_new(char **argv, size_t nr_argv)
+{
+ size_t size = 0, i;
+ char *command;
+ char *pos;
+
+ assert(argv != NULL && nr_argv > 0);
+
+ for (i = 0; i < nr_argv; i++) {
+ if (argv[i] == NULL)
+ continue;
+ size += strlen(argv[i]) + 1;
+ }
+
+ command = malloc(size);
+ if (!command) {
+ say_syserror("popen: Can't allocate command");
+ return NULL;
+ }
+
+ pos = command;
+ for (i = 0; i < nr_argv-1; i++) {
+ if (argv[i] == NULL)
+ continue;
+ strcpy(pos, argv[i]);
+ pos += strlen(argv[i]);
+ *pos++ = ' ';
+ }
+ pos[-1] = '\0';
+
+ return command;
+}
+
+/**
+ * command_free - free memory allocated for command
+ * @command: pointer to free
+ *
+ * To pair with command_new().
+ */
+static inline void
+command_free(char *command)
+{
+ free(command);
+}
+
+/**
+ * handle_new - allocate new popen hanldle with flags specified
+ * @opts: pointer to options to be used
+ * @command: command line string
+ *
+ * Returns pointer to a new initialized popen object
+ * or NULL on error.
+ */
+static struct popen_handle *
+handle_new(struct popen_opts *opts, char *command)
+{
+ struct popen_handle *handle;
+
+ handle = malloc(sizeof(*handle));
+ if (!handle) {
+ say_syserror("popen: Can't allocate handle");
+ return NULL;
+ }
+
+ handle->command = command;
+ handle->wstatus = 0;
+ handle->pid = -1;
+ handle->flags = opts->flags;
+
+ rlist_create(&handle->list);
+
+ /* all fds to -1 */
+ memset(handle->fds, 0xff, sizeof(handle->fds));
+
+ say_debug("popen: handle %p alloc", handle);
+ return handle;
+}
+
+/**
+ * handle_free - free memory allocated for a handle
+ * @handle: popen handle to free
+ *
+ * Just to match handle_new().
+ */
+static void
+handle_free(struct popen_handle *handle)
+{
+ say_debug("popen: handle %p free %p", handle);
+ free(handle);
+}
+
+/**
+ * popen_may_io - test if handle can run io operation
+ * @handle: popen handle
+ * @idx: index of a file descriptor to operate on
+ * @io_flags: popen_flag_bits flags
+ *
+ * Returns true if IO is allowed and false otherwise
+ * (setting @errno).
+ */
+static inline bool
+popen_may_io(struct popen_handle *handle, unsigned int idx,
+ unsigned int io_flags)
+{
+ if (!handle) {
+ errno = ESRCH;
+ return false;
+ }
+
+ if (!(io_flags & handle->flags)) {
+ errno = EINVAL;
+ return false;
+ }
+
+ if (handle->fds[idx] < 0) {
+ errno = EPIPE;
+ return false;
+ }
+
+ return true;
+}
+
+/**
+ * popen_may_pidop - test if handle is valid for pid related operations
+ * @handle: popen handle
+ *
+ * This is shortcut to test if handle is not nil and still have
+ * a valid child process.
+ *
+ * Returns true if ops are allowed and false otherwise
+ * (setting @errno).
+ */
+static inline bool
+popen_may_pidop(struct popen_handle *handle)
+{
+ if (!handle || handle->pid == -1) {
+ errno = ESRCH;
+ return false;
+ }
+ return true;
+}
+
+/**
+ * popen_stat - fill popen object statistics
+ * @handle: popen handle
+ * @st: destination popen_stat to fill
+ *
+ * Returns 0 on succes, -1 otherwise (setting @errno).
+ */
+int
+popen_stat(struct popen_handle *handle, struct popen_stat *st)
+{
+ if (!handle) {
+ errno = ESRCH;
+ return -1;
+ }
+
+ st->pid = handle->pid;
+ st->flags = handle->flags;
+
+ static_assert(lengthof(st->fds) == lengthof(st->fds),
+ "Statistics fds are screwed");
+
+ return 0;
+}
+
+/**
+ * popen_command - get a pointer to former command line
+ * @handle: popen handle
+ *
+ * Returns pointer to a former command line for executiion.
+ * Since it might be big enough it is moved out of popen_stat.
+ *
+ * On error NULL returned.
+ */
+const char *
+popen_command(struct popen_handle *handle)
+{
+ if (!handle) {
+ errno = ESRCH;
+ return NULL;
+ }
+
+ return (const char *)handle->command;
+}
+
+/**
+ * stdX_str - get stdX descriptor string representation
+ * @index: descriptor index
+ */
+static inline const char *
+stdX_str(unsigned int index)
+{
+ static const char * stdX_names[] = {
+ [STDIN_FILENO] = "stdin",
+ [STDOUT_FILENO] = "stdout",
+ [STDERR_FILENO] = "stderr",
+ };
+
+ return index < lengthof(stdX_names) ?
+ stdX_names[index] : "unknown";
+}
+
+/**
+ * popen_write - write data to the child stdin
+ * @handle: popen handle
+ * @buf: data to write
+ * @count: number of bytes to write
+ * @flags: a flag representing stdin peer
+ *
+ * Returns number of bytes written or -1 on error (setting @errno).
+ */
+ssize_t
+popen_write(struct popen_handle *handle, void *buf,
+ size_t count, unsigned int flags)
+{
+ if (!popen_may_io(handle, STDIN_FILENO, flags))
+ return -1;
+
+ if (count > (size_t)SSIZE_MAX) {
+ errno = E2BIG;
+ return -1;
+ }
+
+ say_debug("popen: %d: write idx [%s:%d] buf %p count %zu",
+ handle->pid, stdX_str(STDIN_FILENO),
+ STDIN_FILENO, buf, count);
+
+ return write(handle->fds[STDIN_FILENO], buf, count);
+}
+
+/**
+ * popen_wait_read - wait for data appear on a child's peer
+ * @handle: popen handle
+ * @idx: peer index to wait on
+ * @timeout_msecs: timeout in microseconds
+ *
+ * Returns 1 if there is data to read, -1 on error (setting @errno).
+ */
+static int
+popen_wait_read(struct popen_handle *handle, int idx, int timeout_msecs)
+{
+ struct pollfd pollfd = {
+ .fd = handle->fds[idx],
+ .events = POLLIN,
+ };
+ int ret;
+
+ ret = poll(&pollfd, 1, timeout_msecs);
+ say_debug("popen: %d: poll: ret %d fd [%s:%d] events %#x revents %#x",
+ handle->pid, ret, stdX_str(idx), handle->fds[idx],
+ pollfd.events, pollfd.revents);
+
+ if (ret == 1) {
+ if (pollfd.revents == POLLIN) {
+ return 1;
+ } else {
+ say_error("popen: %d: unexpected revents %#x",
+ handle->pid, pollfd.revents);
+ errno = EINVAL;
+ return -1;
+ }
+ } else if (ret == 0) {
+ errno = EAGAIN;
+ ret = -1;
+ }
+
+ return ret;
+}
+
+/**
+ * popen_read_timeout - read data from a child's peer with timeout
+ * @handle: popen handle
+ * @buf: destination buffer
+ * @count: number of bytes to read
+ * @flags: POPEN_FLAG_FD_STDOUT or POPEN_FLAG_FD_STDERR
+ * @timeout_msecs: time to wait in microseconds if no
+ * data available; ignored if less or equal to zero
+ *
+ * Returns number of bytes read, otherwise -1 returned and
+ * @errno set accordingly.
+ */
+ssize_t
+popen_read_timeout(struct popen_handle *handle, void *buf,
+ size_t count, unsigned int flags,
+ int timeout_msecs)
+{
+ ssize_t ret;
+ int idx;
+
+ idx = flags & POPEN_FLAG_FD_STDOUT ?
+ STDOUT_FILENO : STDERR_FILENO;
+
+ if (!popen_may_io(handle, idx, flags))
+ return -1;
+
+ if (count > (size_t)SSIZE_MAX) {
+ errno = E2BIG;
+ return -1;
+ }
+
+ say_debug("popen: %d: read idx [%s:%d] buf %p count %zu "
+ "fds %d timeout_msecs %d",
+ handle->pid, stdX_str(idx), idx, buf, count,
+ handle->fds[idx], timeout_msecs);
+
+ if (timeout_msecs > 0) {
+ ret = popen_wait_read(handle, idx, timeout_msecs);
+ if (ret < 0) {
+ if (errno != EAGAIN) {
+ say_syserror("popen: %d: data wait failed",
+ handle->pid);
+ }
+ return -1;
+ }
+ }
+
+ return read(handle->fds[idx], buf, count);
+}
+
+/**
+ * wstatus_str - encode signal status into human readable form
+ * @buf: destination buffer
+ * @size: buffer size
+ * @wstatus: status to encode
+ *
+ * Operates on S_DEBUG level only simply because snprintf
+ * is pretty heavy in performance, otherwise @buf remains
+ * untouched.
+ *
+ * Returns pointer to @buf with encoded string.
+ */
+static char *
+wstatus_str(char *buf, size_t size, int wstatus)
+{
+ static const char fmt[] =
+ "wstatus %#x exited %d status %d "
+ "signaled %d wtermsig %d "
+ "stopped %d stopsig %d "
+ "coredump %d continued %d";
+
+ assert(size > 0);
+
+ if (say_log_level_is_enabled(S_DEBUG)) {
+ snprintf(buf, size, fmt, wstatus,
+ WIFEXITED(wstatus),
+ WIFEXITED(wstatus) ?
+ WEXITSTATUS(wstatus) : -1,
+ WIFSIGNALED(wstatus),
+ WIFSIGNALED(wstatus) ?
+ WTERMSIG(wstatus) : -1,
+ WIFSTOPPED(wstatus),
+ WIFSTOPPED(wstatus) ?
+ WSTOPSIG(wstatus) : -1,
+ WCOREDUMP(wstatus),
+ WIFCONTINUED(wstatus));
+ }
+
+ return buf;
+}
+
+/**
+ * popen_notify_sigchld - notify popen subsistem about SIGCHLD event
+ * @pid: PID of a process which changed its state
+ * @wstatus: signal status of a process
+ *
+ * The function is called from global SIGCHLD watcher in libev so
+ * we need to figure out if it is our process which possibly been
+ * terminated.
+ *
+ * Note the libev calls for wait() by self so we don't need
+ * to reap children.
+ */
+static void
+popen_notify_sigchld(pid_t pid, int wstatus)
+{
+ struct popen_handle *handle;
+ char buf[128];
+
+ say_debug("popen: sigchld notify %d (%s)",
+ pid, wstatus_str(buf, sizeof(buf), wstatus));
+
+ handle = popen_find(pid);
+ if (!handle)
+ return;
+
+ handle->wstatus = wstatus;
+ if (WIFEXITED(wstatus) || WIFSIGNALED(wstatus)) {
+ assert(handle->pid == pid);
+ /*
+ * libev calls for waitpid by self so
+ * we don't have to wait here.
+ */
+ popen_unregister(handle);
+ /*
+ * Since SIGCHLD may come to us not
+ * due to exit/kill reason (consider
+ * a case when someone stopped a child
+ * process) we should continue wathcing
+ * state changes, thus we stop monitoring
+ * dead children only.
+ */
+ say_debug("popen: ev_child_stop %d", handle->pid);
+ ev_child_stop(EV_DEFAULT_ &handle->ev_sigchld);
+ handle->pid = -1;
+ }
+}
+
+/**
+ * ev_sigchld_cb - handle SIGCHLD from a child process
+ * @w: a child exited
+ * @revents: unused
+ */
+static void
+ev_sigchld_cb(EV_P_ ev_child *w, int revents)
+{
+ (void)revents;
+ /*
+ * Stop watching this child, libev will
+ * remove it from own hashtable.
+ */
+ ev_child_stop(EV_A_ w);
+
+ /*
+ * The reason for a separate helper is that
+ * we might need to notify more subsystems
+ * in future.
+ */
+ popen_notify_sigchld(w->rpid, w->rstatus);
+}
+
+/**
+ * popen_state - get current child state
+ * @handle: popen handle
+ * @state: state of a child process
+ * @exit_code: exit code of a child process
+ *
+ * On success returns 0 sets @state to POPEN_STATE_
+ * constant and @exit_code. On error -1 returned with @errno.
+ */
+int
+popen_state(struct popen_handle *handle, int *state, int *exit_code)
+{
+ if (!handle) {
+ errno = ESRCH;
+ return -1;
+ }
+
+ if (handle->pid != -1) {
+ *state = POPEN_STATE_ALIVE;
+ *exit_code = 0;
+ } else {
+ if (WIFEXITED(handle->wstatus)) {
+ *state = POPEN_STATE_EXITED;
+ *exit_code = WEXITSTATUS(handle->wstatus);
+ } else {
+ *state = POPEN_STATE_SIGNALED;
+ *exit_code = WTERMSIG(handle->wstatus);
+ }
+ }
+
+ return 0;
+}
+
+/**
+ * popen_state_str - get process state string representation
+ * @state: process state
+ */
+const char *
+popen_state_str(unsigned int state)
+{
+ /*
+ * A process may be in a number of states,
+ * running/sleeping/disk sleep/stopped and etc
+ * but we are interested only if it is alive
+ * or dead (via plain exit or kill signal).
+ *
+ * Thus while it present in a system and not
+ * yet reaped we call it "alive".
+ *
+ * Note this is API for lua, so change with
+ * caution if needed.
+ */
+ static const char * state_str[POPEN_STATE_MAX] = {
+ [POPEN_STATE_NONE] = "none",
+ [POPEN_STATE_ALIVE] = "alive",
+ [POPEN_STATE_EXITED] = "exited",
+ [POPEN_STATE_SIGNALED] = "signaled",
+ };
+
+ return state < POPEN_STATE_MAX ? state_str[state] : "unknown";
+}
+
+/**
+ * popen_send_signal - send a signal to a child process
+ * @handle: popen handle
+ * @signo: signal number
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
+int
+popen_send_signal(struct popen_handle *handle, int signo)
+{
+ int ret;
+
+ /*
+ * A child may be killed or exited already.
+ */
+ if (!popen_may_pidop(handle))
+ return -1;
+
+ say_debug("popen: killpg %d signo %d", handle->pid, signo);
+ ret = killpg(handle->pid, signo);
+ if (ret < 0) {
+ say_syserror("popen: Unable to killpg %d signo %d",
+ handle->pid, signo);
+ }
+ return ret;
+}
+
+/**
+ * popen_delete - delete a popen handle
+ * @handle: a popen handle to delete
+ *
+ * The function kills a child process and
+ * close all fds and remove the handle from
+ * a living list and finally frees the handle.
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
+int
+popen_delete(struct popen_handle *handle)
+{
+ size_t i;
+
+ if (!handle) {
+ errno = ESRCH;
+ return -1;
+ }
+
+ if (popen_send_signal(handle, SIGKILL) && errno != ESRCH)
+ return -1;
+
+ for (i = 0; i < lengthof(handle->fds); i++) {
+ if (handle->fds[i] != -1)
+ close(handle->fds[i]);
+ }
+
+ /*
+ * We won't be wathcing this child anymore if
+ * kill signal is not yet delivered.
+ */
+ if (handle->pid != -1) {
+ say_debug("popen: ev_child_stop %d", handle->pid);
+ ev_child_stop(EV_DEFAULT_ &handle->ev_sigchld);
+ }
+
+ rlist_del(&handle->list);
+ command_free(handle->command);
+ handle_free(handle);
+ return 0;
+}
+
+/**
+ * make_pipe - create O_CLOEXEC pipe
+ * @pfd: pipe ends to setup
+ *
+ * Returns 0 on success, -1 on error.
+ */
+static int
+make_pipe(int pfd[2])
+{
+#ifdef TARGET_OS_LINUX
+ if (pipe2(pfd, O_CLOEXEC)) {
+ say_syserror("popen: Can't create pipe2");
+ return -1;
+ }
+#else
+ if (pipe(pfd)) {
+ say_syserror("popen: Can't create pipe");
+ return -1;
+ }
+ if (fcntl(pfd[0], F_SETFL, O_CLOEXEC) ||
+ fcntl(pfd[1], F_SETFL, O_CLOEXEC)) {
+ int saved_errno = errno;
+ say_syserror("popen: Can't unblock pipe");
+ close(pfd[0]), pfd[0] = -1;
+ close(pfd[1]), pfd[1] = -1;
+ errno = saved_errno;
+ return -1;
+ }
+#endif
+ return 0;
+}
+
+/**
+ * close_inherited_fds - close inherited file descriptors
+ * @skip_fds: an array of descriptors which should
+ * be kept opened
+ * @nr_skip_fds: number of elements in @skip_fds
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
+static int
+close_inherited_fds(int *skip_fds, size_t nr_skip_fds)
+{
+#ifdef TARGET_OS_LINUX
+ static const char path[] = "/proc/self/fd";
+ struct dirent *de;
+ int fd_no, fd_dir;
+ DIR *dir;
+ size_t i;
+
+ dir = opendir(path);
+ if (!dir) {
+ say_syserror("popen: fdin: Can't open %s", path);
+ return -1;
+ }
+ fd_dir = dirfd(dir);
+
+ for (de = readdir(dir); de; de = readdir(dir)) {
+ if (!strcmp(de->d_name, ".") ||
+ !strcmp(de->d_name, ".."))
+ continue;
+
+ fd_no = atoi(de->d_name);
+
+ if (fd_no == fd_dir)
+ continue;
+
+ /* We don't expect many numbers here */
+ for (i = 0; i < nr_skip_fds; i++) {
+ if (fd_no == skip_fds[i]) {
+ fd_no = -1;
+ break;
+ }
+ }
+
+ if (fd_no == -1)
+ continue;
+
+ if (close(fd_no)) {
+ int saved_errno = errno;
+ say_syserror("popen: fdin: Can't close %d", fd_no);
+ closedir(dir);
+ errno = saved_errno;
+ return -1;
+ }
+ }
+
+ if (closedir(dir)) {
+ say_syserror("popen: fdin: Can't close %s", path);
+ return -1;
+ }
+#else
+ /* FIXME: What about FreeBSD/MachO? */
+ (void)skip_fds;
+ (void)nr_skip_fds;
+
+ static bool said = false;
+ if (!said) {
+ say_warn("popen: fdin: Skip closing inherited");
+ said = true;
+ }
+#endif
+ return 0;
+}
+
+#ifdef TARGET_OS_LINUX
+extern char **environ;
+#endif
+
+static inline char **
+get_envp(struct popen_opts *opts)
+{
+ if (!opts->env) {
+#ifdef TARGET_OS_LINUX
+ return environ;
+#else
+ static const char **empty_envp[] = { NULL };
+ static bool said = false;
+ if (!said)
+ say_warn("popen: Environment inheritance "
+ "unsupported, passing empty");
+ return (char *)empty_envp;
+#endif
+ }
+ return opts->env;
+}
+
+/**
+ * popen_new - Create new popen handle
+ * @opts: options for popen handle
+ *
+ * This function creates a new child process and passes it
+ * pipe ends to communicate with child's stdin/stdout/stderr
+ * depending on @opts:flags. Where @opts:flags could be
+ * the bitwise "OR" for the following values:
+ *
+ * POPEN_FLAG_FD_STDIN - to write to stdin
+ * POPEN_FLAG_FD_STDOUT - to read from stdout
+ * POPEN_FLAG_FD_STDERR - to read from stderr
+ *
+ * When need to pass /dev/null descriptor into a child
+ * the following values can be used:
+ *
+ * POPEN_FLAG_FD_STDIN_DEVNULL
+ * POPEN_FLAG_FD_STDOUT_DEVNULL
+ * POPEN_FLAG_FD_STDERR_DEVNULL
+ *
+ * These flags have no effect if appropriate POPEN_FLAG_FD_STDx
+ * flags are set.
+ *
+ * When need to completely close the descriptors the
+ * following values can be used:
+ *
+ * POPEN_FLAG_FD_STDIN_CLOSE
+ * POPEN_FLAG_FD_STDOUT_CLOSE
+ * POPEN_FLAG_FD_STDERR_CLOSE
+ *
+ * These flags have no effect if appropriate POPEN_FLAG_FD_STDx
+ * flags are set.
+ *
+ * If none of POPEN_FLAG_FD_STDx flags are specified the child
+ * process will run with all files inherited from a parent.
+ *
+ * Returns pointer to a new popen handle on success,
+ * otherwise NULL returned setting @errno.
+ */
+struct popen_handle *
+popen_new(struct popen_opts *opts)
+{
+ struct popen_handle *handle = NULL;
+ char *command = NULL;
+
+ int pfd[POPEN_FLAG_FD_STDEND_BIT][2] = {
+ {-1, -1}, {-1, -1}, {-1, -1},
+ };
+
+ char **envp = get_envp(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.
+ */
+ int skip_fds[POPEN_FLAG_FD_STDEND_BIT * 2 + 2];
+ size_t nr_skip_fds = 0;
+
+ /*
+ * A caller must preserve space for this.
+ */
+ if (opts->flags & POPEN_FLAG_SHELL) {
+ opts->argv[0] = "sh";
+ opts->argv[1] = "-c";
+ }
+
+ /*
+ * Carry a copy for information sake.
+ */
+ command = command_new(opts->argv, opts->nr_argv);
+
+ say_debug("popen: command '%s' flags %#x", command, opts->flags);
+
+ static_assert(STDIN_FILENO == 0 &&
+ STDOUT_FILENO == 1 &&
+ STDERR_FILENO == 2,
+ "stdin/out/err are not posix compatible");
+
+ static_assert(lengthof(pfd) == lengthof(pfd_map),
+ "Pipes number does not map to fd bits");
+
+ static_assert(POPEN_FLAG_FD_STDIN_BIT == STDIN_FILENO &&
+ POPEN_FLAG_FD_STDOUT_BIT == STDOUT_FILENO &&
+ POPEN_FLAG_FD_STDERR_BIT == STDERR_FILENO,
+ "Popen flags do not match stdX");
+
+ handle = handle_new(opts, command);
+ if (!handle)
+ return NULL;
+
+ skip_fds[nr_skip_fds++] = dev_null_fd_ro;
+ skip_fds[nr_skip_fds++] = dev_null_fd_wr;
+ assert(nr_skip_fds <= lengthof(skip_fds));
+
+ for (i = 0; i < lengthof(pfd_map); i++) {
+ if (opts->flags & pfd_map[i].mask) {
+ if (make_pipe(pfd[i]))
+ goto out_err;
+
+ /*
+ * FIXME: Rather force make_pipe
+ * to allocate new fds with higher
+ * number.
+ */
+ if (pfd[i][0] <= STDERR_FILENO ||
+ pfd[i][1] <= STDERR_FILENO) {
+ say_error("popen: low fds [%s:%d:%d]",
+ stdX_str(i), pfd[i][0],
+ pfd[i][1]);
+ errno = EBADF;
+ goto out_err;
+ }
+
+ skip_fds[nr_skip_fds++] = pfd[i][0];
+ skip_fds[nr_skip_fds++] = pfd[i][1];
+ assert(nr_skip_fds <= lengthof(skip_fds));
+
+ say_debug("popen: created pipe [%s:%d:%d]",
+ stdX_str(i), pfd[i][0], pfd[i][1]);
+ } else if (!(opts->flags & pfd_map[i].mask_devnull) &&
+ !(opts->flags & pfd_map[i].mask_close)) {
+ skip_fds[nr_skip_fds++] = pfd_map[i].fileno;
+
+ say_debug("popen: inherit [%s:%d]",
+ stdX_str(i), pfd_map[i].fileno);
+ }
+ }
+
+ /*
+ * We have to use vfork here because libev has own
+ * at_fork helpers with mutex, so we will have double
+ * lock here and stuck forever otherwise.
+ *
+ * The good news that this affect tx only the
+ * other tarantoll threads are not waiting for
+ * vfork to complete. Also we try to do as minimum
+ * operations before the exec() as possible.
+ */
+ handle->pid = vfork();
+ if (handle->pid < 0) {
+ goto out_err;
+ } else if (handle->pid == 0) {
+ /*
+ * The documentation for libev says that
+ * each new fork should call ev_loop_fork(EV_DEFAULT)
+ * on every new child process, but we're going
+ * to execute bew binary anyway thus everything
+ * related to OS resources will be eliminated except
+ * file descriptors we use for piping. Thus don't
+ * do anything special.
+ */
+
+ /*
+ * We have to be a session leader otherwise
+ * won't be able to kill a group of children.
+ */
+ if (setsid() == -1)
+ goto exit_child;
+
+ if (close_inherited_fds(skip_fds, nr_skip_fds))
+ goto exit_child;
+
+ for (i = 0; i < lengthof(pfd_map); i++) {
+ int fileno = pfd_map[i].fileno;
+ /*
+ * Pass pipe peer to a child.
+ */
+ if (opts->flags & pfd_map[i].mask) {
+ int child_idx = pfd_map[i].child_idx;
+
+ /* put child peer end at known place */
+ if (dup2(pfd[i][child_idx], fileno) < 0)
+ goto exit_child;
+
+ /* parent's pipe no longer needed */
+ if (close(pfd[i][0]) ||
+ close(pfd[i][1]))
+ goto exit_child;
+ continue;
+ }
+
+ /*
+ * Use /dev/null if requested.
+ */
+ if (opts->flags & pfd_map[i].mask_devnull) {
+ if (dup2(*pfd_map[i].dev_null_fd,
+ fileno) < 0) {
+ goto exit_child;
+ }
+ continue;
+ }
+
+ /*
+ * Or close the destination completely, since
+ * we don't if the file in question is already
+ * closed by some other code we don't care if
+ * EBADF happens.
+ */
+ if (opts->flags & pfd_map[i].mask_close) {
+ if (close(fileno) && errno != EBADF)
+ goto exit_child;
+ continue;
+ }
+
+ /*
+ * Otherwise inherit file descriptor
+ * from a parent.
+ */
+ }
+
+ if (close(dev_null_fd_ro) || close(dev_null_fd_wr))
+ goto exit_child;
+
+ if (opts->flags & POPEN_FLAG_SHELL)
+ execve(_PATH_BSHELL, opts->argv, envp);
+ else
+ execve(opts->argv[2], &opts->argv[2], envp);
+exit_child:
+ _exit(errno);
+ unreachable();
+ }
+
+ for (i = 0; i < lengthof(pfd_map); i++) {
+ if (opts->flags & pfd_map[i].mask) {
+ int parent_idx = pfd_map[i].parent_idx;
+ int child_idx = pfd_map[i].child_idx;
+
+ handle->fds[i] = pfd[i][parent_idx];
+ say_debug("popen: keep pipe [%s:%d]",
+ stdX_str(i), handle->fds[i]);
+
+ if (close(pfd[i][child_idx])) {
+ say_syserror("popen: close child [%s:%d]",
+ stdX_str(i),
+ pfd[i][child_idx]);
+ goto out_err;
+ }
+
+ pfd[i][child_idx] = -1;
+ }
+ }
+
+ /*
+ * Link it into global list for force
+ * cleanup on exit.
+ */
+ rlist_add(&popen_head, &handle->list);
+
+ /*
+ * To watch when a child get exited.
+ */
+ popen_register(handle);
+
+ say_debug("popen: ev_child_start %d", handle->pid);
+ ev_child_init(&handle->ev_sigchld, ev_sigchld_cb, handle->pid, 0);
+ ev_child_start(EV_DEFAULT_ &handle->ev_sigchld);
+
+ say_debug("popen: created child %d", handle->pid);
+
+ return handle;
+
+out_err:
+ saved_errno = errno;
+ popen_delete(handle);
+ for (i = 0; i < lengthof(pfd); i++) {
+ if (pfd[i][0] != -1)
+ close(pfd[i][0]);
+ if (pfd[i][1] != -1)
+ close(pfd[i][1]);
+ }
+ errno = saved_errno;
+ return NULL;
+}
+
+/**
+ * popen_init - initialize popen subsystem
+ *
+ * Allocates resource needed for popen management.
+ */
+void
+popen_init(void)
+{
+ static const int flags = O_CLOEXEC;
+ static const char dev_null_path[] = "/dev/null";
+
+ say_debug("popen: initialize subsystem");
+ popen_pids_map = mh_i32ptr_new();
+
+ dev_null_fd_ro = open(dev_null_path, O_RDONLY | flags);
+ if (dev_null_fd_ro < 0)
+ goto out_err;
+ dev_null_fd_wr = open(dev_null_path, O_WRONLY | flags);
+ if (dev_null_fd_wr < 0)
+ goto out_err;
+
+ /*
+ * FIXME: We should allocate them somewhere
+ * after STDERR_FILENO so the child would be
+ * able to find these fd numbers not occupied.
+ * Other option is to use unix scm and send
+ * them to the child on demand.
+ *
+ * For now left as is since we don't close
+ * our main stdX descriptors and they are
+ * inherited when we call first vfork.
+ */
+ if (dev_null_fd_ro <= STDERR_FILENO ||
+ dev_null_fd_wr <= STDERR_FILENO) {
+ say_error("popen: /dev/null %d %d numbers are too low",
+ dev_null_fd_ro, dev_null_fd_wr);
+ goto out_err;
+ }
+
+ return;
+
+out_err:
+ say_syserror("popen: Can't open %s", dev_null_path);
+ if (dev_null_fd_ro >= 0)
+ close(dev_null_fd_ro);
+ if (dev_null_fd_wr >= 0)
+ close(dev_null_fd_wr);
+ mh_i32ptr_delete(popen_pids_map);
+ exit(EXIT_FAILURE);
+}
+
+/**
+ * popen_free - free popen subsystem
+ *
+ * Kills all running children and frees resources.
+ */
+void
+popen_free(void)
+{
+ struct popen_handle *handle, *tmp;
+
+ say_debug("popen: free subsystem");
+
+ close(dev_null_fd_ro);
+ close(dev_null_fd_wr);
+ dev_null_fd_ro = -1;
+ dev_null_fd_wr = -1;
+
+ rlist_foreach_entry_safe(handle, &popen_head, list, tmp) {
+ /*
+ * If children are still running we should move
+ * them out of the pool and kill them all then.
+ * Note though that we don't do an explicit wait
+ * here, OS will reap them anyway, just release
+ * the memory occupied for popen handles.
+ */
+ if (popen_may_pidop(handle))
+ popen_unregister(handle);
+ popen_delete(handle);
+ }
+
+ if (popen_pids_map) {
+ mh_i32ptr_delete(popen_pids_map);
+ popen_pids_map = NULL;
+ }
+}
diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h
new file mode 100644
index 000000000..4f995a5e2
--- /dev/null
+++ b/src/lib/core/popen.h
@@ -0,0 +1,187 @@
+#ifndef TARANTOOL_LIB_CORE_POPEN_H_INCLUDED
+#define TARANTOOL_LIB_CORE_POPEN_H_INCLUDED
+
+#if defined(__cplusplus)
+extern "C" {
+#endif
+
+#include <signal.h>
+#include <errno.h>
+#include <stdbool.h>
+#include <sys/types.h>
+
+#include <small/rlist.h>
+
+#include "third_party/tarantool_ev.h"
+
+enum popen_flag_bits {
+ POPEN_FLAG_NONE = (0 << 0),
+
+ /*
+ * Which fd we should handle.
+ */
+ POPEN_FLAG_FD_STDIN_BIT = 0,
+ POPEN_FLAG_FD_STDIN = (1 << POPEN_FLAG_FD_STDIN_BIT),
+
+ POPEN_FLAG_FD_STDOUT_BIT = 1,
+ POPEN_FLAG_FD_STDOUT = (1 << POPEN_FLAG_FD_STDOUT_BIT),
+
+ POPEN_FLAG_FD_STDERR_BIT = 2,
+ POPEN_FLAG_FD_STDERR = (1 << POPEN_FLAG_FD_STDERR_BIT),
+
+ /*
+ * Number of bits occupied for stdX descriptors.
+ */
+ POPEN_FLAG_FD_STDEND_BIT = POPEN_FLAG_FD_STDERR_BIT + 1,
+
+ /*
+ * Instead of inheriting fds from a parent
+ * rather use /dev/null.
+ */
+ POPEN_FLAG_FD_STDIN_DEVNULL_BIT = 3,
+ POPEN_FLAG_FD_STDIN_DEVNULL = (1 << POPEN_FLAG_FD_STDIN_DEVNULL_BIT),
+ POPEN_FLAG_FD_STDOUT_DEVNULL_BIT= 4,
+ POPEN_FLAG_FD_STDOUT_DEVNULL = (1 << POPEN_FLAG_FD_STDOUT_DEVNULL_BIT),
+ POPEN_FLAG_FD_STDERR_DEVNULL_BIT= 5,
+ POPEN_FLAG_FD_STDERR_DEVNULL = (1 << POPEN_FLAG_FD_STDERR_DEVNULL_BIT),
+
+ /*
+ * Instead of inheriting fds from a parent
+ * close fds completely.
+ */
+ POPEN_FLAG_FD_STDIN_CLOSE_BIT = 6,
+ POPEN_FLAG_FD_STDIN_CLOSE = (1 << POPEN_FLAG_FD_STDIN_CLOSE_BIT),
+ POPEN_FLAG_FD_STDOUT_CLOSE_BIT = 7,
+ POPEN_FLAG_FD_STDOUT_CLOSE = (1 << POPEN_FLAG_FD_STDOUT_CLOSE_BIT),
+ POPEN_FLAG_FD_STDERR_CLOSE_BIT = 8,
+ POPEN_FLAG_FD_STDERR_CLOSE = (1 << POPEN_FLAG_FD_STDERR_CLOSE_BIT),
+
+ /*
+ * Call exec directly or via shell.
+ */
+ POPEN_FLAG_SHELL_BIT = 9,
+ POPEN_FLAG_SHELL = (1 << POPEN_FLAG_SHELL_BIT),
+};
+
+enum popen_states {
+ POPEN_STATE_NONE = 0,
+ POPEN_STATE_ALIVE = 1,
+ POPEN_STATE_EXITED = 2,
+ POPEN_STATE_SIGNALED = 3,
+
+ POPEN_STATE_MAX,
+};
+
+/**
+ * struct popen_handle - an instance of popen object
+ *
+ * @pid: pid of a child process
+ * @command: copy of a command line for statistics
+ * @wstatus: exit status of a child process
+ * @ev_sigchld: needed by the libev to watch children
+ * @flags: popen_flag_bits
+ * @fds: std(in|out|err)
+ */
+struct popen_handle {
+ pid_t pid;
+ char *command;
+ int wstatus;
+ ev_child ev_sigchld;
+ struct rlist list;
+ unsigned int flags;
+ int fds[POPEN_FLAG_FD_STDEND_BIT];
+};
+
+/**
+ * struct popen_opts - options for popen creation
+ *
+ * @argv: argv to execute
+ * @nr_argv: number of elements in @argv
+ * @env: environment (can be empty)
+ * @flags: popen_flag_bits
+ */
+struct popen_opts {
+ char **argv;
+ size_t nr_argv;
+ char **env;
+ unsigned int flags;
+};
+
+/**
+ * struct popen_stat - popen object statistics
+ *
+ * @pid: pid of a child process
+ * @flags: popen_flag_bits
+ * @fds: std(in|out|err)
+ *
+ * This is a short version of struct popen_handle which should
+ * be used by external code and which should be changed/extended
+ * with extreme caution since it is used in Lua code. Consider it
+ * as API for external modules.
+ */
+struct popen_stat {
+ pid_t pid;
+ unsigned int flags;
+ int fds[POPEN_FLAG_FD_STDEND_BIT];
+};
+
+extern int
+popen_stat(struct popen_handle *handle, struct popen_stat *st);
+
+extern const char *
+popen_command(struct popen_handle *handle);
+
+/**
+ * should_retry_io -- test if we should retry IO procedure
+ * @_errno: error code to test
+ */
+static inline bool
+popen_should_retry_io(int _errno)
+{
+ /*
+ * Pipes do check for regular signal delivery
+ * (unlike phys fs like ext4 on which write get
+ * interrupted by the fatal signal only) so that
+ * the write could be interrupted and we should
+ * restart iteration.
+ */
+ if (_errno == EINTR)
+ return true;
+ return false;
+}
+
+extern ssize_t
+popen_write(struct popen_handle *handle, void *buf,
+ size_t count, unsigned int flags);
+
+extern ssize_t
+popen_read_timeout(struct popen_handle *handle, void *buf,
+ size_t count, unsigned int flags,
+ int timeout_msecs);
+
+extern int
+popen_state(struct popen_handle *handle, int *state, int *exit_code);
+
+extern const char *
+popen_state_str(unsigned int state);
+
+extern int
+popen_send_signal(struct popen_handle *handle, int signo);
+
+extern int
+popen_delete(struct popen_handle *handle);
+
+extern struct popen_handle *
+popen_new(struct popen_opts *opts);
+
+extern void
+popen_init(void);
+
+extern void
+popen_free(void);
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif
+
+#endif /* TARANTOOL_LIB_CORE_POPEN_H_INCLUDED */
diff --git a/src/main.cc b/src/main.cc
index 0ff2213b6..87654fb14 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -77,6 +77,7 @@
#include "box/session.h"
#include "systemd.h"
#include "crypto/crypto.h"
+#include "core/popen.h"
static pid_t master_pid = getpid();
static struct pidfh *pid_file_handle;
@@ -614,6 +615,8 @@ tarantool_free(void)
title_free(main_argc, main_argv);
+ popen_free();
+
/* unlink pidfile. */
if (pid_file_handle != NULL && pidfile_remove(pid_file_handle) == -1)
say_syserror("failed to remove pid file '%s'", pid_file);
@@ -801,6 +804,7 @@ main(int argc, char **argv)
exception_init();
fiber_init(fiber_cxx_invoke);
+ popen_init();
coio_init();
coio_enable();
signal_init();
--
2.20.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Tarantool-patches] [PATCH v2 2/5] lua/fio: Add lbox_fio_push_error as a separate helper
2019-12-10 9:48 [Tarantool-patches] [PATCH v2 0/5] popen: Add ability to run external process Cyrill Gorcunov
2019-12-10 9:48 ` [Tarantool-patches] [PATCH v2 1/5] popen: Introduce a backend engine Cyrill Gorcunov
@ 2019-12-10 9:48 ` Cyrill Gorcunov
2019-12-10 9:48 ` [Tarantool-patches] [PATCH v2 3/5] popen/fio: Merge popen engine into fio internal module Cyrill Gorcunov
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Cyrill Gorcunov @ 2019-12-10 9:48 UTC (permalink / raw)
To: tml
Since lbox_fio_pushbool always push boolean first
it is inpossible to use it inside other routines.
Thus make a separate helper lbox_fio_push_error
which will be used in popen code later.
Part-of #4031
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
src/lua/fio.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/lua/fio.c b/src/lua/fio.c
index 7437cc0c6..33ccd5d71 100644
--- a/src/lua/fio.c
+++ b/src/lua/fio.c
@@ -52,6 +52,16 @@
luaT_push_nil_and_error(L); \
})
+static inline int
+lbox_fio_push_error(struct lua_State *L)
+{
+ diag_set(SystemError, "fio: %s", strerror(errno));
+ struct error *e = diag_last_error(diag_get());
+ assert(e != NULL);
+ luaT_pusherror(L, e);
+ return 1;
+}
+
static int
lbox_fio_open(struct lua_State *L)
{
@@ -118,10 +128,7 @@ lbox_fio_pushbool(struct lua_State *L, bool res)
{
lua_pushboolean(L, res);
if (!res) {
- diag_set(SystemError, "fio: %s", strerror(errno));
- struct error *e = diag_last_error(diag_get());
- assert(e != NULL);
- luaT_pusherror(L, e);
+ lbox_fio_push_error(L);
return 2;
}
return 1;
--
2.20.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Tarantool-patches] [PATCH v2 3/5] popen/fio: Merge popen engine into fio internal module
2019-12-10 9:48 [Tarantool-patches] [PATCH v2 0/5] popen: Add ability to run external process Cyrill Gorcunov
2019-12-10 9:48 ` [Tarantool-patches] [PATCH v2 1/5] popen: Introduce a backend engine Cyrill Gorcunov
2019-12-10 9:48 ` [Tarantool-patches] [PATCH v2 2/5] lua/fio: Add lbox_fio_push_error as a separate helper Cyrill Gorcunov
@ 2019-12-10 9:48 ` Cyrill Gorcunov
2019-12-10 9:48 ` [Tarantool-patches] [PATCH v2 4/5] popen/fio: Add ability to run external programs Cyrill Gorcunov
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Cyrill Gorcunov @ 2019-12-10 9:48 UTC (permalink / raw)
To: tml
In the patch we wrap popen calls with lua C interface
which will allow us to use popen inside lua code.
Part-of #4031
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
src/lib/core/coio_file.c | 115 ++++++++++++++
src/lib/core/coio_file.h | 8 +
src/lua/fio.c | 331 +++++++++++++++++++++++++++++++++++++++
3 files changed, 454 insertions(+)
diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
index 62388344e..70bdd572a 100644
--- a/src/lib/core/coio_file.c
+++ b/src/lib/core/coio_file.c
@@ -30,6 +30,7 @@
*/
#include "coio_file.h"
#include "coio_task.h"
+#include "popen.h"
#include "fiber.h"
#include "say.h"
#include "fio.h"
@@ -103,6 +104,15 @@ struct coio_file_task {
const char *source;
const char *dest;
} copyfile;
+
+ struct {
+ struct popen_handle *handle;
+ struct popen_opts *opts;
+ unsigned int flags;
+ int timeout_msecs;
+ size_t count;
+ void *buf;
+ } popen;
};
};
@@ -639,3 +649,108 @@ coio_utime(const char *pathname, double atime, double mtime)
eio_req *req = eio_utime(pathname, atime, mtime, 0, coio_complete, &eio);
return coio_wait_done(req, &eio);
}
+
+static void
+coio_do_popen_read(eio_req *req)
+{
+ struct coio_file_task *eio = req->data;
+ req->result = popen_read_timeout(eio->popen.handle,
+ eio->popen.buf,
+ eio->popen.count,
+ eio->popen.flags,
+ eio->popen.timeout_msecs);
+}
+
+ssize_t
+coio_popen_read(void *handle, void *buf, size_t count,
+ unsigned int flags, int timeout_msecs)
+{
+ INIT_COEIO_FILE(eio);
+ ssize_t res = -1;
+
+ eio.popen.timeout_msecs = timeout_msecs;
+ eio.popen.handle = handle;
+ eio.popen.flags = flags;
+ eio.popen.count = count;
+ eio.popen.buf = buf;
+
+ while (res < 0) {
+ eio_req *req = eio_custom(coio_do_popen_read,
+ EIO_PRI_DEFAULT,
+ coio_complete, &eio);
+ res = coio_wait_done(req, &eio);
+ if (res < 0) {
+ if (!popen_should_retry_io(errno))
+ break;
+ eio.done = false;
+ }
+ }
+
+ return res;
+}
+
+static void
+coio_do_popen_write(eio_req *req)
+{
+ struct coio_file_task *eio = req->data;
+ req->result = popen_write(eio->popen.handle,
+ eio->popen.buf,
+ eio->popen.count,
+ eio->popen.flags);
+}
+
+ssize_t
+coio_popen_write(void *handle, void *buf, size_t count,
+ unsigned int flags)
+{
+ ssize_t left = count, pos = 0;
+ INIT_COEIO_FILE(eio);
+
+ eio.popen.handle = handle;
+ eio.popen.flags = flags;
+
+ while (left > 0) {
+ eio.popen.count = left;
+ eio.popen.buf = buf + pos;
+
+ eio_req *req = eio_custom(coio_do_popen_write,
+ EIO_PRI_DEFAULT,
+ coio_complete, &eio);
+ ssize_t res = coio_wait_done(req, &eio);
+ if (res < 0) {
+ if (!popen_should_retry_io(errno)) {
+ pos = -1;
+ break;
+ }
+ eio.done = false;
+ continue;
+ }
+ left -= res;
+ pos += res;
+ }
+
+ return pos;
+}
+
+static void
+do_coio_popen_new(eio_req *req)
+{
+ struct coio_file_task *eio = req->data;
+
+ eio->popen.handle = popen_new(eio->popen.opts);
+ req->result = eio->popen.handle ? 0 : -1;
+}
+
+void *
+coio_popen_new(struct popen_opts *opts)
+{
+ INIT_COEIO_FILE(eio);
+ eio.popen.opts = opts;
+
+ eio_req *req = eio_custom(do_coio_popen_new,
+ EIO_PRI_DEFAULT,
+ coio_complete, &eio);
+ coio_wait_done(req, &eio);
+
+ return eio.popen.handle;
+}
diff --git a/src/lib/core/coio_file.h b/src/lib/core/coio_file.h
index ac7b1aacf..593769db5 100644
--- a/src/lib/core/coio_file.h
+++ b/src/lib/core/coio_file.h
@@ -85,6 +85,14 @@ int coio_tempdir(char *path, size_t path_len);
int coio_readdir(const char *path, char **buf);
int coio_copyfile(const char *source, const char *dest);
int coio_utime(const char *pathname, double atime, double mtime);
+
+struct popen_opts;
+void *coio_popen_new(struct popen_opts *opts);
+ssize_t coio_popen_read(void *handle, void *buf, size_t count,
+ unsigned int flags, int timeout_msecs);
+ssize_t coio_popen_write(void *handle, void *buf, size_t count,
+ unsigned int flags);
+
#if defined(__cplusplus)
} /* extern "C" */
#endif /* defined(__cplusplus) */
diff --git a/src/lua/fio.c b/src/lua/fio.c
index 33ccd5d71..37209f691 100644
--- a/src/lua/fio.c
+++ b/src/lua/fio.c
@@ -39,6 +39,7 @@
#include <time.h>
#include <errno.h>
#include "coio_task.h"
+#include "popen.h"
#include <lua.h>
#include <lauxlib.h>
@@ -683,6 +684,328 @@ lbox_fio_utime(struct lua_State *L)
return lbox_fio_pushbool(L, coio_utime(pathname, atime, mtime) == 0);
}
+/**
+ * lbox_fio_popen_new - creates a new popen handle and runs a command inside
+ * @command: a command to run
+ * @flags: popen_flag_bits
+ *
+ * Returns pair @handle = data, @err = nil on success,
+ * @handle = nil, err ~= nil on error.
+ */
+static int
+lbox_fio_popen_new(struct lua_State *L)
+{
+ struct popen_handle *handle;
+ struct popen_opts opts = { };
+ size_t i, argv_size;
+ ssize_t nr_env;
+
+ if (lua_gettop(L) < 1 || !lua_istable(L, 1))
+ luaL_error(L, "Usage: fio.run({opts}])");
+
+ lua_pushstring(L, "argv");
+ lua_gettable(L, -2);
+ if (!lua_istable(L, -1))
+ luaL_error(L, "fio.run: {argv=...} is not a table");
+ lua_pop(L, 1);
+
+ lua_pushstring(L, "flags");
+ lua_gettable(L, -2);
+ if (!lua_isnumber(L, -1))
+ luaL_error(L, "fio.run: {flags=...} is not a number");
+ opts.flags = lua_tonumber(L, -1);
+ lua_pop(L, 1);
+
+ lua_pushstring(L, "argc");
+ lua_gettable(L, -2);
+ if (!lua_isnumber(L, -1))
+ luaL_error(L, "fio.run: {argc=...} is not a number");
+ opts.nr_argv = lua_tonumber(L, -1);
+ lua_pop(L, 1);
+
+ if (opts.nr_argv < 1)
+ luaL_error(L, "fio.run: {argc} is too small");
+
+ /*
+ * argv array should contain NULL element at the
+ * end and probably "sh", "-c" at the start, so
+ * reserve enough space for any flags.
+ */
+ opts.nr_argv += 3;
+ argv_size = sizeof(char *) * opts.nr_argv;
+ opts.argv = malloc(argv_size);
+ if (!opts.argv)
+ luaL_error(L, "fio.run: can't allocate argv");
+
+ lua_pushstring(L, "argv");
+ lua_gettable(L, -2);
+ lua_pushnil(L);
+ for (i = 2; lua_next(L, -2) != 0; i++) {
+ assert(i < opts.nr_argv);
+ opts.argv[i] = (char *)lua_tostring(L, -1);
+ lua_pop(L, 1);
+ }
+ lua_pop(L, 1);
+
+ opts.argv[0] = NULL;
+ opts.argv[1] = NULL;
+ opts.argv[opts.nr_argv - 1] = NULL;
+
+ /*
+ * Environment can be filled, empty
+ * to inherit or contain one NULL to
+ * be zapped.
+ */
+ lua_pushstring(L, "envc");
+ lua_gettable(L, -2);
+ if (!lua_isnumber(L, -1)) {
+ free(opts.argv);
+ luaL_error(L, "fio.run: {envc=...} is not a number");
+ }
+ nr_env = lua_tonumber(L, -1);
+ lua_pop(L, 1);
+
+ if (nr_env >= 0) {
+ /* Should be NULL terminating */
+ opts.env = malloc((nr_env + 1) * sizeof(char *));
+ if (!opts.env) {
+ free(opts.argv);
+ luaL_error(L, "fio.run: can't allocate env");
+ }
+
+ lua_pushstring(L, "env");
+ lua_gettable(L, -2);
+ if (!lua_istable(L, -1)) {
+ free(opts.argv);
+ free(opts.env);
+ luaL_error(L, "fio.run: {env=...} is not a table");
+ }
+ lua_pushnil(L);
+ for (i = 0; lua_next(L, -2) != 0; i++) {
+ assert((ssize_t)i <= nr_env);
+ opts.env[i] = (char *)lua_tostring(L, -1);
+ lua_pop(L, 1);
+ }
+ lua_pop(L, 1);
+
+ opts.env[nr_env] = NULL;
+ } else {
+ /*
+ * Just zap it to nil, the popen will
+ * process inheriting by self.
+ */
+ opts.env = NULL;
+ }
+
+ handle = coio_popen_new(&opts);
+
+ free(opts.argv);
+ free(opts.env);
+
+ if (!handle)
+ return lbox_fio_pushsyserror(L);
+
+ lua_pushlightuserdata(L, handle);
+ return 1;
+}
+
+/**
+ * lbox_fio_popen_delete - close a popen handle
+ * @handle: a handle to close
+ *
+ * If there is a running child it get killed first.
+ *
+ * Returns true if a handle is closeed, false otherwise.
+ */
+static int
+lbox_fio_popen_delete(struct lua_State *L)
+{
+ void *handle = lua_touserdata(L, 1);
+ return lbox_fio_pushbool(L, popen_delete(handle) == 0);
+}
+
+/**
+ * lbox_fio_popen_kill - kill popen's child process
+ * @handle: a handle carries child process to kill
+ *
+ * Returns true if process is killed and false
+ * otherwise. Note the process is simply signaled
+ * and it doesn't mean it is killed immediately,
+ * Poll lbox_fio_pstatus if need to find out when
+ * exactly the child is reaped out.
+ */
+static int
+lbox_fio_popen_kill(struct lua_State *L)
+{
+ struct popen_handle *p = lua_touserdata(L, 1);
+ return lbox_fio_pushbool(L, popen_send_signal(p, SIGKILL) == 0);
+}
+
+/**
+ * lbox_fio_popen_term - terminate popen's child process
+ * @handle: a handle carries child process to terminate
+ *
+ * Returns true if process is terminated and false
+ * otherwise.
+ */
+static int
+lbox_fio_popen_term(struct lua_State *L)
+{
+ struct popen_handle *p = lua_touserdata(L, 1);
+ return lbox_fio_pushbool(L, popen_send_signal(p, SIGTERM) == 0);
+}
+
+/**
+ * lbox_fio_popen_state - fetch popen child process status
+ * @handle: a handle to fetch status from
+ *
+ * Returns @err = nil, @reason = POPEN_STATE_x,
+ * @exit_code = 'number' on success, @err ~= nil on error.
+ */
+static int
+lbox_fio_popen_state(struct lua_State *L)
+{
+ struct popen_handle *p = lua_touserdata(L, 1);
+ int state, exit_code, ret;
+
+ ret = popen_state(p, &state, &exit_code);
+ if (ret < 0)
+ return lbox_fio_push_error(L);
+
+ lua_pushnil(L);
+ lua_pushinteger(L, state);
+ lua_pushinteger(L, exit_code);
+ return 3;
+}
+
+/**
+ * lbox_fio_popen_read - read data from a child peer
+ * @handle: a handle of a child process
+ * @buf: destination buffer
+ * @count: number of bytes to read
+ * @timeout_msecs: read timeout, in microseconds, -1 to not use
+ * @flags: which peer to read (stdout,stderr)
+ *
+ * Returns @size = 'read bytes', @err = nil on success,
+ * @size = nil, @err = nil if timeout expired, and
+ * @size = nil, @err ~= nil on error.
+ */
+static int
+lbox_fio_popen_read(struct lua_State *L)
+{
+ struct popen_handle *handle = lua_touserdata(L, 1);
+ uint32_t ctypeid;
+ void *buf = *(char **)luaL_checkcdata(L, 2, &ctypeid);
+ size_t count = lua_tonumber(L, 3);
+ int timeout_msecs = lua_tonumber(L, 4);
+ unsigned int flags = lua_tonumber(L, 5);
+ ssize_t ret;
+
+ ret = coio_popen_read(handle, buf, count,
+ flags, timeout_msecs);
+ if (ret < 0) {
+ if (errno == EAGAIN) {
+ lua_pushnil(L);
+ lua_pushnil(L);
+ return 2;
+ } else
+ return lbox_fio_pushsyserror(L);
+ }
+
+ lua_pushinteger(L, ret);
+ return 1;
+}
+
+/**
+ * lbox_fio_popen_write - write data to a child peer
+ * @handle: a handle of a child process
+ * @buf: source buffer
+ * @count: number of bytes to write
+ * @flags: which peer to write (stdin)
+ *
+ * Returns @size = 'bytes wrote', @err = nil on succes,
+ * @size = nil, @err ~= nil on error.
+ */
+static int
+lbox_fio_popen_write(struct lua_State *L)
+{
+ struct popen_handle *handle = lua_touserdata(L, 1);
+ void *buf = (void *)lua_tostring(L, 2);
+ uint32_t ctypeid = 0;
+ if (buf == NULL)
+ buf = *(char **)luaL_checkcdata(L, 2, &ctypeid);
+ size_t count = lua_tonumber(L, 3);
+ unsigned int flags = lua_tonumber(L, 4);
+ ssize_t ret;
+
+ ret = popen_write(handle, buf, count, flags);
+ if (ret < 0)
+ return lbox_fio_pushsyserror(L);
+
+ lua_pushinteger(L, ret);
+ return 1;
+}
+
+/**
+ * lbox_fio_popen_info - return information about popen handle
+ * @handle: a handle of a child process
+ *
+ * Returns a @table ~= nil, @err = nil on success,
+ * @table = nil, @err ~= nil on error.
+ */
+static int
+lbox_fio_popen_info(struct lua_State *L)
+{
+ struct popen_handle *handle = lua_touserdata(L, 1);
+ int state, exit_code, ret;
+ struct popen_stat st = { };
+
+ if (popen_stat(handle, &st))
+ return lbox_fio_pushsyserror(L);
+
+ ret = popen_state(handle, &state, &exit_code);
+ if (ret < 0)
+ return lbox_fio_pushsyserror(L);
+
+ assert(state < POPEN_STATE_MAX);
+
+ lua_newtable(L);
+
+ lua_pushliteral(L, "pid");
+ lua_pushinteger(L, st.pid);
+ lua_settable(L, -3);
+
+ lua_pushliteral(L, "command");
+ lua_pushstring(L, popen_command(handle));
+ lua_settable(L, -3);
+
+ lua_pushliteral(L, "flags");
+ lua_pushinteger(L, st.flags);
+ lua_settable(L, -3);
+
+ lua_pushliteral(L, "state");
+ lua_pushstring(L, popen_state_str(state));
+ lua_settable(L, -3);
+
+ lua_pushliteral(L, "exit_code");
+ lua_pushinteger(L, exit_code);
+ lua_settable(L, -3);
+
+ lua_pushliteral(L, "stdin");
+ lua_pushinteger(L, st.fds[POPEN_FLAG_FD_STDIN_BIT]);
+ lua_settable(L, -3);
+
+ lua_pushliteral(L, "stdout");
+ lua_pushinteger(L, st.fds[POPEN_FLAG_FD_STDOUT_BIT]);
+ lua_settable(L, -3);
+
+ lua_pushliteral(L, "stderr");
+ lua_pushinteger(L, st.fds[POPEN_FLAG_FD_STDERR_BIT]);
+ lua_settable(L, -3);
+
+ return 1;
+}
+
void
tarantool_lua_fio_init(struct lua_State *L)
{
@@ -726,6 +1049,14 @@ tarantool_lua_fio_init(struct lua_State *L)
{ "fstat", lbox_fio_fstat },
{ "copyfile", lbox_fio_copyfile, },
{ "utime", lbox_fio_utime },
+ { "popen_new", lbox_fio_popen_new },
+ { "popen_delete", lbox_fio_popen_delete },
+ { "popen_kill", lbox_fio_popen_kill },
+ { "popen_term", lbox_fio_popen_term },
+ { "popen_state", lbox_fio_popen_state },
+ { "popen_read", lbox_fio_popen_read },
+ { "popen_write", lbox_fio_popen_write },
+ { "popen_info", lbox_fio_popen_info },
{ NULL, NULL }
};
luaL_register(L, NULL, internal_methods);
--
2.20.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Tarantool-patches] [PATCH v2 4/5] popen/fio: Add ability to run external programs
2019-12-10 9:48 [Tarantool-patches] [PATCH v2 0/5] popen: Add ability to run external process Cyrill Gorcunov
` (2 preceding siblings ...)
2019-12-10 9:48 ` [Tarantool-patches] [PATCH v2 3/5] popen/fio: Merge popen engine into fio internal module Cyrill Gorcunov
@ 2019-12-10 9:48 ` Cyrill Gorcunov
2019-12-10 9:48 ` [Tarantool-patches] [PATCH v2 5/5] test: Add app/popen test Cyrill Gorcunov
2019-12-11 9:29 ` [Tarantool-patches] [PATCH v2 0/5] popen: Add ability to run external process Cyrill Gorcunov
5 siblings, 0 replies; 10+ messages in thread
From: Cyrill Gorcunov @ 2019-12-10 9:48 UTC (permalink / raw)
To: tml
In the patch we add support for running external
programs via fio module.
The interface is the following:
- To create new popen object either call fio.popen,
or fio.run; the difference is that fio.popen uses
simplified arguments list (like fio.popen(command, "rw"))
while fio.run accepts a table of options which will
be extended in future if needed
- To read/write popen:read()/write() methods are supported,
and read() supports timeout argument which implemented
via internal poll() call and allows to test for data present
on a child peer
- To interrupt children work and exit one can use popen:term()
which is soft signal and can be ignored by a child, or
popen:kill() to force exit. Note that signals are not handled
immediately so the next method is needed
- To wait until child is terminated popen:wait() should be
used. It waits in a cycle so be carefull here -- if child
get signaled with popen:term() it might ignore the signal
and popen:wait() will spin forever
- To fetch information about current state of a child process
use popen:info() method
- Once popen no longer needed the popen:close() should be
called to free all resources associated with the object
Part-of #4031
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
src/lua/fio.lua | 578 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 578 insertions(+)
diff --git a/src/lua/fio.lua b/src/lua/fio.lua
index cb224f3d0..b613cfe8c 100644
--- a/src/lua/fio.lua
+++ b/src/lua/fio.lua
@@ -5,6 +5,7 @@ local ffi = require('ffi')
local buffer = require('buffer')
local fiber = require('fiber')
local errno = require('errno')
+local bit = require('bit')
ffi.cdef[[
int umask(int mask);
@@ -519,4 +520,581 @@ fio.path.lexists = function(filename)
return fio.lstat(filename) ~= nil
end
+local popen_methods = { }
+
+--
+-- Make sure the bits are matching POPEN_FLAG_x
+-- flags in popen header. This is api but internal
+-- one, we should not make it visible to users.
+local popen_flag_bits = {
+ STDIN = 0,
+ STDOUT = 1,
+ STDERR = 2,
+
+ STDIN_DEVNULL = 3,
+ STDOUT_DEVNULL = 4,
+ STDERR_DEVNULL = 5,
+
+ STDIN_CLOSE = 6,
+ STDOUT_CLOSE = 7,
+ STDERR_CLOSE = 8,
+
+ SHELL = 9,
+}
+
+local popen_flags = {
+ NONE = 0,
+ STDIN = bit.lshift(1, popen_flag_bits.STDIN),
+ STDOUT = bit.lshift(1, popen_flag_bits.STDOUT),
+ STDERR = bit.lshift(1, popen_flag_bits.STDERR),
+
+ STDIN_DEVNULL = bit.lshift(1, popen_flag_bits.STDIN_DEVNULL),
+ STDOUT_DEVNULL = bit.lshift(1, popen_flag_bits.STDOUT_DEVNULL),
+ STDERR_DEVNULL = bit.lshift(1, popen_flag_bits.STDERR_DEVNULL),
+
+ STDIN_CLOSE = bit.lshift(1, popen_flag_bits.STDIN_CLOSE),
+ STDOUT_CLOSE = bit.lshift(1, popen_flag_bits.STDOUT_CLOSE),
+ STDERR_CLOSE = bit.lshift(1, popen_flag_bits.STDERR_CLOSE),
+
+ SHELL = bit.lshift(1, popen_flag_bits.SHELL),
+}
+
+--
+-- Get default flags for popen
+local function popen_default_flags()
+ local flags = popen_flags.NONE
+
+ -- default flags: close everything and use shell
+ flags = bit.bor(flags, popen_flags.STDIN_CLOSE)
+ flags = bit.bor(flags, popen_flags.STDOUT_CLOSE)
+ flags = bit.bor(flags, popen_flags.STDERR_CLOSE)
+ flags = bit.bor(flags, popen_flags.SHELL)
+
+ return flags
+end
+
+--
+-- Close a popen object and release all resources.
+-- In case if there is a running child process
+-- it will be killed.
+--
+-- Returns @ret = true if popen handle closed, and
+-- @ret = false, @err ~= nil on error.
+popen_methods.close = function(self)
+ local ret, err = internal.popen_delete(self.popen_handle)
+ if err ~= nil then
+ return false, err
+ end
+ self.popen_handle = nil
+ return true
+end
+
+--
+-- Kill a child process
+--
+-- Returns @ret = true on success,
+-- @ret = false, @err ~= nil on error.
+popen_methods.kill = function(self)
+ return internal.popen_kill(self.popen_handle)
+end
+
+--
+-- Terminate a child process
+--
+-- Returns @ret = true on success,
+-- @ret = false, @err ~= nil on error.
+popen_methods.terminate = function(self)
+ return internal.popen_term(self.popen_handle)
+end
+
+--
+-- Fetch a child process state
+--
+-- Returns @err = nil, @state = (1 - if exited,
+-- 2 - if killed by a signal) and @exit_code ~= nil,
+-- otherwise @err ~= nil.
+--
+-- FIXME: Should the @state be a named constant?
+popen_methods.state = function(self)
+ return internal.popen_state(self.popen_handle)
+end
+
+--
+-- Wait until a child process finished its work
+-- and exit then.
+--
+-- Since it goes via coio thread we can't simply
+-- call wait4 (or similar) directly to not block
+-- the whole i/o subsystem. Instead we do an explicit
+-- polling for status change with predefined period
+-- of sleeping.
+--
+-- Returns the same as popen_methods.status.
+popen_methods.wait = function(self)
+ local err, state, code
+ while true do
+ err, state, code = internal.popen_state(self.popen_handle)
+ if err or state then
+ break
+ end
+ fiber.sleep(self.wait_secs)
+ end
+ return err, state, code
+end
+
+--
+-- A map for popen option keys into tfn ('true|false|nil') values
+-- where bits are just set without additional manipulations.
+--
+-- For example stdin=true means to open stdin end for write,
+-- stdin=false to close the end, finally stdin[=nil] means to
+-- provide /dev/null into a child peer.
+local popen_opts_tfn = {
+ stdin = {
+ popen_flags.STDIN,
+ popen_flags.STDIN_CLOSE,
+ popen_flags.STDIN_DEVNULL,
+ },
+ stdout = {
+ popen_flags.STDOUT,
+ popen_flags.STDOUT_CLOSE,
+ popen_flags.STDOUT_DEVNULL,
+ },
+ stderr = {
+ popen_flags.STDERR,
+ popen_flags.STDERR_CLOSE,
+ popen_flags.STDERR_DEVNULL,
+ },
+}
+
+--
+-- A map for popen option keys into tf ('true|false') values
+-- where bits are set on 'true' and clear on 'false'.
+local popen_opts_tf = {
+ shell = {
+ popen_flags.SHELL,
+ },
+}
+
+--
+-- Parses flags options from popen_opts_tfn and
+-- popen_opts_tf tables.
+local function parse_popen_flags(epfx, flags, opts)
+ if opts == nil then
+ return flags
+ end
+ for k,v in pairs(opts) do
+ if popen_opts_tfn[k] == nil then
+ if popen_opts_tf[k] == nil then
+ error(string.format("%s: Unknown key %s", epfx, k))
+ end
+ if v == true then
+ flags = bit.bor(flags, popen_opts_tf[k][1])
+ elseif v == false then
+ flags = bit.band(flags, bit.bnot(popen_opts_tf[k][1]))
+ else
+ error(string.format("%s: Unknown value %s", epfx, v))
+ end
+ else
+ if v == true then
+ flags = bit.band(flags, bit.bnot(popen_opts_tfn[k][2]))
+ flags = bit.band(flags, bit.bnot(popen_opts_tfn[k][3]))
+ flags = bit.bor(flags, popen_opts_tfn[k][1])
+ elseif v == false then
+ flags = bit.band(flags, bit.bnot(popen_opts_tfn[k][1]))
+ flags = bit.band(flags, bit.bnot(popen_opts_tfn[k][3]))
+ flags = bit.bor(flags, popen_opts_tfn[k][2])
+ elseif v == "devnull" then
+ flags = bit.band(flags, bit.bnot(popen_opts_tfn[k][1]))
+ flags = bit.band(flags, bit.bnot(popen_opts_tfn[k][2]))
+ flags = bit.bor(flags, popen_opts_tfn[k][3])
+ else
+ error(string.format("%s: Unknown value %s", epfx, v))
+ end
+ end
+ end
+ return flags
+end
+
+--
+-- popen:read2 - read stream of a child with options
+-- @opts: options table
+--
+-- The options should have the following keys
+--
+-- @flags: stdout=true or stderr=true
+-- @buf: const_char_ptr_t buffer
+-- @size: size of the buffer
+-- @timeout_msecs: read timeout in microsecond
+--
+-- Returns @res = bytes, err = nil in case if read processed
+-- without errors, @res = nil, @err = nil if timeout elapsed
+-- and no data appeared on the peer, @res = nil, @err ~= nil
+-- otherwise.
+popen_methods.read2 = function(self, opts)
+ --
+ -- We can reuse parse_popen_flags since only
+ -- a few flags we're interesed in -- stdout/stderr
+ local flags = parse_popen_flags("fio.popen:read2",
+ popen_flags.NONE,
+ opts['flags'])
+ local timeout_msecs = -1
+ if opts['buf'] == nil then
+ error("fio.popen:read2 {'buf'} key is missed")
+ elseif opts['size'] == nil then
+ error("fio.popen:read2 {'size'} key is missed")
+ elseif opts['timeout_msecs'] ~= nil then
+ timeout_msecs = tonumber(opts['timeout_msecs'])
+ end
+
+ return internal.popen_read(self.popen_handle, opts['buf'],
+ tonumber(opts['size']),
+ timeout_msecs, flags)
+end
+
+-- popen:write2 - write data to a child with options
+-- @opts: options table
+--
+-- The options should have the following keys
+--
+-- @flags: stdin=true
+-- @buf: const_char_ptr_t buffer
+-- @size: size of the buffer
+--
+-- Returns @res = bytes, err = nil in case if write processed
+-- without errors, or @res = nil, @err ~= nil otherwise.
+popen_methods.write2 = function(self, opts)
+ local flags = parse_popen_flags("fio.popen:write2",
+ popen_flags.NONE,
+ opts['flags'])
+ if opts['buf'] == nil then
+ error("fio.popen:write2 {'buf'} key is missed")
+ elseif opts['size'] == nil then
+ error("fio.popen:write2 {'size'} key is missed")
+ end
+ return internal.popen_write(self.popen_handle, opts['buf'],
+ tonumber(opts['size']), flags)
+end
+
+--
+-- Write data to stdin of a child process
+-- @buf: a buffer to write
+-- @size: size of the buffer
+--
+-- Both parameters are optional and some
+-- string could be passed instead
+--
+-- write(str)
+-- write(buf, len)
+--
+-- Returns @res = bytes, @err = nil in case of success,
+-- @res = , @err ~= nil in case of error.
+popen_methods.write = function(self, buf, size)
+ if not ffi.istype(const_char_ptr_t, buf) then
+ buf = tostring(buf)
+ size = #buf
+ end
+ return self:write2({ buf = buf, size = size,
+ flags = { stdin = true }})
+end
+
+--
+-- popen:read - read the output stream in blocked mode
+-- @buf: a buffer to read to, optional
+-- @size: size of the buffer, optional
+-- @opts: options table, optional
+--
+-- The options may have the following keys:
+--
+-- @flags: stdout=true or stderr=true,
+-- to specify which stream to read;
+-- by default stdout is used
+-- @timeout_msecs: read timeout in microcesonds, when
+-- specified the read will be interrupted
+-- after specified time elapced, to make
+-- nonblocking read; de default the read
+-- is blocking operation
+--
+-- The following variants are accepted as well
+--
+-- read() -> str
+-- read(buf) -> len
+-- read(size) -> str
+-- read(buf, size) -> len
+--
+-- FIXME: Should we merged it with plain fio.write()?
+popen_methods.read = function(self, buf, size, opts)
+ local tmpbuf
+
+ -- read() or read(buf)
+ if (not ffi.istype(const_char_ptr_t, buf) and buf == nil) or
+ (ffi.istype(const_char_ptr_t, buf) and size == nil) then
+ -- read @self.read_buf_size bytes at once if nothing specified
+ -- FIXME: should it be configurable on popen creation?
+ size = self.read_buf_size
+ end
+
+ -- read(size)
+ if not ffi.istype(const_char_ptr_t, buf) then
+ size = buf or size
+ tmpbuf = buffer.ibuf()
+ buf = tmpbuf:reserve(size)
+ end
+
+ if opts == nil then
+ opts = {
+ timeout_msecs = -1,
+ flags = { stdout = true },
+ }
+ end
+
+ local res, err = self:read2({ buf = buf, size = size,
+ timeout_msecs = opts['timeout_msecs'],
+ flags = opts['flags'] })
+
+ if res == nil then
+ if tmpbuf ~= nil then
+ tmpbuf:recycle()
+ end
+ return nil, err
+ end
+
+ if tmpbuf ~= nil then
+ tmpbuf:alloc(res)
+ res = ffi.string(tmpbuf.rpos, tmpbuf:size())
+ tmpbuf:recycle()
+ end
+
+ return res
+end
+
+--
+-- popen:info -- obtain information about a popen object
+--
+-- Returns @info table, err == nil on success,
+-- @info = nil, @err ~= nil otherwise.
+--
+-- The @info table contains the following keys:
+--
+-- @pid: pid of a child process
+-- @flags: flags associated (popen_flags bitmap)
+-- @stdout: parent peer for stdout, -1 if closed
+-- @stderr: parent peer for stderr, -1 if closed
+-- @stdin: parent peer for stdin, -1 if closed
+-- @state: alive | exited | signaled | unknown
+-- @exit_code: exit code of a child process if been
+-- exited or killed by a signal
+--
+-- The child should never be in "unknown" state, reserved
+-- for unexpected errors.
+--
+popen_methods.info = function(self)
+ return internal.popen_info(self.popen_handle)
+end
+
+local popen_mt = { __index = popen_methods }
+
+--
+-- Parse "mode" string to flags
+local function parse_popen_mode(epfx, flags, mode)
+ if mode == nil then
+ return flags
+ end
+ for i = 1, #mode do
+ local c = mode:sub(i, i)
+ if c == 'r' then
+ flags = bit.band(flags, bit.bnot(popen_flags.STDOUT_CLOSE))
+ flags = bit.bor(flags, popen_flags.STDOUT)
+ elseif c == 'w' then
+ flags = bit.band(flags, bit.bnot(popen_flags.STDIN_CLOSE))
+ flags = bit.bor(flags, popen_flags.STDIN)
+ else
+ error(string.format("%s: Unknown mode %s", epfx, c))
+ end
+ end
+ return flags
+end
+
+--
+-- Create a new popen object from options
+local function popen_new(opts)
+ local handle, err = internal.popen_new(opts)
+ if err ~= nil then
+ return nil, err
+ end
+
+ local popen_handle = {
+ -- a handle itself for future use
+ popen_handle = handle,
+
+ -- sleeping period for the @wait method
+ wait_secs = 1,
+
+ -- size of a read buffer to allocate
+ -- in case of implicit read
+ read_buf_size = 512,
+ }
+
+ setmetatable(popen_handle, popen_mt)
+ return popen_handle
+end
+
+--
+-- fio.popen - create a new child process and execute a command inside
+-- @command: a command to run
+-- @mode: r - to grab child's stdout for read,
+-- w - to obtain stdin for write
+-- @...: optional parameters table:
+--
+-- stdin=true alias for @mode=w
+-- stdin=false to close STDIN_FILENO inside a child process [*]
+-- stdin="devnull" a child will get /dev/null as STDIN_FILENO
+--
+-- stdout=true alias for @mode=r
+-- stdout=false to close STDOUT_FILENO inside a child process [*]
+-- stdin="devnull" a child will get /dev/null as STDOUT_FILENO
+--
+-- stderr=true to read STDERR_FILENO from a child process
+-- stderr=false to close STDERR_FILENO inside a child process [*]
+-- stdin="devnull" a child will get /dev/null as STDERR_FILENO
+--
+-- shell=true runs a child process via "sh -c" [*]
+-- shell=false invokes a child process executable directly
+--
+-- [*] are default values
+--
+-- Examples:
+--
+-- popen = require('fio').popen("date", "r")
+-- runs date as "sh -c date" to read the output,
+-- closing all file descriptors except stdin inside
+-- a child process
+--
+-- popen = require('fio').popen("/usr/bin/date", "r", {shell=false})
+-- invokes date executable without shell to read the output
+--
+fio.popen = function(command, mode, ...)
+ local flags = popen_default_flags()
+
+ if type(command) ~= 'string' then
+ error("Usage: fio.popen(command[, rw, {opts}])")
+ end
+
+ -- Mode gives simplified flags
+ flags = parse_popen_mode("fio.popen", flags, mode)
+
+ -- Options table can override the mode option
+ if ... ~= nil then
+ flags = parse_popen_flags("fio.popen", flags, ...)
+ end
+
+ local opts = {
+ argv = { command },
+ argc = 1,
+ flags = flags,
+ envc = -1,
+ }
+
+ return popen_new(opts)
+end
+
+--
+-- fio.run - spawn a new process and run a command inside
+-- @opt: table of options
+--
+-- @opts carries of the following options:
+--
+-- @argv: an array of a program to run with
+-- command line options, mandatory
+--
+-- @env: an array of environment variables to
+-- be used inside a process, if not
+-- set then the current environment is
+-- inherited, if set to an empty array
+-- then the environment will be dropped
+--
+-- @mode: "r" to read from stdout, "w" to
+-- write to stdin of a process
+--
+-- @flags: a dictionary to describe communication pipes
+-- and other parameters of a process to run
+--
+-- stdin=true to write into STDIN_FILENO of a child process
+-- stdin=false to close STDIN_FILENO inside a child process [*]
+-- stdin="devnull" a child will get /dev/null as STDIN_FILENO
+--
+-- stdout=true to write into STDOUT_FILENO of a child process
+-- stdout=false to close STDOUT_FILENO inside a child process [*]
+-- stdin="devnull" a child will get /dev/null as STDOUT_FILENO
+--
+-- stderr=true to read STDERR_FILENO from a child process
+-- stderr=false to close STDERR_FILENO inside a child process [*]
+-- stdin="devnull" a child will get /dev/null as STDERR_FILENO
+--
+-- shell=true runs a child process via "sh -c" [*]
+-- shell=false invokes a child process executable directly
+--
+-- [*] are default values
+--
+-- Examples:
+--
+-- popen = require('fio').run({argv={"date"},flags={stdout=true}})
+-- popen:read()
+-- popen:close()
+--
+-- Execute 'date' command inside a shell, read the result
+-- and close the popen object
+--
+-- popen = require('fio').run({argv={"/usr/bin/echo", "-n", "hello"},
+-- flags={stdout=true, shell=false}})
+-- popen:read()
+-- popen:close()
+--
+-- Execute /usr/bin/echo with arguments '-n','hello' directly
+-- without using a shell, read the result and close the popen
+-- object
+--
+fio.run = function(opts)
+ local flags = popen_default_flags()
+
+ if opts == nil or type(opts) ~= 'table' then
+ error("Usage: fio.run({argv={},envp={},mode={},flags={}})")
+ end
+
+ -- Test for required arguments
+ if opts["argv"] == nil then
+ error(string.format("fio.run: argv key is missing"))
+ end
+
+ -- Prepare options
+ for k,v in pairs(opts) do
+ if k == "mode" then
+ flags = parse_popen_mode("fio.run", flags, v)
+ elseif k == "argv" then
+ if #v == 0 then
+ error(string.format("fio.run: argv key is empty"))
+ end
+ elseif k == "flags" then
+ flags = parse_popen_flags("fio.run", flags, opts["flags"])
+ end
+ end
+
+ -- Update parsed flags
+ opts["flags"] = flags
+
+ -- We will need a number of args for speed sake
+ opts["argc"] = #opts["argv"]
+
+ -- Same applies to environment (note though
+ -- that env={} is pretty valid and env=nil
+ -- as well.
+ if opts["env"] ~= nil then
+ opts["envc"] = #opts["env"]
+ else
+ opts["envc"] = -1
+ end
+
+ return popen_new(opts)
+end
+
return fio
--
2.20.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Tarantool-patches] [PATCH v2 5/5] test: Add app/popen test
2019-12-10 9:48 [Tarantool-patches] [PATCH v2 0/5] popen: Add ability to run external process Cyrill Gorcunov
` (3 preceding siblings ...)
2019-12-10 9:48 ` [Tarantool-patches] [PATCH v2 4/5] popen/fio: Add ability to run external programs Cyrill Gorcunov
@ 2019-12-10 9:48 ` Cyrill Gorcunov
2019-12-11 9:29 ` [Tarantool-patches] [PATCH v2 0/5] popen: Add ability to run external process Cyrill Gorcunov
5 siblings, 0 replies; 10+ messages in thread
From: Cyrill Gorcunov @ 2019-12-10 9:48 UTC (permalink / raw)
To: tml
To test basic functionality of popen code.
Fixes #4031
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
test/app/popen.result | 179 ++++++++++++++++++++++++++++++++++++++++
test/app/popen.test.lua | 68 +++++++++++++++
2 files changed, 247 insertions(+)
create mode 100644 test/app/popen.result
create mode 100644 test/app/popen.test.lua
diff --git a/test/app/popen.result b/test/app/popen.result
new file mode 100644
index 000000000..99efbe512
--- /dev/null
+++ b/test/app/popen.result
@@ -0,0 +1,179 @@
+-- test-run result file version 2
+fio = require('fio')
+ | ---
+ | ...
+buffer = require('buffer')
+ | ---
+ | ...
+ffi = require('ffi')
+ | ---
+ | ...
+
+test_run = require('test_run').new()
+ | ---
+ | ...
+
+local err, reason, exit_code
+ | ---
+ | ...
+buf = buffer.ibuf()
+ | ---
+ | ...
+
+--
+-- Trivial echo output
+--
+script = "echo -n 1 2 3 4 5"
+ | ---
+ | ...
+popen = fio.popen(script, "r")
+ | ---
+ | ...
+popen:wait() -- wait echo to finish
+ | ---
+ | - null
+ | - 1
+ | - 0
+ | ...
+popen:read() -- read the output
+ | ---
+ | - 1 2 3 4 5
+ | ...
+popen:close() -- release the popen
+ | ---
+ | - true
+ | ...
+
+--
+-- Test info and killing of a child process
+--
+script = "while [ 1 ]; do sleep 10; done"
+ | ---
+ | ...
+popen = fio.popen(script, "r")
+ | ---
+ | ...
+popen:kill()
+ | ---
+ | - true
+ | ...
+--
+-- Killing child may be queued and depends on
+-- system load, so we may get ESRCH here.
+err, reason, exit_code = popen:wait()
+ | ---
+ | ...
+popen:state()
+ | ---
+ | - null
+ | - 3
+ | - 9
+ | ...
+info = popen:info()
+ | ---
+ | ...
+info["state"]
+ | ---
+ | - signaled
+ | ...
+info["flags"]
+ | ---
+ | - 834
+ | ...
+info["exit_code"]
+ | ---
+ | - 9
+ | ...
+popen:close()
+ | ---
+ | - true
+ | ...
+
+--
+-- Test for stdin/out stream
+--
+script="prompt=''; read -n 5 prompt; echo -n $prompt"
+ | ---
+ | ...
+popen = fio.popen(script, "rw")
+ | ---
+ | ...
+popen:write("input")
+ | ---
+ | - 5
+ | ...
+popen:read()
+ | ---
+ | - input
+ | ...
+popen:close()
+ | ---
+ | - true
+ | ...
+
+--
+-- Test reading stderr (simply redirect stdout to stderr)
+--
+script = "echo -n 1 2 3 4 5 1>&2"
+ | ---
+ | ...
+popen = fio.popen(script, "rw", {stderr = true})
+ | ---
+ | ...
+popen:wait()
+ | ---
+ | - null
+ | - 1
+ | - 0
+ | ...
+size = 128
+ | ---
+ | ...
+dst = buf:reserve(size)
+ | ---
+ | ...
+res, err = popen:read2({buf = dst, size = size, nil, flags = {stderr = true}})
+ | ---
+ | ...
+res = buf:alloc(res)
+ | ---
+ | ...
+ffi.string(buf.rpos, buf:size())
+ | ---
+ | - 1 2 3 4 5
+ | ...
+buf:recycle()
+ | ---
+ | ...
+popen:close()
+ | ---
+ | - true
+ | ...
+
+--
+-- Test timeouts: just wait for 10 microseconds
+-- to elapse, then write data and re-read for sure.
+--
+script = "prompt=''; read -n 5 prompt && echo -n $prompt;"
+ | ---
+ | ...
+popen = fio.popen(script, "rw")
+ | ---
+ | ...
+popen:read(nil, nil, {timeout_msecs = 10, flags = {stdout = true}})
+ | ---
+ | - null
+ | - null
+ | ...
+popen:write("input")
+ | ---
+ | - 5
+ | ...
+popen:read()
+ | ---
+ | - input
+ | ...
+popen:close()
+ | ---
+ | - true
+ | ...
diff --git a/test/app/popen.test.lua b/test/app/popen.test.lua
new file mode 100644
index 000000000..b6c8cbbba
--- /dev/null
+++ b/test/app/popen.test.lua
@@ -0,0 +1,68 @@
+fio = require('fio')
+buffer = require('buffer')
+ffi = require('ffi')
+
+test_run = require('test_run').new()
+
+local err, reason, exit_code
+buf = buffer.ibuf()
+
+--
+-- Trivial echo output
+--
+script = "echo -n 1 2 3 4 5"
+popen = fio.popen(script, "r")
+popen:wait() -- wait echo to finish
+popen:read() -- read the output
+popen:close() -- release the popen
+
+--
+-- Test info and killing of a child process
+--
+script = "while [ 1 ]; do sleep 10; done"
+popen = fio.popen(script, "r")
+popen:kill()
+--
+-- Killing child may be queued and depends on
+-- system load, so we may get ESRCH here.
+err, reason, exit_code = popen:wait()
+popen:state()
+info = popen:info()
+info["state"]
+info["flags"]
+info["exit_code"]
+popen:close()
+
+--
+-- Test for stdin/out stream
+--
+script="prompt=''; read -n 5 prompt; echo -n $prompt"
+popen = fio.popen(script, "rw")
+popen:write("input")
+popen:read()
+popen:close()
+
+--
+-- Test reading stderr (simply redirect stdout to stderr)
+--
+script = "echo -n 1 2 3 4 5 1>&2"
+popen = fio.popen(script, "rw", {stderr = true})
+popen:wait()
+size = 128
+dst = buf:reserve(size)
+res, err = popen:read2({buf = dst, size = size, nil, flags = {stderr = true}})
+res = buf:alloc(res)
+ffi.string(buf.rpos, buf:size())
+buf:recycle()
+popen:close()
+
+--
+-- Test timeouts: just wait for 10 microseconds
+-- to elapse, then write data and re-read for sure.
+--
+script = "prompt=''; read -n 5 prompt && echo -n $prompt;"
+popen = fio.popen(script, "rw")
+popen:read(nil, nil, {timeout_msecs = 10, flags = {stdout = true}})
+popen:write("input")
+popen:read()
+popen:close()
--
2.20.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 0/5] popen: Add ability to run external process
2019-12-10 9:48 [Tarantool-patches] [PATCH v2 0/5] popen: Add ability to run external process Cyrill Gorcunov
` (4 preceding siblings ...)
2019-12-10 9:48 ` [Tarantool-patches] [PATCH v2 5/5] test: Add app/popen test Cyrill Gorcunov
@ 2019-12-11 9:29 ` Cyrill Gorcunov
5 siblings, 0 replies; 10+ messages in thread
From: Cyrill Gorcunov @ 2019-12-11 9:29 UTC (permalink / raw)
To: tml
On Tue, Dec 10, 2019 at 12:48:50PM +0300, Cyrill Gorcunov wrote:
> In this series we provide a way to execute external binaries
> from inside of Lua scripts, control children process and
> communicate with their stdin/out/err streams.
Drop this series please. I've just sent out v3.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/5] popen: Introduce a backend engine
2019-12-10 9:48 ` [Tarantool-patches] [PATCH v2 1/5] popen: Introduce a backend engine Cyrill Gorcunov
@ 2019-12-26 4:33 ` Konstantin Osipov
2019-12-26 7:04 ` Cyrill Gorcunov
0 siblings, 1 reply; 10+ messages in thread
From: Konstantin Osipov @ 2019-12-26 4:33 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tml
> +/**
> + * command_new - allocates a command string from argv array
Would be nice to say why you need to linearise the command at all
- is it for logging, or error messages, or what?
> + * @argv: an array with pointers to argv strings
> + * @nr_argv: number of elements in the @argv
> + *
> + * Returns a new string or NULL on error.
> + */
> +static inline char *
> +command_new(char **argv, size_t nr_argv)
_new/_delete are usually used for classes/objects. command is not
a standalone class, so a better name for the function is
alloc_argv or similar.
having a separate command_free(0) IMO is over-engineering, as well
as separate handle_free and popen_delete(). I would inline
handle_free() and popen_delete() into popen, as well as
handle_new(). If not, I would at least move all free/destroy
functions close together, so that the code is easier to make ends
of - right now popen_delete() as at the end of a long file, while
command_new/handle_new - at the beginnign.
> +ssize_t
> +popen_write(struct popen_handle *handle, void *buf,
> + size_t count, unsigned int flags)
> +{
> + if (!popen_may_io(handle, STDIN_FILENO, flags))
> + return -1;
> +
> + if (count > (size_t)SSIZE_MAX) {
> + errno = E2BIG;
> + return -1;
> + }
> +
> + say_debug("popen: %d: write idx [%s:%d] buf %p count %zu",
> + handle->pid, stdX_str(STDIN_FILENO),
> + STDIN_FILENO, buf, count);
> +
> + return write(handle->fds[STDIN_FILENO], buf, count);
> +}
I think popen_write() should work like follows:
while (not error and not written the full contents of the buffer)
{
rc = write()
// handle errors
// advance write position
// if written_size != buf_size coio_fiber_yield_timeout() until the descriptor
// becomes ready.
}
For that to work, the descriptor should be set to non-blocking on
parent side right after fork.
Why are you allowing a partial write here? Why are you not
accepting an optional timeout?
> + */
> +static int
> +popen_wait_read(struct popen_handle *handle, int idx, int timeout_msecs)
> +{
> + struct pollfd pollfd = {
> + .fd = handle->fds[idx],
> + .events = POLLIN,
> + };
> + int ret;
> +
> + ret = poll(&pollfd, 1, timeout_msecs);
Here you block the event loop for timeout_msecs. Why aren't you
using coio_fiber_yield_timeout()?
The timeout should be in ev_tstamp format, not integer.
popen_read(), similar to popen_write() should be reading the
requested amount or error, not return a partial read.
> +#else
> + /* FIXME: What about FreeBSD/MachO?
freebsd has fdsecfs
mac has proc_pidinfo()
>
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/5] popen: Introduce a backend engine
2019-12-26 4:33 ` Konstantin Osipov
@ 2019-12-26 7:04 ` Cyrill Gorcunov
2019-12-26 7:12 ` Konstantin Osipov
0 siblings, 1 reply; 10+ messages in thread
From: Cyrill Gorcunov @ 2019-12-26 7:04 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: tml
On Thu, Dec 26, 2019 at 07:33:54AM +0300, Konstantin Osipov wrote:
> > +/**
> > + * command_new - allocates a command string from argv array
>
> Would be nice to say why you need to linearise the command at all
> - is it for logging, or error messages, or what?
ok
> > + * @argv: an array with pointers to argv strings
> > + * @nr_argv: number of elements in the @argv
> > + *
> > + * Returns a new string or NULL on error.
> > + */
> > +static inline char *
> > +command_new(char **argv, size_t nr_argv)
>
> _new/_delete are usually used for classes/objects. command is not
> a standalone class, so a better name for the function is
> alloc_argv or similar.
sure, will do
> having a separate command_free(0) IMO is over-engineering, as well
> as separate handle_free and popen_delete(). I would inline
> handle_free() and popen_delete() into popen, as well as
> handle_new(). If not, I would at least move all free/destroy
> functions close together, so that the code is easier to make ends
> of - right now popen_delete() as at the end of a long file, while
> command_new/handle_new - at the beginnign.
ok, will do
> > +ssize_t
> > +popen_write(struct popen_handle *handle, void *buf,
> > + size_t count, unsigned int flags)
> > +{
> > + if (!popen_may_io(handle, STDIN_FILENO, flags))
> > + return -1;
> > +
> > + if (count > (size_t)SSIZE_MAX) {
> > + errno = E2BIG;
> > + return -1;
> > + }
> > +
> > + say_debug("popen: %d: write idx [%s:%d] buf %p count %zu",
> > + handle->pid, stdX_str(STDIN_FILENO),
> > + STDIN_FILENO, buf, count);
> > +
> > + return write(handle->fds[STDIN_FILENO], buf, count);
> > +}
>
> I think popen_write() should work like follows:
>
> while (not error and not written the full contents of the buffer)
> {
> rc = write()
> // handle errors
> // advance write position
> // if written_size != buf_size coio_fiber_yield_timeout() until the descriptor
> // becomes ready.
> }
>
> For that to work, the descriptor should be set to non-blocking on
> parent side right after fork.
>
> Why are you allowing a partial write here? Why are you not
> accepting an optional timeout?
Because it is v2 of the series, which is obsolete. In v6
we already process timeouts.
>
> > + */
> > +static int
> > +popen_wait_read(struct popen_handle *handle, int idx, int timeout_msecs)
> > +{
> > + struct pollfd pollfd = {
> > + .fd = handle->fds[idx],
> > + .events = POLLIN,
> > + };
> > + int ret;
> > +
> > + ret = poll(&pollfd, 1, timeout_msecs);
>
> Here you block the event loop for timeout_msecs. Why aren't you
> using coio_fiber_yield_timeout()?
>
> The timeout should be in ev_tstamp format, not integer.
Already addressed in v6
> popen_read(), similar to popen_write() should be reading the
> requested amount or error, not return a partial read.
>
> > +#else
> > + /* FIXME: What about FreeBSD/MachO?
>
> freebsd has fdsecfs
> mac has proc_pidinfo()
Thanks a lot for review and this info about freebsd/macho, Kostya!
As to fdsecfs/proc_pidinfo -- I simply don't have these machines
to test on. I think we will address these platforms on top of the
series.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/5] popen: Introduce a backend engine
2019-12-26 7:04 ` Cyrill Gorcunov
@ 2019-12-26 7:12 ` Konstantin Osipov
0 siblings, 0 replies; 10+ messages in thread
From: Konstantin Osipov @ 2019-12-26 7:12 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tml
* Cyrill Gorcunov <gorcunov@gmail.com> [19/12/26 10:05]:
> > The timeout should be in ev_tstamp format, not integer.
>
> Already addressed in v6
Right, because you keep adding me to To: when you post, so the
patch is routed outside the mailing list.
I will take a look at v6
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-12-26 7:12 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-10 9:48 [Tarantool-patches] [PATCH v2 0/5] popen: Add ability to run external process Cyrill Gorcunov
2019-12-10 9:48 ` [Tarantool-patches] [PATCH v2 1/5] popen: Introduce a backend engine Cyrill Gorcunov
2019-12-26 4:33 ` Konstantin Osipov
2019-12-26 7:04 ` Cyrill Gorcunov
2019-12-26 7:12 ` Konstantin Osipov
2019-12-10 9:48 ` [Tarantool-patches] [PATCH v2 2/5] lua/fio: Add lbox_fio_push_error as a separate helper Cyrill Gorcunov
2019-12-10 9:48 ` [Tarantool-patches] [PATCH v2 3/5] popen/fio: Merge popen engine into fio internal module Cyrill Gorcunov
2019-12-10 9:48 ` [Tarantool-patches] [PATCH v2 4/5] popen/fio: Add ability to run external programs Cyrill Gorcunov
2019-12-10 9:48 ` [Tarantool-patches] [PATCH v2 5/5] test: Add app/popen test Cyrill Gorcunov
2019-12-11 9:29 ` [Tarantool-patches] [PATCH v2 0/5] popen: Add ability to run external process Cyrill Gorcunov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox