Re: [Qemu-devel] [PATCH v5 2/5] guest agent: qemu-ga daemon

[Michael Roth <mdroth@linux.vnet.ibm.com>]

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);
>>>
>>
>