QEMU Guest Agent (virtagent) v5

Signed-off-by Michael Roth <[email protected]>

---
 qga/guest-agent-command-state.c |   73 +++++++++++++++++++++++++++++++++++++++
 qga/guest-agent-core.h          |   25 +++++++++++++

 2 files changed, 98 insertions(+), 0 deletions(-)
 create mode 100644 qga/guest-agent-command-state.c
 create mode 100644 qga/guest-agent-core.h

qga/guest-agent-command-state.c new file +20 -0

new file mode 100644
index 0000000..969da23
@@ -0,0 +1,73 @@
+/*
+ * QEMU Guest Agent command state interfaces
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Michael Roth      <[email protected]>
+ *
+ * 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 <glib.h>
+#include "qga/guest-agent-core.h"
+
+struct GACommandState {
+    GSList *groups;
+};
+
+typedef struct GACommandGroup {
+    void (*init)(void);

qga/guest-agent-command-state.c new file +40 -0

+}
+
+static void ga_command_group_init(gpointer opaque, gpointer unused)
+{
+    GACommandGroup *cg = opaque;
+
+    g_assert(cg);
+    if (cg->init) {
+        cg->init();
+    }
+}
+
+void ga_command_state_init_all(GACommandState *cs)
+{
+    g_assert(cs);
+    g_slist_foreach(cs->groups, ga_command_group_init, NULL);
+}
+
+static void ga_command_group_cleanup(gpointer opaque, gpointer unused)
+{
+    GACommandGroup *cg = opaque;
+
+    g_assert(cg);
+    if (cg->cleanup) {
+        cg->cleanup();
+    }
+}
+
+void ga_command_state_cleanup_all(GACommandState *cs)
+{
+    g_assert(cs);
+    g_slist_foreach(cs->groups, ga_command_group_cleanup, NULL);
+}
+
+GACommandState *ga_command_state_new(void)
+{
+    GACommandState *cs = g_malloc0(sizeof(GACommandState));
+    cs->groups = NULL;
+    return cs;
+}

qga/guest-agent-core.h new file +25 -0

new file mode 100644
index 0000000..688f120
@@ -0,0 +1,25 @@
+/*
+ * QEMU Guest Agent core declarations
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Adam Litke        <[email protected]>
+ *  Michael Roth      <[email protected]>
+ *
+ * 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 "qapi/qmp-core.h"
+#include "qemu-common.h"
+
+#define QGA_VERSION "1.0"
+
+typedef struct GACommandState GACommandState;
+
+void ga_command_state_add(GACommandState *cs,
+                          void (*init)(void),
+                          void (*cleanup)(void));
+void ga_command_state_init_all(GACommandState *cs);
+void ga_command_state_cleanup_all(GACommandState *cs);
+GACommandState *ga_command_state_new(void);

Quoted 1 line · On Tue, 14 Jun 2011 15:06:21 -0500 Michael Roth<[email protected]>…

Quoted 4 lines

Signed-off-by Michael Roth<[email protected]>

---
  qga/guest-agent-command-state.c |   73 +++++++++++++++++++++++++++++++++++++++
  qga/guest-agent-core.h          |   25 +++++++++++++

It's not possible to build this. I see that the last patch has the Makefile changes, but what we want is that a patch introducing a .c file also updates the Makefile, so that the .c file can be built.

Quoted 30 lines

  2 files changed, 98 insertions(+), 0 deletions(-)

create mode 100644 qga/guest-agent-command-state.c create mode 100644 qga/guest-agent-core.h

qga/guest-agent-command-state.c new file +20 -0

new file mode 100644
index 0000000..969da23
@@ -0,0 +1,73 @@
+/*
+ * QEMU Guest Agent command state interfaces
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Michael Roth<[email protected]>
+ *
+ * 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<glib.h>
+#include "qga/guest-agent-core.h"
+
+struct GACommandState {
+    GSList *groups;
+};
+
+typedef struct GACommandGroup {
+    void (*init)(void);

qga/guest-agent-command-state.c line 30

Quoted 10 lines

qga/guest-agent-command-state.c new file +10 -0

+    void (*cleanup)(void);
+} GACommandGroup;
+
+/* handle init/cleanup for stateful guest commands */
+
+void ga_command_state_add(GACommandState *cs,
+                          void (*init)(void),
+                          void (*cleanup)(void))
+{
+    GACommandGroup *cg = g_malloc0(sizeof(GACommandGroup));

This is linked against qemu-ga only, right? Otherwise I think we should use qemu_mallocz(). And you probably want to check against NULL.

qga/guest-agent-command-state.c line 33

Quoted 3 lines

qga/guest-agent-command-state.c new file +3 -0

+    cg->init = init;
+    cg->cleanup = cleanup;
+    cs->groups = g_slist_append(cs->groups, cg);

Not that I'm asking to you change anything here, but we're going to get a funny mix with QObjects :)

