Re: [Qemu-devel] [PULL v4 8/9] io: add QIOChannelCommand class

[Paolo Bonzini]

On 18/12/2015 13:21, Daniel P. Berrange wrote:
> +#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, , 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);

Hi Daniel,

Coverity complains here that step is never set to 1.

Paolo

> +} 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 */
> +

Re: [Qemu-devel] [PULL v4 8/9] io: add QIOChannelCommand class

[Paolo Bonzini]

> 
> +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, , 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,

[Qemu-devel] [PULL v4 8/9] io: add QIOChannelCommand class

[Daniel P. Berrange]

Add a QIOChannel subclass that is capable of performing I/O
to/from a separate process, via a pair of pipes. The command
can be used for unidirectional or bi-directional I/O.

Signed-off-by: Daniel P. Berrange 
---
 include/io/channel-command.h|  91 ++
 io/Makefile.objs|   1 +
 io/channel-command.c| 357 
 tests/.gitignore|   2 +
 tests/Makefile  |   3 +
 tests/test-io-channel-command.c | 129 +++
 trace-events|   6 +
 7 files changed, 589 insertions(+)
 create mode 100644 include/io/channel-command.h
 create mode 100644 io/channel-command.c
 create mode 100644 tests/test-io-channel-command.c

diff --git a/include/io/channel-command.h b/include/io/channel-command.h
new file mode 100644
index 000..bd3c599
--- /dev/null
+++ b/include/io/channel-command.h
@@ -0,0 +1,91 @@
+/*
+ * QEMU I/O channels external command driver
+ *
+ * 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 .
+ *
+ */
+
+#ifndef QIO_CHANNEL_COMMAND_H__
+#define QIO_CHANNEL_COMMAND_H__
+
+#include "io/channel.h"
+
+#define TYPE_QIO_CHANNEL_COMMAND "qio-channel-command"
+#define QIO_CHANNEL_COMMAND(obj) \
+OBJECT_CHECK(QIOChannelCommand, (obj), TYPE_QIO_CHANNEL_COMMAND)
+
+typedef struct QIOChannelCommand QIOChannelCommand;
+
+
+/**
+ * QIOChannelCommand:
+ *
+ * The QIOChannelCommand class provides a channel implementation
+ * that can transport data with an externally running command
+ * via its stdio streams.
+ */
+
+struct QIOChannelCommand {
+QIOChannel parent;
+int writefd;
+int readfd;
+pid_t pid;
+};
+
+
+/**
+ * qio_channel_command_new_pid:
+ * @writefd: the FD connected to the command's stdin
+ * @readfd: the FD connected to the command's stdout
+ * @pid: the PID of the running child command
+ * @errp: pointer to an uninitialized error object
+ *
+ * Create a channel for performing I/O with the
+ * previously spawned command identified by @pid.
+ * The two file descriptors provide the connection
+ * to command's stdio streams, either one or which
+ * may be -1 to indicate that stream is not open.
+ *
+ * The channel will take ownership of the process
+ * @pid and will kill it when closing the channel.
+ * Similarly it will take responsibility for
+ * closing the file descriptors @writefd and @readfd.
+ *
+ * Returns: the command channel object, or NULL on error
+ */
+QIOChannelCommand *
+qio_channel_command_new_pid(int writefd,
+int readfd,
+pid_t pid);
+
+/**
+ * qio_channel_command_new_spawn:
+ * @argv: the NULL terminated list of command arguments
+ * @flags: the I/O mode, one of O_RDONLY, O_WRONLY, O_RDWR
+ * @errp: pointer to an uninitialized error object
+ *
+ * Create a channel for performing I/O with the
+ * command to be spawned with arguments @argv.
+ *
+ * Returns: the command channel object, or NULL on error
+ */
+QIOChannelCommand *
+qio_channel_command_new_spawn(const char *const argv[],
+  int flags,
+  Error **errp);
+
+
+#endif /* QIO_CHANNEL_COMMAND_H__ */
diff --git a/io/Makefile.objs b/io/Makefile.objs
index e3771b1..1a58ccb 100644
--- a/io/Makefile.objs
+++ b/io/Makefile.objs
@@ -1,4 +1,5 @@
 io-obj-y = channel.o
+io-obj-y += channel-command.o
 io-obj-y += channel-file.o
 io-obj-y += channel-socket.o
 io-obj-y += channel-tls.o
diff --git a/io/channel-command.c b/io/channel-command.c
new file mode 100644
index 000..598fdab
--- /dev/null
+++ b/io/channel-command.c
@@ -0,0 +1,357 @@
+/*
+ * QEMU I/O channels external command driver
+ *
+ * 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