[Qemu-devel] [PULL 0/9] qemu-ga fixes and {get, set}-{time, vcpus} commands

[Qemu-devel] [PULL 0/9] qemu-ga fixes and {get, set}-{time, vcpus} commands

[Michael Roth]

The following changes since commit fe3cc14fd83e0c8f376d849ccd0fc3433388442d:

  Merge remote-tracking branch 'quintela/migration.next' into staging (2013-03-11 08:30:34 -0500)

are available in the git repository at:


  git://github.com/mdroth/qemu.git qga-pull-3-11-2013

for you to fetch changes up to cbb65fc27fd97a3b020df7fce9db4ce09e3956ad:

  qga: implement qmp_guest_set_vcpus() for Linux with sysfs (2013-03-11 18:58:30 -0500)

----------------------------------------------------------------
Laszlo Ersek (3):
      qga: introduce guest-get-vcpus / guest-set-vcpus with stubs
      qga: implement qmp_guest_get_vcpus() for Linux with sysfs
      qga: implement qmp_guest_set_vcpus() for Linux with sysfs

Lei Li (3):
      qga: cast to int for DWORD type
      qga: add guest-get-time command
      qga: add guest-set-time command

Michael Roth (2):
      qemu-ga: make guest-sync-delimited available during fsfreeze
      qemu-ga: use key-value store to avoid recycling fd handles after restart

Stefan Hajnoczi (1):
      qemu-ga: fix confusing GAChannelMethod comparison

 qga/channel-win32.c    |    2 +-
 qga/commands-posix.c   |  268 ++++++++++++++++++++++++++++++++++++++++++++++--
 qga/commands-win32.c   |   23 +++++
 qga/guest-agent-core.h |    1 +
 qga/main.c             |  185 +++++++++++++++++++++++++++++++++
 qga/qapi-schema.json   |  111 ++++++++++++++++++++
 qga/service-win32.c    |    2 +-
 7 files changed, 584 insertions(+), 8 deletions(-)

[Qemu-devel] [PATCH 1/9] qga: cast to int for DWORD type

[Michael Roth]

From: Lei Li <lilei@linux.vnet.ibm.com>

This patch fixes a compiler warning when cross-build:

qga/service-win32.c: In function 'printf_win_error':
qga/service-win32.c:32:5: warning: format '%d' expects argument of type 'int',
                          but argument 3 has type 'DWORD' [-Wformat]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/service-win32.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/service-win32.c b/qga/service-win32.c
index 0905456..843398a 100644
--- a/qga/service-win32.c
+++ b/qga/service-win32.c
@@ -29,7 +29,7 @@ static int printf_win_error(const char *text)
         MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
         (char *)&message, 0,
         NULL);
-    n = printf("%s. (Error: %d) %s", text, err, message);
+    n = printf("%s. (Error: %d) %s", text, (int)err, message);
     LocalFree(message);
 
     return n;
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/9] qemu-ga: fix confusing GAChannelMethod comparison

[Michael Roth]

From: Stefan Hajnoczi <stefanha@redhat.com>

