[Tarantool-patches] [PATCH 0/2] popen: fix unit test

[Tarantool-patches] [PATCH 0/2] popen: fix unit test

[Cyrill Gorcunov]

Sasha, take a look please, do I use BUILDDIR correctly?
I need to run test/unit/popen-child as a helper process.

Cyrill Gorcunov (2):
  popen: do not require space for shell args
  test: unit/popen -- provide a child process

 src/lib/core/popen.c     |  2 +-
 test/unit/CMakeLists.txt |  4 +++
 test/unit/popen-child.c  | 71 ++++++++++++++++++++++++++++++++++++++++
 test/unit/popen.c        | 18 ++++++----
 4 files changed, 87 insertions(+), 8 deletions(-)
 create mode 100644 test/unit/popen-child.c

-- 
2.20.1

[Tarantool-patches] [PATCH 1/2] popen: do not require space for shell args

[Cyrill Gorcunov]

In case of direct execute without using a shell there
is no need to require a caller to allocate redundant
space, lets pass executable name in first argument.

Since this is yet testing api we're allowed to change
without breaking aything.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/popen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index a0630e3d9..6b6062215 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -945,7 +945,7 @@ popen_new(struct popen_opts *opts)
 		if (opts->flags & POPEN_FLAG_SHELL)
 			execve(_PATH_BSHELL, opts->argv, envp);
 		else
-			execve(opts->argv[2], &opts->argv[2], envp);
+			execve(opts->argv[0], opts->argv, envp);
 exit_child:
 		_exit(errno);
 		unreachable();
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 1/2] popen: do not require space for shell args

[Alexander Turenko]

I still think that we should remove the POPEN_FLAG_SHELL option,
because, in brief, it will simplify the popen engine API (see the last
point in [1]), but okay: let's proceed with the fix and postpone the
decision a bit.

LGTM.

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/014608.html

On Tue, Mar 24, 2020 at 01:03:46PM +0300, Cyrill Gorcunov wrote:
> In case of direct execute without using a shell there
> is no need to require a caller to allocate redundant
> space, lets pass executable name in first argument.
> 
> Since this is yet testing api we're allowed to change
> without breaking aything.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/lib/core/popen.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
> index a0630e3d9..6b6062215 100644
> --- a/src/lib/core/popen.c
> +++ b/src/lib/core/popen.c
> @@ -945,7 +945,7 @@ popen_new(struct popen_opts *opts)
>  		if (opts->flags & POPEN_FLAG_SHELL)
>  			execve(_PATH_BSHELL, opts->argv, envp);
>  		else
> -			execve(opts->argv[2], &opts->argv[2], envp);
> +			execve(opts->argv[0], opts->argv, envp);
>  exit_child:
>  		_exit(errno);
>  		unreachable();
> -- 
> 2.20.1
> 

[Tarantool-patches] [PATCH 2/2] test: unit/popen -- provide a child process

[Cyrill Gorcunov]

Testing via plain C interface with a shell is not stable,
the shell might simply be misconfigured or not found and
we will simply stuck forever (the signal handling in libev
is tricky and requires at least idle cycles or similar to
pass event processing). Thus lets rather run a program we
know is presenting in the system (popen-child executable).

Fixes #4811

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 test/unit/CMakeLists.txt |  4 +++
 test/unit/popen-child.c  | 71 ++++++++++++++++++++++++++++++++++++++++
 test/unit/popen.c        | 18 ++++++----
 3 files changed, 86 insertions(+), 7 deletions(-)
 create mode 100644 test/unit/popen-child.c

diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index bc6aebdcb..e1d506f58 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -246,5 +246,9 @@ target_link_libraries(swim_errinj.test unit swim)
 add_executable(merger.test merger.test.c)
 target_link_libraries(merger.test unit core box)
 
+#
+# Client for popen.test
+add_executable(popen-child popen-child.c)
+
 add_executable(popen.test popen.c)
 target_link_libraries(popen.test misc unit core)
