Patchwork [v5,2/5] guest agent: qemu-ga daemon

login
register
mail settings
Submitter Michael Roth
Date June 14, 2011, 8:06 p.m.
Message ID <1308081985-32394-3-git-send-email-mdroth@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/100428/
State New
Headers show

Comments

Michael Roth - June 14, 2011, 8:06 p.m.
This is the actual guest daemon, it listens for requests over a
virtio-serial/isa-serial/unix socket channel and routes them through
to dispatch routines, and writes the results back to the channel in
a manner similar to QMP.

A shorthand invocation:

  qemu-ga -d

Is equivalent to:

  qemu-ga -c virtio-serial -p /dev/virtio-ports/org.qemu.guest_agent \
          -p /var/run/qemu-guest-agent.pid -d

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qemu-ga.c              |  631 ++++++++++++++++++++++++++++++++++++++++++++++++
 qga/guest-agent-core.h |    4 +
 2 files changed, 635 insertions(+), 0 deletions(-)
 create mode 100644 qemu-ga.c
Luiz Capitulino - June 16, 2011, 6:42 p.m.
On Tue, 14 Jun 2011 15:06:22 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> This is the actual guest daemon, it listens for requests over a
> virtio-serial/isa-serial/unix socket channel and routes them through
> to dispatch routines, and writes the results back to the channel in
> a manner similar to QMP.
> 
> A shorthand invocation:
> 
>   qemu-ga -d
> 
> Is equivalent to:
> 
>   qemu-ga -c virtio-serial -p /dev/virtio-ports/org.qemu.guest_agent \
>           -p /var/run/qemu-guest-agent.pid -d
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Would be nice to have a more complete description, like explaining how to
do a simple test.

And this can't be built...

> ---
>  qemu-ga.c              |  631 ++++++++++++++++++++++++++++++++++++++++++++++++
>  qga/guest-agent-core.h |    4 +
>  2 files changed, 635 insertions(+), 0 deletions(-)
>  create mode 100644 qemu-ga.c
> 
> diff --git a/qemu-ga.c b/qemu-ga.c
> new file mode 100644
> index 0000000..df08d8c
> --- /dev/null
> +++ b/qemu-ga.c
> @@ -0,0 +1,631 @@
> +/*
> + * QEMU Guest Agent
> + *
> + * Copyright IBM Corp. 2011
> + *
> + * Authors:
> + *  Adam Litke        <aglitke@linux.vnet.ibm.com>
> + *  Michael Roth      <mdroth@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <stdbool.h>
> +#include <glib.h>
> +#include <gio/gio.h>
> +#include <getopt.h>
> +#include <termios.h>
> +#include <syslog.h>
> +#include "qemu_socket.h"
> +#include "json-streamer.h"
> +#include "json-parser.h"
> +#include "qint.h"
> +#include "qjson.h"
> +#include "qga/guest-agent-core.h"
> +#include "qga-qmp-commands.h"
> +#include "module.h"
> +
> +#define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent"
> +#define QGA_PIDFILE_DEFAULT "/var/run/qemu-va.pid"
> +#define QGA_BAUDRATE_DEFAULT B38400 /* for isa-serial channels */
> +#define QGA_TIMEOUT_DEFAULT 30*1000 /* ms */
> +
> +struct GAState {
> +    const char *proxy_path;

Where is this used?

> +    JSONMessageParser parser;
> +    GMainLoop *main_loop;
> +    guint conn_id;
> +    GSocket *conn_sock;
> +    GIOChannel *conn_channel;
> +    guint listen_id;
> +    GSocket *listen_sock;
> +    GIOChannel *listen_channel;
> +    const char *path;
> +    const char *method;
> +    bool virtio; /* fastpath to check for virtio to deal with poll() quirks */
> +    GACommandState *command_state;
> +    GLogLevelFlags log_level;
> +    FILE *log_file;
> +    bool logging_enabled;
> +};
> +
> +static void usage(const char *cmd)
> +{
> +    printf(
> +"Usage: %s -c <channel_opts>\n"
> +"QEMU Guest Agent %s\n"
> +"\n"
> +"  -c, --channel     channel method: one of unix-connect, virtio-serial, or\n"
> +"                    isa-serial (virtio-serial is the default)\n"
> +"  -p, --path        channel path (%s is the default for virtio-serial)\n"
> +"  -l, --logfile     set logfile path, logs to stderr by default\n"
> +"  -f, --pidfile     specify pidfile (default is %s)\n"
> +"  -v, --verbose     log extra debugging information\n"
> +"  -V, --version     print version information and exit\n"
> +"  -d, --daemonize   become a daemon\n"
> +"  -h, --help        display this help and exit\n"
> +"\n"
> +"Report bugs to <mdroth@linux.vnet.ibm.com>\n"
> +    , cmd, QGA_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT);
> +}
> +
> +static void conn_channel_close(GAState *s);
> +
> +static const char *ga_log_level_str(GLogLevelFlags level)
> +{
> +    switch (level & G_LOG_LEVEL_MASK) {
> +        case G_LOG_LEVEL_ERROR:
> +            return "error";
> +        case G_LOG_LEVEL_CRITICAL:
> +            return "critical";
> +        case G_LOG_LEVEL_WARNING:
> +            return "warning";
> +        case G_LOG_LEVEL_MESSAGE:
> +            return "message";
> +        case G_LOG_LEVEL_INFO:
> +            return "info";
> +        case G_LOG_LEVEL_DEBUG:
> +            return "debug";
> +        default:
> +            return "user";
> +    }
> +}
> +
> +bool ga_logging_enabled(GAState *s)
> +{
> +    return s->logging_enabled;
> +}
> +
> +void ga_disable_logging(GAState *s)
> +{
> +    s->logging_enabled = false;
> +}
> +
> +void ga_enable_logging(GAState *s)
> +{
> +    s->logging_enabled = true;
> +}

Just to check I got this right, this is needed because of the fsfreeze
command, correct? Isn't it better to have a more descriptive name, like
fsfrozen?

First I thought this was about a log file. Then I realized this was
probably about letting the user log in, but we don't seem to have this
functionality so I got confused.

> +
> +static void ga_log(const gchar *domain, GLogLevelFlags level,
> +                   const gchar *msg, gpointer opaque)
> +{
> +    GAState *s = opaque;
> +    GTimeVal time;
> +    const char *level_str = ga_log_level_str(level);
> +
> +    if (!ga_logging_enabled(s)) {
> +        return;
> +    }
> +
> +    level &= G_LOG_LEVEL_MASK;
> +    if (g_strcmp0(domain, "syslog") == 0) {
> +        syslog(LOG_INFO, "%s: %s", level_str, msg);
> +    } else if (level & s->log_level) {
> +        g_get_current_time(&time);
> +        fprintf(s->log_file,
> +                "%lu.%lu: %s: %s\n", time.tv_sec, time.tv_usec, level_str, msg);
> +        fflush(s->log_file);
> +    }
> +}
> +
> +static void become_daemon(const char *pidfile)
> +{
> +    pid_t pid, sid;
> +    int pidfd;
> +    char *pidstr = NULL;
> +
> +    pid = fork();
> +    if (pid < 0) {
> +        exit(EXIT_FAILURE);
> +    }
> +    if (pid > 0) {
> +        exit(EXIT_SUCCESS);
> +    }
> +
> +    pidfd = open(pidfile, O_CREAT|O_RDWR, S_IRUSR|S_IWUSR);

O_WRONLY is enough.

> +    if (!pidfd || lockf(pidfd, F_TLOCK, 0)) {
> +        g_error("Cannot lock pid file");
> +    }

Are you sure we need lockf() here? I think using O_EXCL is enough for
pid files.

> +
> +    if (ftruncate(pidfd, 0) || lseek(pidfd, 0, SEEK_SET)) {
> +        g_critical("Cannot truncate pid file");
> +        goto fail;

You can use O_TRUNC and the file pointer is already positioned at the
beginning of the file, no need to call lseek().

> +    }
> +    if (asprintf(&pidstr, "%d", getpid()) == -1) {
> +        g_critical("Cannot allocate memory");
> +        goto fail;
> +    }
> +    if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
> +        g_critical("Failed to write pid file");
> +        goto fail;
> +    }

You're not freeing pidstr, nor closing the file (although I think you
do this because of the lockf() call()).

> +
> +    umask(0);
> +    sid = setsid();
> +    if (sid < 0) {
> +        goto fail;
> +    }
> +    if ((chdir("/")) < 0) {
> +        goto fail;
> +    }
> +
> +    close(STDIN_FILENO);
> +    close(STDOUT_FILENO);
> +    close(STDERR_FILENO);
> +    return;
> +
> +fail:
> +    if (pidstr) {
> +        free(pidstr);
> +    }

This check is not needed.

> +    unlink(pidfile);
> +    g_error("failed to daemonize");
> +}
> +
> +static int conn_channel_send_payload(GIOChannel *channel, QObject *payload)
> +{
> +    gsize count, written = 0;
> +    int ret;
> +    const char *buf;
> +    QString *payload_qstr;
> +    GIOStatus status;
> +    GError *err = NULL;
> +
> +    if (!payload || !channel) {
> +        ret = -EINVAL;
> +        goto out;
> +    }

Just do "return -EINVAL" instead of using goto, but I wonder if this should
be an assert() instead.

> +
> +    payload_qstr = qobject_to_json(payload);
> +    if (!payload_qstr) {
> +        ret = -EINVAL;
> +        goto out;
> +    }

return -EINVAL.

> +
> +    buf = qstring_get_str(payload_qstr);
> +    count = strlen(buf);
> +
> +    while (count) {
> +        g_debug("sending data, count: %d", (int)count);
> +        status = g_io_channel_write_chars(channel, buf, count, &written, &err);
> +        if (err != NULL) {
> +            g_warning("error writing payload to channel: %s", err->message);
> +            ret = err->code;
> +            goto out_free;
> +        }
> +        if (status == G_IO_STATUS_NORMAL) {
> +            count -= written;
> +        } else if (status == G_IO_STATUS_ERROR || status == G_IO_STATUS_EOF) {
> +            ret = -EPIPE;
> +            goto out_free;
> +        }
> +    }
> +
> +    status = g_io_channel_write_chars(channel, (char *)"\n", 1, &written, &err);
> +    if (err != NULL) {
> +        g_warning("error sending newline: %s", err->message);
> +        ret = err->code;
> +        goto out_free;
> +    } else if (status == G_IO_STATUS_ERROR || status == G_IO_STATUS_EOF) {
> +        ret = -EPIPE;
> +        goto out_free;
> +    }

This wants to be a function.

> +
> +    g_io_channel_flush(channel, &err);
> +    if (err != NULL) {
> +        g_warning("error flushing payload: %s", err->message);
> +        ret = err->code;
> +        goto out_free;
> +    }
> +
> +    ret = 0;
> +
> +out_free:
> +    QDECREF(payload_qstr);
> +    if (err != NULL) {
> +        g_error_free(err);
> +    }

No need to check against NULL.

> +out:
> +    return ret;
> +}
> +
> +static void process_command(GAState *s, QDict *req)
> +{
> +    QObject *rsp = NULL;
> +    Error *err = NULL;
> +    int ret;
> +
> +    g_assert(req);
> +    g_debug("processing command");
> +    rsp = qmp_dispatch(QOBJECT(req));
> +    if (rsp) {
> +        if (err) {

Apart from its initialization, 'err' is read only. Either, you want to
use qmp_dispatch_err() here or you have to modify qmp_dispatch() to also
accept an Error argument.

If you want to send a "return" or "error" QMP kind of response, then you
have to do the latter.

> +            g_warning("command failed: %s", error_get_pretty(err));
> +        }
> +        ret = conn_channel_send_payload(s->conn_channel, rsp);
> +        if (ret) {
> +            g_warning("error sending payload: %s", strerror(ret));
> +        }
> +        qobject_decref(rsp);
> +    } else {
> +        g_warning("error getting response");
> +        if (err) {
> +            g_warning("dispatch failed: %s", error_get_pretty(err));
> +        }
> +    }
> +}
> +
> +/* handle requests/control events coming in over the channel */
> +static void process_event(JSONMessageParser *parser, QList *tokens)
> +{
> +    GAState *s = container_of(parser, GAState, parser);
> +    QObject *obj;
> +    QDict *qdict;
> +    Error *err = NULL;
> +
> +    g_assert(s && parser);
> +
> +    g_debug("process_event: called");
> +    obj = json_parser_parse_err(tokens, NULL, &err);
> +    if (!obj || qobject_type(obj) != QTYPE_QDICT) {
> +        g_warning("failed to parse event");

Two possible leaks here. You have to call qobject_decref() if obj != NULL
and there's a missing call to error_free(). But you don't use 'err' anyway,
so you could pass NULL there.

Oh, btw, the guest agent doesn't seem to report errors back to its client...
Not even json error messages, is this intended?

> +        return;
> +    } else {
> +        g_debug("parse successful");
> +        qdict = qobject_to_qdict(obj);
> +        g_assert(qdict);
> +    }

Superfluous else clause.

> +
> +    /* handle host->guest commands */
> +    if (qdict_haskey(qdict, "execute")) {
> +        process_command(s, qdict);
> +    } else {
> +        g_warning("unrecognized payload format, ignoring");
> +    }
> +
> +    QDECREF(qdict);
> +}
> +
> +static gboolean conn_channel_read(GIOChannel *channel, GIOCondition condition,
> +                                  gpointer data)
> +{
> +    GAState *s = data;
> +    gchar buf[1024];
> +    gsize count;
> +    GError *err = NULL;
> +    memset(buf, 0, 1024);
> +    GIOStatus status = g_io_channel_read_chars(channel, buf, 1024,
> +                                               &count, &err);
> +    if (err != NULL) {
> +        g_warning("error reading channel: %s", err->message);
> +        conn_channel_close(s);
> +        g_error_free(err);
> +        return false;
> +    }
> +    switch (status) {
> +    case G_IO_STATUS_ERROR:
> +        g_warning("problem");
> +        return false;
> +    case G_IO_STATUS_NORMAL:
> +        g_debug("read data, count: %d, data: %s", (int)count, buf);
> +        json_message_parser_feed(&s->parser, (char *)buf, (int)count);
> +    case G_IO_STATUS_AGAIN:
> +        /* virtio causes us to spin here when no process is attached to
> +         * host-side chardev. sleep a bit to mitigate this
> +         */
> +        if (s->virtio) {
> +            usleep(100*1000);
> +        }
> +        return true;
> +    case G_IO_STATUS_EOF:
> +        g_debug("received EOF");
> +        conn_channel_close(s);
> +        if (s->virtio) {
> +            return true;
> +        }
> +        return false;
> +    default:
> +        g_warning("unknown channel read status, closing");
> +        conn_channel_close(s);
> +        return false;
> +    }
> +    return true;
> +}
> +
> +static int conn_channel_add(GAState *s, int fd)
> +{
> +    GIOChannel *conn_channel;
> +    guint conn_id;
> +    GError *err = NULL;
> +
> +    g_assert(s && !s->conn_channel);
> +    conn_channel = g_io_channel_unix_new(fd);
> +    g_assert(conn_channel);
> +    g_io_channel_set_encoding(conn_channel, NULL, &err);
> +    if (err != NULL) {
> +        g_warning("error setting channel encoding to binary");
> +        g_error_free(err);
> +        return -1;
> +    }
> +    conn_id = g_io_add_watch(conn_channel, G_IO_IN | G_IO_HUP,
> +                             conn_channel_read, s);
> +    if (err != NULL) {
> +        g_warning("error adding io watch: %s", err->message);
> +        g_error_free(err);
> +        return -1;
> +    }
> +    s->conn_channel = conn_channel;
> +    s->conn_id = conn_id;
> +    return 0;
> +}
> +
> +static gboolean listen_channel_accept(GIOChannel *channel,
> +                                      GIOCondition condition, gpointer data)
> +{
> +    GAState *s = data;
> +    GError *err = NULL;
> +    g_assert(channel != NULL);
> +    int ret;
> +    bool accepted = false;
> +
> +    s->conn_sock = g_socket_accept(s->listen_sock, NULL, &err);
> +    if (err != NULL) {
> +        g_warning("error converting fd to gsocket: %s", err->message);
> +        g_error_free(err);
> +        goto out;
> +    }
> +    ret = conn_channel_add(s, g_socket_get_fd(s->conn_sock));
> +    if (ret) {
> +        g_warning("error setting up connection");
> +        goto out;
> +    }
> +    accepted = true;
> +
> +out:
> +    /* only accept 1 connection at a time */
> +    return !accepted;
> +}
> +
> +/* start polling for readable events on listen fd, listen_fd=0
> + * indicates we should use the existing s->listen_channel
> + */
> +static int listen_channel_add(GAState *s, int listen_fd)
> +{
> +    GError *err = NULL;
> +    guint listen_id;
> +
> +    if (listen_fd) {
> +        s->listen_channel = g_io_channel_unix_new(listen_fd);
> +        if (s->listen_sock) {
> +            g_object_unref(s->listen_sock);
> +        }
> +        s->listen_sock = g_socket_new_from_fd(listen_fd, &err);
> +        if (err != NULL) {
> +            g_warning("error converting fd to gsocket: %s", err->message);
> +            g_error_free(err);
> +            return -1;
> +        }
> +    }
> +    listen_id = g_io_add_watch(s->listen_channel, G_IO_IN,
> +                               listen_channel_accept, s);
> +    if (err != NULL) {
> +        g_warning("error adding io watch: %s", err->message);
> +        g_error_free(err);
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +/* cleanup state for closed connection/session, start accepting new
> + * connections if we're in listening mode
> + */
> +static void conn_channel_close(GAState *s)
> +{
> +    if (strcmp(s->method, "unix-listen") == 0) {
> +        g_io_channel_shutdown(s->conn_channel, true, NULL);
> +        g_object_unref(s->conn_sock);
> +        s->conn_sock = NULL;
> +        listen_channel_add(s, 0);
> +    } else if (strcmp(s->method, "virtio-serial") == 0) {
> +        /* we spin on EOF for virtio-serial, so back off a bit. also,
> +         * dont close the connection in this case, it'll resume normal
> +         * operation when another process connects to host chardev
> +         */
> +        usleep(100*1000);
> +        goto out_noclose;
> +    }
> +    g_io_channel_unref(s->conn_channel);
> +    s->conn_channel = NULL;
> +    s->conn_id = 0;
> +out_noclose:
> +    return;
> +}
> +
> +static void init_guest_agent(GAState *s)
> +{
> +    struct termios tio;
> +    int ret, fd;
> +
> +    if (s->method == NULL) {
> +        /* try virtio-serial as our default */
> +        s->method = "virtio-serial";
> +    }
> +
> +    if (s->path == NULL) {
> +        if (strcmp(s->method, "virtio-serial") != 0) {
> +            g_error("must specify a path for this channel");
> +        }
> +        /* try the default path for the virtio-serial port */
> +        s->path = QGA_VIRTIO_PATH_DEFAULT;
> +    }
> +
> +    if (strcmp(s->method, "virtio-serial") == 0) {
> +        s->virtio = true;
> +        fd = qemu_open(s->path, O_RDWR);
> +        if (fd == -1) {
> +            g_error("error opening channel: %s", strerror(errno));
> +        }
> +        ret = fcntl(fd, F_GETFL);
> +        if (ret < 0) {
> +            g_error("error getting channel flags: %s", strerror(errno));
> +        }
> +        ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK | O_ASYNC);
> +        if (ret < 0) {
> +            g_error("error setting channel flags: %s", strerror(errno));
> +        }
> +        ret = conn_channel_add(s, fd);
> +        if (ret) {
> +            g_error("error adding channel to main loop");
> +        }
> +    } else if (strcmp(s->method, "isa-serial") == 0) {
> +        fd = qemu_open(s->path, O_RDWR | O_NOCTTY);
> +        if (fd == -1) {
> +            g_error("error opening channel: %s", strerror(errno));
> +        }
> +        tcgetattr(fd, &tio);
> +        /* set up serial port for non-canonical, dumb byte streaming */
> +        tio.c_iflag &= ~(IGNBRK | BRKINT | IGNPAR | PARMRK | INPCK | ISTRIP |
> +                         INLCR | IGNCR | ICRNL | IXON | IXOFF | IXANY |
> +                         IMAXBEL);
> +        tio.c_oflag = 0;
> +        tio.c_lflag = 0;
> +        tio.c_cflag |= QGA_BAUDRATE_DEFAULT;
> +        /* 1 available byte min or reads will block (we'll set non-blocking
> +         * elsewhere, else we have to deal with read()=0 instead)
> +         */
> +        tio.c_cc[VMIN] = 1;
> +        tio.c_cc[VTIME] = 0;
> +        /* flush everything waiting for read/xmit, it's garbage at this point */
> +        tcflush(fd, TCIFLUSH);
> +        tcsetattr(fd, TCSANOW, &tio);
> +        ret = conn_channel_add(s, fd);
> +        if (ret) {
> +            g_error("error adding channel to main loop");
> +        }
> +    } else if (strcmp(s->method, "unix-listen") == 0) {
> +        fd = unix_listen(s->path, NULL, strlen(s->path));
> +        if (fd == -1) {
> +            g_error("error opening path: %s", strerror(errno));
> +        }
> +        ret = listen_channel_add(s, fd);
> +        if (ret) {
> +            g_error("error binding/listening to specified socket");
> +        }
> +    } else {
> +        g_error("unsupported channel method/type: %s", s->method);
> +    }
> +
> +    json_message_parser_init(&s->parser, process_event);
> +    s->main_loop = g_main_loop_new(NULL, false);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    const char *sopt = "hVvdc:p:l:f:";
> +    const char *method = NULL, *path = NULL, *pidfile = QGA_PIDFILE_DEFAULT;
> +    struct option lopt[] = {
> +        { "help", 0, NULL, 'h' },
> +        { "version", 0, NULL, 'V' },
> +        { "logfile", 0, NULL, 'l' },
> +        { "pidfile", 0, NULL, 'f' },
> +        { "verbose", 0, NULL, 'v' },
> +        { "channel", 0, NULL, 'c' },
> +        { "path", 0, NULL, 'p' },
> +        { "daemonize", 0, NULL, 'd' },
> +        { NULL, 0, NULL, 0 }
> +    };
> +    int opt_ind = 0, ch, daemonize = 0;
> +    GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
> +    FILE *log_file = stderr;
> +    GAState *s;
> +
> +    g_type_init();
> +    g_thread_init(NULL);
> +
> +    while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
> +        switch (ch) {
> +        case 'c':
> +            method = optarg;
> +            break;
> +        case 'p':
> +            path = optarg;
> +            break;
> +        case 'l':
> +            log_file = fopen(optarg, "a");
> +            if (!log_file) {
> +                g_error("unable to open specified log file: %s",
> +                        strerror(errno));
> +            }
> +            break;
> +        case 'f':
> +            pidfile = optarg;
> +            break;
> +        case 'v':
> +            /* enable all log levels */
> +            log_level = G_LOG_LEVEL_MASK;
> +            break;
> +        case 'V':
> +            printf("QEMU Guest Agent %s\n", QGA_VERSION);
> +            return 0;
> +        case 'd':
> +            daemonize = 1;
> +            break;
> +        case 'h':
> +            usage(argv[0]);
> +            return 0;
> +        case '?':
> +            g_error("Unknown option, try '%s --help' for more information.",
> +                    argv[0]);
> +        }
> +    }
> +
> +    if (daemonize) {
> +        g_debug("starting daemon");
> +        become_daemon(pidfile);
> +    }
> +
> +    s = g_malloc0(sizeof(GAState));
> +    s->conn_id = 0;
> +    s->conn_channel = NULL;
> +    s->path = path;
> +    s->method = method;
> +    s->command_state = ga_command_state_new();
> +    ga_command_state_init(s, s->command_state);
> +    ga_command_state_init_all(s->command_state);
> +    s->log_file = log_file;
> +    s->log_level = log_level;
> +    g_log_set_default_handler(ga_log, s);
> +    g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
> +    s->logging_enabled = true;
> +
> +    module_call_init(MODULE_INIT_QAPI);
> +    init_guest_agent(s);
> +
> +    g_main_loop_run(s->main_loop);
> +
> +    ga_command_state_cleanup_all(s->command_state);
> +
> +    return 0;
> +}
> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> index 688f120..66d1729 100644
> --- a/qga/guest-agent-core.h
> +++ b/qga/guest-agent-core.h

No license text.