In commit 7868e26e5930f49ca942311885776b938dcf3b77
("qemu-ga: add initial win32 support") support was added for qemu-ga on
Windows using virtio-serial.  Other channel methods (ISA serial and UNIX
domain socket) are not supported on Windows.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/channel-win32.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/channel-win32.c b/qga/channel-win32.c
index 16bf44a..7ed98d7 100644
--- a/qga/channel-win32.c
+++ b/qga/channel-win32.c
@@ -287,7 +287,7 @@ GIOStatus ga_channel_write_all(GAChannel *c, const char *buf, size_t size)
 static gboolean ga_channel_open(GAChannel *c, GAChannelMethod method,
                                 const gchar *path)
 {
-    if (!method == GA_CHANNEL_VIRTIO_SERIAL) {
+    if (method != GA_CHANNEL_VIRTIO_SERIAL) {
         g_critical("unsupported communication method");
         return false;
     }
-- 
1.7.9.5

[Qemu-devel] [PATCH 3/9] qemu-ga: make guest-sync-delimited available during fsfreeze

[Michael Roth]

We currently maintain a whitelist of commands that are safe during
fsfreeze. During fsfreeze, we disable all commands that aren't part of
that whitelist.

guest-sync-delimited meets the criteria for being whitelisted, and is
also required for qemu-ga clients that rely on guest-sync-delimited for
re-syncing the channel after a timeout.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Cc: qemu-stable@nongnu.org
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qga/main.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/qga/main.c b/qga/main.c
index db281a5..ad75171 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -85,6 +85,7 @@ static const char *ga_freeze_whitelist[] = {
     "guest-ping",
     "guest-info",
     "guest-sync",
+    "guest-sync-delimited",
     "guest-fsfreeze-status",
     "guest-fsfreeze-thaw",
     NULL
-- 
1.7.9.5

[Qemu-devel] [PATCH 4/9] qemu-ga: use key-value store to avoid recycling fd handles after restart

[Michael Roth]

Hosts hold on to handles provided by guest-file-open for periods that can
span beyond the life of the qemu-ga process that issued them. Since these
are issued starting from 0 on every restart, we run the risk of issuing
duplicate handles after restarts/reboots.

As a result, users with a stale copy of these handles may end up
reading/writing corrupted data due to their existing handles effectively
being re-assigned to an unexpected file or offset.

We unfortunately do not issue handles as strings, but as integers, so a
solution such as using UUIDs can't be implemented without introducing a
new interface.

As a workaround, we fix this by implementing a persistent key-value store
that will be used to track the value of the last handle that was issued
across restarts/reboots to avoid issuing duplicates.

The store is automatically written to the same directory we currently
set via --statedir to track fsfreeze state, and so should be applicable
for stable releases where this flag is supported.

A follow-up can use this same store for handling fsfreeze state, but
that change is cosmetic and left out for now.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Cc: qemu-stable@nongnu.org

* fixed guest_file_handle_add() return value from uint64_t to int64_t
---
 qga/commands-posix.c   |   25 +++++--
 qga/guest-agent-core.h |    1 +
 qga/main.c             |  184 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 204 insertions(+), 6 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 7a0202e..1c2aff3 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -129,14 +129,22 @@ static struct {
     QTAILQ_HEAD(, GuestFileHandle) filehandles;
 } guest_file_state;
 
-static void guest_file_handle_add(FILE *fh)
+static int64_t guest_file_handle_add(FILE *fh, Error **errp)
 {
     GuestFileHandle *gfh;
+    int64_t handle;
+
+    handle = ga_get_fd_handle(ga_state, errp);
+    if (error_is_set(errp)) {
+        return 0;
+    }
 
     gfh = g_malloc0(sizeof(GuestFileHandle));
-    gfh->id = fileno(fh);
+    gfh->id = handle;
     gfh->fh = fh;
     QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
+
+    return handle;
 }
 
 static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
@@ -158,7 +166,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
 {
     FILE *fh;
     int fd;
-    int64_t ret = -1;
+    int64_t ret = -1, handle;
 
     if (!has_mode) {
         mode = "r";
@@ -184,9 +192,14 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
         return -1;
     }
 
-    guest_file_handle_add(fh);
-    slog("guest-file-open, handle: %d", fd);
-    return fd;
+    handle = guest_file_handle_add(fh, err);
+    if (error_is_set(err)) {
+        fclose(fh);
+        return -1;
+    }
+
+    slog("guest-file-open, handle: %d", handle);
+    return handle;
 }
 
 void qmp_guest_file_close(int64_t handle, Error **err)
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index 3354598..624a559 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -35,6 +35,7 @@ bool ga_is_frozen(GAState *s);
 void ga_set_frozen(GAState *s);
 void ga_unset_frozen(GAState *s);
 const char *ga_fsfreeze_hook(GAState *s);
+int64_t ga_get_fd_handle(GAState *s, Error **errp);
 
 #ifndef _WIN32
 void reopen_fd_to_null(int fd);
diff --git a/qga/main.c b/qga/main.c
index ad75171..99346e1 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -15,6 +15,7 @@
 #include <stdbool.h>
 #include <glib.h>
 #include <getopt.h>
+#include <glib/gstdio.h>
 #ifndef _WIN32
 #include <syslog.h>
 #include <sys/wait.h>
@@ -30,6 +31,7 @@
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/dispatch.h"
 #include "qga/channel.h"
+#include "qemu/bswap.h"
 #ifdef _WIN32
 #include "qga/service-win32.h"
 #include <windows.h>
@@ -53,6 +55,11 @@
 #endif
 #define QGA_SENTINEL_BYTE 0xFF
 
+typedef struct GAPersistentState {
+#define QGA_PSTATE_DEFAULT_FD_COUNTER 1000
+    int64_t fd_counter;
+} GAPersistentState;
+
 struct GAState {
     JSONMessageParser parser;
     GMainLoop *main_loop;
@@ -76,6 +83,8 @@ struct GAState {
 #ifdef CONFIG_FSFREEZE
     const char *fsfreeze_hook;
 #endif
+    const gchar *pstate_filepath;
+    GAPersistentState pstate;
 };
 
 struct GAState *ga_state;
@@ -725,6 +734,171 @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
 }
 #endif
 
+static void set_persistent_state_defaults(GAPersistentState *pstate)
+{
+    g_assert(pstate);
+    pstate->fd_counter = QGA_PSTATE_DEFAULT_FD_COUNTER;
+}
+
+static void persistent_state_from_keyfile(GAPersistentState *pstate,
+                                          GKeyFile *keyfile)
+{
+    g_assert(pstate);
+    g_assert(keyfile);
+    /* if any fields are missing, either because the file was tampered with
+     * by agents of chaos, or because the field wasn't present at the time the
+     * file was created, the best we can ever do is start over with the default
+     * values. so load them now, and ignore any errors in accessing key-value
+     * pairs
+     */
+    set_persistent_state_defaults(pstate);
+
+    if (g_key_file_has_key(keyfile, "global", "fd_counter", NULL)) {
+        pstate->fd_counter =
+            g_key_file_get_int64(keyfile, "global", "fd_counter", NULL);
+    }
+}
+
+static void persistent_state_to_keyfile(const GAPersistentState *pstate,
+                                        GKeyFile *keyfile)
+{
+    g_assert(pstate);
+    g_assert(keyfile);
+
+    g_key_file_set_int64(keyfile, "global", "fd_counter", pstate->fd_counter);
+}
+
+static gboolean write_persistent_state(const GAPersistentState *pstate,
+                                       const gchar *path)
+{
+    GKeyFile *keyfile = g_key_file_new();
+    GError *gerr = NULL;
+    gboolean ret = true;
+    gchar *data = NULL;
+    gsize data_len;
+
+    g_assert(pstate);
+
+    persistent_state_to_keyfile(pstate, keyfile);
+    data = g_key_file_to_data(keyfile, &data_len, &gerr);
+    if (gerr) {
+        g_critical("failed to convert persistent state to string: %s",
+                   gerr->message);
+        ret = false;
+        goto out;
+    }
+
+    g_file_set_contents(path, data, data_len, &gerr);
+    if (gerr) {
+        g_critical("failed to write persistent state to %s: %s",
+                    path, gerr->message);
+        ret = false;
+        goto out;
+    }
+
+out:
+    if (gerr) {
+        g_error_free(gerr);
+    }
+    if (keyfile) {
+        g_key_file_free(keyfile);
+    }
+    g_free(data);
+    return ret;
+}
+
+static gboolean read_persistent_state(GAPersistentState *pstate,
+                                      const gchar *path, gboolean frozen)
+{
+    GKeyFile *keyfile = NULL;
+    GError *gerr = NULL;
+    struct stat st;
+    gboolean ret = true;
+
+    g_assert(pstate);
+
+    if (stat(path, &st) == -1) {
+        /* it's okay if state file doesn't exist, but any other error
+         * indicates a permissions issue or some other misconfiguration
+         * that we likely won't be able to recover from.
+         */
+        if (errno != ENOENT) {
+            g_critical("unable to access state file at path %s: %s",
+                       path, strerror(errno));
+            ret = false;
+            goto out;
+        }
+
+        /* file doesn't exist. initialize state to default values and
+         * attempt to save now. (we could wait till later when we have
+         * modified state we need to commit, but if there's a problem,
+         * such as a missing parent directory, we want to catch it now)
+         *
+         * there is a potential scenario where someone either managed to
+         * update the agent from a version that didn't use a key store
+         * while qemu-ga thought the filesystem was frozen, or
+         * deleted the key store prior to issuing a fsfreeze, prior
+         * to restarting the agent. in this case we go ahead and defer
+         * initial creation till we actually have modified state to
+         * write, otherwise fail to recover from freeze.
+         */
+        set_persistent_state_defaults(pstate);
+        if (!frozen) {
+            ret = write_persistent_state(pstate, path);
+            if (!ret) {
+                g_critical("unable to create state file at path %s", path);
+                ret = false;
+                goto out;
+            }
+        }
+        ret = true;
+        goto out;
+    }
+
+    keyfile = g_key_file_new();
+    g_key_file_load_from_file(keyfile, path, 0, &gerr);
+    if (gerr) {
+        g_critical("error loading persistent state from path: %s, %s",
+                   path, gerr->message);
+        ret = false;
+        goto out;
+    }
+
+    persistent_state_from_keyfile(pstate, keyfile);
+
+out:
+    if (keyfile) {
+        g_key_file_free(keyfile);
+    }
+    if (gerr) {
+        g_error_free(gerr);
+    }
+
+    return ret;
+}
+
+int64_t ga_get_fd_handle(GAState *s, Error **errp)
+{
+    int64_t handle;
+
+    g_assert(s->pstate_filepath);
+    /* we blacklist commands and avoid operations that potentially require
+     * writing to disk when we're in a frozen state. this includes opening
+     * new files, so we should never get here in that situation
+     */
+    g_assert(!ga_is_frozen(s));
+
+    handle = s->pstate.fd_counter++;
+    if (s->pstate.fd_counter < 0) {
+        s->pstate.fd_counter = 0;
+    }
+    if (!write_persistent_state(&s->pstate, s->pstate_filepath)) {
+        error_setg(errp, "failed to commit persistent state to disk");
+    }
+
+    return handle;
+}
+
 int main(int argc, char **argv)
 {
     const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
@@ -854,7 +1028,9 @@ int main(int argc, char **argv)
     ga_enable_logging(s);
     s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen",
                                                  state_dir);
+    s->pstate_filepath = g_strdup_printf("%s/qga.state", state_dir);
     s->frozen = false;
+
 #ifndef _WIN32
     /* check if a previous instance of qemu-ga exited with filesystems' state
      * marked as frozen. this could be a stale value (a non-qemu-ga process
@@ -911,6 +1087,14 @@ int main(int argc, char **argv)
         }
     }
 
+    /* load persistent state from disk */
+    if (!read_persistent_state(&s->pstate,
+                               s->pstate_filepath,
+                               ga_is_frozen(s))) {
+        g_critical("failed to load persistent state");
+        goto out_bad;
+    }
+
     if (blacklist) {
         s->blacklist = blacklist;
         do {
-- 
1.7.9.5

[Qemu-devel] [PATCH 5/9] qga: add guest-get-time command

[Michael Roth]

From: Lei Li <lilei@linux.vnet.ibm.com>

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

*added stub for w32

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c |   16 ++++++++++++++++
 qga/commands-win32.c |    6 ++++++
 qga/qapi-schema.json |   13 +++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1c2aff3..c83d26d 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -119,6 +119,22 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
     /* succeded */
 }
 
+int64_t qmp_guest_get_time(Error **errp)
+{
+   int ret;
+   qemu_timeval tq;
+   int64_t time_ns;
+
+   ret = qemu_gettimeofday(&tq);
+   if (ret < 0) {
+       error_setg_errno(errp, errno, "Failed to get time");
+       return -1;
+   }
+
+   time_ns = tq.tv_sec * 1000000000LL + tq.tv_usec * 1000;
+   return time_ns;
+}
+
 typedef struct GuestFileHandle {
     uint64_t id;
     FILE *fh;
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 7e8ecb3..3ebb856 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -278,6 +278,12 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **err)
     return NULL;
 }
 
+int64_t qmp_guest_get_time(Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return -1;
+}
+
 /* register init/cleanup routines for stateful command groups */
 void ga_command_state_init(GAState *s, GACommandState *cs)
 {
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index d91d903..bb0f75e 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -83,6 +83,19 @@
 { 'command': 'guest-ping' }
 
 ##
+# @guest-get-time:
+#
+# Get the information about guest time relative to the Epoch
+# of 1970-01-01 in UTC.
+#
+# Returns: Time in nanoseconds.
+#
+# Since 1.5
+##
+{ 'command': 'guest-get-time',
+  'returns': 'int' }
+
+##
 # @GuestAgentCommandInfo:
 #
 # Information about guest agent commands.
-- 
1.7.9.5

[Qemu-devel] [PATCH 6/9] qga: add guest-set-time command

[Michael Roth]

From: Lei Li <lilei@linux.vnet.ibm.com>

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

*added stub for w32

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/commands-win32.c |    5 +++++
 qga/qapi-schema.json |   26 ++++++++++++++++++++++++
 3 files changed, 86 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index c83d26d..c253f97 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -135,6 +135,61 @@ int64_t qmp_guest_get_time(Error **errp)
    return time_ns;
 }
 
+void qmp_guest_set_time(int64_t time_ns, Error **errp)
+{
+    int ret;
+    int status;
+    pid_t pid;
+    Error *local_err = NULL;
+    struct timeval tv;
+
+    /* year-2038 will overflow in case time_t is 32bit */
+    if (time_ns / 1000000000 != (time_t)(time_ns / 1000000000)) {
+        error_setg(errp, "Time %" PRId64 " is too large", time_ns);
+        return;
+    }
+
+    tv.tv_sec = time_ns / 1000000000;
+    tv.tv_usec = (time_ns % 1000000000) / 1000;
+
+    ret = settimeofday(&tv, NULL);
+    if (ret < 0) {
+        error_setg_errno(errp, errno, "Failed to set time to guest");
+        return;
+    }
+
+    /* Set the Hardware Clock to the current System Time. */
+    pid = fork();
+    if (pid == 0) {
+        setsid();
+        reopen_fd_to_null(0);
+        reopen_fd_to_null(1);
+        reopen_fd_to_null(2);
+
+        execle("/sbin/hwclock", "hwclock", "-w", NULL, environ);
+        _exit(EXIT_FAILURE);
+    } else if (pid < 0) {
+        error_setg_errno(errp, errno, "failed to create child process");
+        return;
+    }
+
+    ga_wait_child(pid, &status, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (!WIFEXITED(status)) {
+        error_setg(errp, "child process has terminated abnormally");
+        return;
+    }
+
+    if (WEXITSTATUS(status)) {
+        error_setg(errp, "hwclock failed to set hardware clock to system time");
+        return;
+    }
+}
+
 typedef struct GuestFileHandle {
     uint64_t id;
     FILE *fh;
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 3ebb856..622c74d 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -284,6 +284,11 @@ int64_t qmp_guest_get_time(Error **errp)
     return -1;
 }
 
+void qmp_guest_set_time(int64_t time_ns, Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+}
+
 /* register init/cleanup routines for stateful command groups */
 void ga_command_state_init(GAState *s, GACommandState *cs)
 {
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index bb0f75e..437d750 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -96,6 +96,32 @@
   'returns': 'int' }
 
 ##
+# @guest-set-time:
+#
+# Set guest time.
+#
+# When a guest is paused or migrated to a file then loaded
+# from that file, the guest OS has no idea that there
+# was a big gap in the time. Depending on how long the
+# gap was, NTP might not be able to resynchronize the
+# guest.
+#
+# This command tries to set guest time to the given value,
+# then sets the Hardware Clock to the current System Time.
+# This will make it easier for a guest to resynchronize
+# without waiting for NTP.
+#
+# @time: time of nanoseconds, relative to the Epoch of
+#        1970-01-01 in UTC.
+#
+# Returns: Nothing on success.
+#
+# Since: 1.5
+##
+{ 'command': 'guest-set-time',
+  'data': { 'time': 'int' } }
+
+##
 # @GuestAgentCommandInfo:
 #
 # Information about guest agent commands.
-- 
1.7.9.5

[Qemu-devel] [PATCH 7/9] qga: introduce guest-get-vcpus / guest-set-vcpus with stubs

[Michael Roth]

From: Laszlo Ersek <lersek@redhat.com>

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c |   12 +++++++++
 qga/commands-win32.c |   12 +++++++++
 qga/qapi-schema.json |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index c253f97..03a41a2 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1167,6 +1167,18 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
 }
 #endif
 
+GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+
+int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return -1;
+}
+
 /* register init/cleanup routines for stateful command groups */
 void ga_command_state_init(GAState *s, GACommandState *cs)
 {
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 622c74d..b19be9d 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -289,6 +289,18 @@ void qmp_guest_set_time(int64_t time_ns, Error **errp)
     error_set(errp, QERR_UNSUPPORTED);
 }
 
+GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+
+int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return -1;
+}
+
 /* register init/cleanup routines for stateful command groups */
 void ga_command_state_init(GAState *s, GACommandState *cs)
 {
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 437d750..dac4e6f 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -554,3 +554,75 @@
 ##
 { 'command': 'guest-network-get-interfaces',
   'returns': ['GuestNetworkInterface'] }
+
+##
+# @GuestLogicalProcessor:
+#
+# @logical-id: Arbitrary guest-specific unique identifier of the VCPU.
+#
+# @online: Whether the VCPU is enabled.
+#
+# @can-offline: Whether offlining the VCPU is possible. This member is always
+#               filled in by the guest agent when the structure is returned,
+#               and always ignored on input (hence it can be omitted then).
+#
+# Since: 1.5
+##
+{ 'type': 'GuestLogicalProcessor',
+  'data': {'logical-id': 'int',
+           'online': 'bool',
+           '*can-offline': 'bool'} }
+
+##
+# @guest-get-vcpus:
+#
+# Retrieve the list of the guest's logical processors.
+#
+# This is a read-only operation.
+#
+# Returns: The list of all VCPUs the guest knows about. Each VCPU is put on the
+# list exactly once, but their order is unspecified.
+#
+# Since: 1.5
+##
+{ 'command': 'guest-get-vcpus',
+  'returns': ['GuestLogicalProcessor'] }
+
+##
+# @guest-set-vcpus:
+#
+# Attempt to reconfigure (currently: enable/disable) logical processors inside
+# the guest.
+#
+# The input list is processed node by node in order. In each node @logical-id
+# is used to look up the guest VCPU, for which @online specifies the requested
+# state. The set of distinct @logical-id's is only required to be a subset of
+# the guest-supported identifiers. There's no restriction on list length or on
+# repeating the same @logical-id (with possibly different @online field).
+# Preferably the input list should describe a modified subset of
+# @guest-get-vcpus' return value.
+#
+# Returns: The length of the initial sublist that has been successfully
+#          processed. The guest agent maximizes this value. Possible cases:
+#
+#          0:                if the @vcpus list was empty on input. Guest state
+#                            has not been changed. Otherwise,
+#
+#          Error:            processing the first node of @vcpus failed for the
+#                            reason returned. Guest state has not been changed.
+#                            Otherwise,
+#
+#          < length(@vcpus): more than zero initial nodes have been processed,
+#                            but not the entire @vcpus list. Guest state has
+#                            changed accordingly. To retrieve the error
+#                            (assuming it persists), repeat the call with the
+#                            successfully processed initial sublist removed.
+#                            Otherwise,
+#
+#          length(@vcpus):   call successful.
+#
+# Since: 1.5
+##
+{ 'command': 'guest-set-vcpus',
+  'data':    {'vcpus': ['GuestLogicalProcessor'] },
+  'returns': 'int' }
-- 
1.7.9.5

[Qemu-devel] [PATCH 8/9] qga: implement qmp_guest_get_vcpus() for Linux with sysfs

[Michael Roth]

From: Laszlo Ersek <lersek@redhat.com>

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c |  146 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 140 insertions(+), 6 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 03a41a2..17cedab 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -15,6 +15,10 @@
 #include <sys/types.h>
 #include <sys/ioctl.h>
 #include <sys/wait.h>
+#include <unistd.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <inttypes.h>
 #include "qga/guest-agent-core.h"
 #include "qga-qmp-commands.h"
 #include "qapi/qmp/qerror.h"
@@ -1111,6 +1115,136 @@ error:
     return NULL;
 }
 
+#define SYSCONF_EXACT(name, err) sysconf_exact((name), #name, (err))
+
+static long sysconf_exact(int name, const char *name_str, Error **err)
+{
+    long ret;
+
+    errno = 0;
+    ret = sysconf(name);
+    if (ret == -1) {
+        if (errno == 0) {
+            error_setg(err, "sysconf(%s): value indefinite", name_str);
+        } else {
+            error_setg_errno(err, errno, "sysconf(%s)", name_str);
+        }
+    }
+    return ret;
+}
+
+/* Transfer online/offline status between @vcpu and the guest system.
+ *
+ * On input either @errp or *@errp must be NULL.
+ *
+ * In system-to-@vcpu direction, the following @vcpu fields are accessed:
+ * - R: vcpu->logical_id
+ * - W: vcpu->online
+ * - W: vcpu->can_offline
+ *
+ * In @vcpu-to-system direction, the following @vcpu fields are accessed:
+ * - R: vcpu->logical_id
+ * - R: vcpu->online
+ *
+ * Written members remain unmodified on error.
+ */
+static void transfer_vcpu(GuestLogicalProcessor *vcpu, bool sys2vcpu,
+                          Error **errp)
+{
+    char *dirpath;
+    int dirfd;
+
+    dirpath = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 "/",
+                              vcpu->logical_id);
+    dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
+    if (dirfd == -1) {
+        error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
+    } else {
+        static const char fn[] = "online";
+        int fd;
+        int res;
+
+        fd = openat(dirfd, fn, sys2vcpu ? O_RDONLY : O_RDWR);
+        if (fd == -1) {
+            if (errno != ENOENT) {
+                error_setg_errno(errp, errno, "open(\"%s/%s\")", dirpath, fn);
+            } else if (sys2vcpu) {
+                vcpu->online = true;
+                vcpu->can_offline = false;
+            } else if (!vcpu->online) {
+                error_setg(errp, "logical processor #%" PRId64 " can't be "
+                           "offlined", vcpu->logical_id);
+            } /* otherwise pretend successful re-onlining */
+        } else {
+            unsigned char status;
+
+            res = pread(fd, &status, 1, 0);
+            if (res == -1) {
+                error_setg_errno(errp, errno, "pread(\"%s/%s\")", dirpath, fn);
+            } else if (res == 0) {
+                error_setg(errp, "pread(\"%s/%s\"): unexpected EOF", dirpath,
+                           fn);
+            } else if (sys2vcpu) {
+                vcpu->online = (status != '0');
+                vcpu->can_offline = true;
+            } else if (vcpu->online != (status != '0')) {
+                status = '0' + vcpu->online;
+                if (pwrite(fd, &status, 1, 0) == -1) {
+                    error_setg_errno(errp, errno, "pwrite(\"%s/%s\")", dirpath,
+                                     fn);
+                }
+            } /* otherwise pretend successful re-(on|off)-lining */
+
+            res = close(fd);
+            g_assert(res == 0);
+        }
+
+        res = close(dirfd);
+        g_assert(res == 0);
+    }
+
+    g_free(dirpath);
+}
+
+GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
+{
+    int64_t current;
+    GuestLogicalProcessorList *head, **link;
+    long sc_max;
+    Error *local_err = NULL;
+
+    current = 0;
+    head = NULL;
+    link = &head;
+    sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, &local_err);
+
+    while (local_err == NULL && current < sc_max) {
+        GuestLogicalProcessor *vcpu;
+        GuestLogicalProcessorList *entry;
+
+        vcpu = g_malloc0(sizeof *vcpu);
+        vcpu->logical_id = current++;
+        vcpu->has_can_offline = true; /* lolspeak ftw */
+        transfer_vcpu(vcpu, true, &local_err);
+
+        entry = g_malloc0(sizeof *entry);
+        entry->value = vcpu;
+
+        *link = entry;
+        link = &entry->next;
+    }
+
+    if (local_err == NULL) {
+        /* there's no guest with zero VCPUs */
+        g_assert(head != NULL);
+        return head;
+    }
+
+    qapi_free_GuestLogicalProcessorList(head);
+    error_propagate(errp, local_err);
+    return NULL;
+}
+
 #else /* defined(__linux__) */
 
 void qmp_guest_suspend_disk(Error **err)