diff --git a/test/unit/popen-child.c b/test/unit/popen-child.c
new file mode 100644
index 000000000..40d99faa4
--- /dev/null
+++ b/test/unit/popen-child.c
@@ -0,0 +1,71 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+
+int
+main(int argc, char *argv[])
+{
+	char buf[1024];
+
+	if (argc < 2) {
+		fprintf(stderr, "Not enough args\n");
+		exit(1);
+	}
+
+	// read -n X and the just echo data read
+	if (!strcmp(argv[1], "read") &&
+	    !strcmp(argv[2], "-n")) {
+		ssize_t nr = (ssize_t)atoi(argv[3]);
+		if (nr <= 0) {
+			fprintf(stderr, "Wrong number of args\n");
+			exit(1);
+		}
+
+		if (nr >= (ssize_t)sizeof(buf)) {
+			fprintf(stderr, "Too many bytes to read\n");
+			exit(1);
+		}
+
+		ssize_t n = read(STDIN_FILENO, buf, nr);
+		if (n != nr) {
+			fprintf(stderr, "Can't read from stdin\n");
+			exit(1);
+		}
+
+		n = write(STDOUT_FILENO, buf, nr);
+		if (n != nr) {
+			fprintf(stderr, "Can't write to stdout\n");
+			exit(1);
+		}
+
+		exit(0);
+	}
+
+	// just echo the data
+	if (!strcmp(argv[1], "echo")) {
+		ssize_t nr = (ssize_t)strlen(argv[2]) + 1;
+		if (nr <= 0) {
+			fprintf(stderr, "Wrong number of bytes\n");
+			exit(1);
+		}
+
+		ssize_t n = write(STDOUT_FILENO, argv[2], nr);
+		if (n != nr) {
+			fprintf(stderr, "Can't write to stdout\n");
+			exit(1);
+		}
+
+		exit(0);
+	}
+
+	// just sleep forever
+	if (!strcmp(argv[1], "loop")) {
+		for (;;)
+			sleep(10);
+		exit(0);
+	}
+
+	fprintf(stderr, "Unknown command passed\n");
+	return 1;
+}
diff --git a/test/unit/popen.c b/test/unit/popen.c
index a40ca514c..44624cd0c 100644
--- a/test/unit/popen.c
+++ b/test/unit/popen.c
@@ -11,9 +11,10 @@
 #include "popen.h"
 #include "say.h"
 
