[Tarantool-patches] [PATCH 00/13] Popen Lua API: preliminary patches

[Tarantool-patches] [PATCH 00/13] Popen Lua API: preliminary patches

[Alexander Turenko]

https://github.com/tarantool/tarantool/issues/4031
https://github.com/tarantool/tarantool/tree/Totktonada/gh-4031-popen-lua-api-preparation

I working on Lua API for popen. During the development I found that
several changes are necessary for the underlying popen engine. This
patch series accumulates them.

Here are bugfixes, API and behaviour changes. Hope they are explained
enough in the commit messages.

Please, give more attention to the 'popen: decouple logger fd from
stderr' patch: it is easy to make a mistake with those fds juggling and
I have very sparse time to test it as it deserves (I made manual testing
for the described cases however).

Those changes can be partially tested via Lua API draft. It is on my
temporary branch Totktonada/gh-4031-popen-12-workbranch (see also
test/app-tap/popen.test.lua on the branch).

Alexander Turenko (13):
  popen: require popen handle to be non-NULL
  popen: remove retval from popen_state()
  popen: add missed diag_set in popen_signal/delete
  popen: add logging of fds closed in a child
  say: allow to set a logger file descriptor
  popen: decouple logger fd from stderr
  popen: add const qualifier to popen_write_timeout
  popen: unblock popen_read_timeout at a first byte
  popen: remove redundant fd check before perform IO
  popen: add missed diag_set() in popen IO functions
  coio: fix obsoleted comment in coio_write_timeout
  coio: add *_noxc read / write functions
  popen: use of exception safe functions for IO

 src/lib/core/coio.cc   |  43 +++++-
 src/lib/core/coio.h    |  41 ++++++
 src/lib/core/popen.c   | 320 ++++++++++++++++++++++++++++++-----------
 src/lib/core/popen.h   |   4 +-
 src/lib/core/say.c     |   6 +
 src/lib/core/say.h     |   7 +-
 test/unit/popen.c      |   8 +-
 test/unit/popen.result |  13 +-
 8 files changed, 343 insertions(+), 99 deletions(-)

-- 
2.25.0

[Tarantool-patches] [PATCH 01/13] popen: require popen handle to be non-NULL

[Alexander Turenko]

Further commits will add proper entries into the diagnostics area for
failures inside popen functions. We should either report handle == NULL
case via the diagnostics area or ensure that the NULL handle case is not
possible.

The latter approach is implemented in this commit. There are two
reasons for this:

* This way simplifies function contracts (one less kind of failure).
* The popen Lua module (that will be implemented in the further commits)
  will not construct any logic using NULL as a handle. When 'NULL
  handle' error is not possible in the C API, it will be easier to
  verify that this failure is not possible the Lua API.

A user of the C API should take care to don't call those functions with
NULL handle.