Quoted 71 lines

qga/guest-agent-command-state.c new file +40 -0

+}
+
+static void ga_command_group_init(gpointer opaque, gpointer unused)
+{
+    GACommandGroup *cg = opaque;
+
+    g_assert(cg);
+    if (cg->init) {
+        cg->init();
+    }
+}
+
+void ga_command_state_init_all(GACommandState *cs)
+{
+    g_assert(cs);
+    g_slist_foreach(cs->groups, ga_command_group_init, NULL);
+}
+
+static void ga_command_group_cleanup(gpointer opaque, gpointer unused)
+{
+    GACommandGroup *cg = opaque;
+
+    g_assert(cg);
+    if (cg->cleanup) {
+        cg->cleanup();
+    }
+}
+
+void ga_command_state_cleanup_all(GACommandState *cs)
+{
+    g_assert(cs);
+    g_slist_foreach(cs->groups, ga_command_group_cleanup, NULL);
+}
+
+GACommandState *ga_command_state_new(void)
+{
+    GACommandState *cs = g_malloc0(sizeof(GACommandState));
+    cs->groups = NULL;
+    return cs;
+}

qga/guest-agent-core.h new file +25 -0

new file mode 100644
index 0000000..688f120
@@ -0,0 +1,25 @@
+/*
+ * QEMU Guest Agent core declarations
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Adam Litke<[email protected]>
+ *  Michael Roth<[email protected]>
+ *
+ * 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 "qapi/qmp-core.h"
+#include "qemu-common.h"
+
+#define QGA_VERSION "1.0"
+
+typedef struct GACommandState GACommandState;
+
+void ga_command_state_add(GACommandState *cs,
+                          void (*init)(void),
+                          void (*cleanup)(void));
+void ga_command_state_init_all(GACommandState *cs);
+void ga_command_state_cleanup_all(GACommandState *cs);
+GACommandState *ga_command_state_new(void);

Quoted 4 lines · Luiz Capitulino

Quoted 1 line · On Tue, 14 Jun 2011 15:06:21 -0500 Michael Roth<[email protected]>…

Quoted 9 lines

Quoted 4 lines

Signed-off-by Michael Roth<[email protected]>

---
  qga/guest-agent-command-state.c |   73 +++++++++++++++++++++++++++++++++++++++
  qga/guest-agent-core.h          |   25 +++++++++++++

It's not possible to build this. I see that the last patch has the Makefile changes, but what we want is that a patch introducing a .c file also updates the Makefile, so that the .c file can be built.

I made it a point to test build each C file as introduced, but I'm not sure there's a way of doing incremental builds without modifying the Makefile manually anyway, since we'll be missing a main() function until qemu-ga.c. Or maybe I'm just missing something...any ideas?

Quoted 45 lines

Quoted 31 lines

  2 files changed, 98 insertions(+), 0 deletions(-)

create mode 100644 qga/guest-agent-command-state.c create mode 100644 qga/guest-agent-core.h

qga/guest-agent-command-state.c new file +20 -0

new file mode 100644
index 0000000..969da23
@@ -0,0 +1,73 @@
+/*
+ * QEMU Guest Agent command state interfaces
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Michael Roth<[email protected]>
+ *
+ * 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<glib.h>
+#include "qga/guest-agent-core.h"
+
+struct GACommandState {
+    GSList *groups;
+};
+
+typedef struct GACommandGroup {
+    void (*init)(void);

qga/guest-agent-command-state.c line 30

Quoted 10 lines

qga/guest-agent-command-state.c new file +10 -0

+    void (*cleanup)(void);
+} GACommandGroup;
+
+/* handle init/cleanup for stateful guest commands */
+
+void ga_command_state_add(GACommandState *cs,
+                          void (*init)(void),
+                          void (*cleanup)(void))
+{
+    GACommandGroup *cg = g_malloc0(sizeof(GACommandGroup));

This is linked against qemu-ga only, right? Otherwise I think we should use qemu_mallocz(). And you probably want to check against NULL.

Quoted 7 lines

qga/guest-agent-command-state.c line 33

Quoted 3 lines

qga/guest-agent-command-state.c new file +3 -0

+    cg->init = init;
+    cg->cleanup = cleanup;
+    cs->groups = g_slist_append(cs->groups, cg);

Not that I'm asking to you change anything here, but we're going to get a funny mix with QObjects :)

