Re: [Qemu-devel] [PATCH v6 2/4] guest agent: qemu-ga daemon

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

On 07/08/2011 09:36 AM, Luiz Capitulino wrote:
> On Tue,  5 Jul 2011 08:21:38 -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
>
> I think you meant -f /var/run/qemu-guest-agent.pid
>

Yup, sorry.

>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>>   Makefile               |   10 +-
>>   qemu-ga.c              |  651 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   qga/guest-agent-core.h |    4 +
>>   3 files changed, 661 insertions(+), 4 deletions(-)
>>   create mode 100644 qemu-ga.c
>>
>> diff --git a/Makefile b/Makefile
>> index 6c3ba71..b2e8593 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -140,7 +140,7 @@ endif
>>   ######################################################################
>>
>>   qemu-img.o: qemu-img-cmds.h
>> -qemu-img.o qemu-tool.o qemu-nbd.o qemu-io.o cmd.o: $(GENERATED_HEADERS)
>> +qemu-img.o qemu-tool.o qemu-nbd.o qemu-io.o cmd.o qemu-ga.o: $(GENERATED_HEADERS)
>>
>>   qemu-img$(EXESUF): qemu-img.o qemu-tool.o qemu-error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o
>>
>> @@ -163,7 +163,7 @@ check-qfloat: check-qfloat.o qfloat.o $(CHECK_PROG_DEPS)
>>   check-qjson: check-qjson.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o qjson.o json-streamer.o json-lexer.o json-parser.o error.o qerror.o qemu-error.o $(CHECK_PROG_DEPS)
>>
>>   qapi-dir := qapi-generated
>> -$(qapi-obj-y) test-visitor.o test-qmp-commands.o: QEMU_CFLAGS += -I $(qapi-dir)
>> +$(qapi-obj-y) test-visitor.o test-qmp-commands.o qemu-ga$(EXESUF): QEMU_CFLAGS += -I $(qapi-dir)
>>
>>   $(qapi-dir)/test-qapi-types.c: $(qapi-dir)/test-qapi-types.h
>>   $(qapi-dir)/test-qapi-types.h: $(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-types.py
>> @@ -183,13 +183,15 @@ test-qmp-commands: test-qmp-commands.o qfloat.o qint.o qdict.o qstring.o qlist.o
>>
>>   QGALIB=qga/guest-agent-command-state.o
>>
>> +qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o module.o qapi/qmp-dispatch.o qapi/qmp-registry.o
>> +
>>   QEMULIBS=libhw32 libhw64 libuser libdis libdis-user
>>
>>   clean:
>>   # avoid old build problems by removing potentially incorrect old files
>>   	rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h gen-op-arm.h
>>   	rm -f qemu-options.def
>> -	rm -f *.o *.d *.a *.lo $(TOOLS) TAGS cscope.* *.pod *~ */*~
>> +	rm -f *.o *.d *.a *.lo $(TOOLS) qemu-ga TAGS cscope.* *.pod *~ */*~
>>   	rm -Rf .libs
>>   	rm -f slirp/*.o slirp/*.d audio/*.o audio/*.d block/*.o block/*.d net/*.o net/*.d fsdev/*.o fsdev/*.d ui/*.o ui/*.d qapi/*.o qapi/*.d qga/*.o qga/*.d
>>   	rm -f qemu-img-cmds.h
>> @@ -385,4 +387,4 @@ tarbin:
>>   	$(mandir)/man8/qemu-nbd.8
>>
>>   # Include automatically generated dependency files
>> --include $(wildcard *.d audio/*.d slirp/*.d block/*.d net/*.d ui/*.d qapi/*.d)
>> +-include $(wildcard *.d audio/*.d slirp/*.d block/*.d net/*.d ui/*.d qapi/*.d qga/*.d)
>> diff --git a/qemu-ga.c b/qemu-ga.c
>> new file mode 100644
>> index 0000000..649c16a
>> --- /dev/null
>> +++ b/qemu-ga.c
>> @@ -0,0 +1,651 @@
>> +/*
>> + * 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 "module.h"
>> +#include "signal.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 {
>> +    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 struct GAState *ga_state;
>> +
>> +static void quit_handler(int sig)
>> +{
>> +    g_debug("recieved signal num %d, quitting");
>> +
>> +    if (g_main_loop_is_running(ga_state->main_loop)) {
>> +        g_main_loop_quit(ga_state->main_loop);
>> +    }
>> +}
>> +
>> +static void register_signal_handlers(void)
>> +{
>> +    struct sigaction sigact;
>> +    int ret;
>> +
>> +    sigact.sa_handler = quit_handler;
>> +
>> +    ret = sigaction(SIGINT,&sigact, NULL);
>> +    if (ret == -1) {
>> +        g_error("error configuring signal handler: %s", strerror(errno));
>> +    }
>> +    ret = sigaction(SIGTERM,&sigact, NULL);
>> +    if (ret == -1) {
>> +        g_error("error configuring signal handler: %s", strerror(errno));
>> +    }
>> +}
>> +
>> +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_WRONLY|O_EXCL, S_IRUSR|S_IWUSR);
>> +    if (pidfd == -1) {
>> +        g_error("Cannot create pid file, %s", strerror(errno));
>> +    }
>> +
>> +    if (asprintf(&pidstr, "%d", getpid()) == -1) {
>> +        g_critical("Cannot allocate memory");
>> +        goto fail;
>> +    }
>> +    if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
>> +        free(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);
>> +    free(pidstr);
>> +    return;
>> +
>> +fail:
>> +    unlink(pidfile);
>> +    g_error("failed to daemonize");
>> +}
>> +
>> +static int conn_channel_send_buf(GIOChannel *channel, const char *buf,
>> +                                 gsize count)
>> +{
>> +    GError *err = NULL;
>> +    gsize written = 0;
>> +    GIOStatus status;
>> +
>> +    while (count) {
>> +        status = g_io_channel_write_chars(channel, buf, count,&written,&err);
>> +        g_debug("sending data, count: %d", (int)count);
>> +        if (err != NULL) {
>> +            g_warning("error sending newline: %s", err->message);
>> +            return err->code;
>> +        }
>> +        if (status == G_IO_STATUS_ERROR || status == G_IO_STATUS_EOF) {
>> +            return -EPIPE;
>> +        }
>> +
>> +        if (status == G_IO_STATUS_NORMAL) {
>> +            count -= written;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int conn_channel_send_payload(GIOChannel *channel, QObject *payload)
>> +{
>> +    int ret = 0;
>> +    const char *buf;
>> +    QString *payload_qstr;
>> +    GError *err = NULL;
>> +
>> +    g_assert(payload&&  channel);
>> +
>> +    payload_qstr = qobject_to_json(payload);
>> +    if (!payload_qstr) {
>> +        return -EINVAL;
>> +    }
>
> You can do:
>
>   qstring_append_chr(payload_qstr, '\n');
>
> so that you avoid the additional conn_channel_send_buf() call below.
>

Nice!

>> +
>> +    buf = qstring_get_str(payload_qstr);
>> +    ret = conn_channel_send_buf(channel, buf, strlen(buf));
>> +    if (ret) {
>> +        goto out_free;
>> +    }
>> +
>> +    ret = conn_channel_send_buf(channel, "\n", 1);
>> +    if (ret) {
>> +        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;
>> +    }
>> +
>> +out_free:
>> +    QDECREF(payload_qstr);
>> +    if (err) {
>> +        g_error_free(err);
>> +    }
>> +    return ret;
>> +}
>> +
>> +static void process_command(GAState *s, QDict *req)
>> +{
>> +    QObject *rsp = NULL;
>> +    int ret;
>> +
>> +    g_assert(req);
>> +    g_debug("processing command");
>> +    rsp = qmp_dispatch(QOBJECT(req));
>> +    if (rsp) {
>> +        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");
>> +    }
>> +}
>> +
>> +/* 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) {
>> +        qobject_decref(obj);
>> +        g_warning("failed to parse event");
>
> No error is returned to the client, is this intended?
>

Hmm, nope... originally the errors got rolled into the response in 
qmp_dispatch(). But when I reworked qmp_dispatch() to take a pre-parsed 
QDict instead of a token list this functionality was lost..

Which makes me wonder if I should move the parsing back into 
qmp_dispatch()... I'll take a look. But yah, these errors should get 
returned.

>> +        return;
>> +    }
>> +
>> +    if (err) {
>> +        error_free(err);
>> +        g_warning("failed to parse event: %s", error_get_pretty(err));
>
> Same here.
>
>> +        return;
>> +    }
>> +
>> +    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 {
>
> And here.
>
>> +        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, new==true
>> + * indicates we should use the existing s->listen_channel
>> + */
>> +static int listen_channel_add(GAState *s, int listen_fd, bool new)
>> +{
>> +    GError *err = NULL;
>> +    guint listen_id;
>> +
>> +    if (new) {
>> +        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, false);
>> +    } 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));
>> +        }
>
> Can't O_NONBLOCK and O_ASYNC be set as open() flags?
>
>> +        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, true);
>> +        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[] = {
>
> Can be const.
>
>> +        { "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;
>> +
>> +    while ((ch = getopt_long(argc, argv, sopt, lopt,&opt_ind)) != -1) {
>> +        switch (ch) {
>> +        case 'c':
>> +            method = optarg;
>
> Why don't you call this option 'm' ('method') then?
>

That's probably better.

>> +            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]);
>
> g_error() documentation from:
>
>   http://developer.gnome.org/glib/2.29/glib-Message-Logging.html#g-error
>
> Says:
>
>   "This function will result in a core dump; don't use it for errors you expect.
>    Using this function indicates a bug in your program, i.e. an assertion failure."
>
> But I see it's being used beyond assertions.
>

I've been using it to bail on unrecoverable error states. Might be a bit 
extreme for unknown options though.

>> +        }
>> +    }
>> +
>> +    if (daemonize) {
>> +        g_debug("starting daemon");
>> +        become_daemon(pidfile);
>> +    }
>> +
>> +    g_type_init();
>> +    g_thread_init(NULL);
>> +
>> +    s = qemu_mallocz(sizeof(GAState));
>> +    s->conn_id = 0;
>> +    s->conn_channel = NULL;
>> +    s->path = path;
>> +    s->method = method;
>> +    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;
>> +    ga_state = s;
>> +
>> +    module_call_init(MODULE_INIT_QAPI);
>> +    init_guest_agent(ga_state);
>> +    register_signal_handlers();
>> +
>> +    g_main_loop_run(ga_state->main_loop);
>> +
>> +    unlink(pidfile);
>> +
>> +    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);
>