> @@ -15,6 +15,7 @@
>  
>  #define QGA_VERSION "1.0"
>  
> +typedef struct GAState GAState;
>  typedef struct GACommandState GACommandState;
>  
>  void ga_command_state_add(GACommandState *cs,
> @@ -23,3 +24,6 @@ void ga_command_state_add(GACommandState *cs,
>  void ga_command_state_init_all(GACommandState *cs);
>  void ga_command_state_cleanup_all(GACommandState *cs);
>  GACommandState *ga_command_state_new(void);
> +bool ga_logging_enabled(GAState *s);
> +void ga_disable_logging(GAState *s);
> +void ga_enable_logging(GAState *s);
Michael Roth - June 17, 2011, 7:21 p.m.
On 06/16/2011 01:42 PM, Luiz Capitulino wrote:
> On Tue, 14 Jun 2011 15:06:22 -0500
> Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>
>> This is the actual guest daemon, it listens for requests over a
>> virtio-serial/isa-serial/unix socket channel and routes them through
>> to dispatch routines, and writes the results back to the channel in
>> a manner similar to QMP.
>>
>> A shorthand invocation:
>>
>>    qemu-ga -d
>>
>> Is equivalent to:
>>
>>    qemu-ga -c virtio-serial -p /dev/virtio-ports/org.qemu.guest_agent \
>>            -p /var/run/qemu-guest-agent.pid -d
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>
> Would be nice to have a more complete description, like explaining how to
> do a simple test.
>
> And this can't be built...
>
>> ---
>>   qemu-ga.c              |  631 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   qga/guest-agent-core.h |    4 +
>>   2 files changed, 635 insertions(+), 0 deletions(-)
>>   create mode 100644 qemu-ga.c
>>
>> diff --git a/qemu-ga.c b/qemu-ga.c
>> new file mode 100644
>> index 0000000..df08d8c
>> --- /dev/null
>> +++ b/qemu-ga.c
>> @@ -0,0 +1,631 @@
>> +/*
>> + * QEMU Guest Agent
>> + *
>> + * Copyright IBM Corp. 2011
>> + *
>> + * Authors:
>> + *  Adam Litke<aglitke@linux.vnet.ibm.com>
>> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +#include<stdlib.h>
>> +#include<stdio.h>
>> +#include<stdbool.h>
>> +#include<glib.h>
>> +#include<gio/gio.h>
>> +#include<getopt.h>
>> +#include<termios.h>
>> +#include<syslog.h>
>> +#include "qemu_socket.h"
>> +#include "json-streamer.h"
>> +#include "json-parser.h"
>> +#include "qint.h"
>> +#include "qjson.h"
>> +#include "qga/guest-agent-core.h"
>> +#include "qga-qmp-commands.h"
>> +#include "module.h"
>> +
>> +#define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent"
>> +#define QGA_PIDFILE_DEFAULT "/var/run/qemu-va.pid"
>> +#define QGA_BAUDRATE_DEFAULT B38400 /* for isa-serial channels */
>> +#define QGA_TIMEOUT_DEFAULT 30*1000 /* ms */
>> +
>> +struct GAState {
>> +    const char *proxy_path;
>
> Where is this used?
>

Nowhere actually. Will remove.

>> +    JSONMessageParser parser;
>> +    GMainLoop *main_loop;
>> +    guint conn_id;
>> +    GSocket *conn_sock;
>> +    GIOChannel *conn_channel;
>> +    guint listen_id;
>> +    GSocket *listen_sock;
>> +    GIOChannel *listen_channel;
>> +    const char *path;
>> +    const char *method;
>> +    bool virtio; /* fastpath to check for virtio to deal with poll() quirks */
>> +    GACommandState *command_state;
>> +    GLogLevelFlags log_level;
>> +    FILE *log_file;
>> +    bool logging_enabled;
>> +};
>> +
>> +static void usage(const char *cmd)
>> +{
>> +    printf(
>> +"Usage: %s -c<channel_opts>\n"
>> +"QEMU Guest Agent %s\n"
>> +"\n"
>> +"  -c, --channel     channel method: one of unix-connect, virtio-serial, or\n"
>> +"                    isa-serial (virtio-serial is the default)\n"
>> +"  -p, --path        channel path (%s is the default for virtio-serial)\n"
>> +"  -l, --logfile     set logfile path, logs to stderr by default\n"
>> +"  -f, --pidfile     specify pidfile (default is %s)\n"
>> +"  -v, --verbose     log extra debugging information\n"
>> +"  -V, --version     print version information and exit\n"
>> +"  -d, --daemonize   become a daemon\n"
>> +"  -h, --help        display this help and exit\n"
>> +"\n"
>> +"Report bugs to<mdroth@linux.vnet.ibm.com>\n"
>> +    , cmd, QGA_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT);
>> +}
>> +
>> +static void conn_channel_close(GAState *s);
>> +
>> +static const char *ga_log_level_str(GLogLevelFlags level)
>> +{
>> +    switch (level&  G_LOG_LEVEL_MASK) {
>> +        case G_LOG_LEVEL_ERROR:
>> +            return "error";
>> +        case G_LOG_LEVEL_CRITICAL:
>> +            return "critical";
>> +        case G_LOG_LEVEL_WARNING:
>> +            return "warning";
>> +        case G_LOG_LEVEL_MESSAGE:
>> +            return "message";
>> +        case G_LOG_LEVEL_INFO:
>> +            return "info";
>> +        case G_LOG_LEVEL_DEBUG:
>> +            return "debug";
>> +        default:
>> +            return "user";
>> +    }
>> +}
>> +
>> +bool ga_logging_enabled(GAState *s)
>> +{
>> +    return s->logging_enabled;
>> +}
>> +
>> +void ga_disable_logging(GAState *s)
>> +{
>> +    s->logging_enabled = false;
>> +}
>> +
>> +void ga_enable_logging(GAState *s)
>> +{
>> +    s->logging_enabled = true;
>> +}
>
> Just to check I got this right, this is needed because of the fsfreeze
> command, correct? Isn't it better to have a more descriptive name, like
> fsfrozen?
>
> First I thought this was about a log file. Then I realized this was
> probably about letting the user log in, but we don't seem to have this
> functionality so I got confused.
>

Yup, this is currently due to fsfreeze support. From the perspective of 
the fsfreeze command the explicit "is_frozen" check makes more sense, 
but the reason it affects other RPCs is because because we can't log 
stuff in that state. If an RPC shoots itself in the foot by writing to a 
frozen filesystem we don't really care so much, and up until recently 
that case was handled with a pthread_cancel timeout mechanism (was 
removed for the time being, will re-implement using a child process most 
likely).

What we don't want to do is give a host a way to bypass the expectation 
we set for guest owners by allowing RPCs to be logged. So that's what 
the check is based on, rather than lower level details like *why* 
logging is disabled. Also, I'd really like to avoid a precedence where a 
single RPC can place restrictions on all the others, so the logging 
aspect seemed general enough that it doesn't necessarily provide that 
precedence.

This has come up a few times without any real consensus. I can probably 
go either way, but I think the check for logging is easier to set 
expectations with: "if logging is important from an auditing 
perspective, don't execute this if logging is disabled". beyond that, 
same behavior/symantics you'd get with anything that causes a write() on 
a filesystem in a frozen state: block.

>> +
>> +static void ga_log(const gchar *domain, GLogLevelFlags level,
>> +                   const gchar *msg, gpointer opaque)
>> +{
>> +    GAState *s = opaque;
>> +    GTimeVal time;
>> +    const char *level_str = ga_log_level_str(level);
>> +
>> +    if (!ga_logging_enabled(s)) {
>> +        return;
>> +    }
>> +
>> +    level&= G_LOG_LEVEL_MASK;
>> +    if (g_strcmp0(domain, "syslog") == 0) {
>> +        syslog(LOG_INFO, "%s: %s", level_str, msg);
>> +    } else if (level&  s->log_level) {
>> +        g_get_current_time(&time);
>> +        fprintf(s->log_file,
>> +                "%lu.%lu: %s: %s\n", time.tv_sec, time.tv_usec, level_str, msg);
>> +        fflush(s->log_file);
>> +    }
>> +}
>> +
>> +static void become_daemon(const char *pidfile)
>> +{
>> +    pid_t pid, sid;
>> +    int pidfd;
>> +    char *pidstr = NULL;
>> +
>> +    pid = fork();
>> +    if (pid<  0) {
>> +        exit(EXIT_FAILURE);
>> +    }
>> +    if (pid>  0) {
>> +        exit(EXIT_SUCCESS);
>> +    }
>> +
>> +    pidfd = open(pidfile, O_CREAT|O_RDWR, S_IRUSR|S_IWUSR);
>
> O_WRONLY is enough.
>
>> +    if (!pidfd || lockf(pidfd, F_TLOCK, 0)) {
>> +        g_error("Cannot lock pid file");
>> +    }
>
> Are you sure we need lockf() here? I think using O_EXCL is enough for
> pid files.
>

O_EXCL + O_CREAT? Wouldn't we have issues if the pid file wasn't cleaned 
up from a previous run?

>> +
>> +    if (ftruncate(pidfd, 0) || lseek(pidfd, 0, SEEK_SET)) {
>> +        g_critical("Cannot truncate pid file");
>> +        goto fail;
>
> You can use O_TRUNC and the file pointer is already positioned at the
> beginning of the file, no need to call lseek().
>
>> +    }
>> +    if (asprintf(&pidstr, "%d", getpid()) == -1) {
>> +        g_critical("Cannot allocate memory");
>> +        goto fail;
>> +    }
>> +    if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
>> +        g_critical("Failed to write pid file");
>> +        goto fail;
>> +    }
>
> You're not freeing pidstr, nor closing the file (although I think you
> do this because of the lockf() call()).
>

pidstr gets free'd in the fail: block. And yeah, closing would give up 
the lock prematurely.

>> +
>> +    umask(0);
>> +    sid = setsid();
>> +    if (sid<  0) {
>> +        goto fail;
>> +    }
>> +    if ((chdir("/"))<  0) {
>> +        goto fail;
>> +    }
>> +
>> +    close(STDIN_FILENO);
>> +    close(STDOUT_FILENO);
>> +    close(STDERR_FILENO);
>> +    return;
>> +
>> +fail:
>> +    if (pidstr) {
>> +        free(pidstr);
>> +    }
>
> This check is not needed.
>
>> +    unlink(pidfile);
>> +    g_error("failed to daemonize");
>> +}
>> +
>> +static int conn_channel_send_payload(GIOChannel *channel, QObject *payload)
>> +{
>> +    gsize count, written = 0;
>> +    int ret;
>> +    const char *buf;
>> +    QString *payload_qstr;
>> +    GIOStatus status;
>> +    GError *err = NULL;
>> +
>> +    if (!payload || !channel) {
>> +        ret = -EINVAL;
>> +        goto out;
>> +    }
>
> Just do "return -EINVAL" instead of using goto, but I wonder if this should
> be an assert() instead.
>

Yah, looks like it.

>> +
>> +    payload_qstr = qobject_to_json(payload);
>> +    if (!payload_qstr) {
>> +        ret = -EINVAL;
>> +        goto out;
>> +    }
>
> return -EINVAL.
>
>> +
>> +    buf = qstring_get_str(payload_qstr);
>> +    count = strlen(buf);
>> +
>> +    while (count) {
>> +        g_debug("sending data, count: %d", (int)count);
>> +        status = g_io_channel_write_chars(channel, buf, count,&written,&err);
>> +        if (err != NULL) {
>> +            g_warning("error writing payload to channel: %s", err->message);
>> +            ret = err->code;
>> +            goto out_free;
>> +        }
>> +        if (status == G_IO_STATUS_NORMAL) {
>> +            count -= written;
>> +        } else if (status == G_IO_STATUS_ERROR || status == G_IO_STATUS_EOF) {
>> +            ret = -EPIPE;
>> +            goto out_free;
>> +        }
>> +    }
>> +
>> +    status = g_io_channel_write_chars(channel, (char *)"\n", 1,&written,&err);
>> +    if (err != NULL) {
>> +        g_warning("error sending newline: %s", err->message);
>> +        ret = err->code;
>> +        goto out_free;
>> +    } else if (status == G_IO_STATUS_ERROR || status == G_IO_STATUS_EOF) {
>> +        ret = -EPIPE;
>> +        goto out_free;
>> +    }
>
> This wants to be a function.
>
>> +
>> +    g_io_channel_flush(channel,&err);
>> +    if (err != NULL) {
>> +        g_warning("error flushing payload: %s", err->message);
>> +        ret = err->code;
>> +        goto out_free;
>> +    }
>> +
>> +    ret = 0;
>> +
>> +out_free:
>> +    QDECREF(payload_qstr);
>> +    if (err != NULL) {
>> +        g_error_free(err);
>> +    }
>
> No need to check against NULL.
>
>> +out:
>> +    return ret;
>> +}
>> +
>> +static void process_command(GAState *s, QDict *req)
>> +{
>> +    QObject *rsp = NULL;
>> +    Error *err = NULL;
>> +    int ret;
>> +
>> +    g_assert(req);
>> +    g_debug("processing command");
>> +    rsp = qmp_dispatch(QOBJECT(req));
>> +    if (rsp) {
>> +        if (err) {
>
> Apart from its initialization, 'err' is read only. Either, you want to
> use qmp_dispatch_err() here or you have to modify qmp_dispatch() to also
> accept an Error argument.
>
> If you want to send a "return" or "error" QMP kind of response, then you
> have to do the latter.
>

qmp_dispatch() does the Error -> QMP error conversion. The unused err is 
cruft from when a seperate worker thread did the dispatch, will remove it.

>> +            g_warning("command failed: %s", error_get_pretty(err));
>> +        }
>> +        ret = conn_channel_send_payload(s->conn_channel, rsp);
>> +        if (ret) {
>> +            g_warning("error sending payload: %s", strerror(ret));
>> +        }
>> +        qobject_decref(rsp);
>> +    } else {
>> +        g_warning("error getting response");
>> +        if (err) {
>> +            g_warning("dispatch failed: %s", error_get_pretty(err));
>> +        }
>> +    }
>> +}
>> +
>> +/* handle requests/control events coming in over the channel */
>> +static void process_event(JSONMessageParser *parser, QList *tokens)
>> +{
>> +    GAState *s = container_of(parser, GAState, parser);
>> +    QObject *obj;
>> +    QDict *qdict;
>> +    Error *err = NULL;
>> +
>> +    g_assert(s&&  parser);
>> +
>> +    g_debug("process_event: called");
>> +    obj = json_parser_parse_err(tokens, NULL,&err);
>> +    if (!obj || qobject_type(obj) != QTYPE_QDICT) {
>> +        g_warning("failed to parse event");
>
> Two possible leaks here. You have to call qobject_decref() if obj != NULL
> and there's a missing call to error_free(). But you don't use 'err' anyway,
> so you could pass NULL there.
>
> Oh, btw, the guest agent doesn't seem to report errors back to its client...
> Not even json error messages, is this intended?
>

Hmm, yes it's intended, but I probably should send those to the client...

You do get non-parse errors though: missing arguments, etc.

I'll fix this to do what QMP does here.

>> +        return;
>> +    } else {
>> +        g_debug("parse successful");
>> +        qdict = qobject_to_qdict(obj);
>> +        g_assert(qdict);
>> +    }
>
> Superfluous else clause.
>
>> +
>> +    /* handle host->guest commands */
>> +    if (qdict_haskey(qdict, "execute")) {
>> +        process_command(s, qdict);
>> +    } else {
>> +        g_warning("unrecognized payload format, ignoring");
>> +    }
>> +
>> +    QDECREF(qdict);
>> +}
>> +
>> +static gboolean conn_channel_read(GIOChannel *channel, GIOCondition condition,
>> +                                  gpointer data)
>> +{
>> +    GAState *s = data;
>> +    gchar buf[1024];
>> +    gsize count;
>> +    GError *err = NULL;
>> +    memset(buf, 0, 1024);
>> +    GIOStatus status = g_io_channel_read_chars(channel, buf, 1024,
>> +&count,&err);
>> +    if (err != NULL) {
>> +        g_warning("error reading channel: %s", err->message);
>> +        conn_channel_close(s);
>> +        g_error_free(err);
>> +        return false;
>> +    }
>> +    switch (status) {
>> +    case G_IO_STATUS_ERROR:
>> +        g_warning("problem");
>> +        return false;
>> +    case G_IO_STATUS_NORMAL:
>> +        g_debug("read data, count: %d, data: %s", (int)count, buf);
>> +        json_message_parser_feed(&s->parser, (char *)buf, (int)count);
>> +    case G_IO_STATUS_AGAIN:
>> +        /* virtio causes us to spin here when no process is attached to
>> +         * host-side chardev. sleep a bit to mitigate this
>> +         */
>> +        if (s->virtio) {
>> +            usleep(100*1000);
>> +        }
>> +        return true;
>> +    case G_IO_STATUS_EOF:
>> +        g_debug("received EOF");
>> +        conn_channel_close(s);
>> +        if (s->virtio) {
>> +            return true;
>> +        }
>> +        return false;
>> +    default:
>> +        g_warning("unknown channel read status, closing");
>> +        conn_channel_close(s);
>> +        return false;
>> +    }
>> +    return true;
>> +}
>> +
>> +static int conn_channel_add(GAState *s, int fd)
>> +{
>> +    GIOChannel *conn_channel;
>> +    guint conn_id;
>> +    GError *err = NULL;
>> +
>> +    g_assert(s&&  !s->conn_channel);
>> +    conn_channel = g_io_channel_unix_new(fd);
>> +    g_assert(conn_channel);
>> +    g_io_channel_set_encoding(conn_channel, NULL,&err);
>> +    if (err != NULL) {
>> +        g_warning("error setting channel encoding to binary");
>> +        g_error_free(err);
>> +        return -1;
>> +    }
>> +    conn_id = g_io_add_watch(conn_channel, G_IO_IN | G_IO_HUP,
>> +                             conn_channel_read, s);
>> +    if (err != NULL) {
>> +        g_warning("error adding io watch: %s", err->message);
>> +        g_error_free(err);
>> +        return -1;
>> +    }
>> +    s->conn_channel = conn_channel;
>> +    s->conn_id = conn_id;
>> +    return 0;
>> +}
>> +
>> +static gboolean listen_channel_accept(GIOChannel *channel,
>> +                                      GIOCondition condition, gpointer data)
>> +{
>> +    GAState *s = data;
>> +    GError *err = NULL;
>> +    g_assert(channel != NULL);
>> +    int ret;
>> +    bool accepted = false;
>> +
>> +    s->conn_sock = g_socket_accept(s->listen_sock, NULL,&err);
>> +    if (err != NULL) {
>> +        g_warning("error converting fd to gsocket: %s", err->message);
>> +        g_error_free(err);
>> +        goto out;
>> +    }
>> +    ret = conn_channel_add(s, g_socket_get_fd(s->conn_sock));
>> +    if (ret) {
>> +        g_warning("error setting up connection");
>> +        goto out;
>> +    }
>> +    accepted = true;
>> +
>> +out:
>> +    /* only accept 1 connection at a time */
>> +    return !accepted;
>> +}
>> +
>> +/* start polling for readable events on listen fd, listen_fd=0
>> + * indicates we should use the existing s->listen_channel
>> + */
>> +static int listen_channel_add(GAState *s, int listen_fd)
>> +{
>> +    GError *err = NULL;
>> +    guint listen_id;
>> +
>> +    if (listen_fd) {
>> +        s->listen_channel = g_io_channel_unix_new(listen_fd);
>> +        if (s->listen_sock) {
>> +            g_object_unref(s->listen_sock);
>> +        }
>> +        s->listen_sock = g_socket_new_from_fd(listen_fd,&err);
>> +        if (err != NULL) {
>> +            g_warning("error converting fd to gsocket: %s", err->message);
>> +            g_error_free(err);
>> +            return -1;
>> +        }
>> +    }
>> +    listen_id = g_io_add_watch(s->listen_channel, G_IO_IN,
>> +                               listen_channel_accept, s);
>> +    if (err != NULL) {
>> +        g_warning("error adding io watch: %s", err->message);
>> +        g_error_free(err);
>> +        return -1;
>> +    }
>> +    return 0;
>> +}
>> +
>> +/* cleanup state for closed connection/session, start accepting new
>> + * connections if we're in listening mode
>> + */
>> +static void conn_channel_close(GAState *s)
>> +{
>> +    if (strcmp(s->method, "unix-listen") == 0) {
>> +        g_io_channel_shutdown(s->conn_channel, true, NULL);
>> +        g_object_unref(s->conn_sock);
>> +        s->conn_sock = NULL;
>> +        listen_channel_add(s, 0);
>> +    } else if (strcmp(s->method, "virtio-serial") == 0) {
>> +        /* we spin on EOF for virtio-serial, so back off a bit. also,
>> +         * dont close the connection in this case, it'll resume normal
>> +         * operation when another process connects to host chardev
>> +         */
>> +        usleep(100*1000);
>> +        goto out_noclose;
>> +    }
>> +    g_io_channel_unref(s->conn_channel);
>> +    s->conn_channel = NULL;
>> +    s->conn_id = 0;
>> +out_noclose:
>> +    return;
>> +}
>> +
>> +static void init_guest_agent(GAState *s)
>> +{
>> +    struct termios tio;
>> +    int ret, fd;
>> +
>> +    if (s->method == NULL) {
>> +        /* try virtio-serial as our default */
>> +        s->method = "virtio-serial";
>> +    }
>> +
>> +    if (s->path == NULL) {
>> +        if (strcmp(s->method, "virtio-serial") != 0) {
>> +            g_error("must specify a path for this channel");
>> +        }
>> +        /* try the default path for the virtio-serial port */
>> +        s->path = QGA_VIRTIO_PATH_DEFAULT;
>> +    }
>> +
>> +    if (strcmp(s->method, "virtio-serial") == 0) {
>> +        s->virtio = true;
>> +        fd = qemu_open(s->path, O_RDWR);
>> +        if (fd == -1) {
>> +            g_error("error opening channel: %s", strerror(errno));
>> +        }
>> +        ret = fcntl(fd, F_GETFL);
>> +        if (ret<  0) {
>> +            g_error("error getting channel flags: %s", strerror(errno));
>> +        }
>> +        ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK | O_ASYNC);
>> +        if (ret<  0) {
>> +            g_error("error setting channel flags: %s", strerror(errno));
>> +        }
>> +        ret = conn_channel_add(s, fd);
>> +        if (ret) {
>> +            g_error("error adding channel to main loop");
>> +        }
>> +    } else if (strcmp(s->method, "isa-serial") == 0) {
>> +        fd = qemu_open(s->path, O_RDWR | O_NOCTTY);
>> +        if (fd == -1) {
>> +            g_error("error opening channel: %s", strerror(errno));
>> +        }
>> +        tcgetattr(fd,&tio);
>> +        /* set up serial port for non-canonical, dumb byte streaming */
>> +        tio.c_iflag&= ~(IGNBRK | BRKINT | IGNPAR | PARMRK | INPCK | ISTRIP |
>> +                         INLCR | IGNCR | ICRNL | IXON | IXOFF | IXANY |
>> +                         IMAXBEL);
>> +        tio.c_oflag = 0;
>> +        tio.c_lflag = 0;
>> +        tio.c_cflag |= QGA_BAUDRATE_DEFAULT;
>> +        /* 1 available byte min or reads will block (we'll set non-blocking
>> +         * elsewhere, else we have to deal with read()=0 instead)
>> +         */
>> +        tio.c_cc[VMIN] = 1;
>> +        tio.c_cc[VTIME] = 0;
>> +        /* flush everything waiting for read/xmit, it's garbage at this point */
>> +        tcflush(fd, TCIFLUSH);
>> +        tcsetattr(fd, TCSANOW,&tio);
>> +        ret = conn_channel_add(s, fd);
>> +        if (ret) {
>> +            g_error("error adding channel to main loop");
>> +        }
>> +    } else if (strcmp(s->method, "unix-listen") == 0) {
>> +        fd = unix_listen(s->path, NULL, strlen(s->path));
>> +        if (fd == -1) {
>> +            g_error("error opening path: %s", strerror(errno));
>> +        }
>> +        ret = listen_channel_add(s, fd);
>> +        if (ret) {
>> +            g_error("error binding/listening to specified socket");
>> +        }
>> +    } else {
>> +        g_error("unsupported channel method/type: %s", s->method);
>> +    }
>> +
>> +    json_message_parser_init(&s->parser, process_event);
>> +    s->main_loop = g_main_loop_new(NULL, false);
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +    const char *sopt = "hVvdc:p:l:f:";
>> +    const char *method = NULL, *path = NULL, *pidfile = QGA_PIDFILE_DEFAULT;
>> +    struct option lopt[] = {
>> +        { "help", 0, NULL, 'h' },
>> +        { "version", 0, NULL, 'V' },
>> +        { "logfile", 0, NULL, 'l' },
>> +        { "pidfile", 0, NULL, 'f' },
>> +        { "verbose", 0, NULL, 'v' },
>> +        { "channel", 0, NULL, 'c' },
>> +        { "path", 0, NULL, 'p' },
>> +        { "daemonize", 0, NULL, 'd' },
>> +        { NULL, 0, NULL, 0 }
>> +    };
>> +    int opt_ind = 0, ch, daemonize = 0;
>> +    GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
>> +    FILE *log_file = stderr;
>> +    GAState *s;
>> +
>> +    g_type_init();
>> +    g_thread_init(NULL);
>> +
>> +    while ((ch = getopt_long(argc, argv, sopt, lopt,&opt_ind)) != -1) {
>> +        switch (ch) {
>> +        case 'c':
>> +            method = optarg;
>> +            break;
>> +        case 'p':
>> +            path = optarg;
>> +            break;
>> +        case 'l':
>> +            log_file = fopen(optarg, "a");
>> +            if (!log_file) {
>> +                g_error("unable to open specified log file: %s",
>> +                        strerror(errno));
>> +            }
>> +            break;
>> +        case 'f':
>> +            pidfile = optarg;
>> +            break;
>> +        case 'v':
>> +            /* enable all log levels */
>> +            log_level = G_LOG_LEVEL_MASK;
>> +            break;
>> +        case 'V':
>> +            printf("QEMU Guest Agent %s\n", QGA_VERSION);
>> +            return 0;
>> +        case 'd':
>> +            daemonize = 1;
>> +            break;
>> +        case 'h':
>> +            usage(argv[0]);
>> +            return 0;
>> +        case '?':
>> +            g_error("Unknown option, try '%s --help' for more information.",
>> +                    argv[0]);
>> +        }
>> +    }
>> +
>> +    if (daemonize) {
>> +        g_debug("starting daemon");
>> +        become_daemon(pidfile);
>> +    }
>> +
>> +    s = g_malloc0(sizeof(GAState));
>> +    s->conn_id = 0;
>> +    s->conn_channel = NULL;
>> +    s->path = path;
>> +    s->method = method;
>> +    s->command_state = ga_command_state_new();
>> +    ga_command_state_init(s, s->command_state);
>> +    ga_command_state_init_all(s->command_state);
>> +    s->log_file = log_file;
>> +    s->log_level = log_level;
>> +    g_log_set_default_handler(ga_log, s);
>> +    g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
>> +    s->logging_enabled = true;
>> +
>> +    module_call_init(MODULE_INIT_QAPI);
>> +    init_guest_agent(s);
>> +
>> +    g_main_loop_run(s->main_loop);
>> +
>> +    ga_command_state_cleanup_all(s->command_state);
>> +
>> +    return 0;
>> +}
>> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
>> index 688f120..66d1729 100644
>> --- a/qga/guest-agent-core.h
>> +++ b/qga/guest-agent-core.h
>
> No license text.
>
>> @@ -15,6 +15,7 @@
>>
>>   #define QGA_VERSION "1.0"
>>
>> +typedef struct GAState GAState;
>>   typedef struct GACommandState GACommandState;
>>
>>   void ga_command_state_add(GACommandState *cs,
>> @@ -23,3 +24,6 @@ void ga_command_state_add(GACommandState *cs,
>>   void ga_command_state_init_all(GACommandState *cs);
>>   void ga_command_state_cleanup_all(GACommandState *cs);
>>   GACommandState *ga_command_state_new(void);
>> +bool ga_logging_enabled(GAState *s);
>> +void ga_disable_logging(GAState *s);
>> +void ga_enable_logging(GAState *s);
>
Luiz Capitulino - June 17, 2011, 8:13 p.m.
On Fri, 17 Jun 2011 14:21:31 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On 06/16/2011 01:42 PM, Luiz Capitulino wrote:
> > On Tue, 14 Jun 2011 15:06:22 -0500
> > Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
> >
> >> This is the actual guest daemon, it listens for requests over a
> >> virtio-serial/isa-serial/unix socket channel and routes them through
> >> to dispatch routines, and writes the results back to the channel in
> >> a manner similar to QMP.
> >>
> >> A shorthand invocation:
> >>
> >>    qemu-ga -d
> >>
> >> Is equivalent to:
> >>
> >>    qemu-ga -c virtio-serial -p /dev/virtio-ports/org.qemu.guest_agent \
> >>            -p /var/run/qemu-guest-agent.pid -d
> >>
> >> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
> >
> > Would be nice to have a more complete description, like explaining how to
> > do a simple test.
> >
> > And this can't be built...
> >
> >> ---
> >>   qemu-ga.c              |  631 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>   qga/guest-agent-core.h |    4 +
> >>   2 files changed, 635 insertions(+), 0 deletions(-)
> >>   create mode 100644 qemu-ga.c
> >>
> >> diff --git a/qemu-ga.c b/qemu-ga.c
> >> new file mode 100644
> >> index 0000000..df08d8c
> >> --- /dev/null
> >> +++ b/qemu-ga.c
> >> @@ -0,0 +1,631 @@
> >> +/*
> >> + * QEMU Guest Agent
> >> + *
> >> + * Copyright IBM Corp. 2011
> >> + *
> >> + * Authors:
> >> + *  Adam Litke<aglitke@linux.vnet.ibm.com>
> >> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >> + * See the COPYING file in the top-level directory.
> >> + */
> >> +#include<stdlib.h>
> >> +#include<stdio.h>
> >> +#include<stdbool.h>
> >> +#include<glib.h>
> >> +#include<gio/gio.h>
> >> +#include<getopt.h>
> >> +#include<termios.h>
> >> +#include<syslog.h>
> >> +#include "qemu_socket.h"
> >> +#include "json-streamer.h"
> >> +#include "json-parser.h"
> >> +#include "qint.h"
> >> +#include "qjson.h"
> >> +#include "qga/guest-agent-core.h"
> >> +#include "qga-qmp-commands.h"
> >> +#include "module.h"
> >> +
> >> +#define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent"
> >> +#define QGA_PIDFILE_DEFAULT "/var/run/qemu-va.pid"
> >> +#define QGA_BAUDRATE_DEFAULT B38400 /* for isa-serial channels */
> >> +#define QGA_TIMEOUT_DEFAULT 30*1000 /* ms */
> >> +
> >> +struct GAState {
> >> +    const char *proxy_path;
> >
> > Where is this used?
> >
> 
> Nowhere actually. Will remove.
> 
> >> +    JSONMessageParser parser;
> >> +    GMainLoop *main_loop;
> >> +    guint conn_id;
> >> +    GSocket *conn_sock;
> >> +    GIOChannel *conn_channel;
> >> +    guint listen_id;
> >> +    GSocket *listen_sock;
> >> +    GIOChannel *listen_channel;
> >> +    const char *path;
> >> +    const char *method;
> >> +    bool virtio; /* fastpath to check for virtio to deal with poll() quirks */
> >> +    GACommandState *command_state;
> >> +    GLogLevelFlags log_level;
> >> +    FILE *log_file;
> >> +    bool logging_enabled;
> >> +};
> >> +
> >> +static void usage(const char *cmd)
> >> +{
> >> +    printf(
> >> +"Usage: %s -c<channel_opts>\n"
> >> +"QEMU Guest Agent %s\n"
> >> +"\n"
> >> +"  -c, --channel     channel method: one of unix-connect, virtio-serial, or\n"
> >> +"                    isa-serial (virtio-serial is the default)\n"
> >> +"  -p, --path        channel path (%s is the default for virtio-serial)\n"
> >> +"  -l, --logfile     set logfile path, logs to stderr by default\n"
> >> +"  -f, --pidfile     specify pidfile (default is %s)\n"
> >> +"  -v, --verbose     log extra debugging information\n"
> >> +"  -V, --version     print version information and exit\n"
> >> +"  -d, --daemonize   become a daemon\n"
> >> +"  -h, --help        display this help and exit\n"
> >> +"\n"
> >> +"Report bugs to<mdroth@linux.vnet.ibm.com>\n"
> >> +    , cmd, QGA_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT);
> >> +}
> >> +
> >> +static void conn_channel_close(GAState *s);
> >> +
> >> +static const char *ga_log_level_str(GLogLevelFlags level)
> >> +{
> >> +    switch (level&  G_LOG_LEVEL_MASK) {
> >> +        case G_LOG_LEVEL_ERROR:
> >> +            return "error";
> >> +        case G_LOG_LEVEL_CRITICAL:
> >> +            return "critical";
> >> +        case G_LOG_LEVEL_WARNING:
> >> +            return "warning";
> >> +        case G_LOG_LEVEL_MESSAGE:
> >> +            return "message";
> >> +        case G_LOG_LEVEL_INFO:
> >> +            return "info";
> >> +        case G_LOG_LEVEL_DEBUG:
> >> +            return "debug";
> >> +        default:
> >> +            return "user";
> >> +    }
> >> +}
> >> +
> >> +bool ga_logging_enabled(GAState *s)
> >> +{
> >> +    return s->logging_enabled;
> >> +}
> >> +
> >> +void ga_disable_logging(GAState *s)
> >> +{
> >> +    s->logging_enabled = false;
> >> +}
> >> +
> >> +void ga_enable_logging(GAState *s)
> >> +{
> >> +    s->logging_enabled = true;
> >> +}
> >
> > Just to check I got this right, this is needed because of the fsfreeze
> > command, correct? Isn't it better to have a more descriptive name, like
> > fsfrozen?
> >
> > First I thought this was about a log file. Then I realized this was
> > probably about letting the user log in, but we don't seem to have this
> > functionality so I got confused.
> >
> 
> Yup, this is currently due to fsfreeze support. From the perspective of 
> the fsfreeze command the explicit "is_frozen" check makes more sense, 
> but the reason it affects other RPCs is because because we can't log 
> stuff in that state. If an RPC shoots itself in the foot by writing to a 
> frozen filesystem we don't really care so much, and up until recently 
> that case was handled with a pthread_cancel timeout mechanism (was 
> removed for the time being, will re-implement using a child process most 
> likely).
> 
> What we don't want to do is give a host a way to bypass the expectation 
> we set for guest owners by allowing RPCs to be logged. So that's what 
> the check is based on, rather than lower level details like *why* 
> logging is disabled. Also, I'd really like to avoid a precedence where a 
> single RPC can place restrictions on all the others, so the logging 
> aspect seemed general enough that it doesn't necessarily provide that 
> precedence.
> 
> This has come up a few times without any real consensus. I can probably 
> go either way, but I think the check for logging is easier to set 
> expectations with: "if logging is important from an auditing 
> perspective, don't execute this if logging is disabled". beyond that, 
> same behavior/symantics you'd get with anything that causes a write() on 
> a filesystem in a frozen state: block.

While I understand your arguments, I still find the current behavior
confusing: you issue a guest-open-file command and get a QgaLoggingFailed
error. The first question is: what does a log write fail have to do with me
opening a file? The second question is: what should I do? Try again? Give up?

IMHO, making fsfrozen a global state makes more sense. It's the true guest
state after all. This way a client will clearly know what's happening and
what it has to do in order to make its command successful.

Another possible solution is to have the same semantics of a non-blocking
I/O, that's, return EWOULDBLOCK. I find this less confusing than the
current error.

> 
> >> +
> >> +static void ga_log(const gchar *domain, GLogLevelFlags level,
> >> +                   const gchar *msg, gpointer opaque)
> >> +{
> >> +    GAState *s = opaque;
> >> +    GTimeVal time;
> >> +    const char *level_str = ga_log_level_str(level);
> >> +
> >> +    if (!ga_logging_enabled(s)) {
> >> +        return;
> >> +    }
> >> +
> >> +    level&= G_LOG_LEVEL_MASK;
> >> +    if (g_strcmp0(domain, "syslog") == 0) {
> >> +        syslog(LOG_INFO, "%s: %s", level_str, msg);
> >> +    } else if (level&  s->log_level) {
> >> +        g_get_current_time(&time);
> >> +        fprintf(s->log_file,
> >> +                "%lu.%lu: %s: %s\n", time.tv_sec, time.tv_usec, level_str, msg);
> >> +        fflush(s->log_file);
> >> +    }
> >> +}
> >> +
> >> +static void become_daemon(const char *pidfile)
> >> +{
> >> +    pid_t pid, sid;
> >> +    int pidfd;
> >> +    char *pidstr = NULL;
> >> +
> >> +    pid = fork();
> >> +    if (pid<  0) {
> >> +        exit(EXIT_FAILURE);
> >> +    }
> >> +    if (pid>  0) {
> >> +        exit(EXIT_SUCCESS);
> >> +    }
> >> +
> >> +    pidfd = open(pidfile, O_CREAT|O_RDWR, S_IRUSR|S_IWUSR);
> >
> > O_WRONLY is enough.
> >
> >> +    if (!pidfd || lockf(pidfd, F_TLOCK, 0)) {
> >> +        g_error("Cannot lock pid file");
> >> +    }
> >
> > Are you sure we need lockf() here? I think using O_EXCL is enough for
> > pid files.
> >
> 
> O_EXCL + O_CREAT? Wouldn't we have issues if the pid file wasn't cleaned 
> up from a previous run?

Yes, but that's the intention (and that's how most daemons work afaik). The
only reason for a daemon not to cleanup its pid file is when it exists
abnormally. If this happens, then only the sysadmin can ensure that it's safe
to run another daemon instance.

We could add a -r (--override-existing-pid-file) option, but we should not
do it silently.

Btw, we need to implement a signal handler for SIGTERM.

> >> +
> >> +    if (ftruncate(pidfd, 0) || lseek(pidfd, 0, SEEK_SET)) {
> >> +        g_critical("Cannot truncate pid file");
> >> +        goto fail;
> >
> > You can use O_TRUNC and the file pointer is already positioned at the
> > beginning of the file, no need to call lseek().
> >
> >> +    }
> >> +    if (asprintf(&pidstr, "%d", getpid()) == -1) {
> >> +        g_critical("Cannot allocate memory");
> >> +        goto fail;
> >> +    }
> >> +    if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
> >> +        g_critical("Failed to write pid file");
> >> +        goto fail;
> >> +    }
> >
> > You're not freeing pidstr, nor closing the file (although I think you
> > do this because of the lockf() call()).
> >
> 
> pidstr gets free'd in the fail: block. And yeah, closing would give up 
> the lock prematurely.

