[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL v4 8/9] io: add QIOChannelCommand class
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PULL v4 8/9] io: add QIOChannelCommand class |
Date: |
Fri, 8 Jan 2016 10:11:44 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 |
>
> +static void qio_channel_command_finalize(Object *obj)
> +{
> + QIOChannelCommand *ioc = QIO_CHANNEL_COMMAND(obj);
> + if (ioc->readfd != -1) {
> + close(ioc->readfd);
> + ioc->readfd = -1;
> + }
> + if (ioc->writefd != -1) {
> + close(ioc->writefd);
> + ioc->writefd = -1;
> + }
You're not handling correctly the case where ioc->readfd == ioc->writefd
(possible if both are /dev/null).
> + if (stdinnull || stdoutnull) {
> + devnull = open("/dev/null", O_RDWR);
> + if (!devnull) {
Wrong check, should be "devnull < 0".
> + error_setg_errno(errp, errno,
> + "Unable to open /dev/null");
> + goto error;
> + }
> + }
> +
> + if ((!stdinnull && pipe(stdinfd) < 0) ||
> + (!stdoutnull && pipe(stdoutfd) < 0)) {
> + error_setg_errno(errp, errno,
> + "Unable to open pipe");
> + goto error;
> + }
> +
> + pid = qemu_fork(errp);
> + if (pid < 0) {
> + goto error;
> + }
> +
> + if (pid == 0) { /* child */
> + dup2(stdinnull ? devnull : stdinfd[0], STDIN_FILENO);
> + dup2(stdoutnull ? devnull : stdoutfd[1], STDOUT_FILENO);
> + /* Leave stderr connected to qemu's stderr */
> +
> + if (!stdinnull) {
> + close(stdinfd[0]);
> + close(stdinfd[1]);
> + }
> + if (!stdoutnull) {
> + close(stdoutfd[0]);
> + close(stdoutfd[1]);
> + }
devnull should be closed here if it is not -1...
> + execv(argv[0], (char * const *)argv);
> + _exit(1);
> + }
> + if (!stdinnull) {
> + close(stdinfd[0]);
> + }
> + if (!stdoutnull) {
> + close(stdoutfd[1]);
> + }
> +
> + ioc = qio_channel_command_new_pid(stdinnull ? devnull : stdinfd[1],
> + stdoutnull ? devnull : stdoutfd[0],
> + pid);
> + trace_qio_channel_command_new_spawn(ioc, argv[0], flags);
> + return ioc;
> +
> + error:
... and here too.
Paolo
> + if (stdinfd[0] != -1) {
> + close(stdinfd[0]);
> + }
> + if (stdinfd[1] != -1) {
> + close(stdinfd[1]);
> + }
> + if (stdoutfd[0] != -1) {
> + close(stdoutfd[0]);
> + }
> + if (stdoutfd[1] != -1) {
> + close(stdoutfd[1]);
> + }
> + return NULL;
> +}
> +
> +#else /* WIN32 */
> +QIOChannelCommand *
> +qio_channel_command_new_spawn(const char *const argv[],
> + int flags,
> + Error **errp)
> +{
> + error_setg_errno(errp, ENOSYS,
> + "Command spawn not supported on this platform");
> + return NULL;
> +}
> +#endif /* WIN32 */
> +
> +#ifndef WIN32
> +static int qio_channel_command_abort(QIOChannelCommand *ioc,
> + Error **errp)
> +{
> + pid_t ret;
> + int status;
> + int step = 0;
> +
> + /* See if intermediate process has exited; if not, try a nice
> + * SIGTERM followed by a more severe SIGKILL.
> + */
> + rewait:
> + trace_qio_channel_command_abort(ioc, ioc->pid);
> + ret = waitpid(ioc->pid, &status, WNOHANG);
> + trace_qio_channel_command_wait(ioc, ioc->pid, ret, status);
> + if (ret == (pid_t)-1) {
> + if (errno == EINTR) {
> + goto rewait;
> + } else {
> + error_setg_errno(errp, errno,
> + "Cannot wait on pid %llu",
> + (unsigned long long)ioc->pid);
> + return -1;
> + }
> + } else if (ret == 0) {
> + if (step == 0) {
> + kill(ioc->pid, SIGTERM);
> + } else if (step == 1) {
> + kill(ioc->pid, SIGKILL);
> + } else {
> + error_setg(errp,
> + "Process %llu refused to die",
> + (unsigned long long)ioc->pid);
> + return -1;
> + }
> + usleep(10 * 1000);
> + goto rewait;
> + }
> +
> + return 0;
> +}
> +#endif /* ! WIN32 */
> +
> +
> +static void qio_channel_command_init(Object *obj)
> +{
> + QIOChannelCommand *ioc = QIO_CHANNEL_COMMAND(obj);
> + ioc->readfd = -1;
> + ioc->writefd = -1;
> + ioc->pid = -1;
> +}
> +
> +static void qio_channel_command_finalize(Object *obj)
> +{
> + QIOChannelCommand *ioc = QIO_CHANNEL_COMMAND(obj);
> + if (ioc->readfd != -1) {
> + close(ioc->readfd);
> + ioc->readfd = -1;
> + }
> + if (ioc->writefd != -1) {
> + close(ioc->writefd);
> + ioc->writefd = -1;
> + }
> + if (ioc->pid > 0) {
> +#ifndef WIN32
> + qio_channel_command_abort(ioc, NULL);
> +#endif
> + }
> +}
> +
> +
> +static ssize_t qio_channel_command_readv(QIOChannel *ioc,
> + const struct iovec *iov,
> + size_t niov,
> + int **fds,
> + size_t *nfds,
> + Error **errp)
> +{
> + QIOChannelCommand *cioc = QIO_CHANNEL_COMMAND(ioc);
> + ssize_t ret;
> +
> + retry:
> + ret = readv(cioc->readfd, iov, niov);
> + if (ret < 0) {
> + if (errno == EAGAIN ||
> + errno == EWOULDBLOCK) {
> + return QIO_CHANNEL_ERR_BLOCK;
> + }
> + if (errno == EINTR) {
> + goto retry;
> + }
> +
> + error_setg_errno(errp, errno,
> + "Unable to read from command");
> + return -1;
> + }
> +
> + return ret;
> +}
> +
> +static ssize_t qio_channel_command_writev(QIOChannel *ioc,
> + const struct iovec *iov,
> + size_t niov,
> + int *fds,
> + size_t nfds,
> + Error **errp)
> +{
> + QIOChannelCommand *cioc = QIO_CHANNEL_COMMAND(ioc);
> + ssize_t ret;
> +
> + retry:
> + ret = writev(cioc->writefd, iov, niov);
> + if (ret <= 0) {
> + if (errno == EAGAIN ||
> + errno == EWOULDBLOCK) {
> + return QIO_CHANNEL_ERR_BLOCK;
> + }
> + if (errno == EINTR) {
> + goto retry;
> + }
> + error_setg_errno(errp, errno, "%s",
> + "Unable to write to command");
> + return -1;
> + }
> + return ret;
> +}
> +
> +static int qio_channel_command_set_blocking(QIOChannel *ioc,
> + bool enabled,
> + Error **errp)
> +{
> + QIOChannelCommand *cioc = QIO_CHANNEL_COMMAND(ioc);
> +
> + if (enabled) {
> + qemu_set_block(cioc->writefd);
> + qemu_set_block(cioc->readfd);
> + } else {
> + qemu_set_nonblock(cioc->writefd);
> + qemu_set_nonblock(cioc->readfd);
> + }
> +
> + return 0;
> +}
> +
> +
> +static int qio_channel_command_close(QIOChannel *ioc,
> + Error **errp)
> +{
> + QIOChannelCommand *cioc = QIO_CHANNEL_COMMAND(ioc);
> + int rv = 0;
> +
> + /* We close FDs before killing, because that
> + * gives a better chance of clean shutdown
> + */
> + if (close(cioc->writefd) < 0) {
> + rv = -1;
> + }
> + if (close(cioc->readfd) < 0) {
> + rv = -1;
> + }
> +#ifndef WIN32
> + if (qio_channel_command_abort(cioc, errp) < 0) {
> + return -1;
> + }
> +#endif
> + if (rv < 0) {
> + error_setg_errno(errp, errno, "%s",
> + "Unable to close command");
> + }
> + return rv;
> +}
> +
> +
> +static GSource *qio_channel_command_create_watch(QIOChannel *ioc,
> + GIOCondition condition)
> +{
> + QIOChannelCommand *cioc = QIO_CHANNEL_COMMAND(ioc);
> + return qio_channel_create_fd_pair_watch(ioc,
> + cioc->readfd,
> + cioc->writefd,
> + condition);
> +}
> +
> +
> +static void qio_channel_command_class_init(ObjectClass *klass,
> + void *class_data G_GNUC_UNUSED)
> +{
> + QIOChannelClass *ioc_klass = QIO_CHANNEL_CLASS(klass);
> +
> + ioc_klass->io_writev = qio_channel_command_writev;
> + ioc_klass->io_readv = qio_channel_command_readv;
> + ioc_klass->io_set_blocking = qio_channel_command_set_blocking;
> + ioc_klass->io_close = qio_channel_command_close;
> + ioc_klass->io_create_watch = qio_channel_command_create_watch;
> +}
> +
> +static const TypeInfo qio_channel_command_info = {
> + .parent = TYPE_QIO_CHANNEL,
> + .name = TYPE_QIO_CHANNEL_COMMAND,
> + .instance_size = sizeof(QIOChannelCommand),
> + .instance_init = qio_channel_command_init,
> + .instance_finalize = qio_channel_command_finalize,
> + .class_init = qio_channel_command_class_init,
> +};
> +
> +static void qio_channel_command_register_types(void)
> +{
> + type_register_static(&qio_channel_command_info);
> +}
> +
> +type_init(qio_channel_command_register_types);
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 810b4f0..cc9aeec 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -24,6 +24,8 @@ test-cutils
> test-hbitmap
> test-int128
> test-iov
> +test-io-channel-command
> +test-io-channel-command.fifo
> test-io-channel-file
> test-io-channel-file.txt
> test-io-channel-socket
> diff --git a/tests/Makefile b/tests/Makefile
> index 9d95350..40c3855 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -88,6 +88,7 @@ check-unit-y += tests/test-io-task$(EXESUF)
> check-unit-y += tests/test-io-channel-socket$(EXESUF)
> check-unit-y += tests/test-io-channel-file$(EXESUF)
> check-unit-$(CONFIG_GNUTLS) += tests/test-io-channel-tls$(EXESUF)
> +check-unit-y += tests/test-io-channel-command$(EXESUF)
>
> check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
>
> @@ -482,6 +483,8 @@ tests/test-io-channel-file$(EXESUF):
> tests/test-io-channel-file.o \
> tests/test-io-channel-tls$(EXESUF): tests/test-io-channel-tls.o \
> tests/crypto-tls-x509-helpers.o tests/pkix_asn1_tab.o \
> tests/io-channel-helpers.o $(test-io-obj-y)
> +tests/test-io-channel-command$(EXESUF): tests/test-io-channel-command.o \
> + tests/io-channel-helpers.o $(test-io-obj-y)
>
> libqos-obj-y = tests/libqos/pci.o tests/libqos/fw_cfg.o tests/libqos/malloc.o
> libqos-obj-y += tests/libqos/i2c.o tests/libqos/libqos.o
> diff --git a/tests/test-io-channel-command.c b/tests/test-io-channel-command.c
> new file mode 100644
> index 0000000..03cac36
> --- /dev/null
> +++ b/tests/test-io-channel-command.c
> @@ -0,0 +1,129 @@
> +/*
> + * QEMU I/O channel command test
> + *
> + * Copyright (c) 2015 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see
> <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "io/channel-command.h"
> +#include "io-channel-helpers.h"
> +
> +#ifndef WIN32
> +static void test_io_channel_command_fifo(bool async)
> +{
> +#define TEST_FIFO "tests/test-io-channel-command.fifo"
> + QIOChannel *src, *dst;
> + QIOChannelTest *test;
> + char *srcfifo = g_strdup_printf("PIPE:%s,wronly", TEST_FIFO);
> + char *dstfifo = g_strdup_printf("PIPE:%s,rdonly", TEST_FIFO);
> + const char *srcargv[] = {
> + "/bin/socat", "-", srcfifo, NULL,
> + };
> + const char *dstargv[] = {
> + "/bin/socat", dstfifo, "-", NULL,
> + };
> +
> + unlink(TEST_FIFO);
> + if (access("/bin/socat", X_OK) < 0) {
> + return; /* Pretend success if socat is not present */
> + }
> + if (mkfifo(TEST_FIFO, 0600) < 0) {
> + abort();
> + }
> + src = QIO_CHANNEL(qio_channel_command_new_spawn(srcargv,
> + O_WRONLY,
> + &error_abort));
> + dst = QIO_CHANNEL(qio_channel_command_new_spawn(dstargv,
> + O_RDONLY,
> + &error_abort));
> +
> + test = qio_channel_test_new();
> + qio_channel_test_run_threads(test, async, src, dst);
> + qio_channel_test_validate(test);
> +
> + object_unref(OBJECT(src));
> + object_unref(OBJECT(dst));
> +
> + g_free(srcfifo);
> + g_free(dstfifo);
> + unlink(TEST_FIFO);
> +}
> +
> +
> +static void test_io_channel_command_fifo_async(void)
> +{
> + test_io_channel_command_fifo(true);
> +}
> +
> +static void test_io_channel_command_fifo_sync(void)
> +{
> + test_io_channel_command_fifo(false);
> +}
> +
> +
> +static void test_io_channel_command_echo(bool async)
> +{
> + QIOChannel *ioc;
> + QIOChannelTest *test;
> + const char *socatargv[] = {
> + "/bin/socat", "-", "-", NULL,
> + };
> +
> + if (access("/bin/socat", X_OK) < 0) {
> + return; /* Pretend success if socat is not present */
> + }
> +
> + ioc = QIO_CHANNEL(qio_channel_command_new_spawn(socatargv,
> + O_RDWR,
> + &error_abort));
> + test = qio_channel_test_new();
> + qio_channel_test_run_threads(test, async, ioc, ioc);
> + qio_channel_test_validate(test);
> +
> + object_unref(OBJECT(ioc));
> +}
> +
> +
> +static void test_io_channel_command_echo_async(void)
> +{
> + test_io_channel_command_echo(true);
> +}
> +
> +static void test_io_channel_command_echo_sync(void)
> +{
> + test_io_channel_command_echo(false);
> +}
> +#endif
> +
> +int main(int argc, char **argv)
> +{
> + module_call_init(MODULE_INIT_QOM);
> +
> + g_test_init(&argc, &argv, NULL);
> +
> +#ifndef WIN32
> + g_test_add_func("/io/channel/command/fifo/sync",
> + test_io_channel_command_fifo_sync);
> + g_test_add_func("/io/channel/command/fifo/async",
> + test_io_channel_command_fifo_async);
> + g_test_add_func("/io/channel/command/echo/sync",
> + test_io_channel_command_echo_sync);
> + g_test_add_func("/io/channel/command/echo/async",
> + test_io_channel_command_echo_async);
> +#endif
> +
> + return g_test_run();
> +}
> diff --git a/trace-events b/trace-events
> index ae6ad22..6f03638 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1858,3 +1858,9 @@ qio_channel_websock_handshake_pending(void *ioc, int
> status) "Websock handshake
> qio_channel_websock_handshake_reply(void *ioc) "Websock handshake reply
> ioc=%p"
> qio_channel_websock_handshake_fail(void *ioc) "Websock handshake fail ioc=%p"
> qio_channel_websock_handshake_complete(void *ioc) "Websock handshake
> complete ioc=%p"
> +
> +# io/channel-command.c
> +qio_channel_command_new_pid(void *ioc, int writefd, int readfd, int pid)
> "Command new pid ioc=%p writefd=%d readfd=%d pid=%d"
> +qio_channel_command_new_spawn(void *ioc, const char *binary, int flags)
> "Command new spawn ioc=%p binary=%s flags=%d"
> +qio_channel_command_abort(void *ioc, int pid) "Command abort ioc=%p pid=%d"
> +qio_channel_command_wait(void *ioc, int pid, int ret, int status) "Command
> abort ioc=%p pid=%d ret=%d status=%d"
>