+static char popen_child_path[PATH_MAX];
+
 #define TEST_POPEN_COMMON_FLAGS			\
 	(POPEN_FLAG_SETSID		|	\
-	POPEN_FLAG_SHELL		|	\
 	POPEN_FLAG_RESTORE_SIGNALS)
 
 /**
@@ -40,8 +41,8 @@ popen_write_exit(void)
 {
 	struct popen_handle *handle;
 	char *child_argv[] = {
-		"/bin/sh", "-c",
-		"prompt=''; read -n 5 prompt; echo $prompt",
+		popen_child_path,
+		"read", "-n", "6",
 		NULL,
 	};
 
@@ -108,8 +109,8 @@ popen_read_exit(void)
 {
 	struct popen_handle *handle;
 	char *child_argv[] = {
-		"/bin/sh", "-c",
-		"echo 1 2 3 4 5",
+		popen_child_path,
+		"echo", "1 2 3 4 5",
 		NULL,
 	};
 
@@ -165,8 +166,8 @@ popen_kill(void)
 {
 	struct popen_handle *handle;
 	char *child_argv[] = {
-		"/bin/sh", "-c",
-		"while [ 1 ]; do sleep 10; done",
+		popen_child_path,
+		"loop",
 		NULL,
 	};
 
@@ -237,6 +238,9 @@ main(int argc, char *argv[])
 	//say_logger_init(NULL, S_DEBUG, 0, "plain", 0);
 	memory_init();
 
+	snprintf(popen_child_path, sizeof(popen_child_path),
+		 "%s/test/unit/popen-child", getenv("BUILDDIR"));
+
 	fiber_init(fiber_c_invoke);
 	popen_init();
 	coio_init();
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 2/2] test: unit/popen -- provide a child process

[Alexander Turenko]

I think it is good thing to depend less on a system environment.

It is not clear why the problem appears in the first place and why it is
gone after the patch. This confuses me a bit.

Anyway, I'll CC Nikita to do necessary amount runs with and withour the
patch to say for sure whether it actually helps versus flaky test fails.
Nikita, can you please verify it?

Other than that the patch looks okay except several minor points, see
below.

Whether you'll fix those point or not, I think the patch does not
require re-review with me: Nikita's check should be enough.

LGTM.

WBR, Alexander Turenko.

On Tue, Mar 24, 2020 at 01:03:47PM +0300, Cyrill Gorcunov wrote:
> Testing via plain C interface with a shell is not stable,
> the shell might simply be misconfigured or not found and
> we will simply stuck forever (the signal handling in libev
> is tricky and requires at least idle cycles or similar to
> pass event processing). Thus lets rather run a program we
> know is presenting in the system (popen-child executable).
> 
> Fixes #4811
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  test/unit/CMakeLists.txt |  4 +++
>  test/unit/popen-child.c  | 71 ++++++++++++++++++++++++++++++++++++++++
>  test/unit/popen.c        | 18 ++++++----
>  3 files changed, 86 insertions(+), 7 deletions(-)
>  create mode 100644 test/unit/popen-child.c

> 
> diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
> index bc6aebdcb..e1d506f58 100644
> --- a/test/unit/CMakeLists.txt
> +++ b/test/unit/CMakeLists.txt
> @@ -246,5 +246,9 @@ target_link_libraries(swim_errinj.test unit swim)
>  add_executable(merger.test merger.test.c)
>  target_link_libraries(merger.test unit core box)
>  
> +#
> +# Client for popen.test
> +add_executable(popen-child popen-child.c)
> +

Let's add the generated executable to .gitignore (for in-source build).

> +	// read -n X and the just echo data read

Nit: Our style guide follows kernel's one and forbids C99 style
comments.

> +	if (!strcmp(argv[1], "read") &&
> +	    !strcmp(argv[2], "-n")) {
> +		ssize_t nr = (ssize_t)atoi(argv[3]);
> +		if (nr <= 0) {
> +			fprintf(stderr, "Wrong number of args\n");
> +			exit(1);
> +		}
> +
> +		if (nr >= (ssize_t)sizeof(buf)) {
> +			fprintf(stderr, "Too many bytes to read\n");
> +			exit(1);
> +		}
> +
> +		ssize_t n = read(STDIN_FILENO, buf, nr);
> +		if (n != nr) {
> +			fprintf(stderr, "Can't read from stdin\n");
> +			exit(1);
> +		}

I understand that `nr` is small here and it is hard to find a condition
when a singletone read will be partial, but since the patch is about
stability of testing (and we maybe will add more cases: maybe for even
border conditions like pipe buffer overrun) I would handle this
situation just in case and read in a loop.

The same for write below.

> +
> +		n = write(STDOUT_FILENO, buf, nr);
> +		if (n != nr) {
> +			fprintf(stderr, "Can't write to stdout\n");
> +			exit(1);
> +		}
> @@ -237,6 +238,9 @@ main(int argc, char *argv[])
>  	//say_logger_init(NULL, S_DEBUG, 0, "plain", 0);
>  	memory_init();
>  
> +	snprintf(popen_child_path, sizeof(popen_child_path),
> +		 "%s/test/unit/popen-child", getenv("BUILDDIR"));
> +

It works for in-source and out-of-source build when is run under
test-run. It is okay in fact.

However I think it is convenient to have a default that, say, allows to
run ./test/unit/popen.test from a build directory w/o manual setting of
BUILDDIR. It seems logical to set BUILDDIR to '.' by default (if
getenv() returns NULL).

Re: [Tarantool-patches] [PATCH 2/2] test: unit/popen -- provide a child process

[Nikita Pettik]

On 26 Mar 03:47, Alexander Turenko wrote:
> I think it is good thing to depend less on a system environment.
> 
> It is not clear why the problem appears in the first place and why it is
> gone after the patch. This confuses me a bit.
> 
> Anyway, I'll CC Nikita to do necessary amount runs with and withour the
> patch to say for sure whether it actually helps versus flaky test fails.
> Nikita, can you please verify it?

Yep, I've already verified - patch really fixes the problem.
 
> Other than that the patch looks okay except several minor points, see
> below.
> 
> Whether you'll fix those point or not, I think the patch does not
> require re-review with me: Nikita's check should be enough.
> 
> LGTM.
> 
> WBR, Alexander Turenko.
> 
> On Tue, Mar 24, 2020 at 01:03:47PM +0300, Cyrill Gorcunov wrote:
> > Testing via plain C interface with a shell is not stable,
> > the shell might simply be misconfigured or not found and
> > we will simply stuck forever (the signal handling in libev
> > is tricky and requires at least idle cycles or similar to
> > pass event processing). Thus lets rather run a program we
> > know is presenting in the system (popen-child executable).
> > 
> > Fixes #4811
> > 
> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> > ---
> >  test/unit/CMakeLists.txt |  4 +++
> >  test/unit/popen-child.c  | 71 ++++++++++++++++++++++++++++++++++++++++
> >  test/unit/popen.c        | 18 ++++++----
> >  3 files changed, 86 insertions(+), 7 deletions(-)
> >  create mode 100644 test/unit/popen-child.c
> 
> > 
> > diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
> > index bc6aebdcb..e1d506f58 100644
> > --- a/test/unit/CMakeLists.txt
> > +++ b/test/unit/CMakeLists.txt
> > @@ -246,5 +246,9 @@ target_link_libraries(swim_errinj.test unit swim)
> >  add_executable(merger.test merger.test.c)
> >  target_link_libraries(merger.test unit core box)
> >  
> > +#
> > +# Client for popen.test
> > +add_executable(popen-child popen-child.c)
> > +
> 
> Let's add the generated executable to .gitignore (for in-source build).
> 
> > +	// read -n X and the just echo data read
> 
> Nit: Our style guide follows kernel's one and forbids C99 style
> comments.
> 
> > +	if (!strcmp(argv[1], "read") &&
> > +	    !strcmp(argv[2], "-n")) {
> > +		ssize_t nr = (ssize_t)atoi(argv[3]);
> > +		if (nr <= 0) {
> > +			fprintf(stderr, "Wrong number of args\n");
> > +			exit(1);
> > +		}
> > +
> > +		if (nr >= (ssize_t)sizeof(buf)) {
> > +			fprintf(stderr, "Too many bytes to read\n");
> > +			exit(1);
> > +		}
> > +
> > +		ssize_t n = read(STDIN_FILENO, buf, nr);
> > +		if (n != nr) {
> > +			fprintf(stderr, "Can't read from stdin\n");
> > +			exit(1);
> > +		}
> 
> I understand that `nr` is small here and it is hard to find a condition
> when a singletone read will be partial, but since the patch is about
> stability of testing (and we maybe will add more cases: maybe for even
> border conditions like pipe buffer overrun) I would handle this
> situation just in case and read in a loop.
> 
> The same for write below.
> 
> > +
> > +		n = write(STDOUT_FILENO, buf, nr);
> > +		if (n != nr) {
> > +			fprintf(stderr, "Can't write to stdout\n");
> > +			exit(1);
> > +		}
> > @@ -237,6 +238,9 @@ main(int argc, char *argv[])
> >  	//say_logger_init(NULL, S_DEBUG, 0, "plain", 0);
> >  	memory_init();
> >  
> > +	snprintf(popen_child_path, sizeof(popen_child_path),
> > +		 "%s/test/unit/popen-child", getenv("BUILDDIR"));
> > +
> 
> It works for in-source and out-of-source build when is run under
> test-run. It is okay in fact.
> 
> However I think it is convenient to have a default that, say, allows to
> run ./test/unit/popen.test from a build directory w/o manual setting of
> BUILDDIR. It seems logical to set BUILDDIR to '.' by default (if
> getenv() returns NULL).

[Tarantool-patches] [PATCH v2 2/2] test: unit/popen -- provide a child process

[Cyrill Gorcunov]

Testing via plain C interface with a shell is not stable,
the shell might simply be misconfigured or not found and
we will simply stuck forever (the signal handling in libev
is tricky and requires at least idle cycles or similar to
pass event processing). Thus lets rather run a program we
know is presenting in the system (popen-child executable).

Fixes #4811

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 - fixed comments
 - add entry into .gitignore
 - read/write in a loop

pushed into branch gorcunov/gh-4811-popen-coio-3

 .gitignore               |   1 +
 test/unit/CMakeLists.txt |   4 ++
 test/unit/popen-child.c  | 123 +++++++++++++++++++++++++++++++++++++++
 test/unit/popen.c        |  28 ++++++---
 4 files changed, 148 insertions(+), 8 deletions(-)
 create mode 100644 test/unit/popen-child.c

diff --git a/.gitignore b/.gitignore
index e9508fc43..cda28d79f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -92,6 +92,7 @@ test/unit/fiob
 test/small
 test/var
 test/luajit-tap
+test/unit/popen-child
 third_party/luajit/src/luajit
 third_party/luajit/lib/vmdef.lua
 third_party/luajit/src/buildvm
diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index bc6aebdcb..e1d506f58 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -246,5 +246,9 @@ target_link_libraries(swim_errinj.test unit swim)
 add_executable(merger.test merger.test.c)
 target_link_libraries(merger.test unit core box)
 
+#
+# Client for popen.test
+add_executable(popen-child popen-child.c)
+
 add_executable(popen.test popen.c)
 target_link_libraries(popen.test misc unit core)
diff --git a/test/unit/popen-child.c b/test/unit/popen-child.c
new file mode 100644
index 000000000..d11c4258a
--- /dev/null
+++ b/test/unit/popen-child.c
@@ -0,0 +1,123 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include <errno.h>
+
+static ssize_t
+read_safe(int pfd, char *dest, ssize_t bytes)
+{
+	ssize_t left = bytes;
+	ssize_t off = 0;
+
+	while (left > 0) {
+		ssize_t nr = read(pfd, &dest[off], left);
+		printf("read %zd", nr);
+		if (nr < 0) {
+			if (errno == EAGAIN ||
+			    errno == EWOULDBLOCK ||
+			    errno == EINTR)
+				continue;
+			return nr;
+		} else if (nr == 0) {
+			break;
+		}
+
+		off += nr;
+		left -= nr;
+	}
+
+	return off;
+}
+
+static ssize_t
+write_safe(int pfd, char *dest, ssize_t bytes)
+{
+	ssize_t left = bytes;
+	ssize_t off = 0;
+
+	while (left > 0) {
+		ssize_t nr = write(pfd, &dest[off], left);
+		if (nr < 0) {
+			if (errno == EAGAIN ||
+			    errno == EWOULDBLOCK ||
+			    errno == EINTR)
+				continue;
+			return nr;
+		} else if (nr == 0) {
+			break;
+		}
+
+		off += nr;
+		left -= nr;
+	}
+
+	return off;
+}
+
+int
+main(int argc, char *argv[])
+{
+	char buf[1024];
+
+	if (argc < 2) {
+		fprintf(stderr, "Not enough args\n");
+		exit(1);
+	}
+
+	/* read -n X and the just echo data read */
+	if (!strcmp(argv[1], "read") &&
+	    !strcmp(argv[2], "-n")) {
+		ssize_t nr = (ssize_t)atoi(argv[3]);
+		if (nr <= 0) {
+			fprintf(stderr, "Wrong number of args\n");
+			exit(1);
+		}
+
+		if (nr >= (ssize_t)sizeof(buf)) {
+			fprintf(stderr, "Too many bytes to read\n");
+			exit(1);
+		}
+
+		ssize_t n = read_safe(STDIN_FILENO, buf, nr);
+		if (n != nr) {
+			fprintf(stderr, "Can't read from stdin\n");
+			exit(1);
+		}
+
+		n = write_safe(STDOUT_FILENO, buf, nr);
+		if (n != nr) {
+			fprintf(stderr, "Can't write to stdout\n");
+			exit(1);
+		}
+
+		exit(0);
+	}
+
+	/* just echo the data */
+	if (!strcmp(argv[1], "echo")) {
+		ssize_t nr = (ssize_t)strlen(argv[2]) + 1;
+		if (nr <= 0) {
+			fprintf(stderr, "Wrong number of bytes\n");
+			exit(1);
+		}
+
+		ssize_t n = write_safe(STDOUT_FILENO, argv[2], nr);
+		if (n != nr) {
+			fprintf(stderr, "Can't write to stdout\n");
+			exit(1);
+		}
+
+		exit(0);
+	}
+
+	/* just sleep forever */
+	if (!strcmp(argv[1], "loop")) {
+		for (;;)
+			sleep(10);
+		exit(0);
+	}
+
+	fprintf(stderr, "Unknown command passed\n");
+	return 1;
+}
diff --git a/test/unit/popen.c b/test/unit/popen.c
index a40ca514c..caea9d552 100644
--- a/test/unit/popen.c
+++ b/test/unit/popen.c
@@ -11,9 +11,10 @@
 #include "popen.h"
 #include "say.h"
 