glib is a hard dependency here so i tried to stick with it where possible, since there's a lot of code where i'm kinda forced to use GObject stuff, like all the GIO functions for instance (GObject + GError
+ g_free etc). I think fsfreeze still uses qlist/qemu_free/etc, best I can do is fix that up to relegate all the non-glib stuff to qemu-ga.c and the qapi/qmp stuff it pulls in.

Quoted 73 lines

Quoted 72 lines

qga/guest-agent-command-state.c new file +40 -0

+}
+
+static void ga_command_group_init(gpointer opaque, gpointer unused)
+{
+    GACommandGroup *cg = opaque;
+
+    g_assert(cg);
+    if (cg->init) {
+        cg->init();
+    }
+}
+
+void ga_command_state_init_all(GACommandState *cs)
+{
+    g_assert(cs);
+    g_slist_foreach(cs->groups, ga_command_group_init, NULL);
+}
+
+static void ga_command_group_cleanup(gpointer opaque, gpointer unused)
+{
+    GACommandGroup *cg = opaque;
+
+    g_assert(cg);
+    if (cg->cleanup) {
+        cg->cleanup();
+    }
+}
+
+void ga_command_state_cleanup_all(GACommandState *cs)
+{
+    g_assert(cs);
+    g_slist_foreach(cs->groups, ga_command_group_cleanup, NULL);
+}
+
+GACommandState *ga_command_state_new(void)
+{
+    GACommandState *cs = g_malloc0(sizeof(GACommandState));
+    cs->groups = NULL;
+    return cs;
+}

qga/guest-agent-core.h new file +25 -0