@@ -1134,6 +1268,12 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
     return NULL;
 }
 
+GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+
 #endif
 
 #if !defined(CONFIG_FSFREEZE)
@@ -1167,12 +1307,6 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
 }
 #endif
 
-GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
-{
-    error_set(errp, QERR_UNSUPPORTED);
-    return NULL;
-}
-
 int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
 {
     error_set(errp, QERR_UNSUPPORTED);
-- 
1.7.9.5

[Qemu-devel] [PATCH 9/9] qga: implement qmp_guest_set_vcpus() for Linux with sysfs

[Michael Roth]

From: Laszlo Ersek <lersek@redhat.com>

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c |   38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 17cedab..d7da850 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1245,6 +1245,32 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
     return NULL;
 }
 
+int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
+{
+    int64_t processed;
+    Error *local_err = NULL;
+
+    processed = 0;
+    while (vcpus != NULL) {
+        transfer_vcpu(vcpus->value, false, &local_err);
+        if (local_err != NULL) {
+            break;
+        }
+        ++processed;
+        vcpus = vcpus->next;
+    }
+
+    if (local_err != NULL) {
+        if (processed == 0) {
+            error_propagate(errp, local_err);
+        } else {
+            error_free(local_err);
+        }
+    }
+
+    return processed;
+}
+
 #else /* defined(__linux__) */
 
 void qmp_guest_suspend_disk(Error **err)