pidstr leaks on a successful execution.

> 
> >> +
> >> +    umask(0);
> >> +    sid = setsid();
> >> +    if (sid<  0) {
> >> +        goto fail;
> >> +    }
> >> +    if ((chdir("/"))<  0) {
> >> +        goto fail;
> >> +    }
> >> +
> >> +    close(STDIN_FILENO);
> >> +    close(STDOUT_FILENO);
> >> +    close(STDERR_FILENO);
> >> +    return;
> >> +
> >> +fail:
> >> +    if (pidstr) {
> >> +        free(pidstr);
> >> +    }
> >
> > This check is not needed.
> >
> >> +    unlink(pidfile);
> >> +    g_error("failed to daemonize");
> >> +}
> >> +
> >> +static int conn_channel_send_payload(GIOChannel *channel, QObject *payload)
> >> +{
> >> +    gsize count, written = 0;
> >> +    int ret;
> >> +    const char *buf;
> >> +    QString *payload_qstr;
> >> +    GIOStatus status;
> >> +    GError *err = NULL;
> >> +
> >> +    if (!payload || !channel) {
> >> +        ret = -EINVAL;
> >> +        goto out;
> >> +    }
> >
> > Just do "return -EINVAL" instead of using goto, but I wonder if this should
> > be an assert() instead.
> >
> 
> Yah, looks like it.
> 
> >> +
> >> +    payload_qstr = qobject_to_json(payload);
> >> +    if (!payload_qstr) {
> >> +        ret = -EINVAL;
> >> +        goto out;
> >> +    }
> >
> > return -EINVAL.
> >
> >> +
> >> +    buf = qstring_get_str(payload_qstr);
> >> +    count = strlen(buf);
> >> +
> >> +    while (count) {
> >> +        g_debug("sending data, count: %d", (int)count);
> >> +        status = g_io_channel_write_chars(channel, buf, count,&written,&err);
> >> +        if (err != NULL) {
> >> +            g_warning("error writing payload to channel: %s", err->message);
> >> +            ret = err->code;
> >> +            goto out_free;
> >> +        }
> >> +        if (status == G_IO_STATUS_NORMAL) {
> >> +            count -= written;
> >> +        } else if (status == G_IO_STATUS_ERROR || status == G_IO_STATUS_EOF) {
> >> +            ret = -EPIPE;
> >> +            goto out_free;
> >> +        }
> >> +    }
> >> +
> >> +    status = g_io_channel_write_chars(channel, (char *)"\n", 1,&written,&err);
> >> +    if (err != NULL) {
> >> +        g_warning("error sending newline: %s", err->message);
> >> +        ret = err->code;
> >> +        goto out_free;
> >> +    } else if (status == G_IO_STATUS_ERROR || status == G_IO_STATUS_EOF) {
> >> +        ret = -EPIPE;
> >> +        goto out_free;
> >> +    }
> >
> > This wants to be a function.
> >
> >> +
> >> +    g_io_channel_flush(channel,&err);
> >> +    if (err != NULL) {
> >> +        g_warning("error flushing payload: %s", err->message);
> >> +        ret = err->code;
> >> +        goto out_free;
> >> +    }
> >> +
> >> +    ret = 0;
> >> +
> >> +out_free:
> >> +    QDECREF(payload_qstr);
> >> +    if (err != NULL) {
> >> +        g_error_free(err);
> >> +    }
> >
> > No need to check against NULL.
> >
> >> +out:
> >> +    return ret;
> >> +}
> >> +
> >> +static void process_command(GAState *s, QDict *req)
> >> +{
> >> +    QObject *rsp = NULL;
> >> +    Error *err = NULL;
> >> +    int ret;
> >> +
> >> +    g_assert(req);
> >> +    g_debug("processing command");
> >> +    rsp = qmp_dispatch(QOBJECT(req));
> >> +    if (rsp) {
> >> +        if (err) {
> >
> > Apart from its initialization, 'err' is read only. Either, you want to
> > use qmp_dispatch_err() here or you have to modify qmp_dispatch() to also
> > accept an Error argument.
> >
> > If you want to send a "return" or "error" QMP kind of response, then you
> > have to do the latter.
> >
> 
> qmp_dispatch() does the Error -> QMP error conversion. The unused err is 
> cruft from when a seperate worker thread did the dispatch, will remove it.
> 
> >> +            g_warning("command failed: %s", error_get_pretty(err));
> >> +        }
> >> +        ret = conn_channel_send_payload(s->conn_channel, rsp);
> >> +        if (ret) {
> >> +            g_warning("error sending payload: %s", strerror(ret));
> >> +        }
> >> +        qobject_decref(rsp);
> >> +    } else {
> >> +        g_warning("error getting response");
> >> +        if (err) {
> >> +            g_warning("dispatch failed: %s", error_get_pretty(err));
> >> +        }
> >> +    }
> >> +}
> >> +
> >> +/* handle requests/control events coming in over the channel */
> >> +static void process_event(JSONMessageParser *parser, QList *tokens)
> >> +{
> >> +    GAState *s = container_of(parser, GAState, parser);
> >> +    QObject *obj;
> >> +    QDict *qdict;
> >> +    Error *err = NULL;
> >> +
> >> +    g_assert(s&&  parser);
> >> +
> >> +    g_debug("process_event: called");
> >> +    obj = json_parser_parse_err(tokens, NULL,&err);
> >> +    if (!obj || qobject_type(obj) != QTYPE_QDICT) {
> >> +        g_warning("failed to parse event");
> >
> > Two possible leaks here. You have to call qobject_decref() if obj != NULL
> > and there's a missing call to error_free(). But you don't use 'err' anyway,
> > so you could pass NULL there.
> >
> > Oh, btw, the guest agent doesn't seem to report errors back to its client...
> > Not even json error messages, is this intended?
> >
> 
> Hmm, yes it's intended, but I probably should send those to the client...
> 
> You do get non-parse errors though: missing arguments, etc.
> 
> I'll fix this to do what QMP does here.
> 
> >> +        return;
> >> +    } else {
> >> +        g_debug("parse successful");
> >> +        qdict = qobject_to_qdict(obj);
> >> +        g_assert(qdict);
> >> +    }
> >
> > Superfluous else clause.
> >
> >> +
> >> +    /* handle host->guest commands */
> >> +    if (qdict_haskey(qdict, "execute")) {
> >> +        process_command(s, qdict);
> >> +    } else {
> >> +        g_warning("unrecognized payload format, ignoring");
> >> +    }
> >> +
> >> +    QDECREF(qdict);
> >> +}
> >> +
> >> +static gboolean conn_channel_read(GIOChannel *channel, GIOCondition condition,
> >> +                                  gpointer data)
> >> +{
> >> +    GAState *s = data;
> >> +    gchar buf[1024];
> >> +    gsize count;
> >> +    GError *err = NULL;
> >> +    memset(buf, 0, 1024);
> >> +    GIOStatus status = g_io_channel_read_chars(channel, buf, 1024,
> >> +&count,&err);
> >> +    if (err != NULL) {
> >> +        g_warning("error reading channel: %s", err->message);
> >> +        conn_channel_close(s);
> >> +        g_error_free(err);
> >> +        return false;
> >> +    }
> >> +    switch (status) {
> >> +    case G_IO_STATUS_ERROR:
> >> +        g_warning("problem");
> >> +        return false;
> >> +    case G_IO_STATUS_NORMAL:
> >> +        g_debug("read data, count: %d, data: %s", (int)count, buf);
> >> +        json_message_parser_feed(&s->parser, (char *)buf, (int)count);
> >> +    case G_IO_STATUS_AGAIN:
> >> +        /* virtio causes us to spin here when no process is attached to
> >> +         * host-side chardev. sleep a bit to mitigate this
> >> +         */
> >> +        if (s->virtio) {
> >> +            usleep(100*1000);
> >> +        }
> >> +        return true;
> >> +    case G_IO_STATUS_EOF:
> >> +        g_debug("received EOF");
> >> +        conn_channel_close(s);
> >> +        if (s->virtio) {
> >> +            return true;
> >> +        }
> >> +        return false;
> >> +    default:
> >> +        g_warning("unknown channel read status, closing");
> >> +        conn_channel_close(s);
> >> +        return false;
> >> +    }
> >> +    return true;
> >> +}
> >> +
> >> +static int conn_channel_add(GAState *s, int fd)
> >> +{
> >> +    GIOChannel *conn_channel;
> >> +    guint conn_id;
> >> +    GError *err = NULL;
> >> +
> >> +    g_assert(s&&  !s->conn_channel);
> >> +    conn_channel = g_io_channel_unix_new(fd);
> >> +    g_assert(conn_channel);
> >> +    g_io_channel_set_encoding(conn_channel, NULL,&err);
> >> +    if (err != NULL) {
> >> +        g_warning("error setting channel encoding to binary");
> >> +        g_error_free(err);
> >> +        return -1;
> >> +    }
> >> +    conn_id = g_io_add_watch(conn_channel, G_IO_IN | G_IO_HUP,
> >> +                             conn_channel_read, s);
> >> +    if (err != NULL) {
> >> +        g_warning("error adding io watch: %s", err->message);
> >> +        g_error_free(err);
> >> +        return -1;
> >> +    }
> >> +    s->conn_channel = conn_channel;
> >> +    s->conn_id = conn_id;
> >> +    return 0;
> >> +}
> >> +
> >> +static gboolean listen_channel_accept(GIOChannel *channel,
> >> +                                      GIOCondition condition, gpointer data)
> >> +{
> >> +    GAState *s = data;
> >> +    GError *err = NULL;
> >> +    g_assert(channel != NULL);
> >> +    int ret;
> >> +    bool accepted = false;
> >> +
> >> +    s->conn_sock = g_socket_accept(s->listen_sock, NULL,&err);
> >> +    if (err != NULL) {
> >> +        g_warning("error converting fd to gsocket: %s", err->message);
> >> +        g_error_free(err);
> >> +        goto out;
> >> +    }
> >> +    ret = conn_channel_add(s, g_socket_get_fd(s->conn_sock));
> >> +    if (ret) {
> >> +        g_warning("error setting up connection");
> >> +        goto out;
> >> +    }
> >> +    accepted = true;
> >> +
> >> +out:
> >> +    /* only accept 1 connection at a time */
> >> +    return !accepted;
> >> +}
> >> +
> >> +/* start polling for readable events on listen fd, listen_fd=0
> >> + * indicates we should use the existing s->listen_channel
> >> + */
> >> +static int listen_channel_add(GAState *s, int listen_fd)
> >> +{
> >> +    GError *err = NULL;
> >> +    guint listen_id;
> >> +
> >> +    if (listen_fd) {
> >> +        s->listen_channel = g_io_channel_unix_new(listen_fd);
> >> +        if (s->listen_sock) {
> >> +            g_object_unref(s->listen_sock);
> >> +        }
> >> +        s->listen_sock = g_socket_new_from_fd(listen_fd,&err);
> >> +        if (err != NULL) {
> >> +            g_warning("error converting fd to gsocket: %s", err->message);
> >> +            g_error_free(err);
> >> +            return -1;
> >> +        }
> >> +    }
> >> +    listen_id = g_io_add_watch(s->listen_channel, G_IO_IN,
> >> +                               listen_channel_accept, s);
> >> +    if (err != NULL) {
> >> +        g_warning("error adding io watch: %s", err->message);
> >> +        g_error_free(err);
> >> +        return -1;
> >> +    }
> >> +    return 0;
> >> +}
> >> +
> >> +/* cleanup state for closed connection/session, start accepting new
> >> + * connections if we're in listening mode
> >> + */
> >> +static void conn_channel_close(GAState *s)
> >> +{
> >> +    if (strcmp(s->method, "unix-listen") == 0) {
> >> +        g_io_channel_shutdown(s->conn_channel, true, NULL);
> >> +        g_object_unref(s->conn_sock);
> >> +        s->conn_sock = NULL;
> >> +        listen_channel_add(s, 0);
> >> +    } else if (strcmp(s->method, "virtio-serial") == 0) {
> >> +        /* we spin on EOF for virtio-serial, so back off a bit. also,
> >> +         * dont close the connection in this case, it'll resume normal
> >> +         * operation when another process connects to host chardev
> >> +         */
> >> +        usleep(100*1000);
> >> +        goto out_noclose;
> >> +    }
> >> +    g_io_channel_unref(s->conn_channel);
> >> +    s->conn_channel = NULL;
> >> +    s->conn_id = 0;
> >> +out_noclose:
> >> +    return;
> >> +}
> >> +
> >> +static void init_guest_agent(GAState *s)
> >> +{
> >> +    struct termios tio;
> >> +    int ret, fd;
> >> +
> >> +    if (s->method == NULL) {
> >> +        /* try virtio-serial as our default */
> >> +        s->method = "virtio-serial";
> >> +    }
> >> +
> >> +    if (s->path == NULL) {
> >> +        if (strcmp(s->method, "virtio-serial") != 0) {
> >> +            g_error("must specify a path for this channel");
> >> +        }
> >> +        /* try the default path for the virtio-serial port */
> >> +        s->path = QGA_VIRTIO_PATH_DEFAULT;
> >> +    }
> >> +
> >> +    if (strcmp(s->method, "virtio-serial") == 0) {
> >> +        s->virtio = true;
> >> +        fd = qemu_open(s->path, O_RDWR);
> >> +        if (fd == -1) {
> >> +            g_error("error opening channel: %s", strerror(errno));
> >> +        }
> >> +        ret = fcntl(fd, F_GETFL);
> >> +        if (ret<  0) {
> >> +            g_error("error getting channel flags: %s", strerror(errno));
> >> +        }
> >> +        ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK | O_ASYNC);
> >> +        if (ret<  0) {
> >> +            g_error("error setting channel flags: %s", strerror(errno));
> >> +        }
> >> +        ret = conn_channel_add(s, fd);
> >> +        if (ret) {
> >> +            g_error("error adding channel to main loop");
> >> +        }
> >> +    } else if (strcmp(s->method, "isa-serial") == 0) {
> >> +        fd = qemu_open(s->path, O_RDWR | O_NOCTTY);
> >> +        if (fd == -1) {
> >> +            g_error("error opening channel: %s", strerror(errno));
> >> +        }
> >> +        tcgetattr(fd,&tio);
> >> +        /* set up serial port for non-canonical, dumb byte streaming */
> >> +        tio.c_iflag&= ~(IGNBRK | BRKINT | IGNPAR | PARMRK | INPCK | ISTRIP |
> >> +                         INLCR | IGNCR | ICRNL | IXON | IXOFF | IXANY |
> >> +                         IMAXBEL);
> >> +        tio.c_oflag = 0;
> >> +        tio.c_lflag = 0;
> >> +        tio.c_cflag |= QGA_BAUDRATE_DEFAULT;
> >> +        /* 1 available byte min or reads will block (we'll set non-blocking
> >> +         * elsewhere, else we have to deal with read()=0 instead)
> >> +         */
> >> +        tio.c_cc[VMIN] = 1;
> >> +        tio.c_cc[VTIME] = 0;
> >> +        /* flush everything waiting for read/xmit, it's garbage at this point */
> >> +        tcflush(fd, TCIFLUSH);
> >> +        tcsetattr(fd, TCSANOW,&tio);
> >> +        ret = conn_channel_add(s, fd);
> >> +        if (ret) {
> >> +            g_error("error adding channel to main loop");
> >> +        }
> >> +    } else if (strcmp(s->method, "unix-listen") == 0) {
> >> +        fd = unix_listen(s->path, NULL, strlen(s->path));
> >> +        if (fd == -1) {
> >> +            g_error("error opening path: %s", strerror(errno));
> >> +        }
> >> +        ret = listen_channel_add(s, fd);
> >> +        if (ret) {
> >> +            g_error("error binding/listening to specified socket");
> >> +        }
> >> +    } else {
> >> +        g_error("unsupported channel method/type: %s", s->method);
> >> +    }
> >> +
> >> +    json_message_parser_init(&s->parser, process_event);
> >> +    s->main_loop = g_main_loop_new(NULL, false);
> >> +}
> >> +
> >> +int main(int argc, char **argv)
> >> +{
> >> +    const char *sopt = "hVvdc:p:l:f:";
> >> +    const char *method = NULL, *path = NULL, *pidfile = QGA_PIDFILE_DEFAULT;
> >> +    struct option lopt[] = {
> >> +        { "help", 0, NULL, 'h' },
> >> +        { "version", 0, NULL, 'V' },
> >> +        { "logfile", 0, NULL, 'l' },
> >> +        { "pidfile", 0, NULL, 'f' },
> >> +        { "verbose", 0, NULL, 'v' },
> >> +        { "channel", 0, NULL, 'c' },
> >> +        { "path", 0, NULL, 'p' },
> >> +        { "daemonize", 0, NULL, 'd' },
> >> +        { NULL, 0, NULL, 0 }
> >> +    };
> >> +    int opt_ind = 0, ch, daemonize = 0;
> >> +    GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
> >> +    FILE *log_file = stderr;
> >> +    GAState *s;
> >> +
> >> +    g_type_init();
> >> +    g_thread_init(NULL);
> >> +
> >> +    while ((ch = getopt_long(argc, argv, sopt, lopt,&opt_ind)) != -1) {
> >> +        switch (ch) {
> >> +        case 'c':
> >> +            method = optarg;
> >> +            break;
> >> +        case 'p':
> >> +            path = optarg;
> >> +            break;
> >> +        case 'l':
> >> +            log_file = fopen(optarg, "a");
> >> +            if (!log_file) {
> >> +                g_error("unable to open specified log file: %s",
> >> +                        strerror(errno));
> >> +            }
> >> +            break;
> >> +        case 'f':
> >> +            pidfile = optarg;
> >> +            break;
> >> +        case 'v':
> >> +            /* enable all log levels */
> >> +            log_level = G_LOG_LEVEL_MASK;
> >> +            break;
> >> +        case 'V':
> >> +            printf("QEMU Guest Agent %s\n", QGA_VERSION);
> >> +            return 0;
> >> +        case 'd':
> >> +            daemonize = 1;
> >> +            break;
> >> +        case 'h':
> >> +            usage(argv[0]);
> >> +            return 0;
> >> +        case '?':
> >> +            g_error("Unknown option, try '%s --help' for more information.",
> >> +                    argv[0]);
> >> +        }
> >> +    }
> >> +
> >> +    if (daemonize) {
> >> +        g_debug("starting daemon");
> >> +        become_daemon(pidfile);
> >> +    }
> >> +
> >> +    s = g_malloc0(sizeof(GAState));
> >> +    s->conn_id = 0;
> >> +    s->conn_channel = NULL;
> >> +    s->path = path;
> >> +    s->method = method;
> >> +    s->command_state = ga_command_state_new();
> >> +    ga_command_state_init(s, s->command_state);
> >> +    ga_command_state_init_all(s->command_state);
> >> +    s->log_file = log_file;
> >> +    s->log_level = log_level;
> >> +    g_log_set_default_handler(ga_log, s);
> >> +    g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
> >> +    s->logging_enabled = true;
> >> +
> >> +    module_call_init(MODULE_INIT_QAPI);
> >> +    init_guest_agent(s);
> >> +
> >> +    g_main_loop_run(s->main_loop);
> >> +
> >> +    ga_command_state_cleanup_all(s->command_state);
> >> +
> >> +    return 0;
> >> +}
> >> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> >> index 688f120..66d1729 100644
> >> --- a/qga/guest-agent-core.h
> >> +++ b/qga/guest-agent-core.h
> >
> > No license text.
> >
> >> @@ -15,6 +15,7 @@
> >>
> >>   #define QGA_VERSION "1.0"
> >>
> >> +typedef struct GAState GAState;
> >>   typedef struct GACommandState GACommandState;
> >>
> >>   void ga_command_state_add(GACommandState *cs,
> >> @@ -23,3 +24,6 @@ void ga_command_state_add(GACommandState *cs,
> >>   void ga_command_state_init_all(GACommandState *cs);
> >>   void ga_command_state_cleanup_all(GACommandState *cs);
> >>   GACommandState *ga_command_state_new(void);
> >> +bool ga_logging_enabled(GAState *s);
> >> +void ga_disable_logging(GAState *s);
> >> +void ga_enable_logging(GAState *s);
> >
>
Michael Roth - June 17, 2011, 9:25 p.m.
On 06/17/2011 03:13 PM, Luiz Capitulino wrote:
> On Fri, 17 Jun 2011 14:21:31 -0500
> Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>
>> On 06/16/2011 01:42 PM, Luiz Capitulino wrote:
>>> On Tue, 14 Jun 2011 15:06:22 -0500
>>> Michael Roth<mdroth@linux.vnet.ibm.com>   wrote:
>>>
>>>> This is the actual guest daemon, it listens for requests over a
>>>> virtio-serial/isa-serial/unix socket channel and routes them through
>>>> to dispatch routines, and writes the results back to the channel in
>>>> a manner similar to QMP.
>>>>
>>>> A shorthand invocation:
>>>>
>>>>     qemu-ga -d
>>>>
>>>> Is equivalent to:
>>>>
>>>>     qemu-ga -c virtio-serial -p /dev/virtio-ports/org.qemu.guest_agent \
>>>>             -p /var/run/qemu-guest-agent.pid -d
>>>>
>>>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>>>
>>> Would be nice to have a more complete description, like explaining how to
>>> do a simple test.
>>>
>>> And this can't be built...
>>>
>>>> ---
>>>>    qemu-ga.c              |  631 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    qga/guest-agent-core.h |    4 +
>>>>    2 files changed, 635 insertions(+), 0 deletions(-)
>>>>    create mode 100644 qemu-ga.c
>>>>
>>>> diff --git a/qemu-ga.c b/qemu-ga.c
>>>> new file mode 100644
>>>> index 0000000..df08d8c
>>>> --- /dev/null
>>>> +++ b/qemu-ga.c
>>>> @@ -0,0 +1,631 @@
>>>> +/*
>>>> + * QEMU Guest Agent
>>>> + *
>>>> + * Copyright IBM Corp. 2011
>>>> + *
>>>> + * Authors:
>>>> + *  Adam Litke<aglitke@linux.vnet.ibm.com>
>>>> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>>> + * See the COPYING file in the top-level directory.
>>>> + */
>>>> +#include<stdlib.h>
>>>> +#include<stdio.h>
>>>> +#include<stdbool.h>
>>>> +#include<glib.h>
>>>> +#include<gio/gio.h>
>>>> +#include<getopt.h>
>>>> +#include<termios.h>
>>>> +#include<syslog.h>
>>>> +#include "qemu_socket.h"
>>>> +#include "json-streamer.h"
>>>> +#include "json-parser.h"
>>>> +#include "qint.h"
>>>> +#include "qjson.h"
>>>> +#include "qga/guest-agent-core.h"
>>>> +#include "qga-qmp-commands.h"
>>>> +#include "module.h"
>>>> +
>>>> +#define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent"
>>>> +#define QGA_PIDFILE_DEFAULT "/var/run/qemu-va.pid"
>>>> +#define QGA_BAUDRATE_DEFAULT B38400 /* for isa-serial channels */
>>>> +#define QGA_TIMEOUT_DEFAULT 30*1000 /* ms */
>>>> +
>>>> +struct GAState {
>>>> +    const char *proxy_path;
>>>
>>> Where is this used?
>>>
>>
>> Nowhere actually. Will remove.
>>
>>>> +    JSONMessageParser parser;
>>>> +    GMainLoop *main_loop;
>>>> +    guint conn_id;
>>>> +    GSocket *conn_sock;
>>>> +    GIOChannel *conn_channel;
>>>> +    guint listen_id;
>>>> +    GSocket *listen_sock;
>>>> +    GIOChannel *listen_channel;
>>>> +    const char *path;
>>>> +    const char *method;
>>>> +    bool virtio; /* fastpath to check for virtio to deal with poll() quirks */
>>>> +    GACommandState *command_state;
>>>> +    GLogLevelFlags log_level;
>>>> +    FILE *log_file;
>>>> +    bool logging_enabled;
>>>> +};
>>>> +
>>>> +static void usage(const char *cmd)
>>>> +{
>>>> +    printf(
>>>> +"Usage: %s -c<channel_opts>\n"
>>>> +"QEMU Guest Agent %s\n"
>>>> +"\n"
>>>> +"  -c, --channel     channel method: one of unix-connect, virtio-serial, or\n"
>>>> +"                    isa-serial (virtio-serial is the default)\n"
>>>> +"  -p, --path        channel path (%s is the default for virtio-serial)\n"
>>>> +"  -l, --logfile     set logfile path, logs to stderr by default\n"
>>>> +"  -f, --pidfile     specify pidfile (default is %s)\n"
>>>> +"  -v, --verbose     log extra debugging information\n"
>>>> +"  -V, --version     print version information and exit\n"
>>>> +"  -d, --daemonize   become a daemon\n"
>>>> +"  -h, --help        display this help and exit\n"
>>>> +"\n"
>>>> +"Report bugs to<mdroth@linux.vnet.ibm.com>\n"
>>>> +    , cmd, QGA_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT);
>>>> +}
>>>> +
>>>> +static void conn_channel_close(GAState *s);
>>>> +
>>>> +static const char *ga_log_level_str(GLogLevelFlags level)
>>>> +{
>>>> +    switch (level&   G_LOG_LEVEL_MASK) {
>>>> +        case G_LOG_LEVEL_ERROR:
>>>> +            return "error";
>>>> +        case G_LOG_LEVEL_CRITICAL:
>>>> +            return "critical";
>>>> +        case G_LOG_LEVEL_WARNING:
>>>> +            return "warning";
>>>> +        case G_LOG_LEVEL_MESSAGE:
>>>> +            return "message";
>>>> +        case G_LOG_LEVEL_INFO:
>>>> +            return "info";
>>>> +        case G_LOG_LEVEL_DEBUG:
>>>> +            return "debug";
>>>> +        default:
>>>> +            return "user";
>>>> +    }
>>>> +}
>>>> +
>>>> +bool ga_logging_enabled(GAState *s)
>>>> +{
>>>> +    return s->logging_enabled;
>>>> +}
>>>> +
>>>> +void ga_disable_logging(GAState *s)
>>>> +{
>>>> +    s->logging_enabled = false;
>>>> +}
>>>> +
>>>> +void ga_enable_logging(GAState *s)
>>>> +{
>>>> +    s->logging_enabled = true;
>>>> +}
>>>
>>> Just to check I got this right, this is needed because of the fsfreeze
>>> command, correct? Isn't it better to have a more descriptive name, like
>>> fsfrozen?
>>>
>>> First I thought this was about a log file. Then I realized this was
>>> probably about letting the user log in, but we don't seem to have this
>>> functionality so I got confused.
>>>
>>
>> Yup, this is currently due to fsfreeze support. From the perspective of
>> the fsfreeze command the explicit "is_frozen" check makes more sense,
>> but the reason it affects other RPCs is because because we can't log
>> stuff in that state. If an RPC shoots itself in the foot by writing to a
>> frozen filesystem we don't really care so much, and up until recently
>> that case was handled with a pthread_cancel timeout mechanism (was
>> removed for the time being, will re-implement using a child process most
>> likely).
>>
>> What we don't want to do is give a host a way to bypass the expectation
>> we set for guest owners by allowing RPCs to be logged. So that's what
>> the check is based on, rather than lower level details like *why*
>> logging is disabled. Also, I'd really like to avoid a precedence where a
>> single RPC can place restrictions on all the others, so the logging
>> aspect seemed general enough that it doesn't necessarily provide that
>> precedence.
>>
>> This has come up a few times without any real consensus. I can probably
>> go either way, but I think the check for logging is easier to set
>> expectations with: "if logging is important from an auditing
>> perspective, don't execute this if logging is disabled". beyond that,
>> same behavior/symantics you'd get with anything that causes a write() on
>> a filesystem in a frozen state: block.
>
> While I understand your arguments, I still find the current behavior
> confusing: you issue a guest-open-file command and get a QgaLoggingFailed
> error. The first question is: what does a log write fail have to do with me
> opening a file? The second question is: what should I do? Try again? Give up?
>

I agree it's a better solution for the client there, but at the same time:

guest_privileged_ping():
   if fsfreeze.status == FROZEN:
     syslog("privileged ping, thought you should know")
   else:
     return "error, filesystem frozen"

Seems silly to the client as well. "Why does a ping command care about 
the filesystem state?"

Inability to log is the true error. That's also the case for the 
guest-file-open command. There's nothing wrong with opening a read-only 
file on a frozen filesystem, it's the fact that we can't log the open 
that causes us to bail out..

So, what if we just munge the 2 to give the user the proper clues to fix 
things, and instead return an error like:

"Guest agent failed to log RPC (is the filesystem frozen?)"?

> IMHO, making fsfrozen a global state makes more sense. It's the true guest
> state after all. This way a client will clearly know what's happening and
> what it has to do in order to make its command successful.
>
> Another possible solution is to have the same semantics of a non-blocking
> I/O, that's, return EWOULDBLOCK. I find this less confusing than the
> current error.
>
>>
>>>> +
>>>> +static void ga_log(const gchar *domain, GLogLevelFlags level,
>>>> +                   const gchar *msg, gpointer opaque)
>>>> +{
>>>> +    GAState *s = opaque;
>>>> +    GTimeVal time;
>>>> +    const char *level_str = ga_log_level_str(level);
>>>> +
>>>> +    if (!ga_logging_enabled(s)) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    level&= G_LOG_LEVEL_MASK;
>>>> +    if (g_strcmp0(domain, "syslog") == 0) {
>>>> +        syslog(LOG_INFO, "%s: %s", level_str, msg);
>>>> +    } else if (level&   s->log_level) {
>>>> +        g_get_current_time(&time);
>>>> +        fprintf(s->log_file,
>>>> +                "%lu.%lu: %s: %s\n", time.tv_sec, time.tv_usec, level_str, msg);
>>>> +        fflush(s->log_file);
>>>> +    }
>>>> +}
>>>> +
>>>> +static void become_daemon(const char *pidfile)
>>>> +{
>>>> +    pid_t pid, sid;
>>>> +    int pidfd;
>>>> +    char *pidstr = NULL;
>>>> +
>>>> +    pid = fork();
>>>> +    if (pid<   0) {
>>>> +        exit(EXIT_FAILURE);
>>>> +    }
>>>> +    if (pid>   0) {
>>>> +        exit(EXIT_SUCCESS);
>>>> +    }
>>>> +
>>>> +    pidfd = open(pidfile, O_CREAT|O_RDWR, S_IRUSR|S_IWUSR);
>>>
>>> O_WRONLY is enough.
>>>
>>>> +    if (!pidfd || lockf(pidfd, F_TLOCK, 0)) {
>>>> +        g_error("Cannot lock pid file");
>>>> +    }
>>>
>>> Are you sure we need lockf() here? I think using O_EXCL is enough for
>>> pid files.
>>>
>>
>> O_EXCL + O_CREAT? Wouldn't we have issues if the pid file wasn't cleaned
>> up from a previous run?
>
> Yes, but that's the intention (and that's how most daemons work afaik). The
> only reason for a daemon not to cleanup its pid file is when it exists
> abnormally. If this happens, then only the sysadmin can ensure that it's safe
> to run another daemon instance.
>
> We could add a -r (--override-existing-pid-file) option, but we should not
> do it silently.
>
> Btw, we need to implement a signal handler for SIGTERM.
>
>>>> +
>>>> +    if (ftruncate(pidfd, 0) || lseek(pidfd, 0, SEEK_SET)) {
>>>> +        g_critical("Cannot truncate pid file");
>>>> +        goto fail;
>>>
>>> You can use O_TRUNC and the file pointer is already positioned at the
>>> beginning of the file, no need to call lseek().
>>>
>>>> +    }
>>>> +    if (asprintf(&pidstr, "%d", getpid()) == -1) {
>>>> +        g_critical("Cannot allocate memory");
>>>> +        goto fail;
>>>> +    }
>>>> +    if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
>>>> +        g_critical("Failed to write pid file");
>>>> +        goto fail;
>>>> +    }
>>>
>>> You're not freeing pidstr, nor closing the file (although I think you
>>> do this because of the lockf() call()).
>>>
>>
>> pidstr gets free'd in the fail: block. And yeah, closing would give up
>> the lock prematurely.
>
> pidstr leaks on a successful execution.
>
>>
>>>> +
>>>> +    umask(0);
>>>> +    sid = setsid();
>>>> +    if (sid<   0) {
>>>> +        goto fail;
>>>> +    }
>>>> +    if ((chdir("/"))<   0) {
>>>> +        goto fail;
>>>> +    }
>>>> +
>>>> +    close(STDIN_FILENO);
>>>> +    close(STDOUT_FILENO);
>>>> +    close(STDERR_FILENO);
>>>> +    return;
>>>> +
>>>> +fail:
>>>> +    if (pidstr) {
>>>> +        free(pidstr);
>>>> +    }
>>>
>>> This check is not needed.
>>>
>>>> +    unlink(pidfile);
>>>> +    g_error("failed to daemonize");
>>>> +}
>>>> +
>>>> +static int conn_channel_send_payload(GIOChannel *channel, QObject *payload)
>>>> +{
>>>> +    gsize count, written = 0;
>>>> +    int ret;
>>>> +    const char *buf;
>>>> +    QString *payload_qstr;
>>>> +    GIOStatus status;
>>>> +    GError *err = NULL;
>>>> +
>>>> +    if (!payload || !channel) {
>>>> +        ret = -EINVAL;
>>>> +        goto out;
>>>> +    }
>>>
>>> Just do "return -EINVAL" instead of using goto, but I wonder if this should
>>> be an assert() instead.
>>>
>>
>> Yah, looks like it.
>>
>>>> +
>>>> +    payload_qstr = qobject_to_json(payload);
>>>> +    if (!payload_qstr) {
>>>> +        ret = -EINVAL;
>>>> +        goto out;
>>>> +    }
>>>
>>> return -EINVAL.
>>>
>>>> +
>>>> +    buf = qstring_get_str(payload_qstr);
>>>> +    count = strlen(buf);
>>>> +
>>>> +    while (count) {
>>>> +        g_debug("sending data, count: %d", (int)count);
>>>> +        status = g_io_channel_write_chars(channel, buf, count,&written,&err);
>>>> +        if (err != NULL) {
>>>> +            g_warning("error writing payload to channel: %s", err->message);
>>>> +            ret = err->code;
>>>> +            goto out_free;
>>>> +        }
>>>> +        if (status == G_IO_STATUS_NORMAL) {
>>>> +            count -= written;
>>>> +        } else if (status == G_IO_STATUS_ERROR || status == G_IO_STATUS_EOF) {
>>>> +            ret = -EPIPE;
>>>> +            goto out_free;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    status = g_io_channel_write_chars(channel, (char *)"\n", 1,&written,&err);
>>>> +    if (err != NULL) {
>>>> +        g_warning("error sending newline: %s", err->message);
>>>> +        ret = err->code;
>>>> +        goto out_free;
>>>> +    } else if (status == G_IO_STATUS_ERROR || status == G_IO_STATUS_EOF) {
>>>> +        ret = -EPIPE;
>>>> +        goto out_free;
>>>> +    }
>>>
>>> This wants to be a function.
>>>
>>>> +
>>>> +    g_io_channel_flush(channel,&err);
>>>> +    if (err != NULL) {
>>>> +        g_warning("error flushing payload: %s", err->message);
>>>> +        ret = err->code;
>>>> +        goto out_free;
>>>> +    }
>>>> +
>>>> +    ret = 0;
>>>> +
>>>> +out_free:
>>>> +    QDECREF(payload_qstr);
>>>> +    if (err != NULL) {
>>>> +        g_error_free(err);
>>>> +    }
>>>
>>> No need to check against NULL.
>>>
>>>> +out:
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static void process_command(GAState *s, QDict *req)
>>>> +{
>>>> +    QObject *rsp = NULL;
>>>> +    Error *err = NULL;
>>>> +    int ret;
>>>> +
>>>> +    g_assert(req);
>>>> +    g_debug("processing command");
>>>> +    rsp = qmp_dispatch(QOBJECT(req));
>>>> +    if (rsp) {
>>>> +        if (err) {
>>>
>>> Apart from its initialization, 'err' is read only. Either, you want to
>>> use qmp_dispatch_err() here or you have to modify qmp_dispatch() to also
>>> accept an Error argument.
>>>
>>> If you want to send a "return" or "error" QMP kind of response, then you
>>> have to do the latter.
>>>
>>
>> qmp_dispatch() does the Error ->  QMP error conversion. The unused err is
>> cruft from when a seperate worker thread did the dispatch, will remove it.
>>
>>>> +            g_warning("command failed: %s", error_get_pretty(err));
>>>> +        }
>>>> +        ret = conn_channel_send_payload(s->conn_channel, rsp);
>>>> +        if (ret) {
>>>> +            g_warning("error sending payload: %s", strerror(ret));
>>>> +        }
>>>> +        qobject_decref(rsp);
>>>> +    } else {
>>>> +        g_warning("error getting response");
>>>> +        if (err) {
>>>> +            g_warning("dispatch failed: %s", error_get_pretty(err));
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>> +/* handle requests/control events coming in over the channel */
>>>> +static void process_event(JSONMessageParser *parser, QList *tokens)
>>>> +{
>>>> +    GAState *s = container_of(parser, GAState, parser);
>>>> +    QObject *obj;
>>>> +    QDict *qdict;
>>>> +    Error *err = NULL;
>>>> +
>>>> +    g_assert(s&&   parser);
>>>> +
>>>> +    g_debug("process_event: called");
>>>> +    obj = json_parser_parse_err(tokens, NULL,&err);
>>>> +    if (!obj || qobject_type(obj) != QTYPE_QDICT) {
>>>> +        g_warning("failed to parse event");
>>>
>>> Two possible leaks here. You have to call qobject_decref() if obj != NULL
>>> and there's a missing call to error_free(). But you don't use 'err' anyway,
>>> so you could pass NULL there.
>>>
>>> Oh, btw, the guest agent doesn't seem to report errors back to its client...
>>> Not even json error messages, is this intended?
>>>
>>
>> Hmm, yes it's intended, but I probably should send those to the client...
>>
>> You do get non-parse errors though: missing arguments, etc.
>>
>> I'll fix this to do what QMP does here.
>>
>>>> +        return;
>>>> +    } else {
>>>> +        g_debug("parse successful");
>>>> +        qdict = qobject_to_qdict(obj);
>>>> +        g_assert(qdict);
>>>> +    }
>>>
>>> Superfluous else clause.
>>>
>>>> +
>>>> +    /* handle host->guest commands */
>>>> +    if (qdict_haskey(qdict, "execute")) {
>>>> +        process_command(s, qdict);
>>>> +    } else {
>>>> +        g_warning("unrecognized payload format, ignoring");
>>>> +    }
>>>> +
>>>> +    QDECREF(qdict);
>>>> +}
>>>> +
>>>> +static gboolean conn_channel_read(GIOChannel *channel, GIOCondition condition,
>>>> +                                  gpointer data)
>>>> +{
>>>> +    GAState *s = data;
>>>> +    gchar buf[1024];
>>>> +    gsize count;
>>>> +    GError *err = NULL;
>>>> +    memset(buf, 0, 1024);
>>>> +    GIOStatus status = g_io_channel_read_chars(channel, buf, 1024,
>>>> +&count,&err);
>>>> +    if (err != NULL) {
>>>> +        g_warning("error reading channel: %s", err->message);
>>>> +        conn_channel_close(s);
>>>> +        g_error_free(err);
>>>> +        return false;
>>>> +    }
>>>> +    switch (status) {
>>>> +    case G_IO_STATUS_ERROR:
>>>> +        g_warning("problem");
>>>> +        return false;
>>>> +    case G_IO_STATUS_NORMAL:
>>>> +        g_debug("read data, count: %d, data: %s", (int)count, buf);
>>>> +        json_message_parser_feed(&s->parser, (char *)buf, (int)count);
>>>> +    case G_IO_STATUS_AGAIN:
>>>> +        /* virtio causes us to spin here when no process is attached to
>>>> +         * host-side chardev. sleep a bit to mitigate this
>>>> +         */
>>>> +        if (s->virtio) {
>>>> +            usleep(100*1000);
>>>> +        }
>>>> +        return true;
>>>> +    case G_IO_STATUS_EOF:
>>>> +        g_debug("received EOF");
>>>> +        conn_channel_close(s);
>>>> +        if (s->virtio) {
>>>> +            return true;
>>>> +        }
>>>> +        return false;
>>>> +    default:
>>>> +        g_warning("unknown channel read status, closing");
>>>> +        conn_channel_close(s);
>>>> +        return false;
>>>> +    }
>>>> +    return true;
>>>> +}
>>>> +
>>>> +static int conn_channel_add(GAState *s, int fd)
>>>> +{
>>>> +    GIOChannel *conn_channel;
>>>> +    guint conn_id;
>>>> +    GError *err = NULL;
>>>> +
>>>> +    g_assert(s&&   !s->conn_channel);
>>>> +    conn_channel = g_io_channel_unix_new(fd);
>>>> +    g_assert(conn_channel);
>>>> +    g_io_channel_set_encoding(conn_channel, NULL,&err);
>>>> +    if (err != NULL) {
>>>> +        g_warning("error setting channel encoding to binary");
>>>> +        g_error_free(err);
>>>> +        return -1;
>>>> +    }
>>>> +    conn_id = g_io_add_watch(conn_channel, G_IO_IN | G_IO_HUP,
>>>> +                             conn_channel_read, s);
>>>> +    if (err != NULL) {
>>>> +        g_warning("error adding io watch: %s", err->message);
>>>> +        g_error_free(err);
>>>> +        return -1;
>>>> +    }
>>>> +    s->conn_channel = conn_channel;
>>>> +    s->conn_id = conn_id;
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static gboolean listen_channel_accept(GIOChannel *channel,
>>>> +                                      GIOCondition condition, gpointer data)
>>>> +{
>>>> +    GAState *s = data;
>>>> +    GError *err = NULL;
>>>> +    g_assert(channel != NULL);
>>>> +    int ret;
>>>> +    bool accepted = false;
>>>> +
>>>> +    s->conn_sock = g_socket_accept(s->listen_sock, NULL,&err);
>>>> +    if (err != NULL) {
>>>> +        g_warning("error converting fd to gsocket: %s", err->message);
>>>> +        g_error_free(err);
>>>> +        goto out;
>>>> +    }
>>>> +    ret = conn_channel_add(s, g_socket_get_fd(s->conn_sock));
>>>> +    if (ret) {
>>>> +        g_warning("error setting up connection");
>>>> +        goto out;
>>>> +    }
>>>> +    accepted = true;
>>>> +
>>>> +out:
>>>> +    /* only accept 1 connection at a time */
>>>> +    return !accepted;
>>>> +}
>>>> +
>>>> +/* start polling for readable events on listen fd, listen_fd=0
>>>> + * indicates we should use the existing s->listen_channel
>>>> + */
>>>> +static int listen_channel_add(GAState *s, int listen_fd)
>>>> +{
>>>> +    GError *err = NULL;
>>>> +    guint listen_id;
>>>> +
>>>> +    if (listen_fd) {
>>>> +        s->listen_channel = g_io_channel_unix_new(listen_fd);
>>>> +        if (s->listen_sock) {
>>>> +            g_object_unref(s->listen_sock);
>>>> +        }
>>>> +        s->listen_sock = g_socket_new_from_fd(listen_fd,&err);
>>>> +        if (err != NULL) {
>>>> +            g_warning("error converting fd to gsocket: %s", err->message);
>>>> +            g_error_free(err);
>>>> +            return -1;
>>>> +        }
>>>> +    }
>>>> +    listen_id = g_io_add_watch(s->listen_channel, G_IO_IN,
>>>> +                               listen_channel_accept, s);
>>>> +    if (err != NULL) {
>>>> +        g_warning("error adding io watch: %s", err->message);
>>>> +        g_error_free(err);
>>>> +        return -1;
>>>> +    }
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +/* cleanup state for closed connection/session, start accepting new
>>>> + * connections if we're in listening mode
>>>> + */
>>>> +static void conn_channel_close(GAState *s)
>>>> +{
>>>> +    if (strcmp(s->method, "unix-listen") == 0) {
>>>> +        g_io_channel_shutdown(s->conn_channel, true, NULL);
>>>> +        g_object_unref(s->conn_sock);
>>>> +        s->conn_sock = NULL;
>>>> +        listen_channel_add(s, 0);
>>>> +    } else if (strcmp(s->method, "virtio-serial") == 0) {
>>>> +        /* we spin on EOF for virtio-serial, so back off a bit. also,
>>>> +         * dont close the connection in this case, it'll resume normal
>>>> +         * operation when another process connects to host chardev
>>>> +         */
>>>> +        usleep(100*1000);
>>>> +        goto out_noclose;
>>>> +    }
>>>> +    g_io_channel_unref(s->conn_channel);
>>>> +    s->conn_channel = NULL;
>>>> +    s->conn_id = 0;
>>>> +out_noclose:
>>>> +    return;
>>>> +}
>>>> +
>>>> +static void init_guest_agent(GAState *s)
>>>> +{
>>>> +    struct termios tio;
>>>> +    int ret, fd;
>>>> +
>>>> +    if (s->method == NULL) {
>>>> +        /* try virtio-serial as our default */
>>>> +        s->method = "virtio-serial";
>>>> +    }
>>>> +
>>>> +    if (s->path == NULL) {
>>>> +        if (strcmp(s->method, "virtio-serial") != 0) {
>>>> +            g_error("must specify a path for this channel");
>>>> +        }
>>>> +        /* try the default path for the virtio-serial port */
>>>> +        s->path = QGA_VIRTIO_PATH_DEFAULT;
>>>> +    }
>>>> +
>>>> +    if (strcmp(s->method, "virtio-serial") == 0) {
>>>> +        s->virtio = true;
>>>> +        fd = qemu_open(s->path, O_RDWR);
>>>> +        if (fd == -1) {
>>>> +            g_error("error opening channel: %s", strerror(errno));
>>>> +        }
>>>> +        ret = fcntl(fd, F_GETFL);
>>>> +        if (ret<   0) {
>>>> +            g_error("error getting channel flags: %s", strerror(errno));
>>>> +        }
>>>> +        ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK | O_ASYNC);
>>>> +        if (ret<   0) {
>>>> +            g_error("error setting channel flags: %s", strerror(errno));
>>>> +        }
>>>> +        ret = conn_channel_add(s, fd);
>>>> +        if (ret) {
>>>> +            g_error("error adding channel to main loop");
>>>> +        }
>>>> +    } else if (strcmp(s->method, "isa-serial") == 0) {
>>>> +        fd = qemu_open(s->path, O_RDWR | O_NOCTTY);
>>>> +        if (fd == -1) {
>>>> +            g_error("error opening channel: %s", strerror(errno));
>>>> +        }
>>>> +        tcgetattr(fd,&tio);
>>>> +        /* set up serial port for non-canonical, dumb byte streaming */
>>>> +        tio.c_iflag&= ~(IGNBRK | BRKINT | IGNPAR | PARMRK | INPCK | ISTRIP |
>>>> +                         INLCR | IGNCR | ICRNL | IXON | IXOFF | IXANY |
>>>> +                         IMAXBEL);
>>>> +        tio.c_oflag = 0;
>>>> +        tio.c_lflag = 0;
>>>> +        tio.c_cflag |= QGA_BAUDRATE_DEFAULT;
>>>> +        /* 1 available byte min or reads will block (we'll set non-blocking
>>>> +         * elsewhere, else we have to deal with read()=0 instead)
>>>> +         */
>>>> +        tio.c_cc[VMIN] = 1;
>>>> +        tio.c_cc[VTIME] = 0;
>>>> +        /* flush everything waiting for read/xmit, it's garbage at this point */
>>>> +        tcflush(fd, TCIFLUSH);
>>>> +        tcsetattr(fd, TCSANOW,&tio);
>>>> +        ret = conn_channel_add(s, fd);
>>>> +        if (ret) {
>>>> +            g_error("error adding channel to main loop");
>>>> +        }
>>>> +    } else if (strcmp(s->method, "unix-listen") == 0) {
>>>> +        fd = unix_listen(s->path, NULL, strlen(s->path));
>>>> +        if (fd == -1) {
>>>> +            g_error("error opening path: %s", strerror(errno));
>>>> +        }
>>>> +        ret = listen_channel_add(s, fd);
>>>> +        if (ret) {
>>>> +            g_error("error binding/listening to specified socket");
>>>> +        }
>>>> +    } else {
>>>> +        g_error("unsupported channel method/type: %s", s->method);
>>>> +    }
>>>> +
>>>> +    json_message_parser_init(&s->parser, process_event);
>>>> +    s->main_loop = g_main_loop_new(NULL, false);
>>>> +}
>>>> +
>>>> +int main(int argc, char **argv)
>>>> +{
>>>> +    const char *sopt = "hVvdc:p:l:f:";
>>>> +    const char *method = NULL, *path = NULL, *pidfile = QGA_PIDFILE_DEFAULT;
>>>> +    struct option lopt[] = {
>>>> +        { "help", 0, NULL, 'h' },
>>>> +        { "version", 0, NULL, 'V' },
>>>> +        { "logfile", 0, NULL, 'l' },
>>>> +        { "pidfile", 0, NULL, 'f' },
>>>> +        { "verbose", 0, NULL, 'v' },
>>>> +        { "channel", 0, NULL, 'c' },
>>>> +        { "path", 0, NULL, 'p' },
>>>> +        { "daemonize", 0, NULL, 'd' },
>>>> +        { NULL, 0, NULL, 0 }
>>>> +    };
>>>> +    int opt_ind = 0, ch, daemonize = 0;
>>>> +    GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
>>>> +    FILE *log_file = stderr;
>>>> +    GAState *s;
>>>> +
>>>> +    g_type_init();
>>>> +    g_thread_init(NULL);
>>>> +
>>>> +    while ((ch = getopt_long(argc, argv, sopt, lopt,&opt_ind)) != -1) {
>>>> +        switch (ch) {
>>>> +        case 'c':
>>>> +            method = optarg;
>>>> +            break;
>>>> +        case 'p':
>>>> +            path = optarg;
>>>> +            break;
>>>> +        case 'l':
>>>> +            log_file = fopen(optarg, "a");
>>>> +            if (!log_file) {
>>>> +                g_error("unable to open specified log file: %s",
>>>> +                        strerror(errno));
>>>> +            }
>>>> +            break;
>>>> +        case 'f':
>>>> +            pidfile = optarg;
>>>> +            break;
>>>> +        case 'v':
>>>> +            /* enable all log levels */
>>>> +            log_level = G_LOG_LEVEL_MASK;
>>>> +            break;
>>>> +        case 'V':
>>>> +            printf("QEMU Guest Agent %s\n", QGA_VERSION);
>>>> +            return 0;
>>>> +        case 'd':
>>>> +            daemonize = 1;
>>>> +            break;
>>>> +        case 'h':
>>>> +            usage(argv[0]);
>>>> +            return 0;
>>>> +        case '?':
>>>> +            g_error("Unknown option, try '%s --help' for more information.",
>>>> +                    argv[0]);
>>>> +        }
>>>> +    }
>>>> +
>>>> +    if (daemonize) {
>>>> +        g_debug("starting daemon");
>>>> +        become_daemon(pidfile);
>>>> +    }
>>>> +
>>>> +    s = g_malloc0(sizeof(GAState));
>>>> +    s->conn_id = 0;
>>>> +    s->conn_channel = NULL;
>>>> +    s->path = path;
>>>> +    s->method = method;
>>>> +    s->command_state = ga_command_state_new();
>>>> +    ga_command_state_init(s, s->command_state);
>>>> +    ga_command_state_init_all(s->command_state);
>>>> +    s->log_file = log_file;
>>>> +    s->log_level = log_level;
>>>> +    g_log_set_default_handler(ga_log, s);
>>>> +    g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
>>>> +    s->logging_enabled = true;
>>>> +
>>>> +    module_call_init(MODULE_INIT_QAPI);
>>>> +    init_guest_agent(s);
>>>> +
>>>> +    g_main_loop_run(s->main_loop);
>>>> +
>>>> +    ga_command_state_cleanup_all(s->command_state);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
>>>> index 688f120..66d1729 100644
>>>> --- a/qga/guest-agent-core.h
>>>> +++ b/qga/guest-agent-core.h
>>>
>>> No license text.
>>>
>>>> @@ -15,6 +15,7 @@
>>>>
>>>>    #define QGA_VERSION "1.0"
>>>>
>>>> +typedef struct GAState GAState;
>>>>    typedef struct GACommandState GACommandState;
>>>>
>>>>    void ga_command_state_add(GACommandState *cs,
>>>> @@ -23,3 +24,6 @@ void ga_command_state_add(GACommandState *cs,
>>>>    void ga_command_state_init_all(GACommandState *cs);
>>>>    void ga_command_state_cleanup_all(GACommandState *cs);
>>>>    GACommandState *ga_command_state_new(void);
>>>> +bool ga_logging_enabled(GAState *s);
>>>> +void ga_disable_logging(GAState *s);
>>>> +void ga_enable_logging(GAState *s);
>>>
>>
>
Luiz Capitulino - June 18, 2011, 3:25 a.m.
On Fri, 17 Jun 2011 16:25:32 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On 06/17/2011 03:13 PM, Luiz Capitulino wrote:
> > On Fri, 17 Jun 2011 14:21:31 -0500
> > Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
> >
> >> On 06/16/2011 01:42 PM, Luiz Capitulino wrote:
> >>> On Tue, 14 Jun 2011 15:06:22 -0500
> >>> Michael Roth<mdroth@linux.vnet.ibm.com>   wrote:
> >>>
> >>>> This is the actual guest daemon, it listens for requests over a
> >>>> virtio-serial/isa-serial/unix socket channel and routes them through
> >>>> to dispatch routines, and writes the results back to the channel in
> >>>> a manner similar to QMP.
> >>>>
> >>>> A shorthand invocation:
> >>>>
> >>>>     qemu-ga -d
> >>>>
> >>>> Is equivalent to:
> >>>>
> >>>>     qemu-ga -c virtio-serial -p /dev/virtio-ports/org.qemu.guest_agent \
> >>>>             -p /var/run/qemu-guest-agent.pid -d
> >>>>
> >>>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
> >>>
> >>> Would be nice to have a more complete description, like explaining how to
> >>> do a simple test.
> >>>
> >>> And this can't be built...
> >>>
> >>>> ---
> >>>>    qemu-ga.c              |  631 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>    qga/guest-agent-core.h |    4 +
> >>>>    2 files changed, 635 insertions(+), 0 deletions(-)
> >>>>    create mode 100644 qemu-ga.c
> >>>>
> >>>> diff --git a/qemu-ga.c b/qemu-ga.c
> >>>> new file mode 100644
> >>>> index 0000000..df08d8c
> >>>> --- /dev/null
> >>>> +++ b/qemu-ga.c
> >>>> @@ -0,0 +1,631 @@
> >>>> +/*
> >>>> + * QEMU Guest Agent
> >>>> + *
> >>>> + * Copyright IBM Corp. 2011
> >>>> + *
> >>>> + * Authors:
> >>>> + *  Adam Litke<aglitke@linux.vnet.ibm.com>
> >>>> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
> >>>> + *
> >>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >>>> + * See the COPYING file in the top-level directory.
> >>>> + */
> >>>> +#include<stdlib.h>
> >>>> +#include<stdio.h>
> >>>> +#include<stdbool.h>
> >>>> +#include<glib.h>
> >>>> +#include<gio/gio.h>
> >>>> +#include<getopt.h>
> >>>> +#include<termios.h>
> >>>> +#include<syslog.h>
> >>>> +#include "qemu_socket.h"
> >>>> +#include "json-streamer.h"
> >>>> +#include "json-parser.h"
> >>>> +#include "qint.h"
> >>>> +#include "qjson.h"
> >>>> +#include "qga/guest-agent-core.h"
> >>>> +#include "qga-qmp-commands.h"
> >>>> +#include "module.h"
> >>>> +
> >>>> +#define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent"
> >>>> +#define QGA_PIDFILE_DEFAULT "/var/run/qemu-va.pid"
> >>>> +#define QGA_BAUDRATE_DEFAULT B38400 /* for isa-serial channels */
> >>>> +#define QGA_TIMEOUT_DEFAULT 30*1000 /* ms */
> >>>> +
> >>>> +struct GAState {
> >>>> +    const char *proxy_path;
> >>>
> >>> Where is this used?
> >>>
> >>
> >> Nowhere actually. Will remove.
> >>
> >>>> +    JSONMessageParser parser;
> >>>> +    GMainLoop *main_loop;
> >>>> +    guint conn_id;
> >>>> +    GSocket *conn_sock;
> >>>> +    GIOChannel *conn_channel;
> >>>> +    guint listen_id;
> >>>> +    GSocket *listen_sock;
> >>>> +    GIOChannel *listen_channel;
> >>>> +    const char *path;
> >>>> +    const char *method;
> >>>> +    bool virtio; /* fastpath to check for virtio to deal with poll() quirks */
> >>>> +    GACommandState *command_state;
> >>>> +    GLogLevelFlags log_level;
> >>>> +    FILE *log_file;
> >>>> +    bool logging_enabled;
> >>>> +};
> >>>> +
> >>>> +static void usage(const char *cmd)
> >>>> +{
> >>>> +    printf(
> >>>> +"Usage: %s -c<channel_opts>\n"
> >>>> +"QEMU Guest Agent %s\n"
> >>>> +"\n"
> >>>> +"  -c, --channel     channel method: one of unix-connect, virtio-serial, or\n"
> >>>> +"                    isa-serial (virtio-serial is the default)\n"
> >>>> +"  -p, --path        channel path (%s is the default for virtio-serial)\n"
> >>>> +"  -l, --logfile     set logfile path, logs to stderr by default\n"
> >>>> +"  -f, --pidfile     specify pidfile (default is %s)\n"
> >>>> +"  -v, --verbose     log extra debugging information\n"
> >>>> +"  -V, --version     print version information and exit\n"
> >>>> +"  -d, --daemonize   become a daemon\n"
> >>>> +"  -h, --help        display this help and exit\n"
> >>>> +"\n"
> >>>> +"Report bugs to<mdroth@linux.vnet.ibm.com>\n"
> >>>> +    , cmd, QGA_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT);
> >>>> +}
> >>>> +
> >>>> +static void conn_channel_close(GAState *s);
> >>>> +
> >>>> +static const char *ga_log_level_str(GLogLevelFlags level)
> >>>> +{
> >>>> +    switch (level&   G_LOG_LEVEL_MASK) {
> >>>> +        case G_LOG_LEVEL_ERROR:
> >>>> +            return "error";
> >>>> +        case G_LOG_LEVEL_CRITICAL:
> >>>> +            return "critical";
> >>>> +        case G_LOG_LEVEL_WARNING:
> >>>> +            return "warning";
> >>>> +        case G_LOG_LEVEL_MESSAGE:
> >>>> +            return "message";
> >>>> +        case G_LOG_LEVEL_INFO:
> >>>> +            return "info";
> >>>> +        case G_LOG_LEVEL_DEBUG:
> >>>> +            return "debug";
> >>>> +        default:
> >>>> +            return "user";
> >>>> +    }
> >>>> +}
> >>>> +
> >>>> +bool ga_logging_enabled(GAState *s)
> >>>> +{
> >>>> +    return s->logging_enabled;
> >>>> +}
> >>>> +
> >>>> +void ga_disable_logging(GAState *s)
> >>>> +{
> >>>> +    s->logging_enabled = false;
> >>>> +}
> >>>> +
> >>>> +void ga_enable_logging(GAState *s)
> >>>> +{
> >>>> +    s->logging_enabled = true;
> >>>> +}
> >>>
> >>> Just to check I got this right, this is needed because of the fsfreeze
> >>> command, correct? Isn't it better to have a more descriptive name, like
> >>> fsfrozen?
> >>>
> >>> First I thought this was about a log file. Then I realized this was
> >>> probably about letting the user log in, but we don't seem to have this
> >>> functionality so I got confused.
> >>>
> >>
> >> Yup, this is currently due to fsfreeze support. From the perspective of
> >> the fsfreeze command the explicit "is_frozen" check makes more sense,
> >> but the reason it affects other RPCs is because because we can't log
> >> stuff in that state. If an RPC shoots itself in the foot by writing to a
> >> frozen filesystem we don't really care so much, and up until recently
> >> that case was handled with a pthread_cancel timeout mechanism (was
> >> removed for the time being, will re-implement using a child process most
> >> likely).
> >>
> >> What we don't want to do is give a host a way to bypass the expectation
> >> we set for guest owners by allowing RPCs to be logged. So that's what
> >> the check is based on, rather than lower level details like *why*
> >> logging is disabled. Also, I'd really like to avoid a precedence where a
> >> single RPC can place restrictions on all the others, so the logging
> >> aspect seemed general enough that it doesn't necessarily provide that
> >> precedence.
> >>
> >> This has come up a few times without any real consensus. I can probably
> >> go either way, but I think the check for logging is easier to set
> >> expectations with: "if logging is important from an auditing
> >> perspective, don't execute this if logging is disabled". beyond that,
> >> same behavior/symantics you'd get with anything that causes a write() on
> >> a filesystem in a frozen state: block.
> >
> > While I understand your arguments, I still find the current behavior
> > confusing: you issue a guest-open-file command and get a QgaLoggingFailed
> > error. The first question is: what does a log write fail have to do with me
> > opening a file? The second question is: what should I do? Try again? Give up?
> >
> 
> I agree it's a better solution for the client there, but at the same time:
> 
> guest_privileged_ping():
>    if fsfreeze.status == FROZEN:
>      syslog("privileged ping, thought you should know")
>    else:
>      return "error, filesystem frozen"
> 
> Seems silly to the client as well. "Why does a ping command care about 
> the filesystem state?"
> 
> Inability to log is the true error. That's also the case for the 
> guest-file-open command. 

There's a difference. In the guest ping the inability to log is an
internal agent error, it's not interesting to the client. We could just
ignore the error and reply back (unless we define that guest-privileged-ping
requires writing to disk).

The guest-file-write command, on the other hand, clearly requires to write
to disk, so a client would expect a EWOULDBLOCK error.

EWOULDBLOCK looks good to me. We could define as general rule that
commands don't block, so clients should always expect a EWOULDBLOCK.

> There's nothing wrong with opening a read-only 
> file on a frozen filesystem, it's the fact that we can't log the open 
> that causes us to bail out..

But why do we bail out? Why can't we just ignore the fact we can't log?

> So, what if we just munge the 2 to give the user the proper clues to fix 
> things, and instead return an error like:
> 
> "Guest agent failed to log RPC (is the filesystem frozen?)"?

The 'desc' part of an error is a human error descriptor, clients shouldn't
parse it. The real error is the error class.

> > IMHO, making fsfrozen a global state makes more sense. It's the true guest
> > state after all. This way a client will clearly know what's happening and
> > what it has to do in order to make its command successful.
> >
> > Another possible solution is to have the same semantics of a non-blocking
> > I/O, that's, return EWOULDBLOCK. I find this less confusing than the
> > current error.
> >
> >>
> >>>> +
> >>>> +static void ga_log(const gchar *domain, GLogLevelFlags level,
> >>>> +                   const gchar *msg, gpointer opaque)
> >>>> +{
> >>>> +    GAState *s = opaque;
> >>>> +    GTimeVal time;
> >>>> +    const char *level_str = ga_log_level_str(level);
> >>>> +
> >>>> +    if (!ga_logging_enabled(s)) {
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>> +    level&= G_LOG_LEVEL_MASK;
> >>>> +    if (g_strcmp0(domain, "syslog") == 0) {
> >>>> +        syslog(LOG_INFO, "%s: %s", level_str, msg);
> >>>> +    } else if (level&   s->log_level) {
> >>>> +        g_get_current_time(&time);
> >>>> +        fprintf(s->log_file,
> >>>> +                "%lu.%lu: %s: %s\n", time.tv_sec, time.tv_usec, level_str, msg);
> >>>> +        fflush(s->log_file);
> >>>> +    }
> >>>> +}
> >>>> +
> >>>> +static void become_daemon(const char *pidfile)
> >>>> +{
> >>>> +    pid_t pid, sid;
> >>>> +    int pidfd;
> >>>> +    char *pidstr = NULL;
> >>>> +
> >>>> +    pid = fork();
> >>>> +    if (pid<   0) {
> >>>> +        exit(EXIT_FAILURE);
> >>>> +    }
> >>>> +    if (pid>   0) {
> >>>> +        exit(EXIT_SUCCESS);
> >>>> +    }
> >>>> +
> >>>> +    pidfd = open(pidfile, O_CREAT|O_RDWR, S_IRUSR|S_IWUSR);
> >>>
> >>> O_WRONLY is enough.
> >>>
> >>>> +    if (!pidfd || lockf(pidfd, F_TLOCK, 0)) {
> >>>> +        g_error("Cannot lock pid file");
> >>>> +    }
> >>>
> >>> Are you sure we need lockf() here? I think using O_EXCL is enough for
> >>> pid files.
> >>>
> >>
> >> O_EXCL + O_CREAT? Wouldn't we have issues if the pid file wasn't cleaned
> >> up from a previous run?
> >
> > Yes, but that's the intention (and that's how most daemons work afaik). The
> > only reason for a daemon not to cleanup its pid file is when it exists
> > abnormally. If this happens, then only the sysadmin can ensure that it's safe
> > to run another daemon instance.
> >
> > We could add a -r (--override-existing-pid-file) option, but we should not
> > do it silently.
> >
> > Btw, we need to implement a signal handler for SIGTERM.
> >
> >>>> +
> >>>> +    if (ftruncate(pidfd, 0) || lseek(pidfd, 0, SEEK_SET)) {
> >>>> +        g_critical("Cannot truncate pid file");
> >>>> +        goto fail;
> >>>
> >>> You can use O_TRUNC and the file pointer is already positioned at the
> >>> beginning of the file, no need to call lseek().
> >>>
> >>>> +    }
> >>>> +    if (asprintf(&pidstr, "%d", getpid()) == -1) {
> >>>> +        g_critical("Cannot allocate memory");
> >>>> +        goto fail;
> >>>> +    }
> >>>> +    if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
> >>>> +        g_critical("Failed to write pid file");
> >>>> +        goto fail;
> >>>> +    }
> >>>
> >>> You're not freeing pidstr, nor closing the file (although I think you
> >>> do this because of the lockf() call()).
> >>>
> >>
> >> pidstr gets free'd in the fail: block. And yeah, closing would give up
> >> the lock prematurely.
> >
> > pidstr leaks on a successful execution.
> >
> >>
> >>>> +
> >>>> +    umask(0);
> >>>> +    sid = setsid();
> >>>> +    if (sid<   0) {
> >>>> +        goto fail;
> >>>> +    }
> >>>> +    if ((chdir("/"))<   0) {
> >>>> +        goto fail;
> >>>> +    }
> >>>> +
> >>>> +    close(STDIN_FILENO);
> >>>> +    close(STDOUT_FILENO);
> >>>> +    close(STDERR_FILENO);
> >>>> +    return;
> >>>> +
> >>>> +fail:
> >>>> +    if (pidstr) {
> >>>> +        free(pidstr);
> >>>> +    }
> >>>
> >>> This check is not needed.
> >>>
> >>>> +    unlink(pidfile);
> >>>> +    g_error("failed to daemonize");
> >>>> +}
> >>>> +
> >>>> +static int conn_channel_send_payload(GIOChannel *channel, QObject *payload)
> >>>> +{
> >>>> +    gsize count, written = 0;
> >>>> +    int ret;
> >>>> +    const char *buf;
> >>>> +    QString *payload_qstr;
> >>>> +    GIOStatus status;
> >>>> +    GError *err = NULL;
> >>>> +
> >>>> +    if (!payload || !channel) {
> >>>> +        ret = -EINVAL;
> >>>> +        goto out;
> >>>> +    }
> >>>
> >>> Just do "return -EINVAL" instead of using goto, but I wonder if this should
> >>> be an assert() instead.
> >>>
> >>
> >> Yah, looks like it.
> >>
> >>>> +
> >>>> +    payload_qstr = qobject_to_json(payload);
> >>>> +    if (!payload_qstr) {
> >>>> +        ret = -EINVAL;
> >>>> +        goto out;
> >>>> +    }
> >>>
> >>> return -EINVAL.
> >>>
> >>>> +
> >>>> +    buf = qstring_get_str(payload_qstr);
> >>>> +    count = strlen(buf);
> >>>> +
> >>>> +    while (count) {
> >>>> +        g_debug("sending data, count: %d", (int)count);
> >>>> +        status = g_io_channel_write_chars(channel, buf, count,&written,&err);
> >>>> +        if (err != NULL) {
> >>>> +            g_warning("error writing payload to channel: %s", err->message);
> >>>> +            ret = err->code;
> >>>> +            goto out_free;
> >>>> +        }
> >>>> +        if (status == G_IO_STATUS_NORMAL) {
> >>>> +            count -= written;
> >>>> +        } else if (status == G_IO_STATUS_ERROR || status == G_IO_STATUS_EOF) {
> >>>> +            ret = -EPIPE;
> >>>> +            goto out_free;
> >>>> +        }
> >>>> +    }
> >>>> +
> >>>> +    status = g_io_channel_write_chars(channel, (char *)"\n", 1,&written,&err);
> >>>> +    if (err != NULL) {
> >>>> +        g_warning("error sending newline: %s", err->message);
> >>>> +        ret = err->code;
> >>>> +        goto out_free;
> >>>> +    } else if (status == G_IO_STATUS_ERROR || status == G_IO_STATUS_EOF) {
> >>>> +        ret = -EPIPE;
> >>>> +        goto out_free;
> >>>> +    }
> >>>
> >>> This wants to be a function.
> >>>
> >>>> +
> >>>> +    g_io_channel_flush(channel,&err);
> >>>> +    if (err != NULL) {
> >>>> +        g_warning("error flushing payload: %s", err->message);
> >>>> +        ret = err->code;
> >>>> +        goto out_free;
> >>>> +    }
> >>>> +
> >>>> +    ret = 0;
> >>>> +
> >>>> +out_free:
> >>>> +    QDECREF(payload_qstr);
> >>>> +    if (err != NULL) {
> >>>> +        g_error_free(err);
> >>>> +    }
> >>>
> >>> No need to check against NULL.
> >>>
> >>>> +out:
> >>>> +    return ret;
> >>>> +}
> >>>> +
> >>>> +static void process_command(GAState *s, QDict *req)
> >>>> +{
> >>>> +    QObject *rsp = NULL;
> >>>> +    Error *err = NULL;
> >>>> +    int ret;
> >>>> +
> >>>> +    g_assert(req);
> >>>> +    g_debug("processing command");
> >>>> +    rsp = qmp_dispatch(QOBJECT(req));
> >>>> +    if (rsp) {
> >>>> +        if (err) {
> >>>
> >>> Apart from its initialization, 'err' is read only. Either, you want to
> >>> use qmp_dispatch_err() here or you have to modify qmp_dispatch() to also
> >>> accept an Error argument.
> >>>
> >>> If you want to send a "return" or "error" QMP kind of response, then you
> >>> have to do the latter.
> >>>
> >>
> >> qmp_dispatch() does the Error ->  QMP error conversion. The unused err is
> >> cruft from when a seperate worker thread did the dispatch, will remove it.
> >>
> >>>> +            g_warning("command failed: %s", error_get_pretty(err));
> >>>> +        }
> >>>> +        ret = conn_channel_send_payload(s->conn_channel, rsp);
> >>>> +        if (ret) {
> >>>> +            g_warning("error sending payload: %s", strerror(ret));
> >>>> +        }
> >>>> +        qobject_decref(rsp);
> >>>> +    } else {
> >>>> +        g_warning("error getting response");
> >>>> +        if (err) {
> >>>> +            g_warning("dispatch failed: %s", error_get_pretty(err));
> >>>> +        }
> >>>> +    }
> >>>> +}
> >>>> +
> >>>> +/* handle requests/control events coming in over the channel */
> >>>> +static void process_event(JSONMessageParser *parser, QList *tokens)
> >>>> +{
> >>>> +    GAState *s = container_of(parser, GAState, parser);
> >>>> +    QObject *obj;
> >>>> +    QDict *qdict;
> >>>> +    Error *err = NULL;
> >>>> +
> >>>> +    g_assert(s&&   parser);
> >>>> +
> >>>> +    g_debug("process_event: called");
> >>>> +    obj = json_parser_parse_err(tokens, NULL,&err);
> >>>> +    if (!obj || qobject_type(obj) != QTYPE_QDICT) {
> >>>> +        g_warning("failed to parse event");
> >>>
> >>> Two possible leaks here. You have to call qobject_decref() if obj != NULL
> >>> and there's a missing call to error_free(). But you don't use 'err' anyway,
> >>> so you could pass NULL there.
> >>>
> >>> Oh, btw, the guest agent doesn't seem to report errors back to its client...
> >>> Not even json error messages, is this intended?
> >>>
> >>
> >> Hmm, yes it's intended, but I probably should send those to the client...
> >>
> >> You do get non-parse errors though: missing arguments, etc.
> >>
> >> I'll fix this to do what QMP does here.
> >>
> >>>> +        return;
> >>>> +    } else {
> >>>> +        g_debug("parse successful");
> >>>> +        qdict = qobject_to_qdict(obj);
> >>>> +        g_assert(qdict);
> >>>> +    }
> >>>
> >>> Superfluous else clause.
> >>>
> >>>> +
> >>>> +    /* handle host->guest commands */
> >>>> +    if (qdict_haskey(qdict, "execute")) {
> >>>> +        process_command(s, qdict);
> >>>> +    } else {
> >>>> +        g_warning("unrecognized payload format, ignoring");
> >>>> +    }
> >>>> +
> >>>> +    QDECREF(qdict);
> >>>> +}
> >>>> +
> >>>> +static gboolean conn_channel_read(GIOChannel *channel, GIOCondition condition,
> >>>> +                                  gpointer data)
> >>>> +{
> >>>> +    GAState *s = data;
> >>>> +    gchar buf[1024];
> >>>> +    gsize count;
> >>>> +    GError *err = NULL;
> >>>> +    memset(buf, 0, 1024);
> >>>> +    GIOStatus status = g_io_channel_read_chars(channel, buf, 1024,
> >>>> +&count,&err);
> >>>> +    if (err != NULL) {
> >>>> +        g_warning("error reading channel: %s", err->message);
> >>>> +        conn_channel_close(s);
> >>>> +        g_error_free(err);
> >>>> +        return false;
> >>>> +    }
> >>>> +    switch (status) {
> >>>> +    case G_IO_STATUS_ERROR:
> >>>> +        g_warning("problem");
> >>>> +        return false;
> >>>> +    case G_IO_STATUS_NORMAL:
> >>>> +        g_debug("read data, count: %d, data: %s", (int)count, buf);
> >>>> +        json_message_parser_feed(&s->parser, (char *)buf, (int)count);
> >>>> +    case G_IO_STATUS_AGAIN:
> >>>> +        /* virtio causes us to spin here when no process is attached to
> >>>> +         * host-side chardev. sleep a bit to mitigate this
> >>>> +         */
> >>>> +        if (s->virtio) {
> >>>> +            usleep(100*1000);
> >>>> +        }
> >>>> +        return true;
> >>>> +    case G_IO_STATUS_EOF:
> >>>> +        g_debug("received EOF");
> >>>> +        conn_channel_close(s);
> >>>> +        if (s->virtio) {
> >>>> +            return true;
> >>>> +        }
> >>>> +        return false;
> >>>> +    default:
> >>>> +        g_warning("unknown channel read status, closing");
> >>>> +        conn_channel_close(s);
> >>>> +        return false;
> >>>> +    }
> >>>> +    return true;
> >>>> +}
> >>>> +
> >>>> +static int conn_channel_add(GAState *s, int fd)
> >>>> +{
> >>>> +    GIOChannel *conn_channel;
> >>>> +    guint conn_id;
> >>>> +    GError *err = NULL;
> >>>> +
> >>>> +    g_assert(s&&   !s->conn_channel);
> >>>> +    conn_channel = g_io_channel_unix_new(fd);
> >>>> +    g_assert(conn_channel);
> >>>> +    g_io_channel_set_encoding(conn_channel, NULL,&err);
> >>>> +    if (err != NULL) {
> >>>> +        g_warning("error setting channel encoding to binary");
> >>>> +        g_error_free(err);
> >>>> +        return -1;
> >>>> +    }
> >>>> +    conn_id = g_io_add_watch(conn_channel, G_IO_IN | G_IO_HUP,
> >>>> +                             conn_channel_read, s);
> >>>> +    if (err != NULL) {
> >>>> +        g_warning("error adding io watch: %s", err->message);
> >>>> +        g_error_free(err);
> >>>> +        return -1;
> >>>> +    }
> >>>> +    s->conn_channel = conn_channel;
> >>>> +    s->conn_id = conn_id;
> >>>> +    return 0;
> >>>> +}
> >>>> +
> >>>> +static gboolean listen_channel_accept(GIOChannel *channel,
> >>>> +                                      GIOCondition condition, gpointer data)
> >>>> +{
> >>>> +    GAState *s = data;
> >>>> +    GError *err = NULL;
> >>>> +    g_assert(channel != NULL);
> >>>> +    int ret;
> >>>> +    bool accepted = false;
> >>>> +
> >>>> +    s->conn_sock = g_socket_accept(s->listen_sock, NULL,&err);
> >>>> +    if (err != NULL) {
> >>>> +        g_warning("error converting fd to gsocket: %s", err->message);
> >>>> +        g_error_free(err);
> >>>> +        goto out;
> >>>> +    }
> >>>> +    ret = conn_channel_add(s, g_socket_get_fd(s->conn_sock));
> >>>> +    if (ret) {
> >>>> +        g_warning("error setting up connection");
> >>>> +        goto out;
> >>>> +    }
> >>>> +    accepted = true;
> >>>> +
> >>>> +out:
> >>>> +    /* only accept 1 connection at a time */
> >>>> +    return !accepted;
> >>>> +}
> >>>> +
> >>>> +/* start polling for readable events on listen fd, listen_fd=0
> >>>> + * indicates we should use the existing s->listen_channel
> >>>> + */
> >>>> +static int listen_channel_add(GAState *s, int listen_fd)
> >>>> +{
> >>>> +    GError *err = NULL;
> >>>> +    guint listen_id;
> >>>> +
> >>>> +    if (listen_fd) {
> >>>> +        s->listen_channel = g_io_channel_unix_new(listen_fd);
> >>>> +        if (s->listen_sock) {
> >>>> +            g_object_unref(s->listen_sock);
> >>>> +        }
> >>>> +        s->listen_sock = g_socket_new_from_fd(listen_fd,&err);
> >>>> +        if (err != NULL) {
> >>>> +            g_warning("error converting fd to gsocket: %s", err->message);
> >>>> +            g_error_free(err);
> >>>> +            return -1;
> >>>> +        }
> >>>> +    }
> >>>> +    listen_id = g_io_add_watch(s->listen_channel, G_IO_IN,
> >>>> +                               listen_channel_accept, s);
> >>>> +    if (err != NULL) {
> >>>> +        g_warning("error adding io watch: %s", err->message);
> >>>> +        g_error_free(err);
> >>>> +        return -1;
> >>>> +    }
> >>>> +    return 0;
> >>>> +}
> >>>> +
> >>>> +/* cleanup state for closed connection/session, start accepting new
> >>>> + * connections if we're in listening mode
> >>>> + */
> >>>> +static void conn_channel_close(GAState *s)
> >>>> +{
> >>>> +    if (strcmp(s->method, "unix-listen") == 0) {
> >>>> +        g_io_channel_shutdown(s->conn_channel, true, NULL);
> >>>> +        g_object_unref(s->conn_sock);
> >>>> +        s->conn_sock = NULL;
> >>>> +        listen_channel_add(s, 0);
> >>>> +    } else if (strcmp(s->method, "virtio-serial") == 0) {
> >>>> +        /* we spin on EOF for virtio-serial, so back off a bit. also,
> >>>> +         * dont close the connection in this case, it'll resume normal
> >>>> +         * operation when another process connects to host chardev
> >>>> +         */
> >>>> +        usleep(100*1000);
> >>>> +        goto out_noclose;
> >>>> +    }
> >>>> +    g_io_channel_unref(s->conn_channel);
> >>>> +    s->conn_channel = NULL;
> >>>> +    s->conn_id = 0;
> >>>> +out_noclose:
> >>>> +    return;
> >>>> +}
> >>>> +
> >>>> +static void init_guest_agent(GAState *s)
> >>>> +{
> >>>> +    struct termios tio;
> >>>> +    int ret, fd;
> >>>> +
> >>>> +    if (s->method == NULL) {
> >>>> +        /* try virtio-serial as our default */
> >>>> +        s->method = "virtio-serial";
> >>>> +    }
> >>>> +
> >>>> +    if (s->path == NULL) {
> >>>> +        if (strcmp(s->method, "virtio-serial") != 0) {
> >>>> +            g_error("must specify a path for this channel");
> >>>> +        }
> >>>> +        /* try the default path for the virtio-serial port */
> >>>> +        s->path = QGA_VIRTIO_PATH_DEFAULT;
> >>>> +    }
> >>>> +
> >>>> +    if (strcmp(s->method, "virtio-serial") == 0) {
> >>>> +        s->virtio = true;
> >>>> +        fd = qemu_open(s->path, O_RDWR);
> >>>> +        if (fd == -1) {
> >>>> +            g_error("error opening channel: %s", strerror(errno));
> >>>> +        }
> >>>> +        ret = fcntl(fd, F_GETFL);
> >>>> +        if (ret<   0) {
> >>>> +            g_error("error getting channel flags: %s", strerror(errno));
> >>>> +        }
> >>>> +        ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK | O_ASYNC);
> >>>> +        if (ret<   0) {
> >>>> +            g_error("error setting channel flags: %s", strerror(errno));
> >>>> +        }
> >>>> +        ret = conn_channel_add(s, fd);
> >>>> +        if (ret) {
> >>>> +            g_error("error adding channel to main loop");
> >>>> +        }
> >>>> +    } else if (strcmp(s->method, "isa-serial") == 0) {
> >>>> +        fd = qemu_open(s->path, O_RDWR | O_NOCTTY);
> >>>> +        if (fd == -1) {
> >>>> +            g_error("error opening channel: %s", strerror(errno));
> >>>> +        }
> >>>> +        tcgetattr(fd,&tio);
> >>>> +        /* set up serial port for non-canonical, dumb byte streaming */
> >>>> +        tio.c_iflag&= ~(IGNBRK | BRKINT | IGNPAR | PARMRK | INPCK | ISTRIP |
> >>>> +                         INLCR | IGNCR | ICRNL | IXON | IXOFF | IXANY |
> >>>> +                         IMAXBEL);
> >>>> +        tio.c_oflag = 0;
> >>>> +        tio.c_lflag = 0;
> >>>> +        tio.c_cflag |= QGA_BAUDRATE_DEFAULT;
> >>>> +        /* 1 available byte min or reads will block (we'll set non-blocking
> >>>> +         * elsewhere, else we have to deal with read()=0 instead)
> >>>> +         */
> >>>> +        tio.c_cc[VMIN] = 1;
> >>>> +        tio.c_cc[VTIME] = 0;
> >>>> +        /* flush everything waiting for read/xmit, it's garbage at this point */
> >>>> +        tcflush(fd, TCIFLUSH);
> >>>> +        tcsetattr(fd, TCSANOW,&tio);
> >>>> +        ret = conn_channel_add(s, fd);
> >>>> +        if (ret) {
> >>>> +            g_error("error adding channel to main loop");
> >>>> +        }
> >>>> +    } else if (strcmp(s->method, "unix-listen") == 0) {
> >>>> +        fd = unix_listen(s->path, NULL, strlen(s->path));
> >>>> +        if (fd == -1) {
> >>>> +            g_error("error opening path: %s", strerror(errno));
> >>>> +        }
> >>>> +        ret = listen_channel_add(s, fd);
> >>>> +        if (ret) {
> >>>> +            g_error("error binding/listening to specified socket");
> >>>> +        }
> >>>> +    } else {
> >>>> +        g_error("unsupported channel method/type: %s", s->method);
> >>>> +    }
> >>>> +
> >>>> +    json_message_parser_init(&s->parser, process_event);
> >>>> +    s->main_loop = g_main_loop_new(NULL, false);
> >>>> +}
> >>>> +
> >>>> +int main(int argc, char **argv)
> >>>> +{
> >>>> +    const char *sopt = "hVvdc:p:l:f:";
> >>>> +    const char *method = NULL, *path = NULL, *pidfile = QGA_PIDFILE_DEFAULT;
> >>>> +    struct option lopt[] = {
> >>>> +        { "help", 0, NULL, 'h' },
> >>>> +        { "version", 0, NULL, 'V' },
> >>>> +        { "logfile", 0, NULL, 'l' },
> >>>> +        { "pidfile", 0, NULL, 'f' },
> >>>> +        { "verbose", 0, NULL, 'v' },
> >>>> +        { "channel", 0, NULL, 'c' },
> >>>> +        { "path", 0, NULL, 'p' },
> >>>> +        { "daemonize", 0, NULL, 'd' },
> >>>> +        { NULL, 0, NULL, 0 }
> >>>> +    };
> >>>> +    int opt_ind = 0, ch, daemonize = 0;
> >>>> +    GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
> >>>> +    FILE *log_file = stderr;
> >>>> +    GAState *s;
> >>>> +
> >>>> +    g_type_init();
> >>>> +    g_thread_init(NULL);
> >>>> +
> >>>> +    while ((ch = getopt_long(argc, argv, sopt, lopt,&opt_ind)) != -1) {
> >>>> +        switch (ch) {
> >>>> +        case 'c':
> >>>> +            method = optarg;
> >>>> +            break;
> >>>> +        case 'p':
> >>>> +            path = optarg;
> >>>> +            break;
> >>>> +        case 'l':
> >>>> +            log_file = fopen(optarg, "a");
> >>>> +            if (!log_file) {
> >>>> +                g_error("unable to open specified log file: %s",
> >>>> +                        strerror(errno));
> >>>> +            }
> >>>> +            break;
> >>>> +        case 'f':
> >>>> +            pidfile = optarg;
> >>>> +            break;
> >>>> +        case 'v':
> >>>> +            /* enable all log levels */
> >>>> +            log_level = G_LOG_LEVEL_MASK;
> >>>> +            break;
> >>>> +        case 'V':
> >>>> +            printf("QEMU Guest Agent %s\n", QGA_VERSION);
> >>>> +            return 0;
> >>>> +        case 'd':
> >>>> +            daemonize = 1;
> >>>> +            break;
> >>>> +        case 'h':
> >>>> +            usage(argv[0]);
> >>>> +            return 0;
> >>>> +        case '?':
> >>>> +            g_error("Unknown option, try '%s --help' for more information.",
> >>>> +                    argv[0]);
> >>>> +        }
> >>>> +    }
> >>>> +
> >>>> +    if (daemonize) {
> >>>> +        g_debug("starting daemon");
> >>>> +        become_daemon(pidfile);
> >>>> +    }
> >>>> +
> >>>> +    s = g_malloc0(sizeof(GAState));
> >>>> +    s->conn_id = 0;
> >>>> +    s->conn_channel = NULL;
> >>>> +    s->path = path;
> >>>> +    s->method = method;
> >>>> +    s->command_state = ga_command_state_new();
> >>>> +    ga_command_state_init(s, s->command_state);
> >>>> +    ga_command_state_init_all(s->command_state);
> >>>> +    s->log_file = log_file;
> >>>> +    s->log_level = log_level;
> >>>> +    g_log_set_default_handler(ga_log, s);
> >>>> +    g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
> >>>> +    s->logging_enabled = true;
> >>>> +
> >>>> +    module_call_init(MODULE_INIT_QAPI);
> >>>> +    init_guest_agent(s);
> >>>> +
> >>>> +    g_main_loop_run(s->main_loop);
> >>>> +
> >>>> +    ga_command_state_cleanup_all(s->command_state);
> >>>> +
> >>>> +    return 0;
> >>>> +}
> >>>> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> >>>> index 688f120..66d1729 100644
> >>>> --- a/qga/guest-agent-core.h
> >>>> +++ b/qga/guest-agent-core.h
> >>>
> >>> No license text.
> >>>
> >>>> @@ -15,6 +15,7 @@
> >>>>
> >>>>    #define QGA_VERSION "1.0"
> >>>>
> >>>> +typedef struct GAState GAState;
> >>>>    typedef struct GACommandState GACommandState;
> >>>>
> >>>>    void ga_command_state_add(GACommandState *cs,
> >>>> @@ -23,3 +24,6 @@ void ga_command_state_add(GACommandState *cs,
> >>>>    void ga_command_state_init_all(GACommandState *cs);
> >>>>    void ga_command_state_cleanup_all(GACommandState *cs);
> >>>>    GACommandState *ga_command_state_new(void);
> >>>> +bool ga_logging_enabled(GAState *s);
> >>>> +void ga_disable_logging(GAState *s);
> >>>> +void ga_enable_logging(GAState *s);
> >>>
> >>
> >
>
Michael Roth - June 19, 2011, 7 p.m.
On 06/17/2011 10:25 PM, Luiz Capitulino wrote:
> On Fri, 17 Jun 2011 16:25:32 -0500
> Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>
>> On 06/17/2011 03:13 PM, Luiz Capitulino wrote:
>>> On Fri, 17 Jun 2011 14:21:31 -0500
>>> Michael Roth<mdroth@linux.vnet.ibm.com>   wrote:
>>>
>>>> On 06/16/2011 01:42 PM, Luiz Capitulino wrote:
>>>>> On Tue, 14 Jun 2011 15:06:22 -0500
>>>>> Michael Roth<mdroth@linux.vnet.ibm.com>    wrote:
>>>>>
>>>>>> This is the actual guest daemon, it listens for requests over a
>>>>>> virtio-serial/isa-serial/unix socket channel and routes them through
>>>>>> to dispatch routines, and writes the results back to the channel in
>>>>>> a manner similar to QMP.
>>>>>>
>>>>>> A shorthand invocation:
>>>>>>
>>>>>>      qemu-ga -d
>>>>>>
>>>>>> Is equivalent to:
>>>>>>
>>>>>>      qemu-ga -c virtio-serial -p /dev/virtio-ports/org.qemu.guest_agent \
>>>>>>              -p /var/run/qemu-guest-agent.pid -d
>>>>>>
>>>>>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>>>>>
>>>>> Would be nice to have a more complete description, like explaining how to
>>>>> do a simple test.
>>>>>
>>>>> And this can't be built...
>>>>>
>>>>>> ---
>>>>>>     qemu-ga.c              |  631 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>     qga/guest-agent-core.h |    4 +
>>>>>>     2 files changed, 635 insertions(+), 0 deletions(-)
>>>>>>     create mode 100644 qemu-ga.c
>>>>>>
>>>>>> diff --git a/qemu-ga.c b/qemu-ga.c
>>>>>> new file mode 100644
>>>>>> index 0000000..df08d8c
>>>>>> --- /dev/null
>>>>>> +++ b/qemu-ga.c
>>>>>> @@ -0,0 +1,631 @@
>>>>>> +/*
>>>>>> + * QEMU Guest Agent
>>>>>> + *
>>>>>> + * Copyright IBM Corp. 2011
>>>>>> + *
>>>>>> + * Authors:
>>>>>> + *  Adam Litke<aglitke@linux.vnet.ibm.com>
>>>>>> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
>>>>>> + *
>>>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>>>>> + * See the COPYING file in the top-level directory.
>>>>>> + */
>>>>>> +#include<stdlib.h>
>>>>>> +#include<stdio.h>
>>>>>> +#include<stdbool.h>
>>>>>> +#include<glib.h>
>>>>>> +#include<gio/gio.h>
>>>>>> +#include<getopt.h>
>>>>>> +#include<termios.h>
>>>>>> +#include<syslog.h>
>>>>>> +#include "qemu_socket.h"
>>>>>> +#include "json-streamer.h"
>>>>>> +#include "json-parser.h"
>>>>>> +#include "qint.h"
>>>>>> +#include "qjson.h"
>>>>>> +#include "qga/guest-agent-core.h"
>>>>>> +#include "qga-qmp-commands.h"
>>>>>> +#include "module.h"
>>>>>> +
>>>>>> +#define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent"
>>>>>> +#define QGA_PIDFILE_DEFAULT "/var/run/qemu-va.pid"
>>>>>> +#define QGA_BAUDRATE_DEFAULT B38400 /* for isa-serial channels */
>>>>>> +#define QGA_TIMEOUT_DEFAULT 30*1000 /* ms */
>>>>>> +
>>>>>> +struct GAState {
>>>>>> +    const char *proxy_path;
>>>>>
>>>>> Where is this used?
>>>>>
>>>>
>>>> Nowhere actually. Will remove.
>>>>
>>>>>> +    JSONMessageParser parser;
>>>>>> +    GMainLoop *main_loop;
>>>>>> +    guint conn_id;
>>>>>> +    GSocket *conn_sock;
>>>>>> +    GIOChannel *conn_channel;
>>>>>> +    guint listen_id;
>>>>>> +    GSocket *listen_sock;
>>>>>> +    GIOChannel *listen_channel;
>>>>>> +    const char *path;
>>>>>> +    const char *method;
>>>>>> +    bool virtio; /* fastpath to check for virtio to deal with poll() quirks */
>>>>>> +    GACommandState *command_state;
>>>>>> +    GLogLevelFlags log_level;
>>>>>> +    FILE *log_file;
>>>>>> +    bool logging_enabled;
>>>>>> +};
>>>>>> +
>>>>>> +static void usage(const char *cmd)
>>>>>> +{
>>>>>> +    printf(
>>>>>> +"Usage: %s -c<channel_opts>\n"
>>>>>> +"QEMU Guest Agent %s\n"
>>>>>> +"\n"
>>>>>> +"  -c, --channel     channel method: one of unix-connect, virtio-serial, or\n"
>>>>>> +"                    isa-serial (virtio-serial is the default)\n"
>>>>>> +"  -p, --path        channel path (%s is the default for virtio-serial)\n"
>>>>>> +"  -l, --logfile     set logfile path, logs to stderr by default\n"
>>>>>> +"  -f, --pidfile     specify pidfile (default is %s)\n"
>>>>>> +"  -v, --verbose     log extra debugging information\n"
>>>>>> +"  -V, --version     print version information and exit\n"
>>>>>> +"  -d, --daemonize   become a daemon\n"
>>>>>> +"  -h, --help        display this help and exit\n"
>>>>>> +"\n"
>>>>>> +"Report bugs to<mdroth@linux.vnet.ibm.com>\n"
>>>>>> +    , cmd, QGA_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT);
>>>>>> +}
>>>>>> +
>>>>>> +static void conn_channel_close(GAState *s);
>>>>>> +
>>>>>> +static const char *ga_log_level_str(GLogLevelFlags level)
>>>>>> +{
>>>>>> +    switch (level&    G_LOG_LEVEL_MASK) {
>>>>>> +        case G_LOG_LEVEL_ERROR:
>>>>>> +            return "error";
>>>>>> +        case G_LOG_LEVEL_CRITICAL:
>>>>>> +            return "critical";
>>>>>> +        case G_LOG_LEVEL_WARNING:
>>>>>> +            return "warning";
>>>>>> +        case G_LOG_LEVEL_MESSAGE:
>>>>>> +            return "message";
>>>>>> +        case G_LOG_LEVEL_INFO:
>>>>>> +            return "info";
>>>>>> +        case G_LOG_LEVEL_DEBUG:
>>>>>> +            return "debug";
>>>>>> +        default:
>>>>>> +            return "user";
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>> +bool ga_logging_enabled(GAState *s)
>>>>>> +{
>>>>>> +    return s->logging_enabled;
>>>>>> +}
>>>>>> +
>>>>>> +void ga_disable_logging(GAState *s)
>>>>>> +{
>>>>>> +    s->logging_enabled = false;
>>>>>> +}
>>>>>> +
>>>>>> +void ga_enable_logging(GAState *s)
>>>>>> +{
>>>>>> +    s->logging_enabled = true;
>>>>>> +}
>>>>>
>>>>> Just to check I got this right, this is needed because of the fsfreeze
>>>>> command, correct? Isn't it better to have a more descriptive name, like
>>>>> fsfrozen?
>>>>>
>>>>> First I thought this was about a log file. Then I realized this was
>>>>> probably about letting the user log in, but we don't seem to have this
>>>>> functionality so I got confused.
>>>>>
>>>>
>>>> Yup, this is currently due to fsfreeze support. From the perspective of
>>>> the fsfreeze command the explicit "is_frozen" check makes more sense,
>>>> but the reason it affects other RPCs is because because we can't log
>>>> stuff in that state. If an RPC shoots itself in the foot by writing to a
>>>> frozen filesystem we don't really care so much, and up until recently
>>>> that case was handled with a pthread_cancel timeout mechanism (was
>>>> removed for the time being, will re-implement using a child process most
>>>> likely).
>>>>
>>>> What we don't want to do is give a host a way to bypass the expectation
>>>> we set for guest owners by allowing RPCs to be logged. So that's what
>>>> the check is based on, rather than lower level details like *why*
>>>> logging is disabled. Also, I'd really like to avoid a precedence where a
>>>> single RPC can place restrictions on all the others, so the logging
>>>> aspect seemed general enough that it doesn't necessarily provide that
>>>> precedence.
>>>>
>>>> This has come up a few times without any real consensus. I can probably
>>>> go either way, but I think the check for logging is easier to set
>>>> expectations with: "if logging is important from an auditing
>>>> perspective, don't execute this if logging is disabled". beyond that,
>>>> same behavior/symantics you'd get with anything that causes a write() on
>>>> a filesystem in a frozen state: block.
>>>
>>> While I understand your arguments, I still find the current behavior
>>> confusing: you issue a guest-open-file command and get a QgaLoggingFailed
>>> error. The first question is: what does a log write fail have to do with me
>>> opening a file? The second question is: what should I do? Try again? Give up?
>>>
>>
>> I agree it's a better solution for the client there, but at the same time:
>>
>> guest_privileged_ping():
>>     if fsfreeze.status == FROZEN:
>>       syslog("privileged ping, thought you should know")
>>     else:
>>       return "error, filesystem frozen"
>>
>> Seems silly to the client as well. "Why does a ping command care about
>> the filesystem state?"
>>
>> Inability to log is the true error. That's also the case for the
>> guest-file-open command.
>
> There's a difference. In the guest ping the inability to log is an
> internal agent error, it's not interesting to the client. We could just
> ignore the error and reply back (unless we define that guest-privileged-ping
> requires writing to disk).
>

To clarify, these's 2 different types of errors here and their relation 
to fsfreeze is causing the separation to get munged a bit:

1) Logging/accounting errors: even though we try to make it clear 
wherever possible this solution is based on a "trusted" hypervisor that 
already has substantial privileges over guests (direct access to images, 
etc), one of our internal use cases is the ability for customers to 
properly account for actions initiated by the guest agent. Shutdowns, 
whats files were read/written, etc.

For some commands, this accounting is not important. guest-ping, or 
guest-info, for instance, is uninteresting from a security accounting 
perspective. So logging is in fact optional and logging failures are 
silently ignored.

guest-shutdown, guest-file-open, guest-fsfreeze, however, are important. 
So the QgaLoggingError we currently report really means "this command 
requires logging, but logging has been disabled". You're right that this 
is because of fsfreeze, but it's not because of the filesystem state. 
guest-file-open on a network mount that wasn't frozen would *still*, by 
design, report an error due to inability to log.

For this case I do see your point about the error not being very helpful 
to users, which is why I think something like:

"Guest agent failed to log RPC due to frozen filesystems"

Is the best way to report it. If we need to fix up the error id to 
something like QgaLoggingToFrozenFilesystem I'd be fine with that.

2) EWOULDBLOCK or I/O errors due to writing to frozen filesystems: this 
is currently treated as a PEBKAC and not enforced/reported at all. I 
wouldn't be against disabling guest-file-write as a side-effect of 
freezing, but that's not totally correct. Writes to non-frozen 
filesystems like networked or sysfs mounts is technically still okay, 
for instance. We store path information in guest-file-open and use it to 
scan against fsfreeze's state info about frozen mounts to handle this a 
little better.

Even then you still have RPCs like guest-shutdown that may do a syslog() 
that would cause the command to freeze, or in the future we may add the 
ability for the guest agent to execute user/distro-installed scripts 
that may/may not need to write to the filesystem. Some these might even 
be intended to run when the filesystem is frozen...cleanup scripts for 
databases and whatnot for snapshotting is something I've seen come up.

We can

a) be very restrictive, which I'm not totally against...my initial 
inclination was to disable everything except 
guest-fsfreeze-thaw/guest-fsfreeze-status while frozen, but this has 
some limitations that aren't as obscure/unlikely as one would hope.

b) we can be completely unrestrictive about it and give users the same 
experiences they'd get at a command-line if they, or another user on the 
system, was messing with fsfreeze.

c) try to capture some common cases, but make it clear that you can 
still shoot yourself in the foot using the guest agent on a frozen 
filesystem, evaluating when to enforce this in code on a case-by-case basis.

Personally I like b), mostly because it's the least work, but also 
because it's actually the easiest way to set expectations about fsfreeze.

> The guest-file-write command, on the other hand, clearly requires to write
> to disk, so a client would expect a EWOULDBLOCK error.
>
> EWOULDBLOCK looks good to me. We could define as general rule that
> commands don't block, so clients should always expect a EWOULDBLOCK.
>

This falls under 2)

>> There's nothing wrong with opening a read-only
>> file on a frozen filesystem, it's the fact that we can't log the open
>> that causes us to bail out..
>
> But why do we bail out? Why can't we just ignore the fact we can't log?
>

This falls under 1)

>> So, what if we just munge the 2 to give the user the proper clues to fix
>> things, and instead return an error like:
>>
>> "Guest agent failed to log RPC (is the filesystem frozen?)"?
>
> The 'desc' part of an error is a human error descriptor, clients shouldn't
> parse it. The real error is the error class.
>

As mentioned above, we could change the error id itself to capture both 
the immediate error and the underlying error.
Luiz Capitulino - June 20, 2011, 2:16 p.m.
On Sun, 19 Jun 2011 14:00:30 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On 06/17/2011 10:25 PM, Luiz Capitulino wrote:
> > On Fri, 17 Jun 2011 16:25:32 -0500
> > Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
> >
> >> On 06/17/2011 03:13 PM, Luiz Capitulino wrote:
> >>> On Fri, 17 Jun 2011 14:21:31 -0500
> >>> Michael Roth<mdroth@linux.vnet.ibm.com>   wrote:
> >>>
> >>>> On 06/16/2011 01:42 PM, Luiz Capitulino wrote:
> >>>>> On Tue, 14 Jun 2011 15:06:22 -0500
> >>>>> Michael Roth<mdroth@linux.vnet.ibm.com>    wrote:
> >>>>>
> >>>>>> This is the actual guest daemon, it listens for requests over a
> >>>>>> virtio-serial/isa-serial/unix socket channel and routes them through
> >>>>>> to dispatch routines, and writes the results back to the channel in
> >>>>>> a manner similar to QMP.
> >>>>>>
> >>>>>> A shorthand invocation:
> >>>>>>
> >>>>>>      qemu-ga -d
> >>>>>>
> >>>>>> Is equivalent to:
> >>>>>>
> >>>>>>      qemu-ga -c virtio-serial -p /dev/virtio-ports/org.qemu.guest_agent \
> >>>>>>              -p /var/run/qemu-guest-agent.pid -d
> >>>>>>
> >>>>>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
> >>>>>
> >>>>> Would be nice to have a more complete description, like explaining how to
> >>>>> do a simple test.
> >>>>>
> >>>>> And this can't be built...
> >>>>>
> >>>>>> ---
> >>>>>>     qemu-ga.c              |  631 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>     qga/guest-agent-core.h |    4 +
> >>>>>>     2 files changed, 635 insertions(+), 0 deletions(-)
> >>>>>>     create mode 100644 qemu-ga.c
> >>>>>>
> >>>>>> diff --git a/qemu-ga.c b/qemu-ga.c
> >>>>>> new file mode 100644
> >>>>>> index 0000000..df08d8c
> >>>>>> --- /dev/null
> >>>>>> +++ b/qemu-ga.c
> >>>>>> @@ -0,0 +1,631 @@
> >>>>>> +/*
> >>>>>> + * QEMU Guest Agent
> >>>>>> + *
> >>>>>> + * Copyright IBM Corp. 2011
> >>>>>> + *
> >>>>>> + * Authors:
> >>>>>> + *  Adam Litke<aglitke@linux.vnet.ibm.com>
> >>>>>> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
> >>>>>> + *
> >>>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >>>>>> + * See the COPYING file in the top-level directory.
> >>>>>> + */
> >>>>>> +#include<stdlib.h>
> >>>>>> +#include<stdio.h>
> >>>>>> +#include<stdbool.h>
> >>>>>> +#include<glib.h>
> >>>>>> +#include<gio/gio.h>
> >>>>>> +#include<getopt.h>
> >>>>>> +#include<termios.h>
> >>>>>> +#include<syslog.h>
> >>>>>> +#include "qemu_socket.h"
> >>>>>> +#include "json-streamer.h"
> >>>>>> +#include "json-parser.h"
> >>>>>> +#include "qint.h"
> >>>>>> +#include "qjson.h"
> >>>>>> +#include "qga/guest-agent-core.h"
> >>>>>> +#include "qga-qmp-commands.h"
> >>>>>> +#include "module.h"
> >>>>>> +
> >>>>>> +#define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent"
> >>>>>> +#define QGA_PIDFILE_DEFAULT "/var/run/qemu-va.pid"
> >>>>>> +#define QGA_BAUDRATE_DEFAULT B38400 /* for isa-serial channels */
> >>>>>> +#define QGA_TIMEOUT_DEFAULT 30*1000 /* ms */
> >>>>>> +
> >>>>>> +struct GAState {
> >>>>>> +    const char *proxy_path;
> >>>>>
> >>>>> Where is this used?
> >>>>>
> >>>>
> >>>> Nowhere actually. Will remove.
> >>>>
> >>>>>> +    JSONMessageParser parser;
> >>>>>> +    GMainLoop *main_loop;
> >>>>>> +    guint conn_id;
> >>>>>> +    GSocket *conn_sock;
> >>>>>> +    GIOChannel *conn_channel;
> >>>>>> +    guint listen_id;
> >>>>>> +    GSocket *listen_sock;
> >>>>>> +    GIOChannel *listen_channel;
> >>>>>> +    const char *path;
> >>>>>> +    const char *method;
> >>>>>> +    bool virtio; /* fastpath to check for virtio to deal with poll() quirks */
> >>>>>> +    GACommandState *command_state;
> >>>>>> +    GLogLevelFlags log_level;
> >>>>>> +    FILE *log_file;
> >>>>>> +    bool logging_enabled;
> >>>>>> +};
> >>>>>> +
> >>>>>> +static void usage(const char *cmd)
> >>>>>> +{
> >>>>>> +    printf(
> >>>>>> +"Usage: %s -c<channel_opts>\n"
> >>>>>> +"QEMU Guest Agent %s\n"
> >>>>>> +"\n"
> >>>>>> +"  -c, --channel     channel method: one of unix-connect, virtio-serial, or\n"
> >>>>>> +"                    isa-serial (virtio-serial is the default)\n"
> >>>>>> +"  -p, --path        channel path (%s is the default for virtio-serial)\n"
> >>>>>> +"  -l, --logfile     set logfile path, logs to stderr by default\n"
> >>>>>> +"  -f, --pidfile     specify pidfile (default is %s)\n"
> >>>>>> +"  -v, --verbose     log extra debugging information\n"
> >>>>>> +"  -V, --version     print version information and exit\n"
> >>>>>> +"  -d, --daemonize   become a daemon\n"
> >>>>>> +"  -h, --help        display this help and exit\n"
> >>>>>> +"\n"
> >>>>>> +"Report bugs to<mdroth@linux.vnet.ibm.com>\n"
> >>>>>> +    , cmd, QGA_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void conn_channel_close(GAState *s);
> >>>>>> +
> >>>>>> +static const char *ga_log_level_str(GLogLevelFlags level)
> >>>>>> +{
> >>>>>> +    switch (level&    G_LOG_LEVEL_MASK) {
> >>>>>> +        case G_LOG_LEVEL_ERROR:
> >>>>>> +            return "error";
> >>>>>> +        case G_LOG_LEVEL_CRITICAL:
> >>>>>> +            return "critical";
> >>>>>> +        case G_LOG_LEVEL_WARNING:
> >>>>>> +            return "warning";
> >>>>>> +        case G_LOG_LEVEL_MESSAGE:
> >>>>>> +            return "message";
> >>>>>> +        case G_LOG_LEVEL_INFO:
> >>>>>> +            return "info";
> >>>>>> +        case G_LOG_LEVEL_DEBUG:
> >>>>>> +            return "debug";
> >>>>>> +        default:
> >>>>>> +            return "user";
> >>>>>> +    }
> >>>>>> +}
> >>>>>> +
> >>>>>> +bool ga_logging_enabled(GAState *s)
> >>>>>> +{
> >>>>>> +    return s->logging_enabled;
> >>>>>> +}
> >>>>>> +
> >>>>>> +void ga_disable_logging(GAState *s)
> >>>>>> +{
> >>>>>> +    s->logging_enabled = false;
> >>>>>> +}
> >>>>>> +
> >>>>>> +void ga_enable_logging(GAState *s)
> >>>>>> +{
> >>>>>> +    s->logging_enabled = true;
> >>>>>> +}
> >>>>>
> >>>>> Just to check I got this right, this is needed because of the fsfreeze
> >>>>> command, correct? Isn't it better to have a more descriptive name, like
> >>>>> fsfrozen?
> >>>>>
> >>>>> First I thought this was about a log file. Then I realized this was
> >>>>> probably about letting the user log in, but we don't seem to have this
> >>>>> functionality so I got confused.
> >>>>>
> >>>>
> >>>> Yup, this is currently due to fsfreeze support. From the perspective of
> >>>> the fsfreeze command the explicit "is_frozen" check makes more sense,
> >>>> but the reason it affects other RPCs is because because we can't log
> >>>> stuff in that state. If an RPC shoots itself in the foot by writing to a
> >>>> frozen filesystem we don't really care so much, and up until recently
> >>>> that case was handled with a pthread_cancel timeout mechanism (was
> >>>> removed for the time being, will re-implement using a child process most
> >>>> likely).
> >>>>
> >>>> What we don't want to do is give a host a way to bypass the expectation
> >>>> we set for guest owners by allowing RPCs to be logged. So that's what
> >>>> the check is based on, rather than lower level details like *why*
> >>>> logging is disabled. Also, I'd really like to avoid a precedence where a
> >>>> single RPC can place restrictions on all the others, so the logging
> >>>> aspect seemed general enough that it doesn't necessarily provide that
> >>>> precedence.
> >>>>
> >>>> This has come up a few times without any real consensus. I can probably
> >>>> go either way, but I think the check for logging is easier to set
> >>>> expectations with: "if logging is important from an auditing
> >>>> perspective, don't execute this if logging is disabled". beyond that,
> >>>> same behavior/symantics you'd get with anything that causes a write() on
> >>>> a filesystem in a frozen state: block.
> >>>
> >>> While I understand your arguments, I still find the current behavior
> >>> confusing: you issue a guest-open-file command and get a QgaLoggingFailed
> >>> error. The first question is: what does a log write fail have to do with me
> >>> opening a file? The second question is: what should I do? Try again? Give up?
> >>>
> >>
> >> I agree it's a better solution for the client there, but at the same time:
> >>
> >> guest_privileged_ping():
> >>     if fsfreeze.status == FROZEN:
> >>       syslog("privileged ping, thought you should know")
> >>     else:
> >>       return "error, filesystem frozen"
> >>
> >> Seems silly to the client as well. "Why does a ping command care about
> >> the filesystem state?"
> >>
> >> Inability to log is the true error. That's also the case for the
> >> guest-file-open command.
> >
> > There's a difference. In the guest ping the inability to log is an
> > internal agent error, it's not interesting to the client. We could just
> > ignore the error and reply back (unless we define that guest-privileged-ping
> > requires writing to disk).
> >
> 
> To clarify, these's 2 different types of errors here and their relation 
> to fsfreeze is causing the separation to get munged a bit:
> 
> 1) Logging/accounting errors: even though we try to make it clear 
> wherever possible this solution is based on a "trusted" hypervisor that 
> already has substantial privileges over guests (direct access to images, 
> etc), one of our internal use cases is the ability for customers to 
> properly account for actions initiated by the guest agent. Shutdowns, 
> whats files were read/written, etc.

This is fine.

> For some commands, this accounting is not important. guest-ping, or 
> guest-info, for instance, is uninteresting from a security accounting 
> perspective. So logging is in fact optional and logging failures are 
> silently ignored.
> 
> guest-shutdown, guest-file-open, guest-fsfreeze, however, are important. 
> So the QgaLoggingError we currently report really means "this command 
> requires logging, but logging has been disabled". You're right that this 
> is because of fsfreeze, but it's not because of the filesystem state. 
> guest-file-open on a network mount that wasn't frozen would *still*, by 
> design, report an error due to inability to log.

Completely ignoring the file-system state and considering only what you
say about security, this doesn't make sense to me. Actually, I think it's
a flaw, because a client could open the daemon's log file and write garbage
on it.

But, how does the ability to log protects you from anything? I would understand
it's importance if the user had to provide credentials to use the agent,
but s/he doesn't. It's like you were unable to use a linux box because syslogd
is not running.

This looks like a misfeature to me. If you really want to provide it, then
you could provide a --enable-log-error which, when enabled, would make the
daemon not to execute *any* command if the it's able to log its actions.
Otherwise log errors are just ignored.

> For this case I do see your point about the error not being very helpful 
> to users, which is why I think something like:
> 
> "Guest agent failed to log RPC due to frozen filesystems"
> 
> Is the best way to report it. If we need to fix up the error id to 
> something like QgaLoggingToFrozenFilesystem I'd be fine with that.
> 
> 2) EWOULDBLOCK or I/O errors due to writing to frozen filesystems: this 
> is currently treated as a PEBKAC and not enforced/reported at all. I 
> wouldn't be against disabling guest-file-write as a side-effect of 
> freezing, but that's not totally correct. Writes to non-frozen 
> filesystems like networked or sysfs mounts is technically still okay, 
> for instance. We store path information in guest-file-open and use it to 
> scan against fsfreeze's state info about frozen mounts to handle this a 
> little better.

Yes, I agree with you here.

> 
> Even then you still have RPCs like guest-shutdown that may do a syslog() 
> that would cause the command to freeze, or in the future we may add the 
> ability for the guest agent to execute user/distro-installed scripts 
> that may/may not need to write to the filesystem. Some these might even 
> be intended to run when the filesystem is frozen...cleanup scripts for 
> databases and whatnot for snapshotting is something I've seen come up.
> 
> We can
> 
> a) be very restrictive, which I'm not totally against...my initial 
> inclination was to disable everything except 
> guest-fsfreeze-thaw/guest-fsfreeze-status while frozen, but this has 
> some limitations that aren't as obscure/unlikely as one would hope.
> 
> b) we can be completely unrestrictive about it and give users the same 
> experiences they'd get at a command-line if they, or another user on the 
> system, was messing with fsfreeze.
> 
> c) try to capture some common cases, but make it clear that you can 
> still shoot yourself in the foot using the guest agent on a frozen 
> filesystem, evaluating when to enforce this in code on a case-by-case basis.
> 
> Personally I like b), mostly because it's the least work, but also 
> because it's actually the easiest way to set expectations about fsfreeze.
> 
> > The guest-file-write command, on the other hand, clearly requires to write
> > to disk, so a client would expect a EWOULDBLOCK error.
> >
> > EWOULDBLOCK looks good to me. We could define as general rule that
> > commands don't block, so clients should always expect a EWOULDBLOCK.
> >
> 
> This falls under 2)
> 
> >> There's nothing wrong with opening a read-only
> >> file on a frozen filesystem, it's the fact that we can't log the open
> >> that causes us to bail out..
> >
> > But why do we bail out? Why can't we just ignore the fact we can't log?
> >
> 
> This falls under 1)
> 
> >> So, what if we just munge the 2 to give the user the proper clues to fix
> >> things, and instead return an error like:
> >>
> >> "Guest agent failed to log RPC (is the filesystem frozen?)"?
> >
> > The 'desc' part of an error is a human error descriptor, clients shouldn't
> > parse it. The real error is the error class.
> >
> 
> As mentioned above, we could change the error id itself to capture both 
> the immediate error and the underlying error.
>
Michael Roth - June 20, 2011, 11:40 p.m.
On 06/20/2011 09:16 AM, Luiz Capitulino wrote:
> On Sun, 19 Jun 2011 14:00:30 -0500
> Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>
>> On 06/17/2011 10:25 PM, Luiz Capitulino wrote:
>>> On Fri, 17 Jun 2011 16:25:32 -0500
>>> Michael Roth<mdroth@linux.vnet.ibm.com>   wrote:
>>>
>>>> On 06/17/2011 03:13 PM, Luiz Capitulino wrote:
>>>>> On Fri, 17 Jun 2011 14:21:31 -0500
>>>>> Michael Roth<mdroth@linux.vnet.ibm.com>    wrote:
>>>>>
>>>>>> On 06/16/2011 01:42 PM, Luiz Capitulino wrote:
>>>>>>> On Tue, 14 Jun 2011 15:06:22 -0500
>>>>>>> Michael Roth<mdroth@linux.vnet.ibm.com>     wrote:
>>>>>>>
>>>>>>>> This is the actual guest daemon, it listens for requests over a
>>>>>>>> virtio-serial/isa-serial/unix socket channel and routes them through
>>>>>>>> to dispatch routines, and writes the results back to the channel in
>>>>>>>> a manner similar to QMP.
>>>>>>>>
>>>>>>>> A shorthand invocation:
>>>>>>>>
>>>>>>>>       qemu-ga -d
>>>>>>>>
>>>>>>>> Is equivalent to:
>>>>>>>>
>>>>>>>>       qemu-ga -c virtio-serial -p /dev/virtio-ports/org.qemu.guest_agent \
>>>>>>>>               -p /var/run/qemu-guest-agent.pid -d
>>>>>>>>
>>>>>>>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>>>>>>>
>>>>>>> Would be nice to have a more complete description, like explaining how to
>>>>>>> do a simple test.
>>>>>>>
>>>>>>> And this can't be built...
>>>>>>>
>>>>>>>> ---
>>>>>>>>      qemu-ga.c              |  631 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>      qga/guest-agent-core.h |    4 +
>>>>>>>>      2 files changed, 635 insertions(+), 0 deletions(-)
>>>>>>>>      create mode 100644 qemu-ga.c
>>>>>>>>
>>>>>>>> diff --git a/qemu-ga.c b/qemu-ga.c
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..df08d8c
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/qemu-ga.c
>>>>>>>> @@ -0,0 +1,631 @@
>>>>>>>> +/*
>>>>>>>> + * QEMU Guest Agent
>>>>>>>> + *
>>>>>>>> + * Copyright IBM Corp. 2011
>>>>>>>> + *
>>>>>>>> + * Authors:
>>>>>>>> + *  Adam Litke<aglitke@linux.vnet.ibm.com>
>>>>>>>> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
>>>>>>>> + *
>>>>>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>>>>>>> + * See the COPYING file in the top-level directory.
>>>>>>>> + */
>>>>>>>> +#include<stdlib.h>
>>>>>>>> +#include<stdio.h>
>>>>>>>> +#include<stdbool.h>
>>>>>>>> +#include<glib.h>
>>>>>>>> +#include<gio/gio.h>
>>>>>>>> +#include<getopt.h>
>>>>>>>> +#include<termios.h>
>>>>>>>> +#include<syslog.h>
>>>>>>>> +#include "qemu_socket.h"
>>>>>>>> +#include "json-streamer.h"
>>>>>>>> +#include "json-parser.h"
>>>>>>>> +#include "qint.h"
>>>>>>>> +#include "qjson.h"
>>>>>>>> +#include "qga/guest-agent-core.h"
>>>>>>>> +#include "qga-qmp-commands.h"
>>>>>>>> +#include "module.h"
>>>>>>>> +
>>>>>>>> +#define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent"
>>>>>>>> +#define QGA_PIDFILE_DEFAULT "/var/run/qemu-va.pid"
>>>>>>>> +#define QGA_BAUDRATE_DEFAULT B38400 /* for isa-serial channels */
>>>>>>>> +#define QGA_TIMEOUT_DEFAULT 30*1000 /* ms */
>>>>>>>> +
>>>>>>>> +struct GAState {
>>>>>>>> +    const char *proxy_path;
>>>>>>>
>>>>>>> Where is this used?
>>>>>>>
>>>>>>
>>>>>> Nowhere actually. Will remove.
>>>>>>
>>>>>>>> +    JSONMessageParser parser;
>>>>>>>> +    GMainLoop *main_loop;
>>>>>>>> +    guint conn_id;
>>>>>>>> +    GSocket *conn_sock;
>>>>>>>> +    GIOChannel *conn_channel;
>>>>>>>> +    guint listen_id;
>>>>>>>> +    GSocket *listen_sock;
>>>>>>>> +    GIOChannel *listen_channel;
>>>>>>>> +    const char *path;
>>>>>>>> +    const char *method;
>>>>>>>> +    bool virtio; /* fastpath to check for virtio to deal with poll() quirks */
>>>>>>>> +    GACommandState *command_state;
>>>>>>>> +    GLogLevelFlags log_level;
>>>>>>>> +    FILE *log_file;
>>>>>>>> +    bool logging_enabled;
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +static void usage(const char *cmd)
>>>>>>>> +{
>>>>>>>> +    printf(
>>>>>>>> +"Usage: %s -c<channel_opts>\n"
>>>>>>>> +"QEMU Guest Agent %s\n"
>>>>>>>> +"\n"
>>>>>>>> +"  -c, --channel     channel method: one of unix-connect, virtio-serial, or\n"
>>>>>>>> +"                    isa-serial (virtio-serial is the default)\n"
>>>>>>>> +"  -p, --path        channel path (%s is the default for virtio-serial)\n"
>>>>>>>> +"  -l, --logfile     set logfile path, logs to stderr by default\n"
>>>>>>>> +"  -f, --pidfile     specify pidfile (default is %s)\n"
>>>>>>>> +"  -v, --verbose     log extra debugging information\n"
>>>>>>>> +"  -V, --version     print version information and exit\n"
>>>>>>>> +"  -d, --daemonize   become a daemon\n"
>>>>>>>> +"  -h, --help        display this help and exit\n"
>>>>>>>> +"\n"
>>>>>>>> +"Report bugs to<mdroth@linux.vnet.ibm.com>\n"
>>>>>>>> +    , cmd, QGA_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void conn_channel_close(GAState *s);
>>>>>>>> +
>>>>>>>> +static const char *ga_log_level_str(GLogLevelFlags level)
>>>>>>>> +{
>>>>>>>> +    switch (level&     G_LOG_LEVEL_MASK) {
>>>>>>>> +        case G_LOG_LEVEL_ERROR:
>>>>>>>> +            return "error";
>>>>>>>> +        case G_LOG_LEVEL_CRITICAL:
>>>>>>>> +            return "critical";
>>>>>>>> +        case G_LOG_LEVEL_WARNING:
>>>>>>>> +            return "warning";
>>>>>>>> +        case G_LOG_LEVEL_MESSAGE:
>>>>>>>> +            return "message";
>>>>>>>> +        case G_LOG_LEVEL_INFO:
>>>>>>>> +            return "info";
>>>>>>>> +        case G_LOG_LEVEL_DEBUG:
>>>>>>>> +            return "debug";
>>>>>>>> +        default:
>>>>>>>> +            return "user";
>>>>>>>> +    }
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +bool ga_logging_enabled(GAState *s)
>>>>>>>> +{
>>>>>>>> +    return s->logging_enabled;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +void ga_disable_logging(GAState *s)
>>>>>>>> +{
>>>>>>>> +    s->logging_enabled = false;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +void ga_enable_logging(GAState *s)
>>>>>>>> +{
>>>>>>>> +    s->logging_enabled = true;
>>>>>>>> +}
>>>>>>>
>>>>>>> Just to check I got this right, this is needed because of the fsfreeze
>>>>>>> command, correct? Isn't it better to have a more descriptive name, like
>>>>>>> fsfrozen?
>>>>>>>
>>>>>>> First I thought this was about a log file. Then I realized this was
>>>>>>> probably about letting the user log in, but we don't seem to have this
>>>>>>> functionality so I got confused.
>>>>>>>
>>>>>>
>>>>>> Yup, this is currently due to fsfreeze support. From the perspective of
>>>>>> the fsfreeze command the explicit "is_frozen" check makes more sense,
>>>>>> but the reason it affects other RPCs is because because we can't log
>>>>>> stuff in that state. If an RPC shoots itself in the foot by writing to a
>>>>>> frozen filesystem we don't really care so much, and up until recently
>>>>>> that case was handled with a pthread_cancel timeout mechanism (was
>>>>>> removed for the time being, will re-implement using a child process most
>>>>>> likely).
>>>>>>
>>>>>> What we don't want to do is give a host a way to bypass the expectation
>>>>>> we set for guest owners by allowing RPCs to be logged. So that's what
>>>>>> the check is based on, rather than lower level details like *why*
>>>>>> logging is disabled. Also, I'd really like to avoid a precedence where a
>>>>>> single RPC can place restrictions on all the others, so the logging
>>>>>> aspect seemed general enough that it doesn't necessarily provide that
>>>>>> precedence.
>>>>>>
>>>>>> This has come up a few times without any real consensus. I can probably
>>>>>> go either way, but I think the check for logging is easier to set
>>>>>> expectations with: "if logging is important from an auditing
>>>>>> perspective, don't execute this if logging is disabled". beyond that,
>>>>>> same behavior/symantics you'd get with anything that causes a write() on
>>>>>> a filesystem in a frozen state: block.
>>>>>
>>>>> While I understand your arguments, I still find the current behavior
>>>>> confusing: you issue a guest-open-file command and get a QgaLoggingFailed
>>>>> error. The first question is: what does a log write fail have to do with me
>>>>> opening a file? The second question is: what should I do? Try again? Give up?
>>>>>
>>>>
>>>> I agree it's a better solution for the client there, but at the same time:
>>>>
>>>> guest_privileged_ping():
>>>>      if fsfreeze.status == FROZEN:
>>>>        syslog("privileged ping, thought you should know")
>>>>      else:
>>>>        return "error, filesystem frozen"
>>>>
>>>> Seems silly to the client as well. "Why does a ping command care about
>>>> the filesystem state?"
>>>>
>>>> Inability to log is the true error. That's also the case for the
>>>> guest-file-open command.
>>>
>>> There's a difference. In the guest ping the inability to log is an
>>> internal agent error, it's not interesting to the client. We could just
>>> ignore the error and reply back (unless we define that guest-privileged-ping
>>> requires writing to disk).
>>>
>>
>> To clarify, these's 2 different types of errors here and their relation
>> to fsfreeze is causing the separation to get munged a bit:
>>
>> 1) Logging/accounting errors: even though we try to make it clear
>> wherever possible this solution is based on a "trusted" hypervisor that
>> already has substantial privileges over guests (direct access to images,
>> etc), one of our internal use cases is the ability for customers to
>> properly account for actions initiated by the guest agent. Shutdowns,
>> whats files were read/written, etc.
>
> This is fine.
>
>> For some commands, this accounting is not important. guest-ping, or
>> guest-info, for instance, is uninteresting from a security accounting
>> perspective. So logging is in fact optional and logging failures are
>> silently ignored.
>>
>> guest-shutdown, guest-file-open, guest-fsfreeze, however, are important.
>> So the QgaLoggingError we currently report really means "this command
>> requires logging, but logging has been disabled". You're right that this
>> is because of fsfreeze, but it's not because of the filesystem state.
>> guest-file-open on a network mount that wasn't frozen would *still*, by
>> design, report an error due to inability to log.
>
> Completely ignoring the file-system state and considering only what you
> say about security, this doesn't make sense to me. Actually, I think it's
> a flaw, because a client could open the daemon's log file and write garbage
> on it.
>
> But, how does the ability to log protects you from anything? I would understand
> it's importance if the user had to provide credentials to use the agent,
> but s/he doesn't. It's like you were unable to use a linux box because syslogd
> is not running.
>
> This looks like a misfeature to me. If you really want to provide it, then
> you could provide a --enable-log-error which, when enabled, would make the
> daemon not to execute *any* command if the it's able to log its actions.
> Otherwise log errors are just ignored.
>

Hmm, you're right. I'd thought that if you clobber the log file at least 
the RPC that clobbered the log file would get logged, but since we only 
log the guest-file-open, and we do that before the guest-file-write, 
they could overwrite and fabricate the log file history and nothing 
would be obviously suspicious.

Since we can't build any accounting around something like that I'm fine 
with having the logging silently fail in a freeze state for now.

A better thought out accounting mechanism can be added as an optional 
feature later if it comes to that.

>> For this case I do see your point about the error not being very helpful
>> to users, which is why I think something like:
>>
>> "Guest agent failed to log RPC due to frozen filesystems"
>>
>> Is the best way to report it. If we need to fix up the error id to
>> something like QgaLoggingToFrozenFilesystem I'd be fine with that.
>>
>> 2) EWOULDBLOCK or I/O errors due to writing to frozen filesystems: this
>> is currently treated as a PEBKAC and not enforced/reported at all. I
>> wouldn't be against disabling guest-file-write as a side-effect of
>> freezing, but that's not totally correct. Writes to non-frozen
>> filesystems like networked or sysfs mounts is technically still okay,
>> for instance. We store path information in guest-file-open and use it to
>> scan against fsfreeze's state info about frozen mounts to handle this a
>> little better.
>
> Yes, I agree with you here.
>

Treating I/O errors/deadlocks due to fsfreeze as a user issue (as things 
are currently), or working around it? If the latter, a), b), or c)?

>>
>> Even then you still have RPCs like guest-shutdown that may do a syslog()
>> that would cause the command to freeze, or in the future we may add the
>> ability for the guest agent to execute user/distro-installed scripts
>> that may/may not need to write to the filesystem. Some these might even
>> be intended to run when the filesystem is frozen...cleanup scripts for
>> databases and whatnot for snapshotting is something I've seen come up.
>>
>> We can
>>
>> a) be very restrictive, which I'm not totally against...my initial
>> inclination was to disable everything except
>> guest-fsfreeze-thaw/guest-fsfreeze-status while frozen, but this has
>> some limitations that aren't as obscure/unlikely as one would hope.
>>
>> b) we can be completely unrestrictive about it and give users the same
>> experiences they'd get at a command-line if they, or another user on the
>> system, was messing with fsfreeze.
>>
>> c) try to capture some common cases, but make it clear that you can
>> still shoot yourself in the foot using the guest agent on a frozen
>> filesystem, evaluating when to enforce this in code on a case-by-case basis.
>>
>> Personally I like b), mostly because it's the least work, but also
>> because it's actually the easiest way to set expectations about fsfreeze.
>>
>>> The guest-file-write command, on the other hand, clearly requires to write
>>> to disk, so a client would expect a EWOULDBLOCK error.
>>>
>>> EWOULDBLOCK looks good to me. We could define as general rule that
>>> commands don't block, so clients should always expect a EWOULDBLOCK.
>>>
>>
>> This falls under 2)
>>
>>>> There's nothing wrong with opening a read-only
>>>> file on a frozen filesystem, it's the fact that we can't log the open
>>>> that causes us to bail out..
>>>
>>> But why do we bail out? Why can't we just ignore the fact we can't log?
>>>
>>
>> This falls under 1)
>>
>>>> So, what if we just munge the 2 to give the user the proper clues to fix
>>>> things, and instead return an error like:
>>>>
>>>> "Guest agent failed to log RPC (is the filesystem frozen?)"?
>>>
>>> The 'desc' part of an error is a human error descriptor, clients shouldn't
>>> parse it. The real error is the error class.
>>>
>>
>> As mentioned above, we could change the error id itself to capture both
>> the immediate error and the underlying error.
>>
>
Luiz Capitulino - June 21, 2011, 1:38 p.m.
On Mon, 20 Jun 2011 18:40:38 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On 06/20/2011 09:16 AM, Luiz Capitulino wrote:
> > On Sun, 19 Jun 2011 14:00:30 -0500
> > Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
> >
> >> On 06/17/2011 10:25 PM, Luiz Capitulino wrote:
> >>> On Fri, 17 Jun 2011 16:25:32 -0500
> >>> Michael Roth<mdroth@linux.vnet.ibm.com>   wrote:
> >>>
> >>>> On 06/17/2011 03:13 PM, Luiz Capitulino wrote:
> >>>>> On Fri, 17 Jun 2011 14:21:31 -0500
> >>>>> Michael Roth<mdroth@linux.vnet.ibm.com>    wrote:
> >>>>>
> >>>>>> On 06/16/2011 01:42 PM, Luiz Capitulino wrote:
> >>>>>>> On Tue, 14 Jun 2011 15:06:22 -0500
> >>>>>>> Michael Roth<mdroth@linux.vnet.ibm.com>     wrote:
> >>>>>>>
> >>>>>>>> This is the actual guest daemon, it listens for requests over a
> >>>>>>>> virtio-serial/isa-serial/unix socket channel and routes them through
> >>>>>>>> to dispatch routines, and writes the results back to the channel in
> >>>>>>>> a manner similar to QMP.
> >>>>>>>>
> >>>>>>>> A shorthand invocation:
> >>>>>>>>
> >>>>>>>>       qemu-ga -d
> >>>>>>>>
> >>>>>>>> Is equivalent to:
> >>>>>>>>
> >>>>>>>>       qemu-ga -c virtio-serial -p /dev/virtio-ports/org.qemu.guest_agent \
> >>>>>>>>               -p /var/run/qemu-guest-agent.pid -d
> >>>>>>>>
> >>>>>>>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
> >>>>>>>
> >>>>>>> Would be nice to have a more complete description, like explaining how to
> >>>>>>> do a simple test.
> >>>>>>>
> >>>>>>> And this can't be built...
> >>>>>>>
> >>>>>>>> ---
> >>>>>>>>      qemu-ga.c              |  631 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>>      qga/guest-agent-core.h |    4 +
> >>>>>>>>      2 files changed, 635 insertions(+), 0 deletions(-)
> >>>>>>>>      create mode 100644 qemu-ga.c
> >>>>>>>>
> >>>>>>>> diff --git a/qemu-ga.c b/qemu-ga.c
> >>>>>>>> new file mode 100644
> >>>>>>>> index 0000000..df08d8c
> >>>>>>>> --- /dev/null
> >>>>>>>> +++ b/qemu-ga.c
> >>>>>>>> @@ -0,0 +1,631 @@
> >>>>>>>> +/*
> >>>>>>>> + * QEMU Guest Agent
> >>>>>>>> + *
> >>>>>>>> + * Copyright IBM Corp. 2011
> >>>>>>>> + *
> >>>>>>>> + * Authors:
> >>>>>>>> + *  Adam Litke<aglitke@linux.vnet.ibm.com>
> >>>>>>>> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
> >>>>>>>> + *
> >>>>>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >>>>>>>> + * See the COPYING file in the top-level directory.
> >>>>>>>> + */
> >>>>>>>> +#include<stdlib.h>
> >>>>>>>> +#include<stdio.h>
> >>>>>>>> +#include<stdbool.h>
> >>>>>>>> +#include<glib.h>
> >>>>>>>> +#include<gio/gio.h>
> >>>>>>>> +#include<getopt.h>
> >>>>>>>> +#include<termios.h>
> >>>>>>>> +#include<syslog.h>
> >>>>>>>> +#include "qemu_socket.h"
> >>>>>>>> +#include "json-streamer.h"
> >>>>>>>> +#include "json-parser.h"
> >>>>>>>> +#include "qint.h"
> >>>>>>>> +#include "qjson.h"
> >>>>>>>> +#include "qga/guest-agent-core.h"
> >>>>>>>> +#include "qga-qmp-commands.h"
> >>>>>>>> +#include "module.h"
> >>>>>>>> +
> >>>>>>>> +#define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent"
> >>>>>>>> +#define QGA_PIDFILE_DEFAULT "/var/run/qemu-va.pid"
> >>>>>>>> +#define QGA_BAUDRATE_DEFAULT B38400 /* for isa-serial channels */
> >>>>>>>> +#define QGA_TIMEOUT_DEFAULT 30*1000 /* ms */
> >>>>>>>> +
> >>>>>>>> +struct GAState {
> >>>>>>>> +    const char *proxy_path;
> >>>>>>>
> >>>>>>> Where is this used?
> >>>>>>>
> >>>>>>
> >>>>>> Nowhere actually. Will remove.
> >>>>>>
> >>>>>>>> +    JSONMessageParser parser;
> >>>>>>>> +    GMainLoop *main_loop;
> >>>>>>>> +    guint conn_id;
> >>>>>>>> +    GSocket *conn_sock;
> >>>>>>>> +    GIOChannel *conn_channel;
> >>>>>>>> +    guint listen_id;
> >>>>>>>> +    GSocket *listen_sock;
> >>>>>>>> +    GIOChannel *listen_channel;
> >>>>>>>> +    const char *path;
> >>>>>>>> +    const char *method;
> >>>>>>>> +    bool virtio; /* fastpath to check for virtio to deal with poll() quirks */
> >>>>>>>> +    GACommandState *command_state;
> >>>>>>>> +    GLogLevelFlags log_level;
> >>>>>>>> +    FILE *log_file;
> >>>>>>>> +    bool logging_enabled;
> >>>>>>>> +};
> >>>>>>>> +
> >>>>>>>> +static void usage(const char *cmd)
> >>>>>>>> +{
> >>>>>>>> +    printf(
> >>>>>>>> +"Usage: %s -c<channel_opts>\n"
> >>>>>>>> +"QEMU Guest Agent %s\n"
> >>>>>>>> +"\n"
> >>>>>>>> +"  -c, --channel     channel method: one of unix-connect, virtio-serial, or\n"
> >>>>>>>> +"                    isa-serial (virtio-serial is the default)\n"
> >>>>>>>> +"  -p, --path        channel path (%s is the default for virtio-serial)\n"
> >>>>>>>> +"  -l, --logfile     set logfile path, logs to stderr by default\n"
> >>>>>>>> +"  -f, --pidfile     specify pidfile (default is %s)\n"
> >>>>>>>> +"  -v, --verbose     log extra debugging information\n"
> >>>>>>>> +"  -V, --version     print version information and exit\n"
> >>>>>>>> +"  -d, --daemonize   become a daemon\n"
> >>>>>>>> +"  -h, --help        display this help and exit\n"
> >>>>>>>> +"\n"
> >>>>>>>> +"Report bugs to<mdroth@linux.vnet.ibm.com>\n"
> >>>>>>>> +    , cmd, QGA_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT);
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static void conn_channel_close(GAState *s);
> >>>>>>>> +
> >>>>>>>> +static const char *ga_log_level_str(GLogLevelFlags level)
> >>>>>>>> +{
> >>>>>>>> +    switch (level&     G_LOG_LEVEL_MASK) {
> >>>>>>>> +        case G_LOG_LEVEL_ERROR:
> >>>>>>>> +            return "error";
> >>>>>>>> +        case G_LOG_LEVEL_CRITICAL:
> >>>>>>>> +            return "critical";
> >>>>>>>> +        case G_LOG_LEVEL_WARNING:
> >>>>>>>> +            return "warning";
> >>>>>>>> +        case G_LOG_LEVEL_MESSAGE:
> >>>>>>>> +            return "message";
> >>>>>>>> +        case G_LOG_LEVEL_INFO:
> >>>>>>>> +            return "info";
> >>>>>>>> +        case G_LOG_LEVEL_DEBUG:
> >>>>>>>> +            return "debug";
> >>>>>>>> +        default:
> >>>>>>>> +            return "user";
> >>>>>>>> +    }
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +bool ga_logging_enabled(GAState *s)
> >>>>>>>> +{
> >>>>>>>> +    return s->logging_enabled;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +void ga_disable_logging(GAState *s)
> >>>>>>>> +{
> >>>>>>>> +    s->logging_enabled = false;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +void ga_enable_logging(GAState *s)
> >>>>>>>> +{
> >>>>>>>> +    s->logging_enabled = true;
> >>>>>>>> +}
> >>>>>>>
> >>>>>>> Just to check I got this right, this is needed because of the fsfreeze
> >>>>>>> command, correct? Isn't it better to have a more descriptive name, like
> >>>>>>> fsfrozen?
> >>>>>>>
> >>>>>>> First I thought this was about a log file. Then I realized this was
> >>>>>>> probably about letting the user log in, but we don't seem to have this
> >>>>>>> functionality so I got confused.
> >>>>>>>
> >>>>>>
> >>>>>> Yup, this is currently due to fsfreeze support. From the perspective of
> >>>>>> the fsfreeze command the explicit "is_frozen" check makes more sense,
> >>>>>> but the reason it affects other RPCs is because because we can't log
> >>>>>> stuff in that state. If an RPC shoots itself in the foot by writing to a
> >>>>>> frozen filesystem we don't really care so much, and up until recently
> >>>>>> that case was handled with a pthread_cancel timeout mechanism (was
> >>>>>> removed for the time being, will re-implement using a child process most
> >>>>>> likely).
> >>>>>>
> >>>>>> What we don't want to do is give a host a way to bypass the expectation
> >>>>>> we set for guest owners by allowing RPCs to be logged. So that's what
> >>>>>> the check is based on, rather than lower level details like *why*
> >>>>>> logging is disabled. Also, I'd really like to avoid a precedence where a
> >>>>>> single RPC can place restrictions on all the others, so the logging
> >>>>>> aspect seemed general enough that it doesn't necessarily provide that
> >>>>>> precedence.
> >>>>>>
> >>>>>> This has come up a few times without any real consensus. I can probably
> >>>>>> go either way, but I think the check for logging is easier to set
> >>>>>> expectations with: "if logging is important from an auditing
> >>>>>> perspective, don't execute this if logging is disabled". beyond that,
> >>>>>> same behavior/symantics you'd get with anything that causes a write() on
> >>>>>> a filesystem in a frozen state: block.
> >>>>>
> >>>>> While I understand your arguments, I still find the current behavior
> >>>>> confusing: you issue a guest-open-file command and get a QgaLoggingFailed
> >>>>> error. The first question is: what does a log write fail have to do with me
> >>>>> opening a file? The second question is: what should I do? Try again? Give up?
> >>>>>
> >>>>
> >>>> I agree it's a better solution for the client there, but at the same time:
> >>>>
> >>>> guest_privileged_ping():
> >>>>      if fsfreeze.status == FROZEN:
> >>>>        syslog("privileged ping, thought you should know")
> >>>>      else:
> >>>>        return "error, filesystem frozen"
> >>>>
> >>>> Seems silly to the client as well. "Why does a ping command care about
> >>>> the filesystem state?"
> >>>>
> >>>> Inability to log is the true error. That's also the case for the
> >>>> guest-file-open command.
> >>>
> >>> There's a difference. In the guest ping the inability to log is an
> >>> internal agent error, it's not interesting to the client. We could just
> >>> ignore the error and reply back (unless we define that guest-privileged-ping
> >>> requires writing to disk).
> >>>
> >>
> >> To clarify, these's 2 different types of errors here and their relation
> >> to fsfreeze is causing the separation to get munged a bit:
> >>
> >> 1) Logging/accounting errors: even though we try to make it clear
> >> wherever possible this solution is based on a "trusted" hypervisor that
> >> already has substantial privileges over guests (direct access to images,
> >> etc), one of our internal use cases is the ability for customers to
> >> properly account for actions initiated by the guest agent. Shutdowns,
> >> whats files were read/written, etc.
> >
> > This is fine.
> >
> >> For some commands, this accounting is not important. guest-ping, or
> >> guest-info, for instance, is uninteresting from a security accounting
> >> perspective. So logging is in fact optional and logging failures are
> >> silently ignored.
> >>
> >> guest-shutdown, guest-file-open, guest-fsfreeze, however, are important.
> >> So the QgaLoggingError we currently report really means "this command
> >> requires logging, but logging has been disabled". You're right that this
> >> is because of fsfreeze, but it's not because of the filesystem state.
> >> guest-file-open on a network mount that wasn't frozen would *still*, by
> >> design, report an error due to inability to log.
> >
> > Completely ignoring the file-system state and considering only what you
> > say about security, this doesn't make sense to me. Actually, I think it's
> > a flaw, because a client could open the daemon's log file and write garbage
> > on it.
> >
> > But, how does the ability to log protects you from anything? I would understand
> > it's importance if the user had to provide credentials to use the agent,
> > but s/he doesn't. It's like you were unable to use a linux box because syslogd
> > is not running.
> >
> > This looks like a misfeature to me. If you really want to provide it, then
> > you could provide a --enable-log-error which, when enabled, would make the
> > daemon not to execute *any* command if the it's able to log its actions.
> > Otherwise log errors are just ignored.
> >
> 
> Hmm, you're right. I'd thought that if you clobber the log file at least 
> the RPC that clobbered the log file would get logged, but since we only 
> log the guest-file-open, and we do that before the guest-file-write, 
> they could overwrite and fabricate the log file history and nothing 
> would be obviously suspicious.
> 
> Since we can't build any accounting around something like that I'm fine 
> with having the logging silently fail in a freeze state for now.
> 
> A better thought out accounting mechanism can be added as an optional 
> feature later if it comes to that.

Yes.

> >> For this case I do see your point about the error not being very helpful
> >> to users, which is why I think something like:
> >>
> >> "Guest agent failed to log RPC due to frozen filesystems"
> >>
> >> Is the best way to report it. If we need to fix up the error id to
> >> something like QgaLoggingToFrozenFilesystem I'd be fine with that.
> >>
> >> 2) EWOULDBLOCK or I/O errors due to writing to frozen filesystems: this
> >> is currently treated as a PEBKAC and not enforced/reported at all. I
> >> wouldn't be against disabling guest-file-write as a side-effect of
> >> freezing, but that's not totally correct. Writes to non-frozen
> >> filesystems like networked or sysfs mounts is technically still okay,
> >> for instance. We store path information in guest-file-open and use it to
> >> scan against fsfreeze's state info about frozen mounts to handle this a
> >> little better.
> >
> > Yes, I agree with you here.
> >
> 
> Treating I/O errors/deadlocks due to fsfreeze as a user issue (as things 
> are currently), or working around it? If the latter, a), b), or c)?

