[Qemu-devel] [PATCH 7/7] qemu-ga: add guest-sync-delimited

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

guest-sync leaves it as an exercise to the user as to how to reliably
obtain the response to guest-sync if the client had previously read in a
partial response (due qemu-ga previously being restarted mid-"sentence"
due to reboot, forced restart, etc).

qemu-ga handles this situation on its end by having a client precede
their guest-sync request with a 0xFF byte (invalid UTF-8), which
qemu-ga/QEMU JSON parsers will treat as a flush event. Thus we can
reliably flush the qemu-ga parser state in preparation for receiving
the guest-sync request.

guest-sync-delimited provides the same functionality for a client: when
a guest-sync-delimited is issued, qemu-ga will precede it's response
with a 0xFF byte that the client can use as an indicator to flush its
buffer/parser state in preparation for reliably receiving the
guest-sync-delimited response.

It is also useful as an optimization for clients, since, after issuing a
guest-sync-delimited, clients can safely discard all stale data read
from the channel until the 0xFF is found.

More information available on the wiki:

http://wiki.qemu.org/Features/QAPI/GuestAgent#QEMU_Guest_Agent_Protocol

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qapi-schema-guest.json |   48 +++++++++++++++++++++++++++++++++++++++++++++++-
 qemu-ga.c              |   27 ++++++++++++++++++++++-----
 qga/commands-posix.c   |    3 ---
 qga/commands.c         |    6 ++++++
 qga/guest-agent-core.h |    2 ++
 5 files changed, 77 insertions(+), 9 deletions(-)

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index 12b5d4f..cf18876 100644
--- a/qapi-schema-guest.json
+++ b/qapi-schema-guest.json
@@ -1,6 +1,41 @@
 # *-*- Mode: Python -*-*
 
 ##
+#
+# Echo back a unique integer value, and prepend to response a
+# leading sentinel byte (0xFF) the client can check scan for.
+#
+# This is used by clients talking to the guest agent over the
+# wire to ensure the stream is in sync and doesn't contain stale
+# data from previous client. It must be issued upon initial
+# connection, and after any client-side timeouts (including
+# timeouts on receiving a response to this command).
+#
+# After issuing this request, all guest agent responses should be
+# ignored until the response containing the unique integer value
+# the client passed in is returned. Receival of the 0xFF sentinel
+# byte must be handled as an indication that the client's
+# lexer/tokenizer/parser state should be flushed/reset in
+# preparation for reliably receiving the subsequent response. As
+# an optimization, clients may opt to ignore all data until a
+# sentinel value is receiving to avoid unecessary processing of
+# stale data.
+#
+# Similarly, clients should also precede this *request*
+# with a 0xFF byte to make sure the guest agent flushes any
+# partially read JSON data from a previous client connection.
+#
+# @id: randomly generated 64-bit integer
+#
+# Returns: The unique integer id passed in by the client
+#
+# Since: 1.1
+# ##
+{ 'command': 'guest-sync-delimited'
+  'data':    { 'id': 'int' },
+  'returns': 'int' }
+
+##
 # @guest-sync:
 #
 # Echo back a unique integer value
@@ -13,8 +48,19 @@
 # partially-delivered JSON text in such a way that this response
 # can be obtained.
 #
+# In cases where a partial stale response was previously
+# received by the client, this cannot always be done reliably.
+# One particular scenario being if qemu-ga responses are fed
+# character-by-character into a JSON parser. In these situations,
+# using guest-sync-delimited may be optimal.
+#
+# For clients that fetch responses line by line and convert them
+# to JSON objects, guest-sync should be sufficient, but note that
+# in cases where the channel is dirty some attempts at parsing the
+# response may result in a parser error.
+#
 # Such clients should also precede this command
-# with a 0xFF byte to make such the guest agent flushes any
+# with a 0xFF byte to make sure the guest agent flushes any
 # partially read JSON data from a previous session.
 #
 # @id: randomly generated 64-bit integer