new file mode 100644
index 0000000..688f120
@@ -0,0 +1,25 @@
+/*
+ * QEMU Guest Agent core declarations
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Adam Litke<[email protected]>
+ *  Michael Roth<[email protected]>
+ *
+ * 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 "qapi/qmp-core.h"
+#include "qemu-common.h"
+
+#define QGA_VERSION "1.0"
+
+typedef struct GACommandState GACommandState;
+
+void ga_command_state_add(GACommandState *cs,
+                          void (*init)(void),
+                          void (*cleanup)(void));
+void ga_command_state_init_all(GACommandState *cs);
+void ga_command_state_cleanup_all(GACommandState *cs);
+GACommandState *ga_command_state_new(void);

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 <[email protected]>

---
 qemu-ga.c              |  631 ++++++++++++++++++++++++++++++++++++++++++++++++
 qga/guest-agent-core.h |    4 +
 2 files changed, 635 insertions(+), 0 deletions(-)
 create mode 100644 qemu-ga.c

qemu-ga.c new file +26 -0

new file mode 100644
index 0000000..df08d8c
@@ -0,0 +1,631 @@
+/*
+ * QEMU Guest Agent
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Adam Litke        <[email protected]>
+ *  Michael Roth      <[email protected]>
+ *
+ * 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"

qemu-ga.c new file +63 -0

+    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 <[email protected]>\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;
+}

qemu-ga.c new file +28 -0

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

qemu-ga.c new file +9 -0

+
+    umask(0);
+    sid = setsid();
+    if (sid < 0) {
+        goto fail;
+    }
+    if ((chdir("/")) < 0) {
+        goto fail;
+    }

qemu-ga.c new file +7 -0

+    unlink(pidfile);
+    g_error("failed to daemonize");
+}
+
+static int conn_channel_send_payload(GIOChannel *channel, QObject *payload)
+{
+    gsize count, written = 0;

qemu-ga.c new file +19 -0

+
+    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;
+        }
+    }

qemu-ga.c new file +5 -0

+
+    g_io_channel_flush(channel, &err);
+    if (err != NULL) {
+        g_warning("error flushing payload: %s", err->message);
+        ret = err->code;

qemu-ga.c new file +5 -0

+out:
+    return ret;
+}
+
+static void process_command(GAState *s, QDict *req)

qemu-ga.c new file +19 -0

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

qga/guest-agent-core.h +4 -0

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

Quoted 7 lines · On Tue, 14 Jun 2011 15:06:22 -0500 Michael Roth<[email protected]>…

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:

Quoted 8 lines

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<[email protected]>

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

And this can't be built...

Quoted 42 lines

---
  qemu-ga.c              |  631 ++++++++++++++++++++++++++++++++++++++++++++++++
  qga/guest-agent-core.h |    4 +
  2 files changed, 635 insertions(+), 0 deletions(-)

create mode 100644 qemu-ga.c

qemu-ga.c new file +30 -0

new file mode 100644
index 0000000..df08d8c
@@ -0,0 +1,631 @@
+/*
+ * QEMU Guest Agent
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Adam Litke<[email protected]>
+ *  Michael Roth<[email protected]>
+ *
+ * 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"

qemu-ga.c line 36

Quoted 6 lines

qemu-ga.c new file +6 -0

+#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?

Quoted 63 lines

qemu-ga.c new file +63 -0

+    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<[email protected]>\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;
+}

qemu-ga.c line 109

Quoted 10 lines

qemu-ga.c new file +10 -0

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

Quoted 28 lines

qemu-ga.c new file +28 -0

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

qemu-ga.c line 147

Quoted 10 lines

qemu-ga.c new file +10 -0

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

qemu-ga.c line 150

Quoted 3 lines

qemu-ga.c new file +3 -0

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

qemu-ga.c line 154

Quoted 4 lines

qemu-ga.c new file +4 -0

+
+    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().

qemu-ga.c line 163

Quoted 9 lines

qemu-ga.c new file +9 -0

+    }
+    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()).

Quoted 9 lines

qemu-ga.c new file +9 -0

+
+    umask(0);
+    sid = setsid();
+    if (sid<  0) {
+        goto fail;
+    }
+    if ((chdir("/"))<  0) {
+        goto fail;
+    }

qemu-ga.c line 182

Quoted 10 lines

qemu-ga.c new file +10 -0

+
+    close(STDIN_FILENO);
+    close(STDOUT_FILENO);
+    close(STDERR_FILENO);
+    return;
+
+fail:
+    if (pidstr) {
+        free(pidstr);
+    }

This check is not needed.

Quoted 12 lines

qemu-ga.c new file +12 -0

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

qemu-ga.c line 199

Quoted 5 lines

qemu-ga.c new file +5 -0

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

qemu-ga.c line 205

Quoted 6 lines

qemu-ga.c new file +6 -0

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

return -EINVAL.

Quoted 19 lines

qemu-ga.c new file +19 -0

+
+    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;
+        }
+    }

qemu-ga.c line 234

Quoted 10 lines

qemu-ga.c new file +10 -0

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

Quoted 5 lines

qemu-ga.c new file +5 -0

+
+    g_io_channel_flush(channel,&err);
+    if (err != NULL) {
+        g_warning("error flushing payload: %s", err->message);
+        ret = err->code;

qemu-ga.c line 249

Quoted 10 lines

qemu-ga.c new file +10 -0

+        goto out_free;
+    }
+
+    ret = 0;
+
+out_free:
+    QDECREF(payload_qstr);
+    if (err != NULL) {
+        g_error_free(err);
+    }

No need to check against NULL.

Quoted 5 lines

qemu-ga.c new file +5 -0

+out:
+    return ret;
+}
+
+static void process_command(GAState *s, QDict *req)

qemu-ga.c line 264

Quoted 10 lines

qemu-ga.c new file +10 -0

+{
+    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.

Quoted 19 lines

qemu-ga.c new file +19 -0

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

qemu-ga.c line 293

Quoted 10 lines

qemu-ga.c new file +10 -0

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

qemu-ga.c line 299

Quoted 6 lines

qemu-ga.c new file +6 -0

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

Superfluous else clause.

qga/guest-agent-core.h

Quoted 336 lines

qemu-ga.c new file +332 -0

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

qga/guest-agent-core.h +0 -0

index 688f120..66d1729 100644

No license text.

Quoted 15 lines

qga/guest-agent-core.h +4 -0

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

Quoted 66 lines · Luiz Capitulino

Quoted 7 lines · On Tue, 14 Jun 2011 15:06:22 -0500 Michael Roth<[email protected]>…

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:

Quoted 8 lines

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<[email protected]>

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

And this can't be built...

Quoted 42 lines

---
  qemu-ga.c              |  631 ++++++++++++++++++++++++++++++++++++++++++++++++
  qga/guest-agent-core.h |    4 +
  2 files changed, 635 insertions(+), 0 deletions(-)

create mode 100644 qemu-ga.c

qemu-ga.c new file +30 -0

new file mode 100644
index 0000000..df08d8c
@@ -0,0 +1,631 @@
+/*
+ * QEMU Guest Agent
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Adam Litke<[email protected]>
+ *  Michael Roth<[email protected]>
+ *
+ * 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"

Quoted 9 lines

qemu-ga.c line 36

Quoted 6 lines

qemu-ga.c new file +6 -0

+#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.

Quoted 78 lines

Quoted 63 lines

qemu-ga.c new file +63 -0

+    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<[email protected]>\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;
+}

qemu-ga.c line 109

Quoted 10 lines

qemu-ga.c new file +10 -0

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

Quoted 4 lines

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.

Quoted 46 lines

Quoted 29 lines

qemu-ga.c new file +28 -0

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

qemu-ga.c line 147

Quoted 10 lines

qemu-ga.c new file +10 -0

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

Quoted 3 lines

qemu-ga.c new file +3 -0

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

Quoted 3 lines

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?

Quoted 18 lines

qemu-ga.c line 154

Quoted 4 lines

qemu-ga.c new file +4 -0

+
+    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().

Quoted 9 lines

qemu-ga.c new file +9 -0

+    }
+    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;
+    }

Quoted 3 lines

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.

Quoted 35 lines

Quoted 10 lines

qemu-ga.c new file +9 -0

+
+    umask(0);
+    sid = setsid();
+    if (sid<  0) {
+        goto fail;
+    }
+    if ((chdir("/"))<  0) {
+        goto fail;
+    }

qemu-ga.c line 182

Quoted 10 lines

qemu-ga.c new file +10 -0

+
+    close(STDIN_FILENO);
+    close(STDOUT_FILENO);
+    close(STDERR_FILENO);
+    return;
+
+fail:
+    if (pidstr) {
+        free(pidstr);
+    }

This check is not needed.

Quoted 12 lines

qemu-ga.c new file +12 -0

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

Quoted 9 lines

qemu-ga.c line 199

Quoted 5 lines

qemu-ga.c new file +5 -0

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

Quoted 79 lines

qemu-ga.c line 205

Quoted 6 lines

qemu-ga.c new file +6 -0

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

return -EINVAL.

Quoted 19 lines

qemu-ga.c new file +19 -0

+
+    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;
+        }
+    }

qemu-ga.c line 234

Quoted 10 lines

qemu-ga.c new file +10 -0

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

Quoted 5 lines

qemu-ga.c new file +5 -0

+
+    g_io_channel_flush(channel,&err);
+    if (err != NULL) {
+        g_warning("error flushing payload: %s", err->message);
+        ret = err->code;

qemu-ga.c line 249

Quoted 10 lines

qemu-ga.c new file +10 -0

+        goto out_free;
+    }
+
+    ret = 0;
+
+out_free:
+    QDECREF(payload_qstr);
+    if (err != NULL) {
+        g_error_free(err);
+    }

No need to check against NULL.

Quoted 5 lines

qemu-ga.c new file +5 -0

+out:
+    return ret;
+}
+
+static void process_command(GAState *s, QDict *req)

qemu-ga.c line 264

Quoted 10 lines

qemu-ga.c new file +10 -0

+{
+    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.

Quoted 3 lines

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.

Quoted 34 lines

Quoted 19 lines

qemu-ga.c new file +19 -0

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

qemu-ga.c line 293

Quoted 10 lines

qemu-ga.c new file +10 -0

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

Quoted 3 lines

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.

Quoted 364 lines

qemu-ga.c line 299

Quoted 6 lines

qemu-ga.c new file +6 -0

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

Superfluous else clause.

qga/guest-agent-core.h

Quoted 336 lines

qemu-ga.c new file +332 -0

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

qga/guest-agent-core.h +0 -0

index 688f120..66d1729 100644

No license text.

Quoted 15 lines

qga/guest-agent-core.h +4 -0

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