From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 982114696C3 for ; Thu, 26 Mar 2020 03:59:27 +0300 (MSK) Date: Thu, 26 Mar 2020 00:59:26 +0000 From: Nikita Pettik Message-ID: <20200326005926.GA3155@tarantool.org> References: <20200324100347.15405-1-gorcunov@gmail.com> <20200324100347.15405-3-gorcunov@gmail.com> <20200326004737.nu4dcii7zpwesrsp@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200326004737.nu4dcii7zpwesrsp@tkn_work_nb> Subject: Re: [Tarantool-patches] [PATCH 2/2] test: unit/popen -- provide a child process List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tml 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 > > --- > > 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).