diff --git a/qemu-ga.c b/qemu-ga.c
index 1c90e6e..d6f786e 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -41,6 +41,7 @@
 #define QGA_VIRTIO_PATH_DEFAULT "\\\\.\\Global\\org.qemu.guest_agent.0"
 #endif
 #define QGA_PIDFILE_DEFAULT "/var/run/qemu-ga.pid"
+#define QGA_SENTINEL_BYTE 0xFF
 
 struct GAState {
     JSONMessageParser parser;
@@ -54,9 +55,10 @@ struct GAState {
 #ifdef _WIN32
     GAService service;
 #endif
+    bool delimit_response;
 };
 
-static struct GAState *ga_state;
+struct GAState *ga_state;
 
 #ifdef _WIN32
 DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
@@ -198,6 +200,11 @@ static void ga_log(const gchar *domain, GLogLevelFlags level,
     }
 }
 
+void ga_set_response_delimited(GAState *s)
+{
+    s->delimit_response = true;
+}
+
 #ifndef _WIN32
 static void become_daemon(const char *pidfile)
 {
@@ -254,7 +261,7 @@ fail:
 static int send_response(GAState *s, QObject *payload)
 {
     const char *buf;
-    QString *payload_qstr;
+    QString *payload_qstr, *response_qstr;
     GIOStatus status;
 
     g_assert(payload && s->channel);
@@ -264,10 +271,20 @@ static int send_response(GAState *s, QObject *payload)
         return -EINVAL;
     }
 
-    qstring_append_chr(payload_qstr, '\n');
-    buf = qstring_get_str(payload_qstr);
+    if (s->delimit_response) {
+        s->delimit_response = false;
+        response_qstr = qstring_new();
+        qstring_append_chr(response_qstr, QGA_SENTINEL_BYTE);
+        qstring_append(response_qstr, qstring_get_str(payload_qstr));
+        QDECREF(payload_qstr);
+    } else {
+        response_qstr = payload_qstr;
+    }
+
+    qstring_append_chr(response_qstr, '\n');
+    buf = qstring_get_str(response_qstr);
     status = ga_channel_write_all(s->channel, buf, strlen(buf));
-    QDECREF(payload_qstr);
+    QDECREF(response_qstr);
     if (status != G_IO_STATUS_NORMAL) {
         return -EIO;
     }
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 5b77fa9..7b2be2f 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -35,8 +35,6 @@
 #include "qemu-queue.h"
 #include "host-utils.h"
 
-static GAState *ga_state;
-
 static void reopen_fd_to_null(int fd)
 {
     int nullfd;
@@ -909,7 +907,6 @@ error:
 /* register init/cleanup routines for stateful command groups */
 void ga_command_state_init(GAState *s, GACommandState *cs)
 {
-    ga_state = s;
 #if defined(CONFIG_FSFREEZE)
     ga_command_state_add(cs, guest_fsfreeze_init, guest_fsfreeze_cleanup);
 #endif
diff --git a/qga/commands.c b/qga/commands.c
index b27407d..5bcceaa 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -29,6 +29,12 @@ void slog(const gchar *fmt, ...)
     va_end(ap);
 }
 
+int64_t qmp_guest_sync_delimited(int64_t id, Error **errp)
+{
+    ga_set_response_delimited(ga_state);
+    return id;
+}
+
 int64_t qmp_guest_sync(int64_t id, Error **errp)
 {
     return id;
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index b5dfa5b..304525d 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -18,6 +18,7 @@
 
 typedef struct GAState GAState;
 typedef struct GACommandState GACommandState;
+extern GAState *ga_state;
 
 void ga_command_state_init(GAState *s, GACommandState *cs);
 void ga_command_state_add(GACommandState *cs,
@@ -30,3 +31,4 @@ bool ga_logging_enabled(GAState *s);
 void ga_disable_logging(GAState *s);
 void ga_enable_logging(GAState *s);
 void slog(const gchar *fmt, ...);
+void ga_set_response_delimited(GAState *s);
-- 
1.7.4.1