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

[Paolo Bonzini <pbonzini@redhat.com>]

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