Option b) makes sense to me too.

> 
> >>
> >> Even then you still have RPCs like guest-shutdown that may do a syslog()
> >> that would cause the command to freeze, or in the future we may add the
> >> ability for the guest agent to execute user/distro-installed scripts
> >> that may/may not need to write to the filesystem. Some these might even
> >> be intended to run when the filesystem is frozen...cleanup scripts for
> >> databases and whatnot for snapshotting is something I've seen come up.
> >>
> >> We can
> >>
> >> a) be very restrictive, which I'm not totally against...my initial
> >> inclination was to disable everything except
> >> guest-fsfreeze-thaw/guest-fsfreeze-status while frozen, but this has
> >> some limitations that aren't as obscure/unlikely as one would hope.
> >>
> >> b) we can be completely unrestrictive about it and give users the same
> >> experiences they'd get at a command-line if they, or another user on the
> >> system, was messing with fsfreeze.
> >>
> >> c) try to capture some common cases, but make it clear that you can
> >> still shoot yourself in the foot using the guest agent on a frozen
> >> filesystem, evaluating when to enforce this in code on a case-by-case basis.
> >>
> >> Personally I like b), mostly because it's the least work, but also
> >> because it's actually the easiest way to set expectations about fsfreeze.
> >>
> >>> The guest-file-write command, on the other hand, clearly requires to write
> >>> to disk, so a client would expect a EWOULDBLOCK error.
> >>>
> >>> EWOULDBLOCK looks good to me. We could define as general rule that
> >>> commands don't block, so clients should always expect a EWOULDBLOCK.
> >>>
> >>
> >> This falls under 2)
> >>
> >>>> There's nothing wrong with opening a read-only
> >>>> file on a frozen filesystem, it's the fact that we can't log the open
> >>>> that causes us to bail out..
> >>>
> >>> But why do we bail out? Why can't we just ignore the fact we can't log?
> >>>
> >>
> >> This falls under 1)
> >>
> >>>> So, what if we just munge the 2 to give the user the proper clues to fix
> >>>> things, and instead return an error like:
> >>>>
> >>>> "Guest agent failed to log RPC (is the filesystem frozen?)"?
> >>>
> >>> The 'desc' part of an error is a human error descriptor, clients shouldn't
> >>> parse it. The real error is the error class.
> >>>
> >>
> >> As mentioned above, we could change the error id itself to capture both
> >> the immediate error and the underlying error.
> >>
> >
>