Part of #4031
---
 src/lib/core/popen.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 6b6062215..5f74bc3ce 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -163,13 +163,13 @@ popen_may_io(struct popen_handle *handle, unsigned int idx,
 }
 
 /**
- * Test if the handle is not nil and still have
- * a living child process.
+ * Test if the handle still have a living child process.
  */
 static inline bool
 popen_may_pidop(struct popen_handle *handle)
 {
-	if (!handle || handle->pid == -1) {
+	assert(handle != NULL);
+	if (handle->pid == -1) {
 		errno = ESRCH;
 		return false;
 	}
@@ -182,10 +182,7 @@ popen_may_pidop(struct popen_handle *handle)
 int
 popen_stat(struct popen_handle *handle, struct popen_stat *st)
 {
-	if (!handle) {
-		errno = ESRCH;
-		return -1;
-	}
+	assert(handle != NULL);
 
 	st->pid		= handle->pid;
 	st->flags	= handle->flags;
@@ -204,11 +201,7 @@ popen_stat(struct popen_handle *handle, struct popen_stat *st)
 const char *
 popen_command(struct popen_handle *handle)
 {
-	if (!handle) {
-		errno = ESRCH;
-		return NULL;
-	}
-
+	assert(handle != NULL);
 	return (const char *)handle->command;
 }
 
@@ -236,6 +229,8 @@ popen_write_timeout(struct popen_handle *handle, void *buf,
 		    size_t count, unsigned int flags,
 		    ev_tstamp timeout)
 {
+	assert(handle != NULL);
+
 	int idx = STDIN_FILENO;
 
 	if (!(flags & POPEN_FLAG_FD_STDIN)) {
@@ -268,6 +263,8 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
 		   size_t count, unsigned int flags,
 		   ev_tstamp timeout)
 {
+	assert(handle != NULL);
+
 	int idx = flags & POPEN_FLAG_FD_STDOUT ?
 		STDOUT_FILENO : STDERR_FILENO;
 
@@ -380,10 +377,7 @@ popen_sigchld_handler(EV_P_ ev_child *w, int revents)
 int
 popen_state(struct popen_handle *handle, int *state, int *exit_code)
 {
-	if (!handle) {
-		errno = ESRCH;
-		return -1;
-	}
+	assert(handle != NULL);
 
 	if (handle->pid != -1) {
 		*state = POPEN_STATE_ALIVE;
@@ -437,6 +431,8 @@ popen_send_signal(struct popen_handle *handle, int signo)
 {
 	int ret;
 
+	assert(handle != NULL);
+
 	/*
 	 * A child may be killed or exited already.
 	 */
@@ -464,10 +460,7 @@ popen_delete(struct popen_handle *handle)
 {
 	size_t i;
 
-	if (!handle) {
-		errno = ESRCH;
-		return -1;
-	}
+	assert(handle != NULL);
 
 	if (popen_send_signal(handle, SIGKILL) && errno != ESRCH)
 		return -1;
-- 
2.25.0

[Tarantool-patches] [PATCH 02/13] popen: remove retval from popen_state()

[Alexander Turenko]

After the previous commit ('popen: require popen handle to be non-NULL')
it turns out that popen_state() function always succeeds. There is no
reason to return a success / failure value from it.

Part of #4031
---
 src/lib/core/popen.c   |  4 +---
 src/lib/core/popen.h   |  2 +-
 test/unit/popen.c      |  8 +++-----
 test/unit/popen.result | 13 ++++++-------
 4 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 5f74bc3ce..3a0ac4b53 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -374,7 +374,7 @@ popen_sigchld_handler(EV_P_ ev_child *w, int revents)
 /**
  * Get current child state.
  */
-int
+void
 popen_state(struct popen_handle *handle, int *state, int *exit_code)
 {
 	assert(handle != NULL);
@@ -391,8 +391,6 @@ popen_state(struct popen_handle *handle, int *state, int *exit_code)
 			*exit_code = WTERMSIG(handle->wstatus);
 		}
 	}
-
-	return 0;
 }
 
 /**
diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h
index 9cbfed60c..587d6f7df 100644
--- a/src/lib/core/popen.h
+++ b/src/lib/core/popen.h
@@ -163,7 +163,7 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
 		   size_t count, unsigned int flags,
 		   ev_tstamp timeout);
 
-extern int
+extern void
 popen_state(struct popen_handle *handle, int *state, int *exit_code);
 
 extern const char *
diff --git a/test/unit/popen.c b/test/unit/popen.c
index caea9d552..b71e5485b 100644
--- a/test/unit/popen.c
+++ b/test/unit/popen.c
@@ -26,8 +26,7 @@ static int
 wait_exit(struct popen_handle *handle, int *state, int *exit_code)
 {
 	for (;;) {
-		if (popen_state(handle, state, exit_code))
-			return -1;
+		popen_state(handle, state, exit_code);
 		if (*state == POPEN_STATE_EXITED ||
 		    *state == POPEN_STATE_SIGNALED)
 			break;
@@ -61,7 +60,7 @@ popen_write_exit(void)
 	};
 	int rc;
 
-	plan(7);
+	plan(6);
 	header();
 
 	handle = popen_new(&opts);
@@ -69,8 +68,7 @@ popen_write_exit(void)
 	if (handle == NULL)
 		goto out;
 
-	rc = popen_state(handle, &state, &exit_code);
-	ok(rc == 0, "popen_state");
+	popen_state(handle, &state, &exit_code);
 
 	ok(state == POPEN_STATE_ALIVE, "state %s",
 	   popen_state_str(state));
diff --git a/test/unit/popen.result b/test/unit/popen.result
index d7894d1db..f5e6bc2ca 100644
--- a/test/unit/popen.result
+++ b/test/unit/popen.result
@@ -1,14 +1,13 @@
 1..3
 	*** main_f ***
-    1..7
+    1..6
 	*** popen_write_exit ***
     ok 1 - popen_new
-    ok 2 - popen_state
-    ok 3 - state alive
-    ok 4 - write flag check
-    ok 5 - write to pipe
-    ok 6 - child exited
-    ok 7 - popen_delete
+    ok 2 - state alive
+    ok 3 - write flag check
+    ok 4 - write to pipe
+    ok 5 - child exited
+    ok 6 - popen_delete
 	*** popen_write_exit: done ***
 ok 1 - subtests
     1..5
-- 
2.25.0

[Tarantool-patches] [PATCH 03/13] popen: add missed diag_set in popen_signal/delete

[Alexander Turenko]

Lua API will use content of the diagnostics area to report an error to a
caller, so it is critical to always have proper diagnostics at failure.

Part of #4031
---
 src/lib/core/popen.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 3a0ac4b53..3fcbc325a 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -164,16 +164,19 @@ popen_may_io(struct popen_handle *handle, unsigned int idx,
 
 /**
  * Test if the handle still have a living child process.
+ *
+ * Return -1 and set errno to ESRCH when a process does not
+ * exist. Otherwise return 0.
  */
-static inline bool
+static inline int
 popen_may_pidop(struct popen_handle *handle)
 {
 	assert(handle != NULL);
 	if (handle->pid == -1) {
 		errno = ESRCH;
-		return false;
+		return -1;
 	}
-	return true;
+	return 0;
 }
 
 /**
@@ -423,6 +426,10 @@ popen_state_str(unsigned int state)
 
 /**
  * Send a signal to a child process.
+ *
+ * Return 0 at success or -1 at failure (and set a diag).
+ *
+ * Set errno to ESRCH when a process does not exist.
  */
 int
 popen_send_signal(struct popen_handle *handle, int signo)
@@ -434,16 +441,22 @@ popen_send_signal(struct popen_handle *handle, int signo)
 	/*
 	 * A child may be killed or exited already.
 	 */
-	if (!popen_may_pidop(handle))
+	ret = popen_may_pidop(handle);
+	if (ret == 0) {
+		say_debug("popen: kill %d signo %d", handle->pid, signo);
+		assert(handle->pid != -1);
+		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;
-
-	say_debug("popen: kill %d signo %d", handle->pid, signo);
-	ret = kill(handle->pid, signo);
-	if (ret < 0) {
+	} else if (ret < 0) {
 		diag_set(SystemError, "Unable to kill %d signo %d",
 			 handle->pid, signo);
+		return -1;
 	}
-	return ret;
+	return 0;
 }
 
 /**
@@ -460,7 +473,11 @@ popen_delete(struct popen_handle *handle)
 
 	assert(handle != NULL);
 
-	if (popen_send_signal(handle, SIGKILL) && errno != ESRCH)
+	/*
+	 * 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++) {
-- 
2.25.0

[Tarantool-patches] [PATCH 04/13] popen: add logging of fds closed in a child

[Alexander Turenko]

It is useful for debugging popen behaviour around file descriptors.

Part of #4031
---
 src/lib/core/popen.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 3fcbc325a..9d4e6ef3a 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -579,6 +579,8 @@ close_inherited_fds(int *skip_fds, size_t nr_skip_fds)
 		if (fd_no == -1)
 			continue;
 
+		say_debug("popen: close inherited fd [%s:%d]", stdX_str(fd_no),
+			  fd_no);
 		if (close(fd_no)) {
 			int saved_errno = errno;
 			diag_set(SystemError, "fdin: Can't close %d", fd_no);
-- 
2.25.0

[Tarantool-patches] [PATCH 05/13] say: allow to set a logger file descriptor

[Alexander Turenko]

It is necessary to decouple stderr from a logger file descriptor in the
popen implementation.

Part of #4031
---
 src/lib/core/say.c | 6 ++++++
 src/lib/core/say.h | 7 ++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/lib/core/say.c b/src/lib/core/say.c
index dd05285a6..5d572e1ab 100644
--- a/src/lib/core/say.c
+++ b/src/lib/core/say.c
@@ -174,6 +174,12 @@ log_get_fd(void)
 	return log_default->fd;
 }
 
+void
+log_set_fd(int new_fd)
+{
+	log_default->fd = new_fd;
+}
+
 void
 log_set_level(struct log *log, enum say_level level)
 {
diff --git a/src/lib/core/say.h b/src/lib/core/say.h
index e17de659c..3ce7724c4 100644
--- a/src/lib/core/say.h
+++ b/src/lib/core/say.h
@@ -204,10 +204,15 @@ enum say_logger_type
 log_type();
 
 /**
- * Default logger file descriptor.
+ * Accessors for default logger file descriptor.
+ *
+ * It is needed for dark magic inside popen implementation.
+ * Unlikely it is what you want to use anywhere else.
  */
 int
 log_get_fd(void);
+void
+log_set_fd(int new_fd);
 
 /**
  * Set log level. Can be used dynamically.
-- 
2.25.0

[Tarantool-patches] [PATCH 06/13] popen: decouple logger fd from stderr

[Alexander Turenko]

The default logger configuration writes logs to stderr.

Popen implementation holds a logger fd until execve() to be able to
write debug entries or information about a failure from a child. However
when popen flags requires to close stderr in the child, the logger fd
becomes closed: logging will fail.

Another problem appears when a user want to capture stderr and
tarantool's log level is set to debug (7). Since the logger uses stderr
and it is fed to the parent using a pipe, the logger output will not
shown on the 'real' stderr, but will be captured together with child's
program debugging output.

This commit duplicates a logger file descriptor that allows to close or
redirect child's stderr without described side effects.

See also 86ec3a5c4792ea1bba9f644da1e8f301314c8d29 ('popen: add logging
in child process').

Areas for improvements:

* Copy logger fd at module initialization time instead of copying of
  each popen call.

Alternatives:

* Extend logger to allow to accumulate log entries in a buffer. Flush
  the buffer from the parent process. (It is possible since vfork does
  not split a virtual memory space).

Part of #4031
---
 src/lib/core/popen.c | 124 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 115 insertions(+), 9 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 9d4e6ef3a..62920e0c8 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -74,6 +74,59 @@ popen_unregister(struct popen_handle *handle)
 	mh_i32ptr_remove(popen_pids_map, &node, NULL);
 }
 
+/**
+ * Duplicate a file descriptor, but not to std{in,out,err}.
+ *
+ * Return a new fd at success, otherwise return -1 and set a diag.
+ */
+static int
+dup_not_std_streams(int fd)
+{
+	int res = -1;
+	int save_errno = 0;
+
+	/*
+	 * We will call dup() in a loop until it will return
+	 * fd > STDERR_FILENO. The array `discarded_fds` stores
+	 * intermediate fds to close them after all dup() calls.
+	 */
+	static_assert(STDERR_FILENO + 1 == 3,
+		      "Unexpected STDERR_FILENO constant");
+	int discarded_fds[STDERR_FILENO + 1] = {-1, -1, -1};
+
+	for (size_t i = 0; i < lengthof(discarded_fds) + 1; ++i) {
+		int new_fd = dup(fd);
+		if (new_fd < 0) {
+			save_errno = errno;
+			break;
+		}
+
+		/* Found wanted fd. */
+		if (new_fd > STDERR_FILENO) {
+			res = new_fd;
+			break;
+		}
+
+		/* Save to close then. */
+		assert(i < lengthof(discarded_fds));
+		discarded_fds[i] = new_fd;
+	}
+
+	/* Close all intermediate fds (if any). */
+	for (size_t i = 0; i < lengthof(discarded_fds); ++i)
+		if (discarded_fds[i] >= 0)
+			close(discarded_fds[i]);
+
+	/* Report an error if it occurs. */
+	if (res < 0) {
+		errno = save_errno;
+		diag_set(SystemError, "Unable to duplicate an fd %d", fd);
+		return -1;
+	}
+
+	return res;
+}
+
 /**
  * Allocate new popen hanldle with flags specified.
  */
@@ -746,9 +799,34 @@ popen_new(struct popen_opts *opts)
 	 * plus dev/null variants and logfd
 	 */
 	int skip_fds[POPEN_FLAG_FD_STDEND_BIT * 2 + 2 + 1];
-	int log_fd = log_get_fd();
 	size_t nr_skip_fds = 0;
 
+	/*
+	 * We must decouple log file descriptor from stderr in order to
+	 * close or redirect stderr, but keep logging as is until
+	 * execve() call.
+	 *
+	 * The new file descriptor should not have the same number as
+	 * stdin, stdout or stderr.
+	 *
+	 * NB: It is better to acquire it from the parent to catch
+	 * possible error sooner and don't ever call vfork() if we
+	 * reached a file descriptor limit.
+	 */
+	int old_log_fd = log_get_fd();
+	int log_fd = -1;
+	if (old_log_fd >= 0) {
+		log_fd = dup_not_std_streams(old_log_fd);
+		if (log_fd < 0)
+			return NULL;
+		if (fcntl(log_fd, F_SETFL, O_CLOEXEC) != 0) {
+			diag_set(SystemError,
+				 "Unable to set O_CLOEXEC on temporary logfd");
+			close(log_fd);
+			return NULL;
+		}
+	}
+
 	/*
 	 * A caller must preserve space for this.
 	 */
@@ -771,8 +849,11 @@ popen_new(struct popen_opts *opts)
 		      "Popen flags do not match stdX");
 
 	handle = handle_new(opts);
-	if (!handle)
+	if (!handle) {
+		if (log_fd >= 0)
+			close(log_fd);
 		return NULL;
+	}
 
 	if (log_fd >= 0)
 		skip_fds[nr_skip_fds++] = log_fd;
@@ -839,6 +920,20 @@ popen_new(struct popen_opts *opts)
 		 * do anything special.
 		 */
 
+		/*
+		 * Replace the logger fd to its duplicate. It
+		 * should be done before we'll close inherited
+		 * fds: old logger fd may be stderr and stderr may
+		 * be subject to close.
+		 *
+		 * We should also do it before a first call to a
+		 * say_*() function, because otherwise a user may
+		 * capture our debug logs as stderr of the child
+		 * process.
+		 */
+		if (log_fd >= 0)
+			log_set_fd(log_fd);
+
 		/*
 		 * Also don't forget to drop signal handlers
 		 * to default inside a child process since we're
@@ -944,19 +1039,21 @@ popen_new(struct popen_opts *opts)
 			goto exit_child;
 		}
 
-		if (log_fd >= 0) {
-			if (close(log_fd)) {
-				say_error("child: can't close logfd %d",
-					  log_fd);
-				goto exit_child;
-			}
-		}
+		/*
+		 * Return the logger back, because we're in the
+		 * same virtual memory address space as the
+		 * parent.
+		 */
+		if (log_fd >= 0)
+			log_set_fd(old_log_fd);
 
 		if (opts->flags & POPEN_FLAG_SHELL)
 			execve(_PATH_BSHELL, opts->argv, envp);
 		else
 			execve(opts->argv[0], opts->argv, envp);
 exit_child:
+		if (log_fd >= 0)
+			log_set_fd(old_log_fd);
 		_exit(errno);
 		unreachable();
 	}
@@ -987,6 +1084,13 @@ exit_child:
 		}
 	}
 
+	/* Close the temporary logger fd. */
+	if (log_fd >= 0 && close(log_fd) != 0) {
+		diag_set(SystemError, "Can't close temporary logfd %d", log_fd);
+		log_fd = -1;
+		goto out_err;
+	}
+
 	/*
 	 * Link it into global list for force
 	 * cleanup on exit. Note we use this
@@ -1018,6 +1122,8 @@ out_err:
 		if (pfd[i][1] != -1)
 			close(pfd[i][1]);
 	}
+	if (log_fd >= 0)
+		close(log_fd);
 	errno = saved_errno;
 	return NULL;
 }
-- 
2.25.0

[Tarantool-patches] [PATCH 07/13] popen: add const qualifier to popen_write_timeout

[Alexander Turenko]

The buffer is for reading, we're not intend to change it.

Part of #4031
---
 src/lib/core/popen.c | 2 +-
 src/lib/core/popen.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 62920e0c8..dbc700287 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -281,7 +281,7 @@ stdX_str(unsigned int index)
  * Write data to the child stdin.
  */
 int
-popen_write_timeout(struct popen_handle *handle, void *buf,
+popen_write_timeout(struct popen_handle *handle, const void *buf,
 		    size_t count, unsigned int flags,
 		    ev_tstamp timeout)
 {
diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h
index 587d6f7df..97c581c13 100644
--- a/src/lib/core/popen.h
+++ b/src/lib/core/popen.h
@@ -154,7 +154,7 @@ extern const char *
 popen_command(struct popen_handle *handle);
 
 extern int
-popen_write_timeout(struct popen_handle *handle, void *buf,
+popen_write_timeout(struct popen_handle *handle, const void *buf,
 		    size_t count, unsigned int flags,
 		    ev_tstamp timeout);
 
-- 
2.25.0

[Tarantool-patches] [PATCH 08/13] popen: unblock popen_read_timeout at a first byte

[Alexander Turenko]

Before this change popen_read_timeout() waits until a passed buffer will
be fully filled (or until EOF / timeout / IO error occurs). Now it waits
for any amount of data (but at least one byte).

It allows to communicate with an interactive child program: write, read,
repeat.

Part of #4031
---
 src/lib/core/popen.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index dbc700287..660ace0d5 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -345,8 +345,8 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
 		  handle->pid, stdX_str(idx), idx, buf, count,
 		  handle->ios[idx].fd, timeout);
 
-	return coio_read_timeout(&handle->ios[idx], buf,
-				 count, timeout);
+	return coio_read_ahead_timeout(&handle->ios[idx], buf, 1, count,
+				       timeout);
 }
 
 /**
-- 
2.25.0

[Tarantool-patches] [PATCH 09/13] popen: remove redundant fd check before perform IO

[Alexander Turenko]

The function already checks flags to find out whether the file
descriptor should be available for reading / writing. When it is so, the
corresponding fd is great or equal to zero.

The further commits will add missed diagnostics for IO functions and it
is hard to write a meaningful error message for a situation that is not
possible. Moreover, we would obligated to document the error as one of
possible failures in a function contract (while it can't occur).

Part of #4031
---
 src/lib/core/popen.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 660ace0d5..c54e0b211 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -194,8 +194,7 @@ handle_free(struct popen_handle *handle)
  * Test if the handle can run io operation.
  */
 static inline bool
-popen_may_io(struct popen_handle *handle, unsigned int idx,
-	     unsigned int io_flags)
+popen_may_io(struct popen_handle *handle, unsigned int io_flags)
 {
 	if (!handle) {
 		errno = ESRCH;
@@ -207,11 +206,6 @@ popen_may_io(struct popen_handle *handle, unsigned int idx,
 		return false;
 	}
 
-	if (handle->ios[idx].fd < 0) {
-		errno = EPIPE;
-		return false;
-	}
-
 	return true;
 }
 
@@ -294,7 +288,7 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
 	    return -1;
 	}
 
-	if (!popen_may_io(handle, STDIN_FILENO, flags))
+	if (!popen_may_io(handle, flags))
 		return -1;
 
 	if (count > (size_t)SSIZE_MAX) {
@@ -329,7 +323,7 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
 	    return -1;
 	}
 
-	if (!popen_may_io(handle, idx, flags))
+	if (!popen_may_io(handle, flags))
 		return -1;
 
 	if (count > (size_t)SSIZE_MAX) {
-- 
2.25.0

[Tarantool-patches] [PATCH 10/13] popen: add missed diag_set() in popen IO functions

[Alexander Turenko]

Our usual convention for C code is to return a negative value at failure
and set an entry to the diagnostics area.

When code uses this convention consistently, it is much easier to handle
failures when using it: you always know where to find an error type and
message and how to pass the error to a C or Lua caller.

See also the previous commit ('popen: add missed diag_set in
popen_signal/delete').

Part of #4031
---
 src/lib/core/popen.c | 100 +++++++++++++++++++++++++++++++------------
 1 file changed, 72 insertions(+), 28 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index c54e0b211..bf7d597bd 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -191,22 +191,20 @@ handle_free(struct popen_handle *handle)
 }
 
 /**
- * Test if the handle can run io operation.
+ * Test if the handle can run a requested IO operation.
+ *
+ * Returns 0 if so and -1 otherwise (and set a diag).
  */
-static inline bool
+static inline int
 popen_may_io(struct popen_handle *handle, unsigned int io_flags)
 {
-	if (!handle) {
-		errno = ESRCH;
-		return false;
-	}
-
 	if (!(io_flags & handle->flags)) {
-		errno = EINVAL;
-		return false;
+		diag_set(IllegalParams, "popen: handle does not support the "
+			 "requested IO operation");
+		return -1;
 	}
 
-	return true;
+	return 0;
 }
 
 /**
@@ -273,6 +271,27 @@ stdX_str(unsigned int index)
 
 /**
  * Write data to the child stdin.
+ *
+ * Yield until all @a count bytes will be written.
+ *
+ * Returns @a count at success, otherwise returns -1 and set a
+ * diag.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: a parameter check fails:
+ *   - count: data is too big.
+ *   - flags: POPEN_FLAG_FD_STDIN bit is unset.
+ *   - handle: handle does not support the requested IO operation.
+ * - SocketError: an IO error occurs at write().
+ * - TimedOut: @a timeout quota is exceeded.
+ * - FiberIsCancelled: cancelled by an outside code.
+ *
+ * An error may occur after a partial write. There is not way to
+ * enquire amount of written bytes in the case.
+ *
+ * FIXME: Provide an info re amount written bytes in the case.
+ *        Say, return -(written) in the case.
  */
 int
 popen_write_timeout(struct popen_handle *handle, const void *buf,
@@ -281,20 +300,21 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
 {
 	assert(handle != NULL);
 
-	int idx = STDIN_FILENO;
+	if (count > (size_t)SSIZE_MAX) {
+		diag_set(IllegalParams, "popen: data is too big");
+		return -1;
+	}
 
 	if (!(flags & POPEN_FLAG_FD_STDIN)) {
-	    errno = EINVAL;
-	    return -1;
+		diag_set(IllegalParams,
+			 "popen: POPEN_FLAG_FD_STDIN bit is unset");
+		return -1;
 	}
 
-	if (!popen_may_io(handle, flags))
+	if (popen_may_io(handle, flags) != 0)
 		return -1;
 
-	if (count > (size_t)SSIZE_MAX) {
-		errno = E2BIG;
-		return -1;
-	}
+	int idx = STDIN_FILENO;
 
 	say_debug("popen: %d: write idx [%s:%d] buf %p count %zu "
 		  "fds %d timeout %.9g",
@@ -307,6 +327,26 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
 
 /**
  * Read data from a child's peer with timeout.
+ *
+ * Yield until some data will be available for read.
+ *
+ * Returns amount of read bytes at success, otherwise returns -1
+ * and set a diag.
+ *
+ * Zero return value means EOF.
+ *
+ * Note: Less then @a count bytes may be available for read at a
+ * moment, so a return value less then @a count does not mean EOF.
+ *
+ * Possible errors:
+ *
+ * - IllegalParams: a parameter check fails:
+ *   - count: buffer is too big.
+ *   - flags: POPEN_FLAG_FD_STD{OUT,ERR} are unset both.
+ *   - handle: handle does not support the requested IO operation.
+ * - SocketError: an IO error occurs at read().
+ * - TimedOut: @a timeout quota is exceeded.
+ * - FiberIsCancelled: cancelled by an outside code.
  */
 ssize_t
 popen_read_timeout(struct popen_handle *handle, void *buf,
@@ -315,24 +355,28 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
 {
 	assert(handle != NULL);
 
-	int idx = flags & POPEN_FLAG_FD_STDOUT ?
-		STDOUT_FILENO : STDERR_FILENO;
+	if (count > (size_t)SSIZE_MAX) {
+		diag_set(IllegalParams, "popen: buffer is too big");
+		return -1;
+	}
 
 	if (!(flags & (POPEN_FLAG_FD_STDOUT | POPEN_FLAG_FD_STDERR))) {
-	    errno = EINVAL;
-	    return -1;
+		diag_set(IllegalParams, "popen: POPEN_FLAG_FD_STD{OUT,ERR} are "
+			 "unset both");
+		return -1;
 	}
 
-	if (!popen_may_io(handle, flags))
+	if (flags & POPEN_FLAG_FD_STDOUT && flags & POPEN_FLAG_FD_STDERR) {
+		diag_set(IllegalParams, "popen: reading from both stdout and "
+			 "stderr at one call is not supported");
 		return -1;
+	}
 
-	if (count > (size_t)SSIZE_MAX) {
-		errno = E2BIG;
+	if (popen_may_io(handle, flags) != 0)
 		return -1;
-	}
 
-	if (timeout < 0.)
-		timeout = TIMEOUT_INFINITY;
+	int idx = flags & POPEN_FLAG_FD_STDOUT ?
+		STDOUT_FILENO : STDERR_FILENO;
 
 	say_debug("popen: %d: read idx [%s:%d] buf %p count %zu "
 		  "fds %d timeout %.9g",
-- 
2.25.0

[Tarantool-patches] [PATCH 11/13] coio: fix obsoleted comment in coio_write_timeout

[Alexander Turenko]

The comment was added in 52765de65b8304424ef3d7014952f9fa55ee46f4, but
becomes non-actual since 1.6.6-21-gc74abc786 ('Implement special
TimedOut exception type and use it in coio and latch.')

Part of #4031
---
 src/lib/core/coio.cc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/lib/core/coio.cc b/src/lib/core/coio.cc
index e88d724d5..74e6240ce 100644
--- a/src/lib/core/coio.cc
+++ b/src/lib/core/coio.cc
@@ -388,8 +388,8 @@ coio_readn_ahead_timeout(struct ev_io *coio, void *buf, size_t sz, size_t bufsiz
  * fiber until the socket becomes ready, until
  * all data is written.
  *
- * @retval the number of bytes written. Can be less than
- * requested only in case of timeout.
+ * @retval the number of bytes written. Always
+ * equal to @a sz.
  */
 ssize_t
 coio_write_timeout(struct ev_io *coio, const void *buf, size_t sz,
-- 
2.25.0

[Tarantool-patches] [PATCH 12/13] coio: add *_noxc read / write functions

[Alexander Turenko]

The popen implementation is written in C and uses coio read / write
functions. If an exception occurs, it'll pass through the C code. It
should be catched to proceed correctly.

We usually have foo() and foo_xc() (exception) functions when both
variants are necessary. Here I added non-conventional *_noxc() functions
as the temporary solution to postpone refactoring of the code and all
its usages.

Part of #4031
---
 src/lib/core/coio.cc | 39 +++++++++++++++++++++++++++++++++++++++
 src/lib/core/coio.h  | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)

diff --git a/src/lib/core/coio.cc b/src/lib/core/coio.cc
index 74e6240ce..6a113aa47 100644
--- a/src/lib/core/coio.cc
+++ b/src/lib/core/coio.cc
@@ -381,6 +381,26 @@ coio_readn_ahead_timeout(struct ev_io *coio, void *buf, size_t sz, size_t bufsiz
 	return nrd;
 }
 
+/*
+ * FIXME: Rewrite coio_read_ahead_timeout() w/o C++ exceptions and
+ * drop this function.
+ */
+ssize_t
+coio_read_ahead_timeout_noxc(struct ev_io *coio, void *buf, size_t sz,
+			     size_t bufsiz, ev_tstamp timeout)
+{
+	try {
+		return coio_read_ahead_timeout(coio, buf, sz, bufsiz, timeout);
+	} catch (Exception *) {
+		/*
+		 * The exception is already in the diagnostics
+		 * area. Nothing to do.
+		 */
+	}
+	return -1;
+}
+
+
 /** Write sz bytes to socket.
  *
  * Throws SocketError in case of write error. If
@@ -437,6 +457,25 @@ coio_write_timeout(struct ev_io *coio, const void *buf, size_t sz,
 	}
 }
 
+/*
+ * FIXME: Rewrite coio_write_timeout() w/o C++ exceptions and drop
+ * this function.
+ */
+ssize_t
+coio_write_timeout_noxc(struct ev_io *coio, const void *buf, size_t sz,
+			ev_tstamp timeout)
+{
+	try {
+		return coio_write_timeout(coio, buf, sz, timeout);
+	} catch (Exception *) {
+		/*
+		 * The exception is already in the diagnostics
+		 * area. Nothing to do.
+		 */
+	}
+	return -1;
+}
+
 /*
  * Write iov using sio API.
  * Put in an own function to workaround gcc bug with @finally
diff --git a/src/lib/core/coio.h b/src/lib/core/coio.h
index c323955d7..7875a0da1 100644
--- a/src/lib/core/coio.h
+++ b/src/lib/core/coio.h
@@ -130,6 +130,29 @@ coio_read_timeout(struct ev_io *coio, void *buf, size_t sz, ev_tstamp timeout)
 	return coio_read_ahead_timeout(coio, buf, sz, sz, timeout);
 }
 
+/**
+ * Read data with timeout.
+ *
+ * Yield until some data will be available for read.
+ *
+ * Returns amount of read bytes at success, otherwise returns -1
+ * and set a diag.
+ *
+ * Zero return value means EOF.
+ *
+ * Note: Less then @a count bytes may be available for read at a
+ * moment, so a return value less then @a count does not mean EOF.
+ *
+ * Possible errors:
+ *
+ * - SocketError: an IO error occurs at read().
+ * - TimedOut: @a timeout quota is exceeded.
+ * - FiberIsCancelled: cancelled by an outside code.
+ */
+ssize_t
+coio_read_ahead_timeout_noxc(struct ev_io *coio, void *buf, size_t sz,
+			     size_t bufsiz, ev_tstamp timeout);
+
 static inline ssize_t
 coio_readn(struct ev_io *coio, void *buf, size_t sz)
 {
@@ -144,6 +167,24 @@ ssize_t
 coio_write_timeout(struct ev_io *coio, const void *buf, size_t sz,
 		   ev_tstamp timeout);
 
+/**
+ * Write @a count bytes with timeout.
+ *
+ * Yield until all @a count bytes will be written.
+ *
+ * Returns @a count at success, otherwise returns -1 and set a
+ * diag.
+ *
+ * Possible errors:
+ *
+ * - SocketError: an IO error occurs at write().
+ * - TimedOut: @a timeout quota is exceeded.
+ * - FiberIsCancelled: cancelled by an outside code.
+ */
+ssize_t
+coio_write_timeout_noxc(struct ev_io *coio, const void *buf, size_t sz,
+			ev_tstamp timeout);
+
 static inline void
 coio_write(struct ev_io *coio, const void *buf, size_t sz)
 {
-- 
2.25.0

[Tarantool-patches] [PATCH 13/13] popen: use of exception safe functions for IO

[Alexander Turenko]

popen read / write functions provides are written in C and intended to
be used from C: the contract is to return -1 at failure and set an entry
to the diagnostics area. However a C++ exception from coio read / write
functions would pass over a popen function stack frame.

The solution is to use the recently introduced coio exception safe
functions.

Part of #4031
---
 src/lib/core/popen.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index bf7d597bd..d39d8640a 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -321,8 +321,10 @@ popen_write_timeout(struct popen_handle *handle, const void *buf,
 		  handle->pid, stdX_str(idx), idx, buf, count,
 		  handle->ios[idx].fd, timeout);
 
-	return coio_write_timeout(&handle->ios[idx], buf,
-				  count, timeout);
+	int rc = coio_write_timeout_noxc(&handle->ios[idx], buf, count,
+					 timeout);
+	assert(rc < 0 || rc == (ssize_t)count);
+	return rc;
 }
 
 /**
@@ -383,8 +385,8 @@ popen_read_timeout(struct popen_handle *handle, void *buf,
 		  handle->pid, stdX_str(idx), idx, buf, count,
 		  handle->ios[idx].fd, timeout);
 
-	return coio_read_ahead_timeout(&handle->ios[idx], buf, 1, count,
-				       timeout);
+	return coio_read_ahead_timeout_noxc(&handle->ios[idx], buf, 1, count,
+					    timeout);
 }
 
 /**
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 01/13] popen: require popen handle to be non-NULL

[Cyrill Gorcunov]

On Fri, Apr 10, 2020 at 05:50:39AM +0300, Alexander Turenko wrote:
> Further commits will add proper entries into the diagnostics area for
> failures inside popen functions. We should either report handle == NULL
> case via the diagnostics area or ensure that the NULL handle case is not
> possible.
> 
> The latter approach is implemented in this commit. There are two
> reasons for this:
> 
> * This way simplifies function contracts (one less kind of failure).
> * The popen Lua module (that will be implemented in the further commits)
>   will not construct any logic using NULL as a handle. When 'NULL
>   handle' error is not possible in the C API, it will be easier to
>   verify that this failure is not possible the Lua API.
> 
> A user of the C API should take care to don't call those functions with
> NULL handle.
> 
> Part of #4031

Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH 02/13] popen: remove retval from popen_state()

[Cyrill Gorcunov]

On Fri, Apr 10, 2020 at 05:50:40AM +0300, Alexander Turenko wrote:
> After the previous commit ('popen: require popen handle to be non-NULL')
> it turns out that popen_state() function always succeeds. There is no
> reason to return a success / failure value from it.
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH 03/13] popen: add missed diag_set in popen_signal/delete

[Cyrill Gorcunov]

On Fri, Apr 10, 2020 at 05:50:41AM +0300, Alexander Turenko wrote:
> Lua API will use content of the diagnostics area to report an error to a
> caller, so it is critical to always have proper diagnostics at failure.
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH 04/13] popen: add logging of fds closed in a child

[Cyrill Gorcunov]

On Fri, Apr 10, 2020 at 05:50:42AM +0300, Alexander Turenko wrote:
> It is useful for debugging popen behaviour around file descriptors.
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

> ---
>  src/lib/core/popen.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
> index 3fcbc325a..9d4e6ef3a 100644
> --- a/src/lib/core/popen.c
> +++ b/src/lib/core/popen.c
> @@ -579,6 +579,8 @@ close_inherited_fds(int *skip_fds, size_t nr_skip_fds)
>  		if (fd_no == -1)
>  			continue;
>  
> +		say_debug("popen: close inherited fd [%s:%d]", stdX_str(fd_no),
> +			  fd_no);

Can we please shift args a bit, like

		say_debug("popen: close inherited fd [%s:%d]",
			  stdX_str(fd_no), fd_no);

Re: [Tarantool-patches] [PATCH 07/13] popen: add const qualifier to popen_write_timeout

[Cyrill Gorcunov]

On Fri, Apr 10, 2020 at 05:50:45AM +0300, Alexander Turenko wrote:
> The buffer is for reading, we're not intend to change it.
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH 12/13] coio: add *_noxc read / write functions

[Konstantin Osipov]

* Alexander Turenko <alexander.turenko@tarantool.org> [20/04/10 09:28]:
> The popen implementation is written in C and uses coio read / write
> functions. If an exception occurs, it'll pass through the C code. It
> should be catched to proceed correctly.
> 
> We usually have foo() and foo_xc() (exception) functions when both
> variants are necessary. Here I added non-conventional *_noxc() functions
> as the temporary solution to postpone refactoring of the code and all
> its usages.
> 
> Part of #4031
> ---
>  src/lib/core/coio.cc | 39 +++++++++++++++++++++++++++++++++++++++
>  src/lib/core/coio.h  | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 80 insertions(+)
> 
> diff --git a/src/lib/core/coio.cc b/src/lib/core/coio.cc
> index 74e6240ce..6a113aa47 100644
> --- a/src/lib/core/coio.cc
> +++ b/src/lib/core/coio.cc
> @@ -381,6 +381,26 @@ coio_readn_ahead_timeout(struct ev_io *coio, void *buf, size_t sz, size_t bufsiz
>  	return nrd;
>  }
>  
> +/*
> + * FIXME: Rewrite coio_read_ahead_timeout() w/o C++ exceptions and
> + * drop this function.
> + */

Ugh.

Why can't you do it instead? This is necessary for a lot of other
patches, I believe Georgy's patches are doing it as well.


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH 08/13] popen: unblock popen_read_timeout at a first byte

[Cyrill Gorcunov]

On Fri, Apr 10, 2020 at 05:50:46AM +0300, Alexander Turenko wrote:
> Before this change popen_read_timeout() waits until a passed buffer will
> be fully filled (or until EOF / timeout / IO error occurs). Now it waits
> for any amount of data (but at least one byte).
> 
> It allows to communicate with an interactive child program: write, read,
> repeat.
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH 12/13] coio: add *_noxc read / write functions

[Cyrill Gorcunov]

On Fri, Apr 10, 2020 at 11:05:13AM +0300, Konstantin Osipov wrote:
> >  
> > +/*
> > + * FIXME: Rewrite coio_read_ahead_timeout() w/o C++ exceptions and
> > + * drop this function.
> > + */
> 
> Ugh.
> 
> Why can't you do it instead? This is necessary for a lot of other
> patches, I believe Georgy's patches are doing it as well.

Simply because we want a functional popen engine in the upcoming
release. We gonna cleanup/fixup and etc in next iteration. And
unweaving service functions from exception is nontrivial. As
to me such things should be done by separate series.

If you take a look on Georgy's patches with more attention
you'll see that the changes are far more than just modifying
the helpers, they affect context.

Re: [Tarantool-patches] [PATCH 09/13] popen: remove redundant fd check before perform IO

[Cyrill Gorcunov]

On Fri, Apr 10, 2020 at 05:50:47AM +0300, Alexander Turenko wrote:
> The function already checks flags to find out whether the file
> descriptor should be available for reading / writing. When it is so, the
> corresponding fd is great or equal to zero.
> 
> The further commits will add missed diagnostics for IO functions and it
> is hard to write a meaningful error message for a situation that is not
> possible. Moreover, we would obligated to document the error as one of
> possible failures in a function contract (while it can't occur).
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH 10/13] popen: add missed diag_set() in popen IO functions

[Cyrill Gorcunov]

On Fri, Apr 10, 2020 at 05:50:48AM +0300, Alexander Turenko wrote:
> Our usual convention for C code is to return a negative value at failure
> and set an entry to the diagnostics area.
> 
> When code uses this convention consistently, it is much easier to handle
> failures when using it: you always know where to find an error type and
> message and how to pass the error to a C or Lua caller.
> 
> See also the previous commit ('popen: add missed diag_set in
> popen_signal/delete').
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH 11/13] coio: fix obsoleted comment in coio_write_timeout

[Cyrill Gorcunov]

On Fri, Apr 10, 2020 at 05:50:49AM +0300, Alexander Turenko wrote:
> The comment was added in 52765de65b8304424ef3d7014952f9fa55ee46f4, but
> becomes non-actual since 1.6.6-21-gc74abc786 ('Implement special
> TimedOut exception type and use it in coio and latch.')
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH 12/13] coio: add *_noxc read / write functions

[Cyrill Gorcunov]

On Fri, Apr 10, 2020 at 05:50:50AM +0300, Alexander Turenko wrote:
> The popen implementation is written in C and uses coio read / write
> functions. If an exception occurs, it'll pass through the C code. It
> should be catched to proceed correctly.
> 
> We usually have foo() and foo_xc() (exception) functions when both
> variants are necessary. Here I added non-conventional *_noxc() functions
> as the temporary solution to postpone refactoring of the code and all
> its usages.
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH 05/13] say: allow to set a logger file descriptor

[Cyrill Gorcunov]

On Fri, Apr 10, 2020 at 05:50:43AM +0300, Alexander Turenko wrote:
...
>  
>  /**
> - * Default logger file descriptor.
> + * Accessors for default logger file descriptor.
> + *
> + * It is needed for dark magic inside popen implementation.
> + * Unlikely it is what you want to use anywhere else.
>   */

I would rather comment it as "It is needed to keep the
logger fd alive if a child process in the popen engine
needs own fd intersecting with logger." But up to you.

Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH 06/13] popen: decouple logger fd from stderr

[Cyrill Gorcunov]

On Fri, Apr 10, 2020 at 05:50:44AM +0300, Alexander Turenko wrote:
> The default logger configuration writes logs to stderr.
> 
> Popen implementation holds a logger fd until execve() to be able to
> write debug entries or information about a failure from a child. However
> when popen flags requires to close stderr in the child, the logger fd
> becomes closed: logging will fail.
> 
> Another problem appears when a user want to capture stderr and
> tarantool's log level is set to debug (7). Since the logger uses stderr
> and it is fed to the parent using a pipe, the logger output will not
> shown on the 'real' stderr, but will be captured together with child's
> program debugging output.
> 
> This commit duplicates a logger file descriptor that allows to close or
> redirect child's stderr without described side effects.
> 
> See also 86ec3a5c4792ea1bba9f644da1e8f301314c8d29 ('popen: add logging
> in child process').
> 
> Areas for improvements:
> 
> * Copy logger fd at module initialization time instead of copying of
>   each popen call.
> 
> Alternatives:
> 
> * Extend logger to allow to accumulate log entries in a buffer. Flush
>   the buffer from the parent process. (It is possible since vfork does
>   not split a virtual memory space).
> 
> Part of #4031
> ---
>  src/lib/core/popen.c | 124 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 115 insertions(+), 9 deletions(-)
> 
> diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
> index 9d4e6ef3a..62920e0c8 100644
> --- a/src/lib/core/popen.c
> +++ b/src/lib/core/popen.c
> @@ -74,6 +74,59 @@ popen_unregister(struct popen_handle *handle)
>  	mh_i32ptr_remove(popen_pids_map, &node, NULL);
>  }
>  
> +/**
> + * Duplicate a file descriptor, but not to std{in,out,err}.
> + *
> + * Return a new fd at success, otherwise return -1 and set a diag.
> + */
> +static int
> +dup_not_std_streams(int fd)
> +{
> +	int res = -1;
> +	int save_errno = 0;
> +
> +	/*
> +	 * We will call dup() in a loop until it will return
> +	 * fd > STDERR_FILENO. The array `discarded_fds` stores
> +	 * intermediate fds to close them after all dup() calls.
> +	 */
> +	static_assert(STDERR_FILENO + 1 == 3,
> +		      "Unexpected STDERR_FILENO constant");

We already have (in popen_new)

	static_assert(STDIN_FILENO == 0 &&
		      STDOUT_FILENO == 1 &&
		      STDERR_FILENO == 2,
		      "stdin/out/err are not posix compatible");

no need for this again.

> +	int discarded_fds[STDERR_FILENO + 1] = {-1, -1, -1};

And here we could

	int discarded_fds[POPEN_FLAG_FD_STDEND_BIT]

the POPEN_FLAG_FD_STDEND_BIT constant introduced exactly for that.

> +
> +	for (size_t i = 0; i < lengthof(discarded_fds) + 1; ++i) {
> +		int new_fd = dup(fd);
> +		if (new_fd < 0) {
> +			save_errno = errno;
> +			break;
> +		}
> +
> +		/* Found wanted fd. */
> +		if (new_fd > STDERR_FILENO) {
> +			res = new_fd;
> +			break;
> +		}
> +
> +		/* Save to close then. */
> +		assert(i < lengthof(discarded_fds));
> +		discarded_fds[i] = new_fd;
> +	}
> +
> +	/* Close all intermediate fds (if any). */
> +	for (size_t i = 0; i < lengthof(discarded_fds); ++i)
> +		if (discarded_fds[i] >= 0)
> +			close(discarded_fds[i]);

Wrap for() with {} please.

Otherwise looks good.
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH 13/13] popen: use of exception safe functions for IO

[Cyrill Gorcunov]

On Fri, Apr 10, 2020 at 05:50:51AM +0300, Alexander Turenko wrote:
> popen read / write functions provides are written in C and intended to
> be used from C: the contract is to return -1 at failure and set an entry
> to the diagnostics area. However a C++ exception from coio read / write
> functions would pass over a popen function stack frame.
> 
> The solution is to use the recently introduced coio exception safe
> functions.
> 
> Part of #4031
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH 12/13] coio: add *_noxc read / write functions

[Alexander Turenko]

On Fri, Apr 10, 2020 at 11:05:13AM +0300, Konstantin Osipov wrote:
> * Alexander Turenko <alexander.turenko@tarantool.org> [20/04/10 09:28]:
> > The popen implementation is written in C and uses coio read / write
> > functions. If an exception occurs, it'll pass through the C code. It
> > should be catched to proceed correctly.
> > 
> > We usually have foo() and foo_xc() (exception) functions when both
> > variants are necessary. Here I added non-conventional *_noxc() functions
> > as the temporary solution to postpone refactoring of the code and all
> > its usages.
> > 
> > Part of #4031
> > ---
> >  src/lib/core/coio.cc | 39 +++++++++++++++++++++++++++++++++++++++
> >  src/lib/core/coio.h  | 41 +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 80 insertions(+)
> > 
> > diff --git a/src/lib/core/coio.cc b/src/lib/core/coio.cc
> > index 74e6240ce..6a113aa47 100644
> > --- a/src/lib/core/coio.cc
> > +++ b/src/lib/core/coio.cc
> > @@ -381,6 +381,26 @@ coio_readn_ahead_timeout(struct ev_io *coio, void *buf, size_t sz, size_t bufsiz
> >  	return nrd;
> >  }
> >  
> > +/*
> > + * FIXME: Rewrite coio_read_ahead_timeout() w/o C++ exceptions and
> > + * drop this function.
> > + */
> 
> Ugh.
> 
> Why can't you do it instead? This is necessary for a lot of other
> patches, I believe Georgy's patches are doing it as well.

I dislike all those "let's do it dirty for now, later we'll return here"
too. But now I really have no choice: there is no time I can pay here
now.

Sorry.

Re: [Tarantool-patches] [PATCH 04/13] popen: add logging of fds closed in a child

[Alexander Turenko]

On Fri, Apr 10, 2020 at 10:46:42AM +0300, Cyrill Gorcunov wrote:
> On Fri, Apr 10, 2020 at 05:50:42AM +0300, Alexander Turenko wrote:
> > It is useful for debugging popen behaviour around file descriptors.
> > 
> > Part of #4031
> Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
> 
> > ---
> >  src/lib/core/popen.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
> > index 3fcbc325a..9d4e6ef3a 100644
> > --- a/src/lib/core/popen.c
> > +++ b/src/lib/core/popen.c
> > @@ -579,6 +579,8 @@ close_inherited_fds(int *skip_fds, size_t nr_skip_fds)
> >  		if (fd_no == -1)
> >  			continue;
> >  
> > +		say_debug("popen: close inherited fd [%s:%d]", stdX_str(fd_no),
> > +			  fd_no);
> 
> Can we please shift args a bit, like
> 
> 		say_debug("popen: close inherited fd [%s:%d]",
> 			  stdX_str(fd_no), fd_no);

My approach is to keep lines within 80 symbols, but don't break lines
prematurely.

I asked Cyrill what is the general approach he use, because I don't want
change code back and forth due to different styles within one file.

The answer is that one dangling word looks ugly. It is not strict
criteria, however I will try to follow it (at least when editing
Cyrill's code).

Changed:

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 9d4e6ef3a..e5e7e5cfc 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -579,8 +579,8 @@ close_inherited_fds(int *skip_fds, size_t nr_skip_fds)
                if (fd_no == -1)
                        continue;
 
-               say_debug("popen: close inherited fd [%s:%d]", stdX_str(fd_no),
-                         fd_no);
+               say_debug("popen: close inherited fd [%s:%d]",
+                         stdX_str(fd_no), fd_no);
                if (close(fd_no)) {
                        int saved_errno = errno;
                        diag_set(SystemError, "fdin: Can't close %d", fd_no);

Re: [Tarantool-patches] [PATCH 05/13] say: allow to set a logger file descriptor

[Alexander Turenko]

On Fri, Apr 10, 2020 at 11:33:49AM +0300, Cyrill Gorcunov wrote:
> On Fri, Apr 10, 2020 at 05:50:43AM +0300, Alexander Turenko wrote:
> ...
> >  
> >  /**
> > - * Default logger file descriptor.
> > + * Accessors for default logger file descriptor.
> > + *
> > + * It is needed for dark magic inside popen implementation.
> > + * Unlikely it is what you want to use anywhere else.
> >   */
> 
> I would rather comment it as "It is needed to keep the
> logger fd alive if a child process in the popen engine
> needs own fd intersecting with logger." But up to you.

Keep it alive is one of two problems we should solve: the second one is
about capturing of stderr in a parent process. I would not bury into
those details here, but give few common words: we need decouple
descriptors in popen, let's use those functions with caution.

The wording is actually too informal, so it worth to rewrite in more
serious way :)

Changed:

diff --git a/src/lib/core/say.h b/src/lib/core/say.h
index 3ce7724c4..43883f0da 100644
--- a/src/lib/core/say.h
+++ b/src/lib/core/say.h
@@ -206,8 +206,11 @@ log_type();
 /**
  * Accessors for default logger file descriptor.
  *
- * It is needed for dark magic inside popen implementation.
- * Unlikely it is what you want to use anywhere else.
+ * It is needed for decoupling of a logger file descriptor from
+ * stderr in the popen implementation.
+ *
+ * Those functions break logger incapsulation, so use them with
+ * caution.
  */
 int
 log_get_fd(void);

Re: [Tarantool-patches] [PATCH 06/13] popen: decouple logger fd from stderr

[Alexander Turenko]

> > +	/*
> > +	 * We will call dup() in a loop until it will return
> > +	 * fd > STDERR_FILENO. The array `discarded_fds` stores
> > +	 * intermediate fds to close them after all dup() calls.
> > +	 */
> > +	static_assert(STDERR_FILENO + 1 == 3,
> > +		      "Unexpected STDERR_FILENO constant");
> 
> We already have (in popen_new)
> 
> 	static_assert(STDIN_FILENO == 0 &&
> 		      STDOUT_FILENO == 1 &&
> 		      STDERR_FILENO == 2,
> 		      "stdin/out/err are not posix compatible");
> 
> no need for this again.
> 
> > +	int discarded_fds[STDERR_FILENO + 1] = {-1, -1, -1};
> 
> And here we could
> 
> 	int discarded_fds[POPEN_FLAG_FD_STDEND_BIT]
> 
> the POPEN_FLAG_FD_STDEND_BIT constant introduced exactly for that.

Okay, thanks!

> 
> > +
> > +	for (size_t i = 0; i < lengthof(discarded_fds) + 1; ++i) {
> > +		int new_fd = dup(fd);
> > +		if (new_fd < 0) {
> > +			save_errno = errno;
> > +			break;
> > +		}
> > +
> > +		/* Found wanted fd. */
> > +		if (new_fd > STDERR_FILENO) {
> > +			res = new_fd;
> > +			break;
> > +		}
> > +
> > +		/* Save to close then. */
> > +		assert(i < lengthof(discarded_fds));
> > +		discarded_fds[i] = new_fd;
> > +	}
> > +
> > +	/* Close all intermediate fds (if any). */
> > +	for (size_t i = 0; i < lengthof(discarded_fds); ++i)
> > +		if (discarded_fds[i] >= 0)
> > +			close(discarded_fds[i]);
> 
> Wrap for() with {} please.

No problem.

Changed:

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index b8ce77494..ef2f8e2aa 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -90,9 +90,7 @@ dup_not_std_streams(int fd)
         * fd > STDERR_FILENO. The array `discarded_fds` stores
         * intermediate fds to close them after all dup() calls.
         */
-       static_assert(STDERR_FILENO + 1 == 3,
-                     "Unexpected STDERR_FILENO constant");
-       int discarded_fds[STDERR_FILENO + 1] = {-1, -1, -1};
+       int discarded_fds[POPEN_FLAG_FD_STDEND_BIT] = {-1, -1, -1};
 
        for (size_t i = 0; i < lengthof(discarded_fds) + 1; ++i) {
                int new_fd = dup(fd);
@@ -113,9 +111,10 @@ dup_not_std_streams(int fd)
        }
 
        /* Close all intermediate fds (if any). */
-       for (size_t i = 0; i < lengthof(discarded_fds); ++i)
+       for (size_t i = 0; i < lengthof(discarded_fds); ++i) {
                if (discarded_fds[i] >= 0)
                        close(discarded_fds[i]);
+       }
 
        /* Report an error if it occurs. */
        if (res < 0) {

Re: [Tarantool-patches] [PATCH 13/13] popen: use of exception safe functions for IO

[Alexander Turenko]

On Fri, Apr 10, 2020 at 02:50:00PM +0300, Cyrill Gorcunov wrote:
> On Fri, Apr 10, 2020 at 05:50:51AM +0300, Alexander Turenko wrote:
> > popen read / write functions provides are written in C and intended to

Fixed typo: removed 'provides'.

> > be used from C: the contract is to return -1 at failure and set an entry
> > to the diagnostics area. However a C++ exception from coio read / write
> > functions would pass over a popen function stack frame.
> > 
> > The solution is to use the recently introduced coio exception safe
> > functions.
> > 
> > Part of #4031
> Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>

Thanks for the review!

Re: [Tarantool-patches] [PATCH 00/13] Popen Lua API: preliminary patches

[Kirill Yukhin]

Hello,

On 10 апр 05:50, Alexander Turenko wrote:
> https://github.com/tarantool/tarantool/issues/4031
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4031-popen-lua-api-preparation
> 
> I working on Lua API for popen. During the development I found that
> several changes are necessary for the underlying popen engine. This
> patch series accumulates them.
> 
> Here are bugfixes, API and behaviour changes. Hope they are explained
> enough in the commit messages.
> 
> Please, give more attention to the 'popen: decouple logger fd from
> stderr' patch: it is easy to make a mistake with those fds juggling and
> I have very sparse time to test it as it deserves (I made manual testing
> for the described cases however).
> 
> Those changes can be partially tested via Lua API draft. It is on my
> temporary branch Totktonada/gh-4031-popen-12-workbranch (see also
> test/app-tap/popen.test.lua on the branch).
> 
> Alexander Turenko (13):
>   popen: require popen handle to be non-NULL
>   popen: remove retval from popen_state()
>   popen: add missed diag_set in popen_signal/delete
>   popen: add logging of fds closed in a child
>   say: allow to set a logger file descriptor
>   popen: decouple logger fd from stderr
>   popen: add const qualifier to popen_write_timeout
>   popen: unblock popen_read_timeout at a first byte
>   popen: remove redundant fd check before perform IO
>   popen: add missed diag_set() in popen IO functions
>   coio: fix obsoleted comment in coio_write_timeout
>   coio: add *_noxc read / write functions
>   popen: use of exception safe functions for IO

The patchset LGTM.

I've checked it into master.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH 12/13] coio: add *_noxc read / write functions

[Alexander Turenko]

On Fri, Apr 10, 2020 at 11:05:13AM +0300, Konstantin Osipov wrote:
> * Alexander Turenko <alexander.turenko@tarantool.org> [20/04/10 09:28]:
> > The popen implementation is written in C and uses coio read / write
> > functions. If an exception occurs, it'll pass through the C code. It
> > should be catched to proceed correctly.
> > 
> > We usually have foo() and foo_xc() (exception) functions when both
> > variants are necessary. Here I added non-conventional *_noxc() functions
> > as the temporary solution to postpone refactoring of the code and all
> > its usages.
> > 
> > Part of #4031
> > ---
> >  src/lib/core/coio.cc | 39 +++++++++++++++++++++++++++++++++++++++
> >  src/lib/core/coio.h  | 41 +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 80 insertions(+)
> > 
> > diff --git a/src/lib/core/coio.cc b/src/lib/core/coio.cc
> > index 74e6240ce..6a113aa47 100644
> > --- a/src/lib/core/coio.cc
> > +++ b/src/lib/core/coio.cc
> > @@ -381,6 +381,26 @@ coio_readn_ahead_timeout(struct ev_io *coio, void *buf, size_t sz, size_t bufsiz
> >  	return nrd;
> >  }
> >  
> > +/*
> > + * FIXME: Rewrite coio_read_ahead_timeout() w/o C++ exceptions and
> > + * drop this function.
> > + */
> 
> Ugh.
> 
> Why can't you do it instead? This is necessary for a lot of other
> patches, I believe Georgy's patches are doing it as well.

Filed https://github.com/tarantool/tarantool/issues/4881

WBR, Alexander Turenko.