+static char popen_child_path[PATH_MAX];
+
 #define TEST_POPEN_COMMON_FLAGS			\
 	(POPEN_FLAG_SETSID		|	\
-	POPEN_FLAG_SHELL		|	\
 	POPEN_FLAG_RESTORE_SIGNALS)
 
 /**
@@ -40,8 +41,8 @@ popen_write_exit(void)
 {
 	struct popen_handle *handle;
 	char *child_argv[] = {
-		"/bin/sh", "-c",
-		"prompt=''; read -n 5 prompt; echo $prompt",
+		popen_child_path,
+		"read", "-n", "5",
 		NULL,
 	};
 
@@ -108,8 +109,8 @@ popen_read_exit(void)
 {
 	struct popen_handle *handle;
 	char *child_argv[] = {
-		"/bin/sh", "-c",
-		"echo 1 2 3 4 5",
+		popen_child_path,
+		"echo", "1 2 3 4 5",
 		NULL,
 	};
 
@@ -165,8 +166,8 @@ popen_kill(void)
 {
 	struct popen_handle *handle;
 	char *child_argv[] = {
-		"/bin/sh", "-c",
-		"while [ 1 ]; do sleep 10; done",
+		popen_child_path,
+		"loop",
 		NULL,
 	};
 
@@ -234,9 +235,20 @@ main_f(va_list ap)
 int
 main(int argc, char *argv[])
 {
-	//say_logger_init(NULL, S_DEBUG, 0, "plain", 0);
+#if 0
+	say_logger_init(NULL, S_DEBUG, 0, "plain", 0);
+#endif
 	memory_init();
 
+	if (getenv("BUILDDIR") == NULL) {
+		size_t size = sizeof(popen_child_path);
+		strncpy(popen_child_path, "./test/unit/popen-child", size);
+		popen_child_path[size-1] = '\0';
+	} else {
+		snprintf(popen_child_path, sizeof(popen_child_path),
+			 "%s/test/unit/popen-child", getenv("BUILDDIR"));
+	}
+
 	fiber_init(fiber_c_invoke);
 	popen_init();
 	coio_init();
-- 
2.20.1

Re: [Tarantool-patches] [PATCH v2 2/2] test: unit/popen -- provide a child process

[Cyrill Gorcunov]

On Thu, Mar 26, 2020 at 11:31:22AM +0300, Cyrill Gorcunov wrote:
...
> +
> +static ssize_t
> +read_safe(int pfd, char *dest, ssize_t bytes)
> +{
> +	ssize_t left = bytes;
> +	ssize_t off = 0;
> +
> +	while (left > 0) {
> +		ssize_t nr = read(pfd, &dest[off], left);
> +		printf("read %zd", nr);

This is redundant printf, I dropped it and force-pushed
into gorcunov/gh-4811-popen-coio-3

Re: [Tarantool-patches] [PATCH 0/2] popen: fix unit test

[Cyrill Gorcunov]

On Tue, Mar 24, 2020 at 01:03:45PM +0300, Cyrill Gorcunov wrote:
> Sasha, take a look please, do I use BUILDDIR correctly?
> I need to run test/unit/popen-child as a helper process.
> 

branch gorcunov/gh-4811-popen-coio-2
issue https://github.com/tarantool/tarantool/issues/4811

Re: [Tarantool-patches] [PATCH 0/2] popen: fix unit test

[Alexander Turenko]

On Tue, Mar 24, 2020 at 01:03:45PM +0300, Cyrill Gorcunov wrote:
> Sasha, take a look please, do I use BUILDDIR correctly?
> I need to run test/unit/popen-child as a helper process.

Mostly so, but I would give it default '.' to execute w/o test-run
easily. See [1] (at the end).

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/015154.html

Re: [Tarantool-patches] [PATCH 0/2] popen: fix unit test

[Kirill Yukhin]

Hello,

On 24 мар 13:03, Cyrill Gorcunov wrote:
> Sasha, take a look please, do I use BUILDDIR correctly?
> I need to run test/unit/popen-child as a helper process.
> 
> Cyrill Gorcunov (2):
>   popen: do not require space for shell args
>   test: unit/popen -- provide a child process

I've checked your patches into master.

--
Regards, Kirill Yukhin