@@ -1274,6 +1300,12 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
     return NULL;
 }
 
+int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return -1;
+}
+
 #endif
 
 #if !defined(CONFIG_FSFREEZE)
@@ -1307,12 +1339,6 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
 }
 #endif
 
-int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
-{
-    error_set(errp, QERR_UNSUPPORTED);
-    return -1;
-}
-
 /* register init/cleanup routines for stateful command groups */
 void ga_command_state_init(GAState *s, GACommandState *cs)
 {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 4/9] qemu-ga: use key-value store to avoid recycling fd handles after restart

[Peter Crosthwaite]

Hi Michael, Anthony,

On Tue, Mar 12, 2013 at 10:53 AM, Michael Roth
<mdroth@linux.vnet.ibm.com> wrote:
> Hosts hold on to handles provided by guest-file-open for periods that can
> span beyond the life of the qemu-ga process that issued them. Since these
> are issued starting from 0 on every restart, we run the risk of issuing
> duplicate handles after restarts/reboots.
>
> As a result, users with a stale copy of these handles may end up
> reading/writing corrupted data due to their existing handles effectively
> being re-assigned to an unexpected file or offset.
>
> We unfortunately do not issue handles as strings, but as integers, so a
> solution such as using UUIDs can't be implemented without introducing a
> new interface.
>
> As a workaround, we fix this by implementing a persistent key-value store
> that will be used to track the value of the last handle that was issued
> across restarts/reboots to avoid issuing duplicates.
>
> The store is automatically written to the same directory we currently
> set via --statedir to track fsfreeze state, and so should be applicable
> for stable releases where this flag is supported.
>
> A follow-up can use this same store for handling fsfreeze state, but
> that change is cosmetic and left out for now.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Cc: qemu-stable@nongnu.org
>
> * fixed guest_file_handle_add() return value from uint64_t to int64_t
> ---
>  qga/commands-posix.c   |   25 +++++--
>  qga/guest-agent-core.h |    1 +
>  qga/main.c             |  184 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 204 insertions(+), 6 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 7a0202e..1c2aff3 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
[snip]
> +    g_assert(pstate);
> +    g_assert(keyfile);
> +    /* if any fields are missing, either because the file was tampered with
> +     * by agents of chaos, or because the field wasn't present at the time the
> +     * file was created, the best we can ever do is start over with the default
> +     * values. so load them now, and ignore any errors in accessing key-value
> +     * pairs
> +     */
> +    set_persistent_state_defaults(pstate);
> +
> +    if (g_key_file_has_key(keyfile, "global", "fd_counter", NULL)) {
> +        pstate->fd_counter =
> +            g_key_file_get_int64(keyfile, "global", "fd_counter", NULL);

This function (and friends) doesn't exist in glib pre v2.26, breaking
the build for a few distros (RHEL 5.6 and Ubuntu 10.04 are two that I
am building with). Are we mandating glib > 2.26 as of this series (and
should configure be updated) or do we need an alternate implementation
of this code?

Regards,
peter

Re: [Qemu-devel] [PATCH 4/9] qemu-ga: use key-value store to avoid recycling fd handles after restart

[Michael Roth]

On Thu, Mar 14, 2013 at 10:31 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Hi Michael, Anthony,
>
> On Tue, Mar 12, 2013 at 10:53 AM, Michael Roth
> <mdroth@linux.vnet.ibm.com> wrote:
>> Hosts hold on to handles provided by guest-file-open for periods that can
>> span beyond the life of the qemu-ga process that issued them. Since these
>> are issued starting from 0 on every restart, we run the risk of issuing
>> duplicate handles after restarts/reboots.
>>
>> As a result, users with a stale copy of these handles may end up
>> reading/writing corrupted data due to their existing handles effectively
>> being re-assigned to an unexpected file or offset.
>>
>> We unfortunately do not issue handles as strings, but as integers, so a
>> solution such as using UUIDs can't be implemented without introducing a
>> new interface.
>>
>> As a workaround, we fix this by implementing a persistent key-value store
>> that will be used to track the value of the last handle that was issued
>> across restarts/reboots to avoid issuing duplicates.
>>
>> The store is automatically written to the same directory we currently
>> set via --statedir to track fsfreeze state, and so should be applicable
>> for stable releases where this flag is supported.
>>
>> A follow-up can use this same store for handling fsfreeze state, but
>> that change is cosmetic and left out for now.
>>
>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>> Cc: qemu-stable@nongnu.org
>>
>> * fixed guest_file_handle_add() return value from uint64_t to int64_t
>> ---
>>  qga/commands-posix.c   |   25 +++++--
>>  qga/guest-agent-core.h |    1 +
>>  qga/main.c             |  184 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 204 insertions(+), 6 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 7a0202e..1c2aff3 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
> [snip]
>> +    g_assert(pstate);
>> +    g_assert(keyfile);
>> +    /* if any fields are missing, either because the file was tampered with
>> +     * by agents of chaos, or because the field wasn't present at the time the
>> +     * file was created, the best we can ever do is start over with the default
>> +     * values. so load them now, and ignore any errors in accessing key-value
>> +     * pairs
>> +     */
>> +    set_persistent_state_defaults(pstate);
>> +
>> +    if (g_key_file_has_key(keyfile, "global", "fd_counter", NULL)) {
>> +        pstate->fd_counter =
>> +            g_key_file_get_int64(keyfile, "global", "fd_counter", NULL);
>
> This function (and friends) doesn't exist in glib pre v2.26, breaking
> the build for a few distros (RHEL 5.6 and Ubuntu 10.04 are two that I
> am building with). Are we mandating glib > 2.26 as of this series (and
> should configure be updated) or do we need an alternate implementation
> of this code?

Sigh...that certainly wasn't the intention, and something I specifically tried
to avoid doing. GKeyFile was added in 2.6 according to the documentation:

https://developer.gnome.org/glib/2.31/glib-Key-value-file-parser.html

It seems that it may specifically be g_key_file_get_int64() which is
causing the 2.26 dependency.

Would you mind testing with get_int64/set_int64 swapped out for
get_integer/set_integer? If that does the trick I can send an updated
patch tomorrow.

>
> Regards,
> peter
>

Re: [Qemu-devel] [PATCH 4/9] qemu-ga: use key-value store to avoid recycling fd handles after restart

[Peter Crosthwaite]

On Fri, Mar 15, 2013 at 1:52 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> On Thu, Mar 14, 2013 at 10:31 PM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> Hi Michael, Anthony,
>>
>> On Tue, Mar 12, 2013 at 10:53 AM, Michael Roth
>> <mdroth@linux.vnet.ibm.com> wrote:
>>> Hosts hold on to handles provided by guest-file-open for periods that can
>>> span beyond the life of the qemu-ga process that issued them. Since these
>>> are issued starting from 0 on every restart, we run the risk of issuing
>>> duplicate handles after restarts/reboots.
>>>
>>> As a result, users with a stale copy of these handles may end up
>>> reading/writing corrupted data due to their existing handles effectively
>>> being re-assigned to an unexpected file or offset.
>>>
>>> We unfortunately do not issue handles as strings, but as integers, so a
>>> solution such as using UUIDs can't be implemented without introducing a
>>> new interface.
>>>
>>> As a workaround, we fix this by implementing a persistent key-value store
>>> that will be used to track the value of the last handle that was issued
>>> across restarts/reboots to avoid issuing duplicates.
>>>
>>> The store is automatically written to the same directory we currently
>>> set via --statedir to track fsfreeze state, and so should be applicable
>>> for stable releases where this flag is supported.
>>>
>>> A follow-up can use this same store for handling fsfreeze state, but
>>> that change is cosmetic and left out for now.
>>>
>>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>>> Cc: qemu-stable@nongnu.org
>>>
>>> * fixed guest_file_handle_add() return value from uint64_t to int64_t
>>> ---
>>>  qga/commands-posix.c   |   25 +++++--
>>>  qga/guest-agent-core.h |    1 +
>>>  qga/main.c             |  184 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 204 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>>> index 7a0202e..1c2aff3 100644
>>> --- a/qga/commands-posix.c
>>> +++ b/qga/commands-posix.c
>> [snip]
>>> +    g_assert(pstate);
>>> +    g_assert(keyfile);
>>> +    /* if any fields are missing, either because the file was tampered with
>>> +     * by agents of chaos, or because the field wasn't present at the time the
>>> +     * file was created, the best we can ever do is start over with the default
>>> +     * values. so load them now, and ignore any errors in accessing key-value
>>> +     * pairs
>>> +     */
>>> +    set_persistent_state_defaults(pstate);
>>> +
>>> +    if (g_key_file_has_key(keyfile, "global", "fd_counter", NULL)) {
>>> +        pstate->fd_counter =
>>> +            g_key_file_get_int64(keyfile, "global", "fd_counter", NULL);
>>
>> This function (and friends) doesn't exist in glib pre v2.26, breaking
>> the build for a few distros (RHEL 5.6 and Ubuntu 10.04 are two that I
>> am building with). Are we mandating glib > 2.26 as of this series (and
>> should configure be updated) or do we need an alternate implementation
>> of this code?
>
> Sigh...that certainly wasn't the intention, and something I specifically tried
> to avoid doing. GKeyFile was added in 2.6 according to the documentation:
>
> https://developer.gnome.org/glib/2.31/glib-Key-value-file-parser.html
>
> It seems that it may specifically be g_key_file_get_int64() which is
> causing the 2.26 dependency.
>
> Would you mind testing with get_int64/set_int64 swapped out for
> get_integer/set_integer? If that does the trick I can send an updated
> patch tomorrow.
>

That did the trick,

Patch up on list if you want to sign it off or suggested-by it.

Regards,
Peter


>>
>> Regards,
>> peter
>>
>