[tarantool-patches] Re: [PATCH v2] core: Non-blocking io.popen
Stanislav Zudin
szudin at tarantool.org
Mon Jun 3 18:52:35 MSK 2019
On 30.05.2019 21:34, Vladimir Davydov wrote:
> Hello,
>
> Please see a few comments inline. Note I haven't reviewed all parts of
> the patch yet, because it's quite big and there's already something to
> address. I might write more later.
>
> On Wed, May 29, 2019 at 10:08:09AM +0300, Stanislav Zudin wrote:
>> Adds nonblocking implementation of popen.
>> The method is available in namespace fio.
>> fio.popen() returns an object providing facilities
>> for dealing with standard input/output, getting
>> status of the child process.
>>
>> Closes #4031
>>
>> @TarantoolBot document
>> Title: Nonblocking fio.popen
>>
>> handle, err = fio.popen(parameters)
>>
>> fio.popen starts a process and redirects its input/output.
>>
>> parameters - a table containing arguments to run a process.
>> The following arguments are expected:
>>
>> argv - [mandatory] is a table of argument strings passed to
>> the new program. By convention, the first of these strings should
>> contain the filename associated with the file being executed.
>>
>> environment - [optional] is a table of strings, conventionally of the
>> form key=value, which are passed as environment to the new program.
>
> What environment is used by default? Parent's?
yep
>
>>
>> By default stdin, stdout,stderr of the associated process are
>> available for writing/reading using object's methods
>> handle:write() and handle:read().
>> One can override default behavior to redirect streams to/from file
>> or to input/output of another process or to the default input/output
>> of the parent process.
>>
>> stdin - [optional] overrides the child process's standard input.
>> stdout - [optional] overrides the child process's standard output.
>> stderr - [optional] overrides the the child process's standard
>> error output.
>> May accept one of the following values:
>> Handle of the file open with fio.open()
>> Handle of the standard input/output of another process,
>> open by fio.popen
>> A constant defining the parent's STDIN, STDOUT or STDERR.
>> A constants:
>> fio.PIPE - Opens a file descriptor available for reading/writing
>> or redirection to/from another process.
>> fio.DEVNULL - Makes fio.popen redirect the output to /dev/null.
>>
>> On success returns an object providing methods for
>> communication with the running process.
>> Returns nil if underlying functions calls fail;
>> in this case the second return value, err, contains a error message.
>>
>> The object created by fio.popen provides the following methods:
>> read()
>> read2()
>> write()
>> kill()
>> wait()
>> get_status()
>> get_stdin()
>> get_stdout()
>> get_stderr()
>>
>> number handle:get_stdin()
>
> I'm not quite sure about this, but it looks like we don't typically use
> get_ prefix for getter functions in Lua, i.e. we would write simply
> status(), stdin(), etc. Not insisting though - the API is up to Georgy
> and Alexander mainly.
>
Renamed methods to status(), stdin(), stdout(), stderr().
>>
>> Returns handle of the child process's standard input.
>> The handle is available only if it wasn't redirected earlier.
>> Use this handle to setup a redirection
>> from file or other process to the input of the associated process.
>> If handle is unavailable the method returns nil.
>>
>> number handle:get_stdout()
>> number handle:get_stderr()
>>
>> Return STDOUT and STDIN of the associated process accordingly.
>> See handle:get_stdin() for details.
>
> Always? Even if stdin/stdout/stderrr is associated with a file or
> /dev/null?
Updated the comment.
"The handle is available only if it was created with fio.PIPE option."
>
>>
>> rc,err = handle:wait(timeout)
>>
>> The wait() waits for the associated process to terminate
>> and returns the exit status of the command.
>
> I like when an API gives exactly one way to do every thing. Here you
> can get process exit code by either checking wait() return code or by
> calling status(), which is kinda ambiguous. Let's please make this
> function return true/false on success/error so that there's the only
> way to get process status.
Done.
>
>>
>> timeout - an integer specifies number of seconds to wait.
>> If the requested time has elapsed the method returns nil,
>> the second return value, err, contains a error message.
>> To distinguish timeout from the the other errors use errno.
>> If timeout is nil, the method waits infinitely until
>> the associated process is terminated.
>> On success function returns an exit code as a positive number
>> or signal id as a negative number.
>> If failed, rc is nul and err contains a error message.
>>
>> If the associated process is terminated, one can use the following
>> methods get the exit status:
>>
>> rc = handle:get_status()
>>
>> returns nil if process is still running
>> >= 0 if process exited normally
>> < 0 if process was terminated by a signal
>
> Better say
>
> == 0 if the process exited normally
> error code > 0 if the process terminated with an error
> -signal no < 0 if the process was killed by a signal
>
Fixed.
>>
>> rc, err = handle:kill(sig)
>>
>> The kill() sends a specified signal to the associated process.
>> On success the method returns true, if failed - nil and error message.
>
> Since you return 'true' on success, I think we better return 'false' on
> error, not nil.
Fixed.
>
>> If the sig is nil the default "SIGTERM" is being sent to the process.
>> If the signal is unknown then the method fails (with error EINVAL).
>> The following signals are acceptable:
>> "SIGINT"
>> "SIGILL"
>> "SIGABRT"
>> "SIGFPE"
>> "SIGSEGV"
>> "SIGTERM"
>>
>> "SIGHUP"
>> "SIGQUIT"
>> "SIGTRAP"
>> "SIGKILL"
>> "SIGBUS"
>> "SIGSYS"
>> "SIGPIPE"
>> "SIGALRM"
>>
>> "SIGURG"
>> "SIGSTOP"
>> "SIGTSTP"
>> "SIGCONT"
>> "SIGCHLD"
>> "SIGTTIN"
>> "SIGTTOU"
>> "SIGPOLL"
>> "SIGXCPU"
>> "SIGXFSZ"
>> "SIGVTALRM"
>> "SIGPROF"
>> "SIGUSR1"
>> "SIGUSR2"
>
> Not really necessary to enumerate the signals here - they are standard.
> An example of using SIGKILL instead of SIGTERM would be enough. Would be
> cool to allow to pass signal numbers (may be not).
The numbers are acceptable although it's not documented.
Fixed.
>
>>
>> rc,src,err = handle:read(buffer,size)
>> rc,src,err = handle:read2(buffer,size,seconds)
>>
>> read stdout & stderr of the process started by fio.popen
>> read() -> str, source
>> read(buf) -> len, source
>> read(size) -> str, source
>> read(buf, size) -> len, source
>> read2(seconds) -> str, source
>> read2(buf,seconds) -> len, source
>> read2(size,seconds) -> str, source
>> read2(buf, size,seconds) -> len, source
>
> Please use the same function name for both variants - read() - as
> you can figure out what to do by looking at function arguments, no?
>
Well, actually there is an obstacle.
The size and the seconds are both 'number' so we can't distinguish them.
The possible way is to pass the fixed number of arguments, e.g.
read(nil,size) instead of read(size),
read(buf, nil, seconds) instead of read2(buf,seconds)
and so on.
If this approach is acceptable then i'll make the changes.
>>
>> seconds - an integer specifies number of seconds to wait.
>> If the requested time has elapsed the method returns nil.
>>
>> src contains id of the stream: fio.STDOUT or fio.STDERR.
>> If method failed the rc and src are nil and err contains error message.
>>
>> rc, err = handle:write(data, length)
>> Writes specified number of bytes
>> On success returns number of written bytes.
>> If failed the rc is nil and err contains an error message.
>
> write() to the pipe may block if the child process isn't reading its
> stdin hence this function should probably have timeout, too.
ok
>
>> ---
>> Branch: https://github.com/tarantool/tarantool/tree/stanztt/gh-4031-nonblocking-popen
>
> The code doesn't compile on Travis CI. Please fix it and in future check
> Travis CI before submitting a patch.
got it.
>
>> Issue: https://github.com/tarantool/tarantool/issues/4031
>>
>> src/CMakeLists.txt | 1 +
>> src/lib/core/CMakeLists.txt | 1 +
>> src/lib/core/coio_file.c | 243 +++++++++++
>> src/lib/core/coio_file.h | 30 ++
>> src/lib/core/coio_popen.c | 661 ++++++++++++++++++++++++++++++
>> src/lib/core/coio_popen.h | 243 +++++++++++
>> src/lib/core/coio_task.c | 2 +
>> src/lib/core/fiber.h | 4 +-
>> src/lua/fio.c | 289 +++++++++++++
>> src/lua/fio.lua | 262 ++++++++++++
>> src/lua/init.c | 2 +
>> src/lua/lua_signal.c | 99 +++++
>> src/lua/lua_signal.h | 45 ++
>> src/main.cc | 21 +-
>> test/box-tap/fio_popen.sample.txt | 5 +
>> test/box-tap/fio_popen.test.lua | 189 +++++++++
>> test/box-tap/fio_popen_test1.sh | 7 +
>> third_party/libeio/eio.c | 19 +-
>> third_party/libeio/etp.c | 28 ++
>> third_party/libev/ev.c | 2 +
>> 20 files changed, 2149 insertions(+), 4 deletions(-)
>> create mode 100644 src/lib/core/coio_popen.c
>> create mode 100644 src/lib/core/coio_popen.h
>> create mode 100644 src/lua/lua_signal.c
>> create mode 100644 src/lua/lua_signal.h
>> create mode 100644 test/box-tap/fio_popen.sample.txt
>> create mode 100755 test/box-tap/fio_popen.test.lua
>> create mode 100755 test/box-tap/fio_popen_test1.sh
>>
>> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
>> index 68674d06a..cfe46dfe3 100644
>> --- a/src/CMakeLists.txt
>> +++ b/src/CMakeLists.txt
>> @@ -117,6 +117,7 @@ set (server_sources
>> lua/info.c
>> lua/string.c
>> lua/buffer.c
>> + lua/lua_signal.c
>> ${lua_sources}
>> ${PROJECT_SOURCE_DIR}/third_party/lua-yaml/lyaml.cc
>> ${PROJECT_SOURCE_DIR}/third_party/lua-yaml/b64.c
>> diff --git a/src/lib/core/CMakeLists.txt b/src/lib/core/CMakeLists.txt
>> index eb10b11c3..8b1f8d32e 100644
>> --- a/src/lib/core/CMakeLists.txt
>> +++ b/src/lib/core/CMakeLists.txt
>> @@ -26,6 +26,7 @@ set(core_sources
>> trigger.cc
>> mpstream.c
>> port.c
>> + coio_popen.c
>> )
>>
>> if (TARGET_OS_NETBSD)
>> diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
>> index 3359f42bc..93956e533 100644
>> --- a/src/lib/core/coio_file.c
>> +++ b/src/lib/core/coio_file.c
>> @@ -34,6 +34,7 @@
>> #include "say.h"
>> #include "fio.h"
>> #include "errinj.h"
>> +#include "coio_popen.h"
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <dirent.h>
>> @@ -103,6 +104,36 @@ struct coio_file_task {
>> const char *source;
>> const char *dest;
>> } copyfile;
>> +
>> + struct {
>> + char** argv;
>
> We use C-style asterisk - put it closer to the variable name.
>
Fixed.
>> + int argc;
>> + char** environment;
>> + int environment_size;
>
> argv/argc but environment/environment_size - inconsistent.
> Please use the same naming convention, e.g.
>
> argv/argc and envp/envc
>
Fixed.
> or
>
> args/arg_count and env/env_count
>
> However, I don't think you really need the sizes anyway - in Unix's
> execve NULL is used as an end marker, why not do the same?
Accepted.
>
> Also, is there any particular reason to (ab)use coio_file_task rather
> than simply using coio_call? This way you wouldn't need to mess with
> coio_file.c at all and would be able to neatly keep the whole popen
> implementation in one file - coio_popen.c.
ok
>
>> + int stdin_fd;
>> + int stdout_fd;
>> + int stderr_fd;
>> + void *handle;
>
> Please don't use void* for popen handle. You can declare a struct and
> use pointers to it without having its body definition:
>
> struct popen_handle;
>
> struct popen_handle *handle;
>
> The code would look more readable that way.
ok
>
>> + } popen_params;
>> +
>
>> + struct {
>> + void *handle;
>> + } pclose_params;
>
> This one isn't used anywhere at all.
Fixed.
>
>> +
>> + struct {
>> + void *handle;
>> + const void* buf;
>> + size_t count;
>> + size_t *written;
>> + } popen_write;
>> +
>> + struct {
>> + void *handle;
>> + void *buf;
>> + size_t count;
>> + size_t *read_bytes;
>
> 'written', but 'read_bytes' - inconsistent. Pleas strive for consistency
> everywhere in your code.
ok
>
>> + int *output_number;
>> + } popen_read;
>> };
>> };
>>
>> @@ -631,3 +662,215 @@ coio_copyfile(const char *source, const char *dest)
>> eio_req *req = eio_custom(coio_do_copyfile, 0, coio_complete, &eio);
>> return coio_wait_done(req, &eio);
>> }
>> +
>> +static void
>> +coio_do_popen(eio_req *req)
>> +{
>> + struct coio_file_task *eio = (struct coio_file_task *)req->data;
>> + eio->popen_params.handle = coio_popen_impl(eio->popen_params.argv,
>> + eio->popen_params.argc,
>> + eio->popen_params.environment,
>> + eio->popen_params.environment_size,
>> + eio->popen_params.stdin_fd,
>> + eio->popen_params.stdout_fd,
>> + eio->popen_params.stderr_fd);
>> +
>> + eio->result = 0;
>> + eio->errorno = errno;
>> +}
>> +
>> +void *
>> +coio_popen(char** argv, int argc,
>> + char** environment, int environment_size,
>> + int stdin_fd, int stdout_fd, int stderr_fd)
>> +{
>> + INIT_COEIO_FILE(eio);
>> + eio.popen_params.argv = argv;
>> + eio.popen_params.argc = argc;
>> + eio.popen_params.environment = environment;
>> + eio.popen_params.environment_size = environment_size;
>> + eio.popen_params.stdin_fd = stdin_fd;
>> + eio.popen_params.stdout_fd = stdout_fd;
>> + eio.popen_params.stderr_fd = stderr_fd;
>> +
>> + eio_req *req = eio_custom(coio_do_popen, 0,
>> + coio_complete, &eio);
>> + coio_wait_done(req, &eio);
>> + return eio.popen_params.handle;
>> +}
>> +
>> +static void
>> +coio_do_popen_read(eio_req *req)
>> +{
>> + struct coio_file_task *eio = (struct coio_file_task *)req->data;
>> +
>> + int rc = coio_popen_try_to_read(eio->popen_read.handle,
>> + eio->popen_read.buf,
>> + eio->popen_read.count,
>> + eio->popen_read.read_bytes,
>> + eio->popen_read.output_number);
>> +
>> + req->result = rc;
>> + req->errorno = errno;
>> +}
>> +
>> +static int
>> +coio_do_nonblock_popen_read(void *fh, void *buf, size_t count,
>> + size_t *read_bytes, int *source_id)
>> +{
>> + INIT_COEIO_FILE(eio);
>> + eio.popen_read.buf = buf;
>> + eio.popen_read.count = count;
>> + eio.popen_read.handle = fh;
>> + eio.popen_read.read_bytes = read_bytes;
>> + eio.popen_read.output_number = source_id;
>> + eio_req *req = eio_custom(coio_do_popen_read, 0,
>> + coio_complete, &eio);
>> + return coio_wait_done(req, &eio);
>> +}
>> +
>> +ssize_t
>> +coio_popen_read(void *fh, void *buf, size_t count,
>> + int *output_number, int timeout)
>> +{
>> + size_t received = 0;
>> + int rc = coio_popen_try_to_read(fh, buf, count,
>> + &received, output_number);
>
> You call the same function coio_popen_try_to_read from both tx and coio.
> How's that? If it's blocking, it blocks tx, which is unacceptable. If it
> isn't, why call it from coio at all?
coio_popen_try_to_read is not blocking.
But somebody should call fiber_yield(). And coio_file.c contains all
facilities.
>
>> + if (rc == 0) /* The reading's succeeded */
>> + return (ssize_t)received;
>> + else if (rc == -1 && errno != EAGAIN) /* Failed */
>> + return -1;
>> +
>> + /* A blocking operation is expected */
>> +
>> + time_t start_tt;
>> + time(&start_tt);
>
> There's ev_monotonic_now() for checking time - it's much more
> lightweight and precise.
ok
>
>> +
>> + bool in_progress;
>> + do {
>> + if (fiber_is_cancelled()) {
>> + errno = EINTR;
>> + return -1;
>> + }
>> +
>> + if (timeout > 0) {
>> + time_t tt;
>> + time(&tt);
>> + if ((tt - start_tt) > timeout) {
>> + errno = ETIMEDOUT;
>> + return -1;
>> + }
>> + }
>> +
>> + rc = coio_do_nonblock_popen_read(fh, buf, count,
>> + &received, output_number);
>> + in_progress = (rc == -1 && errno == EAGAIN);
>
> Looks like busy waiting to me.
>
>> + } while (in_progress);
>> +
>> + return (rc == 0) ? (ssize_t)received
>> + : -1;
>> +}
>> +
>> +static void
>> +coio_do_popen_write(eio_req *req)
>> +{
>> + struct coio_file_task *eio = (struct coio_file_task *)req->data;
>> +
>> + int rc = coio_popen_try_to_write(eio->popen_write.handle,
>> + eio->popen_write.buf,
>> + eio->popen_write.count,
>> + eio->popen_write.written);
>> +
>> + req->result = rc;
>> + req->errorno = errno;
>> +}
>> +
>> +static int
>> +coio_do_nonblock_popen_write(void *fh, const void *buf, size_t count,
>> + size_t *written)
>> +{
>> + INIT_COEIO_FILE(eio);
>> + eio.popen_write.buf = buf;
>> + eio.popen_write.count = count;
>> + eio.popen_write.handle = fh;
>> + eio.popen_write.written = written;
>> + eio_req *req = eio_custom(coio_do_popen_write, 0,
>> + coio_complete, &eio);
>> + return coio_wait_done(req, &eio);
>> +}
>> +
>> +ssize_t
>> +coio_popen_write(void *fh, const void *buf, size_t count)
>> +{
>> + ssize_t total = 0;
>> + size_t written = 0;
>> + int rc = coio_popen_try_to_write(fh, buf, count,
>> + &written);
>> + if (rc == 0 && written == count) /* The writing's succeeded */
>> + return (ssize_t)written;
>> + else if (rc == -1 && errno != EAGAIN) /* Failed */
>> + return -1;
>> +
>> + /* A blocking operation is expected */
>> + bool in_progress;
>> +
>> + do {
>> + if (fiber_is_cancelled()) {
>> + errno = EINTR;
>> + return -1;
>> + }
>> +
>> + buf += written; /* advance writing position */
>> + total += (ssize_t)written;
>> + count -= written;
>> +
>> + if (count == 0)
>> + return total;
>> +
>> + written = 0;
>> + rc = coio_do_nonblock_popen_write(fh, buf, count,
>> + &written);
>> + in_progress = (rc == 0 && written < count) ||
>> + (rc == -1 && errno == EAGAIN);
>> + } while (in_progress);
>> +
>> + return (rc == 0) ? total
>> + : -1;
>> +}
>> +
>> +int
>> +coio_popen_wait(void *fh, int timeout, int *exit_code)
>> +{
>
> This function doesn't depend on coio_file infrastructure. Why define it
> here at all?
>
popen is part of fio.
fio is implemented in coio_* functions.
>> + time_t start_tt;
>> + time(&start_tt);
>> +
>> + do {
>> + /* Wait for SIGCHLD */
>> + int sig = 0;
>> + int code = 0;
>> +
>> + int rc = coio_popen_get_status(fh, &sig, &code);
>> + if (rc != POPEN_RUNNING) {
>> + *exit_code = (rc == POPEN_EXITED) ? code
>> + : -sig;
>> + return 0;
>> + }
>> +
>> + /* Check for timeout */
>> + if (timeout > 0) {
>> + time_t tt;
>> + time(&tt);
>> + if ((tt - start_tt) > timeout) {
>> + errno = ETIMEDOUT;
>> + return -1;
>> + }
>> + }
>> +
>> + fiber_yield_timeout(0);
>
> Busy waiting...
>
>> +
>> + } while(!fiber_is_cancelled());
>> +
>> + errno = EINTR;
>> + return -1;
>> +}
>> +
>> diff --git a/src/lib/core/coio_popen.c b/src/lib/core/coio_popen.c
>> new file mode 100644
>> index 000000000..b44cbf60b
>> --- /dev/null
>> +++ b/src/lib/core/coio_popen.c
>> @@ -0,0 +1,661 @@
>> +/*
>> + * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
>> + *
>> + * Redistribution and use in source and binary forms, with or
>> + * without modification, are permitted provided that the following
>> + * conditions are met:
>> + *
>> + * 1. Redistributions of source code must retain the above
>> + * copyright notice, this list of conditions and the
>> + * following disclaimer.
>> + *
>> + * 2. Redistributions in binary form must reproduce the above
>> + * copyright notice, this list of conditions and the following
>> + * disclaimer in the documentation and/or other materials
>> + * provided with the distribution.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
>> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
>> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
>> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
>> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
>> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
>> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
>> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
>> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>> + * SUCH DAMAGE.
>> + */
>> +
>> +#include <stddef.h>
>> +#include <signal.h>
>> +#include "coio_popen.h"
>> +#include "coio_task.h"
>> +#include "fiber.h"
>> +#include "say.h"
>> +#include "fio.h"
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <dirent.h>
>> +#include <sys/wait.h>
>> +#include <paths.h>
>> +#include <unistd.h>
>> +#include <sys/types.h>
>> +#include <sys/socket.h>
>> +#include <bits/types/siginfo_t.h>
>> +#include <pthread.h>
>> +
>> +/*
>> + * On OSX this global variable is not declared
>> + * in <unistd.h>
>> + */
>> +extern char **environ;
>> +
>> +
>> +struct popen_data {
>
> popen_handle would be a better name, I guess.
>
> Also, please use the same prefix for all data structures (popen_
> everywhere or coio_popen_ everywhere, not intermittently).
ok
>
>> + /* process id */
>> + pid_t pid;
>> + int fh[3];
>
> This is rather fd, not fh.
>
>> + /*
>> + * Three handles:
>> + * [0] write to stdin of the child process
>> + * [1] read from stdout of the child process
>> + * [2] read from stderr of the child process
>> + */
>
> We usually put comments before variable definition, not after.
> Anyway, would be prudent to say when this cases are valid. Only
> for pipe or for regular files and /dev/null, as well?
>
>> +
>> + /* Handle to /dev/null */
>> + int devnull_fd;
>> +
>> + /* The ID of socket was read recently
>> + * (STDERR_FILENO or STDOUT_FILENO */
>> + int prev_source;
>> +
>> + /*
>> + * Current process status.
>> + * The SIGCHLD handler changes this status.
>> + */
>> + enum popen_status status;
>> +
>> + /* exit status of the associated process. */
>
> This might sound pedantic (and it is), but we are very strict about
> comment formatting. Please always start a comment from a capital letter
> and end it with a dot.
>
>> + int exit_code;
>> +
>> + /*
>> + * Number of the signal that caused the
>> + * assosiated process to terminate
>
> Typo: assosiated => associated. Please enable spell checking and fix
> typos in comments.
>
> Please apply these minor comments to all your code.
>
>> + */
>> + int signal_id;
>> +
>> + /*
>> + * The next entry in the global list
>> + */
>> + struct popen_data* next;
>
> We have rlist/stailq. No need to invent a list.
ok
>
>> +};
>> +
>> +void *
>> +coio_popen_impl(char** argv, int argc,
>> + char** environment, int environment_size,
>> + int stdin_fd, int stdout_fd, int stderr_fd)
>> +{
>> + pid_t pid;
>> + int socket_rd[2] = {-1,-1};
>> + int socket_wr[2] = {-1,-1};
>> + int socket_er[2] = {-1,-1};
>
> It's not a socket. It's a pipe.
ok
>
>> + errno = 0;
>> +
>> + struct popen_data *data = popen_data_new();
>> + if (data == NULL)
>> + return NULL;
>> +
>> + /* Setup a /dev/null if necessary */
>> + bool read_devnull = (stdin_fd == FIO_DEVNULL);
>> + bool write_devnull = (stdout_fd == FIO_DEVNULL) ||
>> + (stderr_fd == FIO_DEVNULL);
>> + int devnull_flags = O_RDWR | O_CREAT;
>> + if (!read_devnull)
>> + devnull_flags = O_WRONLY | O_CREAT;
>> + else if (!write_devnull)
>> + devnull_flags = O_RDONLY | O_CREAT;
>> +
>> + if (read_devnull || write_devnull) {
>> + data->devnull_fd = open("/dev/null", devnull_flags, 0666);
>> + if (data->devnull_fd < 0)
>> + goto on_error;
>> + else {
>> + if (stdin_fd == FIO_DEVNULL)
>> + stdin_fd = data->devnull_fd;
>> + if (stdout_fd == FIO_DEVNULL)
>> + stdout_fd = data->devnull_fd;
>> + if (stderr_fd == FIO_DEVNULL)
>> + stderr_fd = data->devnull_fd;
>> + }
>> + }
>> +
>> + if (stdin_fd == FIO_PIPE) {
>> + /*
>> + * Enable non-blocking for the parent side
>> + * and close-on-exec on the child's side.
>> + * The socketpair on OSX doesn't support
>> + * SOCK_NONBLOCK & SOCK_CLOEXEC flags.
>> + */
>> + if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_rd) < 0 ||
>> + fcntl(socket_rd[0], F_SETFL, O_NONBLOCK) < 0 ||
>> + fcntl(socket_rd[1], F_SETFD, FD_CLOEXEC) < 0) {
>> + goto on_error;
>> + }
>> + }
>> +
>> + if (stdout_fd == FIO_PIPE) {
>> + if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_wr) < 0 ||
>> + fcntl(socket_wr[0], F_SETFL, O_NONBLOCK) < 0 ||
>> + fcntl(socket_wr[1], F_SETFD, FD_CLOEXEC) < 0) {
>> + goto on_error;
>> + }
>> + }
>> +
>> + if (stderr_fd == FIO_PIPE) {
>> + if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_er) < 0 ||
>> + fcntl(socket_er[0], F_SETFL, O_NONBLOCK) < 0 ||
>> + fcntl(socket_er[1], F_SETFD, FD_CLOEXEC) < 0) {
>> + goto on_error;
>> + }
>> + }
>> +
>> + pid = fork();
>
> Please use vfork().
>
I'm afraid it won't work because of at_fork handlers.
>> +int
>> +coio_popen_try_to_read(void *fh, void *buf, size_t count,
>> + size_t *read_bytes, int *source_id)
>> +{
>> + struct popen_data *data = (struct popen_data *)fh;
>> +
>> + if (data == NULL){
>> + errno = EBADF;
>> + return -1;
>> + }
>> +
>> + fd_set rfds;
>> + FD_ZERO(&rfds);
>> + ssize_t received = 0;
>> + int num = 0;
>> +
>> + if (data->fh[STDOUT_FILENO] >= 0) {
>> + FD_SET(data->fh[STDOUT_FILENO], &rfds);
>> + ++num;
>> + }
>> + if (data->fh[STDERR_FILENO] >= 0) {
>> + FD_SET(data->fh[STDERR_FILENO], &rfds);
>> + ++num;
>> + }
>> +
>> + if (num == 0) {
>> + /*
>> + * There are no open handles for reading
>> + */
>> + errno = EBADF;
>> + return -1;
>> + }
>> +
>> + struct timeval tv = {0,0};
>> + int max_h = MAX(data->fh[STDOUT_FILENO],
>> + data->fh[STDERR_FILENO]);
>> +
>> + errno = 0;
>> + int retv = select(max_h + 1, &rfds, NULL, NULL, &tv);
>
> What's going on here? Would be nice to see a friendly comment.
>
> Anyway, why don't you use evio instead - it already uses select() under
> the hood, see ev_io_start.
researching...
>
>> +void
>> +coio_popen_child_is_dead(int sig, siginfo_t *si, void *context)
>> +{
>> + (void)context; /* UNUSED */
>> +
>> + if (sig != SIGCHLD)
>> + return;
>> +
>> + /*
>> + * The sigaction is called with SA_NOCLDWAIT,
>> + * so no need to call waitpid()
>> + */
>> +
>> + popen_lock_data_list();
>
> This function is called from a signal handler => it might deadlock with
> coio_popen_destroy, for instance. Please consider using evio signal
> handlers (take a look at ev_signal_init).
evio drops all information related to the signal and just reports that
the signal had been caught.
And since evio is implemented upon the signalfd() the further calls of
waitpid are useless - the information about child process was discarded.
All you can get is the fact that the process is not running.
So i see the following ways to resolve this problem:
1. Refactoring of evio - to keep siginfo_t received with a signal
(the worst idea IMO).
2. Use signalfd and run the own thread to dispatch SIGCHLD
3. Deal with the lack of child's exit code
4. make the wait() is mandatory, call waitpid() and do not use SIGCHLD
handler at all.
>
>> +
>> + struct popen_data *data = popen_lookup_data_by_pid(si->si_pid);
>> + if (data) {
>> + switch (si->si_code) {
>> + case CLD_EXITED:
>> + data->exit_code = si->si_status;
>> + data->status = POPEN_EXITED;
>> + break;
>> + case CLD_KILLED:
>> + data->signal_id = si->si_status;
>> + data->status = POPEN_KILLED;
>> + break;
>> + case CLD_DUMPED:
>> + /* exit_status makes no sense */
>> + data->status = POPEN_DUMPED;
>> + break;
>> + }
>> +
>> + /*
>> + * We shouldn't close file descriptors here.
>> + * The child process may exit earlier than
>> + * the parent process finishes reading data.
>> + * In this case the reading fails.
>> + */
>> + }
>> +
>> + popen_unlock_data_list();
>> +}
>> diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
>> index fb168e25e..614b8fefe 100644
>> --- a/src/lib/core/fiber.h
>> +++ b/src/lib/core/fiber.h
>> @@ -494,8 +494,8 @@ struct cord {
>> extern __thread struct cord *cord_ptr;
>>
>> #define cord() cord_ptr
>> -#define fiber() cord()->fiber
>> -#define loop() (cord()->loop)
>> +#define fiber() (cord() ? cord()->fiber : NULL)
>> +#define loop() (cord() ? cord()->loop : NULL)
>
> Why is that? A comment would be nice to have.
It's an at_fork() consequences.
Not sure if we can safely remove at_fork handlers.
The parent process is forking to run as a daemon.
>
>> diff --git a/src/lua/lua_signal.c b/src/lua/lua_signal.c
>> new file mode 100644
>> index 000000000..924b0ab51
>> --- /dev/null
>> +++ b/src/lua/lua_signal.c
>> @@ -0,0 +1,99 @@
>> +/*
>> + * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
>> + *
>> + * Redistribution and use in source and binary forms, with or
>> + * without modification, are permitted provided that the following
>> + * conditions are met:
>> + *
>> + * 1. Redistributions of source code must retain the above
>> + * copyright notice, this list of conditions and the
>> + * following disclaimer.
>> + *
>> + * 2. Redistributions in binary form must reproduce the above
>> + * copyright notice, this list of conditions and the following
>> + * disclaimer in the documentation and/or other materials
>> + * provided with the distribution.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
>> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
>> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
>> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
>> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
>> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
>> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
>> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
>> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>> + * SUCH DAMAGE.
>> + */
>> +
>> +#include "lua/lua_signal.h"
>> +#include <sys/types.h>
>> +#include <signal.h>
>> +
>> +#include <lua.h>
>> +#include <lauxlib.h>
>> +#include <lualib.h>
>> +
>> +#include "lua/utils.h"
>> +
>> +#ifndef PUSHTABLE
>> +#define PUSHTABLE(name, method, value) { \
>> + lua_pushliteral(L, name); \
>> + method(L, value); \
>> + lua_settable(L, -3); \
>> +}
>> +#endif /*PUSHTABLE*/
>> +
>> +void
>> +tarantool_lua_signal_init(struct lua_State *L)
>> +{
>> + static const struct luaL_Reg signal_methods[] = {
>> + { NULL, NULL }
>> + };
>> +
>> + luaL_register_module(L, "signal", signal_methods);
>> +
>> + lua_pushliteral(L, "c");
>> + lua_newtable(L);
>> +
>> + lua_pushliteral(L, "signals");
>> + lua_newtable(L);
>> +
>> + PUSHTABLE("SIGINT", lua_pushinteger, SIGINT);
>> + PUSHTABLE("SIGILL", lua_pushinteger, SIGILL);
>> + PUSHTABLE("SIGABRT", lua_pushinteger, SIGABRT);
>> + PUSHTABLE("SIGFPE", lua_pushinteger, SIGFPE);
>> + PUSHTABLE("SIGSEGV", lua_pushinteger, SIGSEGV);
>> + PUSHTABLE("SIGTERM", lua_pushinteger, SIGTERM);
>> +
>> + PUSHTABLE("SIGHUP", lua_pushinteger, SIGHUP);
>> + PUSHTABLE("SIGQUIT", lua_pushinteger, SIGQUIT);
>> + PUSHTABLE("SIGTRAP", lua_pushinteger, SIGTRAP);
>> + PUSHTABLE("SIGKILL", lua_pushinteger, SIGKILL);
>> + PUSHTABLE("SIGBUS", lua_pushinteger, SIGBUS);
>> + PUSHTABLE("SIGSYS", lua_pushinteger, SIGSYS);
>> + PUSHTABLE("SIGPIPE", lua_pushinteger, SIGPIPE);
>> + PUSHTABLE("SIGALRM", lua_pushinteger, SIGALRM);
>> +
>> + PUSHTABLE("SIGURG", lua_pushinteger, SIGURG);
>> + PUSHTABLE("SIGSTOP", lua_pushinteger, SIGSTOP);
>> + PUSHTABLE("SIGTSTP", lua_pushinteger, SIGTSTP);
>> + PUSHTABLE("SIGCONT", lua_pushinteger, SIGCONT);
>> + PUSHTABLE("SIGCHLD", lua_pushinteger, SIGCHLD);
>> + PUSHTABLE("SIGTTIN", lua_pushinteger, SIGTTIN);
>> + PUSHTABLE("SIGTTOU", lua_pushinteger, SIGTTOU);
>> + PUSHTABLE("SIGPOLL", lua_pushinteger, SIGPOLL);
>> + PUSHTABLE("SIGXCPU", lua_pushinteger, SIGXCPU);
>> + PUSHTABLE("SIGXFSZ", lua_pushinteger, SIGXFSZ);
>> + PUSHTABLE("SIGVTALRM", lua_pushinteger, SIGVTALRM);
>> + PUSHTABLE("SIGPROF", lua_pushinteger, SIGPROF);
>> + PUSHTABLE("SIGUSR1", lua_pushinteger, SIGUSR1);
>> + PUSHTABLE("SIGUSR2", lua_pushinteger, SIGUSR2);
>> + lua_settable(L, -3); /* "signals" */
>
> This could be done from Lua - signal numbers are standard AFAIK.
Did you mean to use constants like fio.STDIN?
Ok.
>
>> +
>> + lua_settable(L, -3); /* "c" */
>> + lua_pop(L, 1);
>> +}
>> diff --git a/test/box-tap/fio_popen.sample.txt b/test/box-tap/fio_popen.sample.txt
>> new file mode 100644
>> index 000000000..43cba8c65
>> --- /dev/null
>> +++ b/test/box-tap/fio_popen.sample.txt
>> @@ -0,0 +1,5 @@
>> +AAA
>> +BBB
>> +CCC
>> +DDD
>> +
>
> I think this file should be created by the test in tmp dir. Just don't
> forget to delete it.
>
Ok.
>> diff --git a/test/box-tap/fio_popen.test.lua b/test/box-tap/fio_popen.test.lua
>
> Should be app-tap? Anyway, why tap test? Normal tests are easier to
> write and understand IMO.
>
Ok, let it be app-tap.
What a 'Normal' test? The ones in e.g. test/app directory?
It's hard to find the error when result files are quite big.
Have to use external tools, e.g. 'diff' etc.
>> new file mode 100755
>> index 000000000..0c2bd7e70
>> --- /dev/null
>> +++ b/test/box-tap/fio_popen.test.lua
>> @@ -0,0 +1,189 @@
>> +#!/usr/bin/env tarantool
>> +
>> +local tap = require('tap')
>> +local fio = require('fio')
>> +local errno = require('errno')
>> +local fiber = require('fiber')
>> +local test = tap.test()
>> +
>> +test:plan(5+9+11+9+8)
>> +
>> +-- Preliminaries
>> +local function read_stdout(app)
>> + local ss = ""
>> +
>> + local s,src,err = app:read(128)
>> +
>> + while s ~= nil and s ~= "" do
>> + ss = ss .. s
>> +
>> + s,src,err = app:read(128)
>> + end
>> +
>> + return ss
>> +end
>> +
>> +-- Test 1. Run application, check its status, kill and wait
>> +local app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
>
> Please test passing custom env as well.
ok
>
>> + stdout=fio.STDOUT,
>> + stderr=fio.STDOUT})
>> +test:isnt(app1, nil, "#1. Starting a existing application")
>> +
>> +local rc = app1:get_status()
>> +test:is(rc, nil, "#1. Process is running")
>> +
>> +rc,err = app1:kill()
>> +test:is(rc, true, "#1. Sending kill(15)")
>> +
>> +rc,err = app1:wait(5)
>> +test:is(rc, -15, "#1. Process was killed")
>> +
>> +rc = app1:get_status()
>> +test:is(rc, -15, "#1. Process was killed 2")
>> +
>> +app1 = nil
>> +
>> +-- Test 2. Run application, write to stdin, read from stdout
>> +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
>
> What's the point using '/bin/sh' just to run 'cat'?
>
cat fails without it.
>> + stdout=fio.PIPE,
>> + stdin=fio.PIPE,
>> + stderr=fio.STDOUT})
>> +test:isnt(app1, nil, "#2. Starting a existing application")
>> +
>> +rc = app1:get_status()
>> +test:is(rc, nil, "#2. Process is running")
>> +
>> +local test2str = '123\n456\n789'
>> +
>> +app1:write(test2str)
>> +rc,src,err = app1:read(256)
>> +
>> +test:is(src, fio.STDOUT, "#2. Read from STDOUT")
>> +test:is(rc, test2str, "#2. Received exact string")
>> +
>> +test2str = 'abc\ndef\ngh'
>> +
>> +app1:write(test2str, 4)
>> +local rc2,src2,err = app1:read(6)
>> +
>> +test:is(err, nil, "#2. Reading ok")
>> +test:is(src2, fio.STDOUT, "#2. Read from STDOUT 2")
>> +test:is(rc2, 'abc\n', "#2. Received exact string 2")
>> +
>> +rc,err = app1:kill()
>> +test:is(rc, true, "#2. Sending kill(15)")
>> +
>> +rc,err = app1:wait()
>> +test:is(rc, -15, "#2. Process was killed")
>> +
>> +app1 = nil
>> +
>> +-- Test 3. read from stdout with timeout
>> +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
>> + stdout=fio.PIPE,
>> + stdin=fio.PIPE,
>> + stderr=fio.STDOUT})
>> +test:isnt(app1, nil, "#3. Starting a existing application")
>> +test:is(app1:get_stderr(), nil, "#3. STDERR is redirected")
>> +test:isnt(app1:get_stdout(), nil, "#3. STDOUT is available")
>> +test:isnt(app1:get_stdin(), nil, "#3. STDIN is available")
>> +
>> +
>> +rc = app1:get_status()
>> +test:is(rc, nil, "#3. Process is running")
>> +
>> +rc,src,err = app1:read2(256, 2)
>
> Please use smaller timeouts - we want tests to run real fast.
>
ok
>> +
>> +local e = errno()
>> +test:is(e, errno.ETIMEDOUT, "#3. Timeout")
>> +
>> +
>> +local test2str = '123\n456\n789'
>> +
>> +app1:write(test2str)
>> +rc,src,err = app1:read2(256, 2)
>> +
>> +test:is(err, nil, "#3. Reading ok")
>> +test:is(src, fio.STDOUT, "#3. Read from STDOUT")
>> +test:is(rc, test2str, "#3. Received exact string")
>> +
>> +rc,err = app1:kill('SIGHUP')
>> +test:is(rc, true, "#3. Sending kill(1)")
>> +
>> +rc,err = app1:wait()
>> +test:is(rc, -1, "#3. Process was killed")
>> +
>> +app1 = nil
>> +
>> +-- Test 4. Redirect from file
>> +local build_path = os.getenv("BUILDDIR")
>> +local txt_filename = fio.pathjoin(build_path, 'test/box-tap/fio_popen.sample.txt')
>> +
>> +local txt_file = fio.open(txt_filename, {'O_RDONLY'})
>> +test:isnt(txt_file, nil, "#4. Open existing file for reading")
>> +
>> +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
>> + stdout=fio.PIPE,
>> + stdin=txt_file,
>> + stderr=fio.STDOUT})
>> +test:isnt(app1, nil, "#4. Starting a existing application")
>> +test:is(app1:get_stderr(), nil, "#4. STDERR is redirected")
>> +test:isnt(app1:get_stdout(), nil, "#4. STDOUT is available")
>> +test:is(app1:get_stdin(), nil, "#4. STDIN is redirected")
>> +
>> +rc = app1:get_status()
>> +test:is(rc, nil, "#4. Process is running")
>> +
>> +rc,src,err = app1:read2(256, 2)
>> +
>> +test:is(src, fio.STDOUT, "#4. Read from STDOUT")
>> +
>> +local test2str = 'AAA\nBBB\nCCC\nDDD\n\n'
>> +
>> +test:is(rc, test2str, "#4. Received exact string")
>> +
>> +rc,err = app1:wait()
>> +test:is(rc, 0, "#4. Process's exited")
>> +
>> +app1 = nil
>> +txt_file:close()
>> +
>> +-- Test 5. Redirect output from one process to another
>> +local app_path = fio.pathjoin(build_path, 'test/box-tap/fio_popen_test1.sh')
>> +
>> +app1 = fio.popen({argv = {'/bin/sh', '-c', app_path},
>> + stdout=fio.PIPE,
>> + stderr=fio.STDOUT})
>> +
>> +test:isnt(app1, nil, "#5. Starting application 1")
>> +test:is(app1:get_stderr(), nil, "#5. STDERR is redirected")
>> +test:isnt(app1:get_stdout(), nil, "#5. STDOUT is available")
>> +test:isnt(app1:get_stdin(), nil, "#5. STDIN is available")
>> +
>> +fiber.sleep(1)
>> +
>> +local app2 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
>> + stdout=fio.PIPE,
>> + stdin=app1:get_stdout()})
>> +
>> +test:isnt(app2, nil, "#5. Starting application 2")
>> +
>> +local test2str = '1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n'
>> +
>> +rc = read_stdout(app2)
>> +
>> +test:is(rc, test2str, "#5. Received exact string")
>> +
>> +rc,err = app1:wait()
>> +test:is(rc, 0, "#5. Process's exited 1")
>> +
>> +rc,err = app2:wait()
>> +test:is(rc, 0, "#5. Process's exited 2")
>> +
>> +app1 = nil
>> +app2 = nil
>> +
>> +-- --------------------------------------------------------------
>> +test:check()
>> +os.exit(0)
>> +
>> diff --git a/test/box-tap/fio_popen_test1.sh b/test/box-tap/fio_popen_test1.sh
>> new file mode 100755
>> index 000000000..da54ee19a
>> --- /dev/null
>> +++ b/test/box-tap/fio_popen_test1.sh
>> @@ -0,0 +1,7 @@
>> +#!/bin/bash
>> +for i in {1..10}
>> +do
>> + echo $i
>> +# sleep 1
>
> Commented line. Please remove.
>
Done.
>> +done
>
> Would be nice to see a test that reads/writes much more to the pipes.
>
Ok.
>> +
>> diff --git a/third_party/libeio/eio.c b/third_party/libeio/eio.c
>> index 7351d5dda..e433b1b3b 100644
>> --- a/third_party/libeio/eio.c
>> +++ b/third_party/libeio/eio.c
>> @@ -1741,7 +1741,24 @@ static int eio_threads;
>> static void ecb_cold
>> eio_prefork()
>> {
>> - eio_threads = etp_set_max_parallel(EIO_POOL, 0);
>> + /*
>> + * When fork() is called libeio shuts
>> + * down all working threads.
>> + * But it causes a deadlock if fork() was
>> + * called from the libeio thread.
>> + * To avoid this do not close the
>> + * thread who called fork().
>> + * This behaviour is acceptable for the
>> + * case when fork() is immediately followed
>> + * by exec().
>> + * To clone a process call fork() from the
>> + * main thread.
>> + */
>> +
>> + if (etp_is_in_pool_thread())
>> + eio_threads = etp_get_max_parallel(EIO_POOL);
>> + else
>> + eio_threads = etp_set_max_parallel(EIO_POOL, 0);
>
> Please investigate why it was initially done that way, because
> I'm afraid this change might break something.
>
A usual precautions before fork: stop all thread before fork and resume
after fork.
This works fine if fork() is called in the main thread.
Of course the current workaround has its disadvantages. But they're not
worth mentioning in the case when fork() is followed by exec.
>> }
>>
>> static void ecb_cold
>> diff --git a/third_party/libeio/etp.c b/third_party/libeio/etp.c
>> index 42f7661eb..38c2d1f9b 100644
>> --- a/third_party/libeio/etp.c
>> +++ b/third_party/libeio/etp.c
>> @@ -164,6 +164,21 @@ struct etp_pool_user
>> xmutex_t lock;
>> };
>>
>> +static __thread int is_eio_thread = 0;
>> +
>> +/**
>> + * Check whether the current thread belongs to
>> + * libeio thread pool or just a generic thread.
>> + *
>> + * @return 0 for generic thread
>> + * @return 1 for libeio thread pool
>> + **/
>> +ETP_API_DECL int ecb_cold
>> +etp_is_in_pool_thread()
>> +{
>> + return is_eio_thread;
>> +}
>> +
>> /* worker threads management */
>>
>> static void ecb_cold
>> @@ -322,6 +337,9 @@ X_THREAD_PROC (etp_proc)
>> self.pool = pool;
>> etp_pool_user user; /* per request */
>>
>> +/* Distinguish libeio threads from the generic threads */
>> + is_eio_thread = 1;
>> +
>> etp_proc_init ();
>>
>> /* try to distribute timeouts somewhat evenly (nanosecond part) */
>> @@ -616,3 +634,13 @@ etp_set_max_parallel (etp_pool pool, unsigned int threads)
>> X_UNLOCK (pool->lock);
>> return retval;
>> }
>> +
>> +ETP_API_DECL int ecb_cold
>> +etp_get_max_parallel (etp_pool pool)
>> +{
>> + int retval;
>> + X_LOCK (pool->lock);
>> + retval = pool->wanted;
>> + X_UNLOCK (pool->lock);
>> + return retval;
>> +}
>> diff --git a/third_party/libev/ev.c b/third_party/libev/ev.c
>> index 6a2648591..5fa8293a1 100644
>> --- a/third_party/libev/ev.c
>> +++ b/third_party/libev/ev.c
>> @@ -4214,6 +4214,8 @@ noinline
>> void
>> ev_signal_stop (EV_P_ ev_signal *w) EV_THROW
>> {
>> + if (!loop)
>> + return;
>
> Why is that? At least deserves a comment. Better handle it at the upper
> level.
It's an at_fork() consequences.
See my comments above.
>
> Anyway, all these changes to libev deserve a separate patch with a
> proper change log comment.
>
ok.
>> clear_pending (EV_A_ (W)w);
>> if (expect_false (!ev_is_active (w)))
>> return;
>
>
>
On 31.05.2019 14:49, Vladimir Davydov wrote:> A few more comments.
Please take a look.
>
> On Wed, May 29, 2019 at 10:08:09AM +0300, Stanislav Zudin wrote:
>> diff --git a/src/lib/core/coio_popen.c b/src/lib/core/coio_popen.c
>> +#include <stddef.h>
>> +#include <signal.h>
>> +#include "coio_popen.h"
>> +#include "coio_task.h"
>> +#include "fiber.h"
>> +#include "say.h"
>> +#include "fio.h"
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <dirent.h>
>> +#include <sys/wait.h>
>> +#include <paths.h>
>
> Why do you need this header? Looks like you never use anything from it.
> Same for dirent.h. Please include only those headers you really need.
>
ok
>> +static struct popen_data *
>> +popen_lookup_data_by_pid(pid_t pid)
>> +{
>> + struct popen_data *cur = popen_data_list;
>> + for(; cur; cur = cur->next) {
>> + if (cur->pid == pid)
>> + return cur;
>> + }
>
> Please write a comment explaining why you think linear search is fine
> here and why we don't use some kind of hash or tree.
>
I believe the number of simultaneously running popen processes is rather
small - tens. In this case hash won't give a significant performance boost.
>> +
>> + return NULL;
>> +}
>> +
>> +static void
>> +popen_exclude_from_list(struct popen_data *data)
>> +{
>> + if (popen_data_list == data) {
>> + popen_data_list = data->next;
>> + return;
>> + }
>> +
>> + /* Find the previous entry */
>> + struct popen_data *prev = popen_data_list;
>> + for( ; prev && prev->next != data; prev = prev->next) {
>> + /* empty */;
>> + }
>> +
>> + if (!prev)
>> + return ;
>> + assert(prev->next == data);
>> + prev->next = data->next;
>
> Looks like doubly-linked list (rlist) would suit better here.
>
ok
>> +}
>> +
>> +/*
>> + * Returns next socket to read.
>> + * Use this function when both STDOUT and STDERR outputs
>> + * are ready for reading.
>
> But what does it do? How does it prioritize? Though short, this function
> looks kinda arcane to me due to this bit manipulation magic. Please
> clarify. However, if you switch to ev io, you'll probably won't be
> needing it.
>
It's just a round-robin.
Used in a case when both STDIN & STDOUT are ready for reading while
popen.read() returns only one result. Doesn't let any stream stuck.
>> + * */
>> +static inline int
>> +get_handle_in_order(struct popen_data *data)
>> +{
>> + /*
>> + * Invert the order of handles to be read
>> + */
>> + const int mask = STDERR_FILENO | STDOUT_FILENO;
>> + data->prev_source ^= mask;
>> +
>> + /*
>> + * If handle is not available, invert it back
>> + */
>> + if (data->fh[data->prev_source] < 0)
>> + data->prev_source ^= mask;
>> + /*
>> + * if both reading handles are invalid return -1
>> + */
>> + return data->fh[data->prev_source];
>> +}
>
>> +void *
>> +coio_popen_impl(char** argv, int argc,
>> + char** environment, int environment_size,
>> + int stdin_fd, int stdout_fd, int stderr_fd)
>> +{
>> + pid_t pid;
>> + int socket_rd[2] = {-1,-1};
>> + int socket_wr[2] = {-1,-1};
>> + int socket_er[2] = {-1,-1};
>> + errno = 0;
>> +
>> + struct popen_data *data = popen_data_new();
>> + if (data == NULL)
>> + return NULL;
>> +
>> + /* Setup a /dev/null if necessary */
>> + bool read_devnull = (stdin_fd == FIO_DEVNULL);
>> + bool write_devnull = (stdout_fd == FIO_DEVNULL) ||
>> + (stderr_fd == FIO_DEVNULL);
>> + int devnull_flags = O_RDWR | O_CREAT;
>> + if (!read_devnull)
>> + devnull_flags = O_WRONLY | O_CREAT;
>> + else if (!write_devnull)
>> + devnull_flags = O_RDONLY | O_CREAT;
>> +
>> + if (read_devnull || write_devnull) {
>> + data->devnull_fd = open("/dev/null", devnull_flags, 0666);
>
> No need to pass mode unless you're creating a file.
It doesn't affect the existing files. Thus one line is shorted than two.
:-)
>
>> + if (data->devnull_fd < 0)
>> + goto on_error;
>> + else {
>> + if (stdin_fd == FIO_DEVNULL)
>> + stdin_fd = data->devnull_fd;
>> + if (stdout_fd == FIO_DEVNULL)
>> + stdout_fd = data->devnull_fd;
>> + if (stderr_fd == FIO_DEVNULL)
>> + stderr_fd = data->devnull_fd;
>> + }
>> + }
>> +
>> + if (stdin_fd == FIO_PIPE) {
>> + /*
>> + * Enable non-blocking for the parent side
>> + * and close-on-exec on the child's side.
>> + * The socketpair on OSX doesn't support
>> + * SOCK_NONBLOCK & SOCK_CLOEXEC flags.
>> + */
>> + if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_rd) < 0 ||
>
> I overlooked it when I reviewed the patch last time. Why do you use
> sockets? Why is a simple pipe not enough? Sockets are full-duplex
> while in your case a simplex connection would be enough, no?
Ask Georgy, he insisted on replacing pipes by socketpair.
>
>> + fcntl(socket_rd[0], F_SETFL, O_NONBLOCK) < 0 ||
>> + fcntl(socket_rd[1], F_SETFD, FD_CLOEXEC) < 0) {
>
> You don't really need FD_CLOEXEC for this one as you can close it in the
> child right after fork(). Moreover, looks like you close() them anyway.
>
> We do need to use CLOEXEC for other fds (e.g. xlog or vinyl files), but
> I guess this can be done in a separate patch.
got it.
>
>> + goto on_error;
>> + }
>> + }
>> +
>> + if (stdout_fd == FIO_PIPE) {
>> + if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_wr) < 0 ||
>> + fcntl(socket_wr[0], F_SETFL, O_NONBLOCK) < 0 ||
>> + fcntl(socket_wr[1], F_SETFD, FD_CLOEXEC) < 0) {
>> + goto on_error;
>> + }
>> + }
>> +
>> + if (stderr_fd == FIO_PIPE) {
>> + if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_er) < 0 ||
>> + fcntl(socket_er[0], F_SETFL, O_NONBLOCK) < 0 ||
>> + fcntl(socket_er[1], F_SETFD, FD_CLOEXEC) < 0) {
>
> Looks like quite a bit of repetition. Worth moving this to a separate
> helper function or doing it in a loop somehow instead of copying and
> pasting?
>
i'll think about that.
>> + goto on_error;
>> + }
>> + }
>> +
>> + pid = fork();
>> +
>> + if (pid < 0)
>> + goto on_error;
>> + else if (pid == 0) /* child */ {
>> + /* Setup stdin/stdout */
>> + if (stdin_fd == FIO_PIPE) {
>> + close(socket_rd[0]); /* close parent's side */
>> + stdin_fd = socket_rd[1];
>> + }
>> + if (stdin_fd != STDIN_FILENO) {
>> + dup2(stdin_fd, STDIN_FILENO);
>> + close(stdin_fd);
>> + }
>> +
>> + if (stdout_fd == FIO_PIPE) {
>> + close(socket_wr[0]); /* close parent's side */
>> + stdout_fd = socket_wr[1];
>> + }
>> + if (stdout_fd != STDOUT_FILENO) {
>> + dup2(stdout_fd, STDOUT_FILENO);
>> + if (stdout_fd != STDERR_FILENO)
>> + close(stdout_fd);
>> + }
>> +
>> + if (stderr_fd == FIO_PIPE) {
>> + close(socket_er[0]); /* close parent's side */
>> + stderr_fd = socket_er[1];
>> + }
>> + if (stderr_fd != STDERR_FILENO) {
>> + dup2(stderr_fd, STDERR_FILENO);
>> + if (stderr_fd != STDOUT_FILENO)
>> + close(stderr_fd);
>> + }
>> +
>> + execve( argv[0], argv,
>> + environment ? environment : environ);
>> + _exit(127);
>
> Why _exit(), why not exit()? If there's a reason, please explain in a
> comment. Also, why 127? We prefer to avoid explicit numeric constants.
> May be, you could use EXIT_FAILURE instead?
_exit() doesn't call atexit(), but in particular this case it doesn't
matter.
As to exit code i need to distinguish exit code returned by child
process from execve error.
The native implementation of popen does return 127.
>
> Also, may be we need to write something to stderr in this case, just to
> let the caller now that it's not the child failure, it's execve failure.
Good idea.
>
>> +int
>> +coio_popen_destroy(void *fh)
>> +{
>> + struct popen_data *data = (struct popen_data *)fh;
>> +
>> + if (data == NULL){
>> + errno = EBADF;
>> + return -1;
>> + }
>
> What's the point in returning an error from this function?
> You never check it AFAICS.
>
> Anyway, like I mentioned above, I think that one shouldn't pass NULL for
> a handle to any of popen functions.
>
ok
>> +void
>> +coio_popen_child_is_dead(int sig, siginfo_t *si, void *context)
>
> Judging by the name it looks like this function should merely return
> true/false, not do anything else. Please rename. For instance, we could
> name it coio_popen_on_child_termination or handle_child_death.
ok
>
>> +{
>> + (void)context; /* UNUSED */
>
> Why pass it at all if it's unused?
>
Because C compiler requires the argument to be defined.
[6.9.1.5] If the declarator includes a parameter type list, the
declaration of each parameter shall include an identifier, except for
the special case of a parameter list consisting of a single parameter of
type void, in which case there shall not be an identifier. No
declaration list shall follow.
Changing the compiler flags is too much, IMO.
>> +
>> + if (sig != SIGCHLD)
>> + return;
>
> Why call it if it isn't SIGCHLD?
>
Removed.
>> +
>> + /*
>> + * The sigaction is called with SA_NOCLDWAIT,
>> + * so no need to call waitpid()
>> + */
>> +
>> + popen_lock_data_list();
>> +
>> + struct popen_data *data = popen_lookup_data_by_pid(si->si_pid);
>> + if (data) {
>> + switch (si->si_code) {
>> + case CLD_EXITED:
>> + data->exit_code = si->si_status;
>> + data->status = POPEN_EXITED;
>> + break;
>> + case CLD_KILLED:
>> + data->signal_id = si->si_status;
>> + data->status = POPEN_KILLED;
>> + break;
>> + case CLD_DUMPED:
>> + /* exit_status makes no sense */
>> + data->status = POPEN_DUMPED;
>> + break;
>> + }
>> +
>> + /*
>> + * We shouldn't close file descriptors here.
>> + * The child process may exit earlier than
>> + * the parent process finishes reading data.
>> + * In this case the reading fails.
>> + */
>> + }
>> +
>> + popen_unlock_data_list();
>> +}
>
>> diff --git a/src/lib/core/coio_popen.h b/src/lib/core/coio_popen.h
>> +/**
>> + * Special values of the file descriptors passed to fio.popen
>> + * */
>> +enum {
>> + FIO_PIPE = -2,
>> + /*
>> + * Tells fio.popen to open a handle for
>> + * direct reading/writing
>> + */
>> +
>> + FIO_DEVNULL = -3
>> + /*
>> + * Tells fio.popen to redirect the given standard
>> + * stream into /dev/null
>> + */
>> +};
>> +
>> +/**
>> + * Possible status of the process started via fio.popen
>> + **/
>> +enum popen_status {
>> + POPEN_UNKNOWN = -1,
>
> Why do you need this mysterious status code? Looking at the code, I see
> that you coio_popen_get_status() returns it in case of error. But what
> kind of error? Turns out that this is the case when the handle is NULL.
> This looks pointless to me - one shouldn't pass NULL to that function.
> Am I missing something?
Removed.
>
>> +
>> + POPEN_RUNNING = 1,
>> + /*the process is alive and well*/
>
> Comments should be above constants, not below. Also, please use
> formatting typically used in the code (starts with /**, capital
> letter, a dot at the end).
>
Ok.
>> +
>> + POPEN_EXITED = 2,
>> + /*the process exited*/
>> +
>> + POPEN_KILLED = 3,
>> + /*the process terminated by a signal*/
>> +
>> + POPEN_DUMPED = 4
>> + /* the process terminated abnormally */
>> +};
>
>> +/**
>> + * Returns status of the associated process.
>> + *
>> + * @param fd - handle returned by fio.popen.
>> + * @param signal_id - if not NULL accepts the signal
>> + * sent to terminate the process
>> + * @param exit_code - if not NULL accepts the exit code
>> + * if the process terminated normally.
>> + *
>> + * @return one of the following values:
>> + * POPEN_RUNNING if the process is alive
>> + * POPEN_EXITED if the process was terminated normally
>> + * POPEN_KILLED if the process was terminated by a signal
>> + * POPEN_UNKNOWN an error's occurred
>> + */
>> +int
>> +coio_popen_get_status(void *fh, int *signal_id, int *exit_code);
>> +
>> +/**
>> + * Handle SIGCHLD signal
>> + */
>> +void
>> +coio_popen_child_is_dead(int sig, siginfo_t *si, void *);
>
> Better not use siginfo_t here - instead pass what you really need.
> This makes the API clearer and cuts the dependency list.
>
coio_popen_child_is_dead is passed to sigaction.
To separate it from siginfo_t we need one more function.
No problem, will add.
>> diff --git a/src/lua/fio.c b/src/lua/fio.c
>> +/**
>> + * A wrapper applicable for using with lua's GC.
>> + * */
>> +struct fio_popen_wrap {
>
> Why do you need a wrapper at all?
Right now it contains only a "handle" (a pointer to
struct popen_data *).
Later we may need to keep something else in the managed memory.
It's more readable than just a pointer to pointer.
>
>> + void* handle;
>> +};
>> +static const char* fio_popen_typename = "fio.popen.data";
>> +
>> +static struct fio_popen_wrap *
>> +fio_popen_get_handle_wrap(lua_State *L, int index)
>> +{
>> + luaL_checktype(L, index, LUA_TUSERDATA);
>> + struct fio_popen_wrap *wrap = (struct fio_popen_wrap *)
>> + luaL_checkudata(L, index, fio_popen_typename);
>
> Hmm, why do you use udata? Why not plain cdata? I keep forgetting
> the difference, to tell the truth.
The Lua finalizer deals only with "managed" memory.
It's not aware of cdata and can't release it.
>
>> diff --git a/src/lua/fio.lua b/src/lua/fio.lua
>> +popen_methods.do_read = function(self, buf, size, seconds)
>> + if size == nil or type(size) ~= 'number' then
>> + error('fio.popen.read: invalid size argument')
>> + end
>> + if seconds == nil or type(seconds) ~= 'number' then
>> + error('fio.popen.read: invalid seconds argument')
>> + end
>> +
>> + local tmpbuf
>> + if not ffi.istype(const_char_ptr_t, buf) then
>> + tmpbuf = buffer.ibuf()
>> + buf = tmpbuf:reserve(size)
>> + end
>> +
>> + local res, output_no, err = internal.popen_read(self.fh, buf,
size, seconds)
>> + if res == nil then
>> + if tmpbuf ~= nil then
>> + tmpbuf:recycle()
>> + end
>> + return nil, nil, err
>> + end
>> +
>> + if tmpbuf ~= nil then
>> + tmpbuf:alloc(res)
>> + res = ffi.string(tmpbuf.rpos, tmpbuf:size())
>> + tmpbuf:recycle()
>> + end
>> + return res, output_no
>> +end
>> +
>> +-- read stdout & stderr of the process started by fio.popen
>> +-- read() -> str, source
>> +-- read(buf) -> len, source
>> +-- read(size) -> str, source
>> +-- read(buf, size) -> len, source
>> +-- source contains id of the stream, fio.STDOUT or fio.STDERR
>> +popen_methods.read = function(self, buf, size)
>> + if self.fh == nil then
>> + return nil, nil, 'Invalid object'
>> + end
>> +
>> + if buf == nil and size == nil then
>> + -- read()
>> + size = 512
>
> Why 512? Aren't we supposed to read everything till EOF in this case?
The behaviour was borrowed from fio.read()
The reading everything till EOF causes problems in reading from both
sources: STDIN and STDOUT. While you waiting one source another one may
get stuck.
>
>> + elseif ffi.istype(const_char_ptr_t, buf) and size == nil then
>> + -- read(buf)
>> + size = 512
>> + elseif not ffi.istype(const_char_ptr_t, buf) and
>> + buf ~= nil and size == nil then
>> + -- read(size)
>> + size = tonumber(buf)
>> + buf = nil
>> + elseif ffi.istype(const_char_ptr_t, buf) and size ~= nil then
>> + -- read(buf, size)
>> + size = tonumber(size)
>> + else
>> + error("fio.popen.read: invalid arguments")
>> + end
>> +
>> + return self:do_read(buf, size, -1)
>> +end
>
>> +popen_methods.wait = function(self, timeout)
>> + if self.fh == nil then
>> + return nil, 'Invalid object'
>> + end
>> +
>> + if timeout == nil then
>> + timeout = -1
>> + else
>> + timeout = tonumber(timeout)
>> + end
>> +
>> + local rc, err = internal.popen_wait(self.fh, timeout)
>> + if rc ~= nil then
>> + self.exit_code = tonumber(rc)
>> + self.fh = nil
>
> This is kinda unexpected. What if we want to read the process output
> after waiting for it to complete.
And what do you expect to read after process is complete?
If application is a some kind of linear script it produces some output
and then exits. In this case the caller reads everything till the EOF.
BTW, in our tests test scripts exit earlier than the parent starts
reading their output.
For interactive applications who may wait for input forever we've got a
kill(). It's allowed to read after sending signal and before calling
wait(). In this case we won't miss anything from the child process output.
So my point is: wait() is a synonim to close() (or join() in C++ world).
It releases all resources.
>
> Come to think of it, we might need an explicit knob to destroy an object
> even before it's collected. May be, consider re-adding close() for this?
> Please consult with Alexander and/or Georgy re this.
>
Sasha, George, what do you think?
>> + return rc
>> + else
>> + return nil,err
>> + end
>> +end
>
>> diff --git a/test/box-tap/fio_popen.test.lua
b/test/box-tap/fio_popen.test.lua
>> new file mode 100755
>> index 000000000..0c2bd7e70
>> --- /dev/null
>> +++ b/test/box-tap/fio_popen.test.lua
>> @@ -0,0 +1,189 @@
>> +#!/usr/bin/env tarantool
>> +
>> +local tap = require('tap')
>> +local fio = require('fio')
>> +local errno = require('errno')
>> +local fiber = require('fiber')
>> +local test = tap.test()
>> +
>> +test:plan(5+9+11+9+8)
>> +
>> +-- Preliminaries
>> +local function read_stdout(app)
>> + local ss = ""
>> +
>> + local s,src,err = app:read(128)
>> +
>> + while s ~= nil and s ~= "" do
>> + ss = ss .. s
>> +
>> + s,src,err = app:read(128)
>> + end
>> +
>> + return ss
>> +end
>> +
>> +-- Test 1. Run application, check its status, kill and wait
>> +local app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
>> + stdout=fio.STDOUT,
>> + stderr=fio.STDOUT})
>> +test:isnt(app1, nil, "#1. Starting a existing application")
>> +
>> +local rc = app1:get_status()
>> +test:is(rc, nil, "#1. Process is running")
>> +
>> +rc,err = app1:kill()
>> +test:is(rc, true, "#1. Sending kill(15)")
>> +
>> +rc,err = app1:wait(5)
>> +test:is(rc, -15, "#1. Process was killed")
>> +
>> +rc = app1:get_status()
>> +test:is(rc, -15, "#1. Process was killed 2")
>> +
>> +app1 = nil
>> +
>> +-- Test 2. Run application, write to stdin, read from stdout
>> +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
>> + stdout=fio.PIPE,
>> + stdin=fio.PIPE,
>> + stderr=fio.STDOUT})
>> +test:isnt(app1, nil, "#2. Starting a existing application")
>> +
>> +rc = app1:get_status()
>> +test:is(rc, nil, "#2. Process is running")
>> +
>> +local test2str = '123\n456\n789'
>> +
>> +app1:write(test2str)
>> +rc,src,err = app1:read(256)
>
> Please also test the case when the child writes both to
> stdin and stdout (looks like you missed it).
Did you mean stderr & stdout?
>
>> +
>> +test:is(src, fio.STDOUT, "#2. Read from STDOUT")
>> +test:is(rc, test2str, "#2. Received exact string")
>> +
>> +test2str = 'abc\ndef\ngh'
>> +
>> +app1:write(test2str, 4)
>> +local rc2,src2,err = app1:read(6)
>> +
>> +test:is(err, nil, "#2. Reading ok")
>> +test:is(src2, fio.STDOUT, "#2. Read from STDOUT 2")
>> +test:is(rc2, 'abc\n', "#2. Received exact string 2")
>> +
>> +rc,err = app1:kill()
>> +test:is(rc, true, "#2. Sending kill(15)")
>> +
>> +rc,err = app1:wait()
>> +test:is(rc, -15, "#2. Process was killed")
>> +
>> +app1 = nil
>> +
>> +-- Test 3. read from stdout with timeout
>> +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
>> + stdout=fio.PIPE,
>> + stdin=fio.PIPE,
>> + stderr=fio.STDOUT})
>> +test:isnt(app1, nil, "#3. Starting a existing application")
>> +test:is(app1:get_stderr(), nil, "#3. STDERR is redirected")
>> +test:isnt(app1:get_stdout(), nil, "#3. STDOUT is available")
>> +test:isnt(app1:get_stdin(), nil, "#3. STDIN is available")
>> +
>> +
>> +rc = app1:get_status()
>> +test:is(rc, nil, "#3. Process is running")
>> +
>> +rc,src,err = app1:read2(256, 2)
>> +
>> +local e = errno()
>> +test:is(e, errno.ETIMEDOUT, "#3. Timeout")
>> +
>> +
>> +local test2str = '123\n456\n789'
>> +
>> +app1:write(test2str)
>> +rc,src,err = app1:read2(256, 2)
>> +
>> +test:is(err, nil, "#3. Reading ok")
>> +test:is(src, fio.STDOUT, "#3. Read from STDOUT")
>> +test:is(rc, test2str, "#3. Received exact string")
>> +
>> +rc,err = app1:kill('SIGHUP')
>> +test:is(rc, true, "#3. Sending kill(1)")
>> +
>> +rc,err = app1:wait()
>> +test:is(rc, -1, "#3. Process was killed")
>> +
>> +app1 = nil
>> +
>> +-- Test 4. Redirect from file
>> +local build_path = os.getenv("BUILDDIR")
>> +local txt_filename = fio.pathjoin(build_path,
'test/box-tap/fio_popen.sample.txt')
>> +
>> +local txt_file = fio.open(txt_filename, {'O_RDONLY'})
>> +test:isnt(txt_file, nil, "#4. Open existing file for reading")
>> +
>> +app1 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
>> + stdout=fio.PIPE,
>> + stdin=txt_file,
>> + stderr=fio.STDOUT})
>> +test:isnt(app1, nil, "#4. Starting a existing application")
>> +test:is(app1:get_stderr(), nil, "#4. STDERR is redirected")
>> +test:isnt(app1:get_stdout(), nil, "#4. STDOUT is available")
>> +test:is(app1:get_stdin(), nil, "#4. STDIN is redirected")
>> +
>> +rc = app1:get_status()
>> +test:is(rc, nil, "#4. Process is running")
>> +
>> +rc,src,err = app1:read2(256, 2)
>> +
>> +test:is(src, fio.STDOUT, "#4. Read from STDOUT")
>> +
>> +local test2str = 'AAA\nBBB\nCCC\nDDD\n\n'
>> +
>> +test:is(rc, test2str, "#4. Received exact string")
>> +
>> +rc,err = app1:wait()
>> +test:is(rc, 0, "#4. Process's exited")
>> +
>> +app1 = nil
>> +txt_file:close()
>> +
>> +-- Test 5. Redirect output from one process to another
>> +local app_path = fio.pathjoin(build_path,
'test/box-tap/fio_popen_test1.sh')
>> +
>> +app1 = fio.popen({argv = {'/bin/sh', '-c', app_path},
>> + stdout=fio.PIPE,
>> + stderr=fio.STDOUT})
>> +
>> +test:isnt(app1, nil, "#5. Starting application 1")
>> +test:is(app1:get_stderr(), nil, "#5. STDERR is redirected")
>> +test:isnt(app1:get_stdout(), nil, "#5. STDOUT is available")
>> +test:isnt(app1:get_stdin(), nil, "#5. STDIN is available")
>> +
>> +fiber.sleep(1)
>> +
>> +local app2 = fio.popen({argv = {'/bin/sh', '-c', 'cat'},
>> + stdout=fio.PIPE,
>> + stdin=app1:get_stdout()})
>> +
>> +test:isnt(app2, nil, "#5. Starting application 2")
>> +
>> +local test2str = '1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n'
>> +
>> +rc = read_stdout(app2)
>
> Please also test reading/writing/killing in case when the child is dead.
>
ok
>> +
>> +test:is(rc, test2str, "#5. Received exact string")
>> +
>> +rc,err = app1:wait()
>> +test:is(rc, 0, "#5. Process's exited 1")
>> +
>> +rc,err = app2:wait()
>> +test:is(rc, 0, "#5. Process's exited 2")
>> +
>> +app1 = nil
>> +app2 = nil
>> +
>> +-- --------------------------------------------------------------
>> +test:check()
>> +os.exit(0)
>> +
>> diff --git a/test/box-tap/fio_popen_test1.sh
b/test/box-tap/fio_popen_test1.sh
>> new file mode 100755
>> index 000000000..da54ee19a
>> --- /dev/null
>> +++ b/test/box-tap/fio_popen_test1.sh
>> @@ -0,0 +1,7 @@
>> +#!/bin/bash
>> +for i in {1..10}
>> +do
>> + echo $i
>> +# sleep 1
>> +done
>> +
>
More information about the Tarantool-patches
mailing list