From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55384) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHT5M-0000kP-Op for qemu-devel@nongnu.org; Fri, 08 Jan 2016 04:11:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aHT5H-0007Z7-N2 for qemu-devel@nongnu.org; Fri, 08 Jan 2016 04:11:52 -0500 Received: from mail-wm0-x22c.google.com ([2a00:1450:400c:c09::22c]:35806) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHT5H-0007Yw-Cz for qemu-devel@nongnu.org; Fri, 08 Jan 2016 04:11:47 -0500 Received: by mail-wm0-x22c.google.com with SMTP id f206so127150565wmf.0 for ; Fri, 08 Jan 2016 01:11:47 -0800 (PST) Sender: Paolo Bonzini References: <1450441266-543-1-git-send-email-berrange@redhat.com> <1450441266-543-9-git-send-email-berrange@redhat.com> From: Paolo Bonzini Message-ID: <568F7D50.6000604@redhat.com> Date: Fri, 8 Jan 2016 10:11:44 +0100 MIME-Version: 1.0 In-Reply-To: <1450441266-543-9-git-send-email-berrange@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL v4 8/9] io: add QIOChannelCommand class List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org Cc: Peter Maydell > > +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 . > + * > + */ > + > +#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" >