Patch

diff --git a/qemu-ga.c b/qemu-ga.c
new file mode 100644
index 0000000..df08d8c
--- /dev/null
+++ b/qemu-ga.c
@@ -0,0 +1,631 @@ 
+/*
+ * QEMU Guest Agent
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Adam Litke        <aglitke@linux.vnet.ibm.com>
+ *  Michael Roth      <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include <stdlib.h>
+#include <stdio.h>
+#include <stdbool.h>
+#include <glib.h>
+#include <gio/gio.h>
+#include <getopt.h>
+#include <termios.h>
+#include <syslog.h>
+#include "qemu_socket.h"
+#include "json-streamer.h"
+#include "json-parser.h"
+#include "qint.h"
+#include "qjson.h"
+#include "qga/guest-agent-core.h"
+#include "qga-qmp-commands.h"
+#include "module.h"
+
+#define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent"
+#define QGA_PIDFILE_DEFAULT "/var/run/qemu-va.pid"
+#define QGA_BAUDRATE_DEFAULT B38400 /* for isa-serial channels */
+#define QGA_TIMEOUT_DEFAULT 30*1000 /* ms */
+
+struct GAState {
+    const char *proxy_path;
+    JSONMessageParser parser;
+    GMainLoop *main_loop;
+    guint conn_id;
+    GSocket *conn_sock;
+    GIOChannel *conn_channel;
+    guint listen_id;
+    GSocket *listen_sock;
+    GIOChannel *listen_channel;
+    const char *path;
+    const char *method;
+    bool virtio; /* fastpath to check for virtio to deal with poll() quirks */
+    GACommandState *command_state;
+    GLogLevelFlags log_level;
+    FILE *log_file;
+    bool logging_enabled;
+};
+
+static void usage(const char *cmd)
+{
+    printf(
+"Usage: %s -c <channel_opts>\n"
+"QEMU Guest Agent %s\n"
+"\n"
+"  -c, --channel     channel method: one of unix-connect, virtio-serial, or\n"
+"                    isa-serial (virtio-serial is the default)\n"
+"  -p, --path        channel path (%s is the default for virtio-serial)\n"
+"  -l, --logfile     set logfile path, logs to stderr by default\n"
+"  -f, --pidfile     specify pidfile (default is %s)\n"
+"  -v, --verbose     log extra debugging information\n"
+"  -V, --version     print version information and exit\n"
+"  -d, --daemonize   become a daemon\n"
+"  -h, --help        display this help and exit\n"
+"\n"
+"Report bugs to <mdroth@linux.vnet.ibm.com>\n"
+    , cmd, QGA_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT);
+}
+
+static void conn_channel_close(GAState *s);
+
+static const char *ga_log_level_str(GLogLevelFlags level)
+{
+    switch (level & G_LOG_LEVEL_MASK) {
+        case G_LOG_LEVEL_ERROR:
+            return "error";
+        case G_LOG_LEVEL_CRITICAL:
+            return "critical";
+        case G_LOG_LEVEL_WARNING:
+            return "warning";
+        case G_LOG_LEVEL_MESSAGE:
+            return "message";
+        case G_LOG_LEVEL_INFO:
+            return "info";
+        case G_LOG_LEVEL_DEBUG:
+            return "debug";
+        default:
+            return "user";
+    }
+}
+
+bool ga_logging_enabled(GAState *s)
+{
+    return s->logging_enabled;
+}
+
+void ga_disable_logging(GAState *s)
+{
+    s->logging_enabled = false;
+}
+
+void ga_enable_logging(GAState *s)
+{
+    s->logging_enabled = true;
+}
+
+static void ga_log(const gchar *domain, GLogLevelFlags level,
+                   const gchar *msg, gpointer opaque)
+{
+    GAState *s = opaque;
+    GTimeVal time;
+    const char *level_str = ga_log_level_str(level);
+
+    if (!ga_logging_enabled(s)) {
+        return;
+    }
+
+    level &= G_LOG_LEVEL_MASK;
+    if (g_strcmp0(domain, "syslog") == 0) {
+        syslog(LOG_INFO, "%s: %s", level_str, msg);
+    } else if (level & s->log_level) {
+        g_get_current_time(&time);
+        fprintf(s->log_file,
+                "%lu.%lu: %s: %s\n", time.tv_sec, time.tv_usec, level_str, msg);
+        fflush(s->log_file);
+    }
+}
+
+static void become_daemon(const char *pidfile)
+{
+    pid_t pid, sid;
+    int pidfd;
+    char *pidstr = NULL;
+
+    pid = fork();
+    if (pid < 0) {
+        exit(EXIT_FAILURE);
+    }
+    if (pid > 0) {
+        exit(EXIT_SUCCESS);
+    }
+
+    pidfd = open(pidfile, O_CREAT|O_RDWR, S_IRUSR|S_IWUSR);
+    if (!pidfd || lockf(pidfd, F_TLOCK, 0)) {
+        g_error("Cannot lock pid file");
+    }
+
+    if (ftruncate(pidfd, 0) || lseek(pidfd, 0, SEEK_SET)) {
+        g_critical("Cannot truncate pid file");
+        goto fail;
+    }
+    if (asprintf(&pidstr, "%d", getpid()) == -1) {
+        g_critical("Cannot allocate memory");
+        goto fail;
+    }
+    if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
+        g_critical("Failed to write pid file");
+        goto fail;
+    }
+
+    umask(0);
+    sid = setsid();
+    if (sid < 0) {
+        goto fail;
+    }
+    if ((chdir("/")) < 0) {
+        goto fail;
+    }
+
+    close(STDIN_FILENO);
+    close(STDOUT_FILENO);
+    close(STDERR_FILENO);
+    return;
+
+fail:
+    if (pidstr) {
+        free(pidstr);
+    }
+    unlink(pidfile);
+    g_error("failed to daemonize");
+}
+
+static int conn_channel_send_payload(GIOChannel *channel, QObject *payload)
+{
+    gsize count, written = 0;
+    int ret;
+    const char *buf;
+    QString *payload_qstr;
+    GIOStatus status;
+    GError *err = NULL;
+
+    if (!payload || !channel) {
+        ret = -EINVAL;
+        goto out;
+    }
+
+    payload_qstr = qobject_to_json(payload);
+    if (!payload_qstr) {
+        ret = -EINVAL;
+        goto out;
+    }
+
+    buf = qstring_get_str(payload_qstr);
+    count = strlen(buf);
+
+    while (count) {
+        g_debug("sending data, count: %d", (int)count);
+        status = g_io_channel_write_chars(channel, buf, count, &written, &err);
+        if (err != NULL) {
+            g_warning("error writing payload to channel: %s", err->message);
+            ret = err->code;
+            goto out_free;
+        }
+        if (status == G_IO_STATUS_NORMAL) {
+            count -= written;
+        } else if (status == G_IO_STATUS_ERROR || status == G_IO_STATUS_EOF) {
+            ret = -EPIPE;
+            goto out_free;
+        }
+    }
+
+    status = g_io_channel_write_chars(channel, (char *)"\n", 1, &written, &err);
+    if (err != NULL) {
+        g_warning("error sending newline: %s", err->message);
+        ret = err->code;
+        goto out_free;
+    } else if (status == G_IO_STATUS_ERROR || status == G_IO_STATUS_EOF) {
+        ret = -EPIPE;
+        goto out_free;
+    }
+
+    g_io_channel_flush(channel, &err);
+    if (err != NULL) {
+        g_warning("error flushing payload: %s", err->message);
+        ret = err->code;
+        goto out_free;
+    }
+
+    ret = 0;
+
+out_free:
+    QDECREF(payload_qstr);
+    if (err != NULL) {
+        g_error_free(err);
+    }
+out:
+    return ret;
+}
+
+static void process_command(GAState *s, QDict *req)
+{
+    QObject *rsp = NULL;
+    Error *err = NULL;
+    int ret;
+
+    g_assert(req);
+    g_debug("processing command");
+    rsp = qmp_dispatch(QOBJECT(req));
+    if (rsp) {
+        if (err) {
+            g_warning("command failed: %s", error_get_pretty(err));
+        }
+        ret = conn_channel_send_payload(s->conn_channel, rsp);
+        if (ret) {
+            g_warning("error sending payload: %s", strerror(ret));
+        }
+        qobject_decref(rsp);
+    } else {
+        g_warning("error getting response");
+        if (err) {
+            g_warning("dispatch failed: %s", error_get_pretty(err));
+        }
+    }
+}
+
+/* handle requests/control events coming in over the channel */
+static void process_event(JSONMessageParser *parser, QList *tokens)
+{
+    GAState *s = container_of(parser, GAState, parser);
+    QObject *obj;
+    QDict *qdict;
+    Error *err = NULL;
+
+    g_assert(s && parser);
+
+    g_debug("process_event: called");
+    obj = json_parser_parse_err(tokens, NULL, &err);
+    if (!obj || qobject_type(obj) != QTYPE_QDICT) {
+        g_warning("failed to parse event");
+        return;
+    } else {
+        g_debug("parse successful");
+        qdict = qobject_to_qdict(obj);
+        g_assert(qdict);
+    }
+
+    /* handle host->guest commands */
+    if (qdict_haskey(qdict, "execute")) {
+        process_command(s, qdict);
+    } else {
+        g_warning("unrecognized payload format, ignoring");
+    }
+
+    QDECREF(qdict);
+}
+
+static gboolean conn_channel_read(GIOChannel *channel, GIOCondition condition,
+                                  gpointer data)
+{
+    GAState *s = data;
+    gchar buf[1024];
+    gsize count;
+    GError *err = NULL;
+    memset(buf, 0, 1024);
+    GIOStatus status = g_io_channel_read_chars(channel, buf, 1024,
+                                               &count, &err);
+    if (err != NULL) {
+        g_warning("error reading channel: %s", err->message);
+        conn_channel_close(s);
+        g_error_free(err);
+        return false;
+    }
+    switch (status) {
+    case G_IO_STATUS_ERROR:
+        g_warning("problem");
+        return false;
+    case G_IO_STATUS_NORMAL:
+        g_debug("read data, count: %d, data: %s", (int)count, buf);
+        json_message_parser_feed(&s->parser, (char *)buf, (int)count);
+    case G_IO_STATUS_AGAIN:
+        /* virtio causes us to spin here when no process is attached to
+         * host-side chardev. sleep a bit to mitigate this
+         */
+        if (s->virtio) {
+            usleep(100*1000);
+        }
+        return true;
+    case G_IO_STATUS_EOF:
+        g_debug("received EOF");
+        conn_channel_close(s);
+        if (s->virtio) {
+            return true;
+        }
+        return false;
+    default:
+        g_warning("unknown channel read status, closing");
+        conn_channel_close(s);
+        return false;
+    }
+    return true;
+}
+
+static int conn_channel_add(GAState *s, int fd)
+{
+    GIOChannel *conn_channel;
+    guint conn_id;
+    GError *err = NULL;
+
+    g_assert(s && !s->conn_channel);
+    conn_channel = g_io_channel_unix_new(fd);
+    g_assert(conn_channel);
+    g_io_channel_set_encoding(conn_channel, NULL, &err);
+    if (err != NULL) {
+        g_warning("error setting channel encoding to binary");
+        g_error_free(err);
+        return -1;
+    }
+    conn_id = g_io_add_watch(conn_channel, G_IO_IN | G_IO_HUP,
+                             conn_channel_read, s);
+    if (err != NULL) {
+        g_warning("error adding io watch: %s", err->message);
+        g_error_free(err);
+        return -1;
+    }
+    s->conn_channel = conn_channel;
+    s->conn_id = conn_id;
+    return 0;
+}
+
+static gboolean listen_channel_accept(GIOChannel *channel,
+                                      GIOCondition condition, gpointer data)
+{
+    GAState *s = data;
+    GError *err = NULL;
+    g_assert(channel != NULL);
+    int ret;
+    bool accepted = false;
+
+    s->conn_sock = g_socket_accept(s->listen_sock, NULL, &err);
+    if (err != NULL) {
+        g_warning("error converting fd to gsocket: %s", err->message);
+        g_error_free(err);
+        goto out;
+    }
+    ret = conn_channel_add(s, g_socket_get_fd(s->conn_sock));
+    if (ret) {
+        g_warning("error setting up connection");
+        goto out;
+    }
+    accepted = true;
+
+out:
+    /* only accept 1 connection at a time */
+    return !accepted;
+}
+
+/* start polling for readable events on listen fd, listen_fd=0
+ * indicates we should use the existing s->listen_channel
+ */
+static int listen_channel_add(GAState *s, int listen_fd)
+{
+    GError *err = NULL;
+    guint listen_id;
+
+    if (listen_fd) {
+        s->listen_channel = g_io_channel_unix_new(listen_fd);
+        if (s->listen_sock) {
+            g_object_unref(s->listen_sock);
+        }
+        s->listen_sock = g_socket_new_from_fd(listen_fd, &err);
+        if (err != NULL) {
+            g_warning("error converting fd to gsocket: %s", err->message);
+            g_error_free(err);
+            return -1;
+        }
+    }
+    listen_id = g_io_add_watch(s->listen_channel, G_IO_IN,
+                               listen_channel_accept, s);
+    if (err != NULL) {
+        g_warning("error adding io watch: %s", err->message);
+        g_error_free(err);
+        return -1;
+    }
+    return 0;
+}
+
+/* cleanup state for closed connection/session, start accepting new
+ * connections if we're in listening mode
+ */
+static void conn_channel_close(GAState *s)
+{
+    if (strcmp(s->method, "unix-listen") == 0) {
+        g_io_channel_shutdown(s->conn_channel, true, NULL);
+        g_object_unref(s->conn_sock);
+        s->conn_sock = NULL;
+        listen_channel_add(s, 0);
+    } else if (strcmp(s->method, "virtio-serial") == 0) {
+        /* we spin on EOF for virtio-serial, so back off a bit. also,
+         * dont close the connection in this case, it'll resume normal
+         * operation when another process connects to host chardev
+         */
+        usleep(100*1000);
+        goto out_noclose;
+    }
+    g_io_channel_unref(s->conn_channel);
+    s->conn_channel = NULL;
+    s->conn_id = 0;
+out_noclose:
+    return;
+}
+
+static void init_guest_agent(GAState *s)
+{
+    struct termios tio;
+    int ret, fd;
+
+    if (s->method == NULL) {
+        /* try virtio-serial as our default */
+        s->method = "virtio-serial";
+    }
+
+    if (s->path == NULL) {
+        if (strcmp(s->method, "virtio-serial") != 0) {
+            g_error("must specify a path for this channel");
+        }
+        /* try the default path for the virtio-serial port */
+        s->path = QGA_VIRTIO_PATH_DEFAULT;
+    }
+
+    if (strcmp(s->method, "virtio-serial") == 0) {
+        s->virtio = true;
+        fd = qemu_open(s->path, O_RDWR);
+        if (fd == -1) {
+            g_error("error opening channel: %s", strerror(errno));
+        }
+        ret = fcntl(fd, F_GETFL);
+        if (ret < 0) {
+            g_error("error getting channel flags: %s", strerror(errno));
+        }
+        ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK | O_ASYNC);
+        if (ret < 0) {
+            g_error("error setting channel flags: %s", strerror(errno));
+        }
+        ret = conn_channel_add(s, fd);
+        if (ret) {
+            g_error("error adding channel to main loop");
+        }
+    } else if (strcmp(s->method, "isa-serial") == 0) {
+        fd = qemu_open(s->path, O_RDWR | O_NOCTTY);
+        if (fd == -1) {
+            g_error("error opening channel: %s", strerror(errno));
+        }
+        tcgetattr(fd, &tio);
+        /* set up serial port for non-canonical, dumb byte streaming */
+        tio.c_iflag &= ~(IGNBRK | BRKINT | IGNPAR | PARMRK | INPCK | ISTRIP |
+                         INLCR | IGNCR | ICRNL | IXON | IXOFF | IXANY |
+                         IMAXBEL);
+        tio.c_oflag = 0;
+        tio.c_lflag = 0;
+        tio.c_cflag |= QGA_BAUDRATE_DEFAULT;
+        /* 1 available byte min or reads will block (we'll set non-blocking
+         * elsewhere, else we have to deal with read()=0 instead)
+         */
+        tio.c_cc[VMIN] = 1;
+        tio.c_cc[VTIME] = 0;
+        /* flush everything waiting for read/xmit, it's garbage at this point */
+        tcflush(fd, TCIFLUSH);
+        tcsetattr(fd, TCSANOW, &tio);
+        ret = conn_channel_add(s, fd);
+        if (ret) {
+            g_error("error adding channel to main loop");
+        }
+    } else if (strcmp(s->method, "unix-listen") == 0) {
+        fd = unix_listen(s->path, NULL, strlen(s->path));
+        if (fd == -1) {
+            g_error("error opening path: %s", strerror(errno));
+        }
+        ret = listen_channel_add(s, fd);
+        if (ret) {
+            g_error("error binding/listening to specified socket");
+        }
+    } else {
+        g_error("unsupported channel method/type: %s", s->method);
+    }
+
+    json_message_parser_init(&s->parser, process_event);
+    s->main_loop = g_main_loop_new(NULL, false);
+}
+
+int main(int argc, char **argv)
+{
+    const char *sopt = "hVvdc:p:l:f:";
+    const char *method = NULL, *path = NULL, *pidfile = QGA_PIDFILE_DEFAULT;
+    struct option lopt[] = {
+        { "help", 0, NULL, 'h' },
+        { "version", 0, NULL, 'V' },
+        { "logfile", 0, NULL, 'l' },
+        { "pidfile", 0, NULL, 'f' },
+        { "verbose", 0, NULL, 'v' },
+        { "channel", 0, NULL, 'c' },
+        { "path", 0, NULL, 'p' },
+        { "daemonize", 0, NULL, 'd' },
+        { NULL, 0, NULL, 0 }
+    };
+    int opt_ind = 0, ch, daemonize = 0;
+    GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
+    FILE *log_file = stderr;
+    GAState *s;
+
+    g_type_init();
+    g_thread_init(NULL);
+
+    while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
+        switch (ch) {
+        case 'c':
+            method = optarg;
+            break;
+        case 'p':
+            path = optarg;
+            break;
+        case 'l':
+            log_file = fopen(optarg, "a");
+            if (!log_file) {
+                g_error("unable to open specified log file: %s",
+                        strerror(errno));
+            }
+            break;
+        case 'f':
+            pidfile = optarg;
+            break;
+        case 'v':
+            /* enable all log levels */
+            log_level = G_LOG_LEVEL_MASK;
+            break;
+        case 'V':
+            printf("QEMU Guest Agent %s\n", QGA_VERSION);
+            return 0;
+        case 'd':
+            daemonize = 1;
+            break;
+        case 'h':
+            usage(argv[0]);
+            return 0;
+        case '?':
+            g_error("Unknown option, try '%s --help' for more information.",
+                    argv[0]);
+        }
+    }
+
+    if (daemonize) {
+        g_debug("starting daemon");
+        become_daemon(pidfile);
+    }
+
+    s = g_malloc0(sizeof(GAState));
+    s->conn_id = 0;
+    s->conn_channel = NULL;
+    s->path = path;
+    s->method = method;
+    s->command_state = ga_command_state_new();
+    ga_command_state_init(s, s->command_state);
+    ga_command_state_init_all(s->command_state);
+    s->log_file = log_file;
+    s->log_level = log_level;
+    g_log_set_default_handler(ga_log, s);
+    g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
+    s->logging_enabled = true;
+
+    module_call_init(MODULE_INIT_QAPI);
+    init_guest_agent(s);
+
+    g_main_loop_run(s->main_loop);
+
+    ga_command_state_cleanup_all(s->command_state);
+
+    return 0;
+}
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index 688f120..66d1729 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -15,6 +15,7 @@ 
 
 #define QGA_VERSION "1.0"
 
+typedef struct GAState GAState;
 typedef struct GACommandState GACommandState;
 
 void ga_command_state_add(GACommandState *cs,
@@ -23,3 +24,6 @@  void ga_command_state_add(GACommandState *cs,
 void ga_command_state_init_all(GACommandState *cs);
 void ga_command_state_cleanup_all(GACommandState *cs);
 GACommandState *ga_command_state_new(void);
+bool ga_logging_enabled(GAState *s);
+void ga_disable_logging(GAState *s);
+void ga_enable_logging(GAState *s);