[Qemu-devel] [RFC][PATCH v2 00/11] QEMU Guest Agent: QMP-based host/guest communication (virtagent)

[Qemu-devel] [RFC][PATCH v2 00/11] QEMU Guest Agent: QMP-based host/guest communication (virtagent)

[Michael Roth]

These apply on top of Anthony's glib tree, commit 03d5927deb5e6baebaade1b4c8ff2428a85e125c currently, and can also be obtained from:
git://repo.or.cz/qemu/mdroth.git qga_v2

Patches 1-8 are general json/QAPI-related fixes. Anthony, please consider pulling these into your glib tree. The json fix-ups may need further evaluation, but I'm confident they're at least an improvement. The QAPI ones are mostly trivial fix-ups.

Changes since V1:

- Added guest agent worker thread to execute RPCs in the guest. With this in place we have a reliable timeout mechanism for hung commands, currently set at 30 seconds.
- Add framework for registering init/cleanup routines for stateful RPCs to clean up after themselves after a timeout.
- Added the following RPCs: guest-file-{open,close,read,write,seek}, guest-shutdown, guest-info, and removed stubs for guest-view-file (now deprecated)
- Added GUEST_AGENT_UP/GUEST_AGENT_DOWN QMP events
- Switched to a TCP-style host-initiated 3-way handshake for channel negotiation, this simplifies client negotiation/interaction over the wire
- Added configurable log level/log file/pid file options for guest agent
- Various fixes for bugs/memory leaks and checkpatch.pl fixups

ISSUES/TODOS:

- Fix QMP proxy handling of error responses sent by guest agent, currently ignored
- Add unit tests for guest agent wire protocol
- Add unit tests for QMP interfaces
- Add host-side timeout mechanism for async QMP commands
- Return error for guest commands if guest up event has not yet been recieved
- Make QMP param names more consistent between related commands
- Clean up logging

OVERVIEW

For a better overview of what these patches are meant to accomplish, please reference the RFC for virtagent:

http://comments.gmane.org/gmane.comp.emulators.qemu/96096

These patches integrate the previous virtagent guest agent work directly in QAPI/QMP to leverage it's auto-generated marshalling code. This has numerous benefits:

 - addresses previous concerns over relying on external libraries to handle data encapsulation
 - reduces the need for manual unmarshalling of requests/responses, which makes adding new RPCs much safer/less error-prone, as well as cutting down on redundant code
 - QAPI documentation aligns completely with guest-side RPC implementation
 - is Just Better (TM)

BUILD/USAGE

build:
  ./configure --target-list=x86_64-softmmu
  make
  make qemu-ga #should be built on|for target guest

start guest:
  qemu \
  -drive file=/home/mdroth/vm/rhel6_64_base.raw,snapshot=off,if=virtio \
  -net nic,model=virtio,macaddr=52:54:00:12:34:00 \
  -net tap,script=/etc/qemu-ifup \
  -vnc :1 -m 1024 --enable-kvm \
  -chardev socket,path=/tmp/mon-qmp.sock,server,nowait,id=mon-qmp \
  -qmp mon-qmp \
  -chardev qmp_proxy,id=qmp_proxy \ 
  -device virtio-serial \
  -device virtserialport,chardev=qmp_proxy,name=qcg"

use guest agent:
  ./qemu-ga -h
  ./qemu-ga -c virtio-serial -p /dev/virtio-ports/qcg

start/use qmp:
  mdroth@illuin:~$ sudo socat unix-connect:/tmp/mon-qmp.sock readline
  {"QMP": {"version": {"qemu": {"micro": 50, "minor": 13, "major": 0}, "package": ""}, "capabilities": []}}

  {"execute":"guest-info"}
  {"return": {}}

  {"execute": "guest-info"}
  {"return": {"version": "1.0", "timeout_ms": 30000}}

  {"execute":"guest-file-open", "arguments":{"filename":"/tmp/testqga","mode":"w+"}}
  {"return": 0}
  {"execute":"guest-file-write", "arguments":{"filehandle":0,"data_b64":"aGVsbG8gd29ybGQhCg==","count":13}} // writes "hello world!\n"
  {"return": {"count": 13, "eof": false}}

  {"execute":"guest-file-open", "arguments":{"filename":"/tmp/testqga","mode":"r"}}
  {"return": 1}
  {"execute":"guest-file-read", "arguments":{"filehandle":1,"count":1024}}
  {"return": {"buf": "aGVsbG8gd29ybGQhCg==", "count": 13, "eof": true}}
  {"execute":"guest-file-close","arguments":{"filehandle":1}}
  {"return": {}}

 Makefile                        |   15 +-
 Makefile.objs                   |    1 +
 configure                       |    6 +-
 json-lexer.c                    |   33 ++-
 json-lexer.h                    |    1 +
 json-parser.c                   |    6 +-
 json-streamer.c                 |   35 ++-
 qapi-schema.json                |  140 ++++++++-
 qemu-char.c                     |   55 +++
 qemu-ga.c                       |  711 +++++++++++++++++++++++++++++++++++++++
 qemu-sockets.c                  |    2 +-
 qga/guest-agent-command-state.c |   73 ++++
 qga/guest-agent-commands.c      |  284 ++++++++++++++++
 qga/guest-agent-core.c          |  139 ++++++++
 qga/guest-agent-core.h          |   40 +++
 qga/guest-agent-worker.c        |  173 ++++++++++
 qmp-core.c                      |   13 +-
 qmp-core.h                      |    7 +-
 qmp-gen.py                      |   15 +-
 qmp-proxy-core.h                |   21 ++
 qmp-proxy.c                     |  294 ++++++++++++++++
 vl.c                            |    1 +
 22 files changed, 2023 insertions(+), 42 deletions(-)

[Qemu-devel] [RFC][PATCH v2 01/17] json-lexer: make lexer error-recovery more deterministic

[Michael Roth]

Currently when we reach an error state we effectively flush everything
fed to the lexer, which can put us in a state where we keep feeding
tokens into the parser at arbitrary offsets in the stream. This makes it
difficult for the lexer/tokenizer/parser to get back in sync when bad
input is made by the client.

With these changes we emit an error state/token up to the tokenizer as
soon as we reach an error state, and continue processing any data passed
in rather than bailing out. The reset token will be used to reset the
tokenizer and parser, such that they'll recover state as soon as the
lexer begins generating valid token sequences again.

We also map chr(192,193,245-255) to an error state here, since they are
invalid UTF-8 characters. QMP guest proxy/agent will use chr(255) to
force a flush/reset of previous input for reliable delivery of certain
events, so also we document that thoroughly here.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 json-lexer.c |   33 ++++++++++++++++++++++++++++-----
 json-lexer.h |    1 +
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/json-lexer.c b/json-lexer.c
index 3462c89..bede557 100644
--- a/json-lexer.c
+++ b/json-lexer.c
@@ -105,7 +105,8 @@ static const uint8_t json_lexer[][256] =  {
         ['u'] = IN_DQ_UCODE0,
     },
     [IN_DQ_STRING] = {
-        [1 ... 0xFF] = IN_DQ_STRING,
+        [1 ... 0xBF] = IN_DQ_STRING,
+        [0xC2 ... 0xF4] = IN_DQ_STRING,
         ['\\'] = IN_DQ_STRING_ESCAPE,
         ['"'] = JSON_STRING,
     },
@@ -144,7 +145,8 @@ static const uint8_t json_lexer[][256] =  {
         ['u'] = IN_SQ_UCODE0,
     },
     [IN_SQ_STRING] = {
-        [1 ... 0xFF] = IN_SQ_STRING,
+        [1 ... 0xBF] = IN_SQ_STRING,
+        [0xC2 ... 0xF4] = IN_SQ_STRING,
         ['\\'] = IN_SQ_STRING_ESCAPE,
         ['\''] = JSON_STRING,
     },
@@ -305,10 +307,25 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch)
             new_state = IN_START;
             break;
         case ERROR:
+            /* XXX: To avoid having previous bad input leaving the parser in an
+             * unresponsive state where we consume unpredictable amounts of
+             * subsequent "good" input, percolate this error state up to the
+             * tokenizer/parser by forcing a NULL object to be emitted, then
+             * reset state.
+             *
+             * Also note that this handling is required for reliable channel
+             * negotiation between QMP and the guest agent, since chr(0xFF)
+             * is placed at the beginning of certain events to ensure proper
+             * delivery when the channel is in an unknown state. chr(0xFF) is
+             * never a valid ASCII/UTF-8 sequence, so this should reliably
+             * induce an error/flush state.
+             */
+            lexer->emit(lexer, lexer->token, JSON_ERROR, lexer->x, lexer->y);
             QDECREF(lexer->token);
             lexer->token = qstring_new();
             new_state = IN_START;
-            return -EINVAL;
+            lexer->state = new_state;
+            return 0;
         default:
             break;
         }
@@ -334,7 +351,6 @@ int json_lexer_feed(JSONLexer *lexer, const char *buffer, size_t size)
 
     for (i = 0; i < size; i++) {
         int err;
-
         err = json_lexer_feed_char(lexer, buffer[i]);
         if (err < 0) {
             return err;
@@ -346,7 +362,14 @@ int json_lexer_feed(JSONLexer *lexer, const char *buffer, size_t size)
 
 int json_lexer_flush(JSONLexer *lexer)
 {
-    return lexer->state == IN_START ? 0 : json_lexer_feed_char(lexer, 0);
+    if (lexer->state != IN_START) {
+        lexer->state = IN_START;
+        QDECREF(lexer->token);
+        lexer->token = qstring_new();
+        return -EINVAL;
+    }
+
+    return 0;
 }
 
 void json_lexer_destroy(JSONLexer *lexer)
diff --git a/json-lexer.h b/json-lexer.h
index 3b50c46..10bc0a7 100644
--- a/json-lexer.h
+++ b/json-lexer.h
@@ -25,6 +25,7 @@ typedef enum json_token_type {
     JSON_STRING,
     JSON_ESCAPE,
     JSON_SKIP,
+    JSON_ERROR,
 } JSONTokenType;
 
 typedef struct JSONLexer JSONLexer;
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v2 02/17] json-streamer: add handling for JSON_ERROR token/state

[Michael Roth]

This allows a JSON_ERROR state to be passed to the streamer to force a
flush of the current tokens and pass a NULL token list to the parser
rather that have it churn on bad data. (Alternatively we could just not
pass it to the parser at all, but it may be useful to push there errors
up the stack. NULL token lists are not currently handled by the parser,
the next patch will address that)

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 json-streamer.c |   35 +++++++++++++++++++++++------------
 1 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/json-streamer.c b/json-streamer.c
index a6cb28f..c255c78 100644
--- a/json-streamer.c
+++ b/json-streamer.c
@@ -56,29 +56,40 @@ static void json_message_process_token(JSONLexer *lexer, QString *token, JSONTok
 
     qlist_append(parser->tokens, dict);
 
-    if (parser->brace_count < 0 ||
+    if (type == JSON_ERROR) {
+        goto out_emit_bad;
+    } else if (parser->brace_count < 0 ||
         parser->bracket_count < 0 ||
         (parser->brace_count == 0 &&
          parser->bracket_count == 0)) {
-        parser->brace_count = 0;
-        parser->bracket_count = 0;
-        parser->emit(parser, parser->tokens);
-        QDECREF(parser->tokens);
-        parser->tokens = qlist_new();
-        parser->token_size = 0;
+        goto out_emit;
     } else if (parser->token_size > MAX_TOKEN_SIZE ||
                parser->bracket_count > MAX_NESTING ||
                parser->brace_count > MAX_NESTING) {
         /* Security consideration, we limit total memory allocated per object
          * and the maximum recursion depth that a message can force.
          */
-        parser->brace_count = 0;
-        parser->bracket_count = 0;
-        parser->emit(parser, parser->tokens);
+        goto out_emit;
+    }
+
+    return;
+
+out_emit_bad:
+    /* clear out token list and tell the parser to emit and error
+     * indication by passing it a NULL list
+     */
+    QDECREF(parser->tokens);
+    parser->tokens = NULL;
+out_emit:
+    /* send current list of tokens to parser and reset tokenizer */
+    parser->brace_count = 0;
+    parser->bracket_count = 0;
+    parser->emit(parser, parser->tokens);
+    if (parser->tokens) {
         QDECREF(parser->tokens);
-        parser->tokens = qlist_new();
-        parser->token_size = 0;
     }
+    parser->tokens = qlist_new();
+    parser->token_size = 0;
 }
 
 void json_message_parser_init(JSONMessageParser *parser,
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v2 03/17] json-parser: add handling for NULL token list

[Michael Roth]

Currently a NULL token list will crash the parser, instead we have it
pass back a NULL QObject.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 json-parser.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/json-parser.c b/json-parser.c
index 58e973b..849e215 100644
--- a/json-parser.c
+++ b/json-parser.c
@@ -633,9 +633,13 @@ QObject *json_parser_parse(QList *tokens, va_list *ap)
 QObject *json_parser_parse_err(QList *tokens, va_list *ap, Error **errp)
 {
     JSONParserContext ctxt = {};
-    QList *working = qlist_copy(tokens);
+    QList *working;
     QObject *result;
 
+    if (!tokens) {
+        return NULL;
+    }
+    working = qlist_copy(tokens);
     result = parse_value(&ctxt, &working, ap);
 
     QDECREF(working);
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v2 04/17] qapi: fix function name typo in qmp-gen.py

[Michael Roth]

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qmp-gen.py |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/qmp-gen.py b/qmp-gen.py
index 90069ca..4164692 100644
--- a/qmp-gen.py
+++ b/qmp-gen.py
@@ -2328,7 +2328,7 @@ void qga_init_marshal(void)
             if not s.has_key('command'):
                 continue
             name = s['command']
-            if qmp_is_proxy_command(name):
+            if qmp_is_proxy_cmd(name):
                 ret += mcgen('''
     qga_register_command("%(name)s", &qmp_marshal_%(c_name)s);
 ''',
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v2 05/17] qapi: fix handling for null-return async callbacks

[Michael Roth]

Async commands like 'guest-ping' have NULL retvals. Handle these by
inserting an empty dictionary in the response's "return" field.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qmp-core.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/qmp-core.c b/qmp-core.c
index e33f7a4..9f3d182 100644
--- a/qmp-core.c
+++ b/qmp-core.c
@@ -922,9 +922,12 @@ void qmp_async_complete_command(QmpCommandState *cmd, QObject *retval, Error *er
     rsp = qdict_new();
     if (err) {
         qdict_put_obj(rsp, "error", error_get_qobject(err));
-    } else {
+    } else if (retval) {
         qobject_incref(retval);
         qdict_put_obj(rsp, "return", retval);
+    } else {
+        /* add empty "return" dict, this is the standard for NULL returns */
+        qdict_put_obj(rsp, "return", QOBJECT(qdict_new()));
     }
     if (cmd->tag) {
         qdict_put_obj(rsp, "tag", cmd->tag);
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v2 06/17] qapi: fix memory leak for async marshalling code

[Michael Roth]

When generating the callback function for an async command, if we expect
a QString we copy it into a native char* type, then call the completion
function. We should free it after calling the completion function, since
the completion function will later copy it into a new QString before
adding it to the response object and then passing it on to the wire.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qmp-gen.py |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/qmp-gen.py b/qmp-gen.py
index 4164692..3667ec5 100644
--- a/qmp-gen.py
+++ b/qmp-gen.py
@@ -349,6 +349,10 @@ static void qmp_%(c_name)s_cb(void *qmp__opaque, QObject *qmp__retval, Error *qm
         ret += cgen('    qmp__cb->cb(qmp__cb->opaque, qmp__err);')
     else:
         ret += cgen('    qmp__cb->cb(qmp__cb->opaque, qmp__native_retval, qmp__err);')
+    if retval != 'none' and qmp_type_should_free(retval):
+        ret += cgen('    %(free)s(qmp__native_retval);',
+                    free=qapi_free_func(retval))
+
     ret += cgen('}')
 
     return ret
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v2 07/17] qapi: qmp-gen.py, use basename of path for guard/core prefix

[Michael Roth]

To avoid errors when generating output to a seperate subdirectory, use
only the filename, minus any leading directories, when passing it into
functions to be used as a prefix for header guards, includes, etc.

Also, trim file extensions based on "." seperator instead of assuming a
single-char extension and trimming the last 2 chars

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qmp-gen.py |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/qmp-gen.py b/qmp-gen.py
index 3667ec5..eeef58c 100644
--- a/qmp-gen.py
+++ b/qmp-gen.py
@@ -2047,10 +2047,11 @@ def generate(kind, output):
     enum_types = []
     event_types = {}
     indent_level = 0
+    prefix = output.split("/")[-1].split(".")[0]
 
-    guard = '%s_H' % c_var(output[:-2]).upper()
-    core = '%s-core.h' % output[:-2]
-    header = '%s.h' % output[:-2]
+    guard = '%s_H' % c_var(prefix).upper()
+    core = '%s-core.h' % prefix
+    header = '%s.h' % prefix
 
     if kind.endswith('body') or kind.endswith('header'):
         ret = mcgen('''
@@ -2387,7 +2388,7 @@ void qcfg_options_init(void)
     return ret
 
 def main(args):
-    if len(args) != 2:
+    if len(args) < 2:
         return 1
     if not args[0].startswith('--'):
         return 1
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v2 08/17] qapi: fix Error usage in qemu-sockets.c

[Michael Roth]

Fix spurious errors due to not initializing Error pointer to NULL before
checking for errors.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qemu-sockets.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/qemu-sockets.c b/qemu-sockets.c
index dc8beeb..e709e5f 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -630,7 +630,7 @@ int unix_connect(const char *path)
 {
     QemuOpts *opts;
     int sock;
-    Error *err;
+    Error *err = NULL;
 
     opts = qemu_opts_create(&dummy_opts, NULL, 0, &err);
     if (err) {
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v2 09/17] qmp proxy: core code for proxying qmp requests to guest

[Michael Roth]

This provides a QmpProxy class, 1 instance of which is shared by all QMP
servers/sessions to send/receive QMP requests/responses between QEMU and
the QEMU guest agent.

A single qmp_proxy_send_request() is the only interface currently needed
by a QMP session, QAPI/QMP's existing async support handles all the work
of doing callbacks and routing responses to the proper session.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qapi-schema.json |    7 ++
 qmp-core.c       |    8 ++
 qmp-core.h       |    7 +-
 qmp-proxy-core.h |   21 ++++
 qmp-proxy.c      |  294 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 vl.c             |    1 +
 6 files changed, 332 insertions(+), 6 deletions(-)
 create mode 100644 qmp-proxy-core.h
 create mode 100644 qmp-proxy.c

diff --git a/qapi-schema.json b/qapi-schema.json
index de6c9a3..5292938 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1498,3 +1498,10 @@
 # Since: 0.14.0
 ##
 { 'option': 'vnc', 'data': 'VncConfig', 'implicit': 'address' }
+
+## 0.15.0 Events ##
+{ 'event': 'GUEST_AGENT_UP' }
+{ 'command': 'get-guest-agent-up-event', 'returns': 'GUEST_AGENT_UP' }
+
+{ 'event': 'GUEST_AGENT_RESET' }
+{ 'command': 'get-guest-agent-reset-event', 'returns': 'GUEST_AGENT_RESET' }
diff --git a/qmp-core.c b/qmp-core.c
index 9f3d182..dab50a1 100644
--- a/qmp-core.c
+++ b/qmp-core.c
@@ -937,7 +937,15 @@ void qmp_async_complete_command(QmpCommandState *cmd, QObject *retval, Error *er
     qemu_free(cmd);
 }
 
+extern QmpProxy *qmp_proxy_default;
+
 void qmp_guest_dispatch(const char *name, const QDict *args, Error **errp,
                         QmpGuestCompletionFunc *cb, void *opaque)
 {
+    if (!qmp_proxy_default) {
+        /* TODO: should set errp here */
+        fprintf(stderr, "qmp proxy: no guest proxy found\n");
+        return;
+    }
+    qmp_proxy_send_request(qmp_proxy_default, name, args, errp, cb, opaque);
 }
diff --git a/qmp-core.h b/qmp-core.h
index b676020..114d290 100644
--- a/qmp-core.h
+++ b/qmp-core.h
@@ -4,6 +4,7 @@
 #include "monitor.h"
 #include "qmp-marshal-types.h"
 #include "error_int.h"
+#include "qmp-proxy-core.h"
 
 struct QmpCommandState
 {
@@ -85,11 +86,5 @@ int qmp_state_get_fd(QmpState *sess);
     }                                                        \
 } while(0)
 
-typedef void (QmpGuestCompletionFunc)(void *opaque, QObject *ret_data, Error *err);
-
-void qmp_guest_dispatch(const char *name, const QDict *args, Error **errp,
-                        QmpGuestCompletionFunc *cb, void *opaque);
-
-
 #endif
 
diff --git a/qmp-proxy-core.h b/qmp-proxy-core.h
new file mode 100644
index 0000000..6afdc23
--- /dev/null
+++ b/qmp-proxy-core.h
@@ -0,0 +1,21 @@
+#ifndef QMP_PROXY_CORE_H
+#define QMP_PROXY_CORE_H
+
+#define QMP_PROXY_PATH_DEFAULT "/tmp/qmp-proxy.sock"
+
+typedef void (QmpGuestCompletionFunc)(void *opaque, QObject *ret_data,
+                                      Error *err);
+
+void qmp_guest_dispatch(const char *name, const QDict *args, Error **errp,
+                        QmpGuestCompletionFunc *cb, void *opaque);
+
+typedef struct QmpProxy QmpProxy;
+
+void qmp_proxy_send_request(QmpProxy *p, const char *name,
+                            const QDict *args, Error **errp,
+                            QmpGuestCompletionFunc *cb, void *opaque);
+QmpProxy *qmp_proxy_new(CharDriverState *chr);
+void qmp_proxy_close(QmpProxy *p);
+int qmp_proxy_read(QmpProxy *p, const uint8_t *buf, int len);
+
+#endif
diff --git a/qmp-proxy.c b/qmp-proxy.c
new file mode 100644
index 0000000..a4c7eea
--- /dev/null
+++ b/qmp-proxy.c
@@ -0,0 +1,294 @@
+/*
+ * QMP definitions for communicating with guest agent
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Michael Roth      <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include <glib.h>
+#include "qmp.h"
+#include "qmp-core.h"
+#include "qemu-queue.h"
+#include "json-parser.h"
+#include "json-streamer.h"
+#include "qemu_socket.h"
+
+#define QMP_SENTINEL 0xFF
+
+typedef struct QmpProxyRequest {
+    const char *name;
+    const QDict *args;
+    QmpGuestCompletionFunc *cb;
+    void *opaque;
+    QString *json;
+    QTAILQ_ENTRY(QmpProxyRequest) entry;
+} QmpProxyRequest;
+
+struct QmpProxy {
+    int session_id;
+    JSONMessageParser parser;
+    GString *tx;
+    QEMUTimer *tx_timer;
+    int tx_timer_interval;
+    QTAILQ_HEAD(, QmpProxyRequest) requests;
+    CharDriverState *chr;
+};
+
+static GuestAgentUpEvent guest_agent_up_event;
+static GuestAgentResetEvent guest_agent_reset_event;
+static void qmp_proxy_write(QmpProxy *p);
+
+GuestAgentUpEvent *qmp_get_guest_agent_up_event(Error **errp)
+{
+    return &guest_agent_up_event;
+}
+
+GuestAgentResetEvent *qmp_get_guest_agent_reset_event(Error **errp)
+{
+    return &guest_agent_reset_event;
+}
+
+static int qmp_proxy_cancel_request(QmpProxy *p, QmpProxyRequest *r)
+{
+    if (r && r->cb) {
+        r->cb(r->opaque, NULL, NULL);
+    }
+
+    return 0;
+}
+
+static int qmp_proxy_cancel_all(QmpProxy *p)
+{
+    QmpProxyRequest *r, *tmp;
+    QTAILQ_FOREACH_SAFE(r, &p->requests, entry, tmp) {
+        qmp_proxy_cancel_request(p, r);
+        QTAILQ_REMOVE(&p->requests, r, entry);
+    }
+
+    return 0;
+}
+
+static void qmp_proxy_send_control_event(QmpProxy *p, QDict *evt)
+{
+    QString *json = qobject_to_json(QOBJECT(evt));;
+
+    /* currently there's no reason to send any queued data or control
+     * events ahead of the most recent event, so empty the buffer first
+     */
+    g_string_truncate(p->tx, p->tx->len);
+    g_string_append_c(p->tx, QMP_SENTINEL);
+    g_string_append(p->tx, qstring_get_str(json));
+    QDECREF(json);
+    qmp_proxy_write(p);
+}
+
+static void qmp_proxy_send_host_ack(QmpProxy *p, int guest_sid)
+{
+    QDict *evt = qdict_new();
+
+    qdict_put_obj(evt, "_control_event",
+                  QOBJECT(qstring_from_str("host_ack")));
+    qdict_put_obj(evt, "_control_arg_host_sid",
+                  QOBJECT(qint_from_int(p->session_id)));
+    qdict_put_obj(evt, "_control_arg_guest_sid",
+                  QOBJECT(qint_from_int(guest_sid)));
+
+    qmp_proxy_send_control_event(p, evt);
+}
+
+static void qmp_proxy_send_host_init(QmpProxy *p)
+{
+    QDict *evt = qdict_new();
+
+    p->session_id = g_random_int_range(1, G_MAXINT32);
+    qdict_put_obj(evt, "_control_event",
+                  QOBJECT(qstring_from_str("host_init")));
+    qdict_put_obj(evt, "_control_arg_host_sid",
+                  QOBJECT(qint_from_int(p->session_id)));
+
+    qmp_proxy_send_control_event(p, evt);
+}
+
+static void qmp_proxy_process_control_event(QmpProxy *p, const QDict *evt)
+{
+    const char *cmd;
+    int host_sid, guest_sid;
+
+    cmd = qdict_get_try_str(evt, "_control_event");
+    if (!cmd) {
+        fprintf(stderr, "received NULL control event\n");
+    } else if (strcmp(cmd, "guest_ack") == 0) {
+        host_sid = qdict_get_try_int(evt, "_control_arg_host_sid", 0);
+        if (!host_sid) {
+            fprintf(stderr, "invalid guest_ack: wrong host sid\n");
+            return;
+        }
+        /* guest responded to host_init, return a host_ack */
+        /* reset outstanding requests, then send an ack with the
+         * session id they passed us
+         */
+        guest_sid = qdict_get_try_int(evt, "_control_arg_guest_sid", 0);
+        if (!guest_sid) {
+            fprintf(stderr, "invalid guest_ack: missing guest sid\n");
+            return;
+        }
+        qmp_proxy_send_host_ack(p, guest_sid);
+    } else if (strcmp(cmd, "guest_up") == 0) {
+        /* guest agent [re-]started, cancel any outstanding request and
+         * start negotiation */
+        /* TODO: should generate a qmp event for this as well */
+        signal_notify(&guest_agent_up_event);
+        signal_notify(&guest_agent_reset_event);
+        qmp_proxy_cancel_all(p);
+        qmp_proxy_send_host_init(p);
+    } else if (strcmp(cmd, "guest_reset") == 0) {
+        /* guest agent reset it's state (likely due to a command timeout)
+         * cancel all outstanding requests */
+        signal_notify(&guest_agent_reset_event);
+        qmp_proxy_cancel_all(p);
+    } else {
+        fprintf(stderr, "received unknown control event\n");
+    }
+}
+
+static void qmp_proxy_process_event(JSONMessageParser *parser, QList *tokens)
+{
+    QmpProxy *p = container_of(parser, QmpProxy, parser);
+    QmpProxyRequest *r;
+    QObject *obj;
+    QDict *qdict;
+    Error *err = NULL;
+
+    fprintf(stderr, "qmp proxy: called\n");
+    obj = json_parser_parse_err(tokens, NULL, &err);
+    if (!obj) {
+        fprintf(stderr, "qmp proxy: failed to parse\n");
+        return;
+    } else {
+        fprintf(stderr, "qmp proxy: parse successful\n");
+        qdict = qobject_to_qdict(obj);
+    }
+
+    if (qdict_haskey(qdict, "_control_event")) {
+        /* handle transport-level control event */
+        qmp_proxy_process_control_event(p, qdict);
+    } else if (qdict_haskey(qdict, "return")) {
+        /* handle proxied qmp command response */
+        fprintf(stderr, "received return\n");
+        r = QTAILQ_FIRST(&p->requests);
+        if (!r) {
+            fprintf(stderr, "received return, but no request queued\n");
+            return;
+        }
+        /* XXX: can't assume type here */
+        fprintf(stderr, "recieved response for cmd: %s\nreturn: %s\n",
+                r->name, qstring_get_str(qobject_to_json(QOBJECT(qdict))));
+        r->cb(r->opaque, qdict_get(qdict, "return"), NULL);
+        QTAILQ_REMOVE(&p->requests, r, entry);
+        qemu_free(r);
+        fprintf(stderr, "done handling response\n");
+    } else {
+        fprintf(stderr, "received invalid payload format\n");
+    }
+
+    QDECREF(qdict);
+}
+
+static void qmp_proxy_write_handler(void *opaque)
+{
+    QmpProxy *p = opaque;
+    int max, len;
+
+    if (!p->tx->len) {
+        return;
+    }
+
+    max = qemu_chr_can_read(p->chr);
+    if (max) {
+        len = MIN(max, p->tx->len);
+        qemu_chr_read(p->chr, (uint8_t *)p->tx->str, len);
+        g_string_erase(p->tx, 0, len);
+    }
+
+    if (p->tx->len) {
+        /* defer transfer of remaining data */
+        qemu_mod_timer(p->tx_timer,
+                       qemu_get_clock(rt_clock) + p->tx_timer_interval);
+    }
+}
+
+int qmp_proxy_read(QmpProxy *p, const uint8_t *buf, int len)
+{
+    return json_message_parser_feed(&p->parser, (const char *)buf, len);
+}
+
+void qmp_proxy_write(QmpProxy *p)
+{
+    /* more stuff in our buffer, kick the write handler */
+    qmp_proxy_write_handler(p);
+}
+
+void qmp_proxy_send_request(QmpProxy *p, const char *name,
+                            const QDict *args, Error **errp,
+                            QmpGuestCompletionFunc *cb, void *opaque)
+{
+    QmpProxyRequest *r = qemu_mallocz(sizeof(QmpProxyRequest));
+    QDict *payload = qdict_new();
+    QString *json;
+
+    /* TODO: don't really need to hold on to name/args after encoding */
+    r->name = name;
+    r->args = args;
+    r->cb = cb;
+    r->opaque = opaque;
+
+    qdict_put_obj(payload, "execute", QOBJECT(qstring_from_str(r->name)));
+    /* TODO: casting a const so we can add it to our dictionary. bad. */
+    qdict_put_obj(payload, "arguments", QOBJECT((QDict *)args));
+
+    json = qobject_to_json(QOBJECT((QDict *)payload));
+    if (!json) {
+        goto out_bad;
+    }
+
+    QTAILQ_INSERT_TAIL(&p->requests, r, entry);
+    g_string_append(p->tx, qstring_get_str(json));
+    QDECREF(json);
+    qmp_proxy_write(p);
+    return;
+
+out_bad:
+    cb(opaque, NULL, NULL);
+    qemu_free(r);
+}
+
+QmpProxy *qmp_proxy_new(CharDriverState *chr)
+{
+    QmpProxy *p = qemu_mallocz(sizeof(QmpProxy));
+
+    signal_init(&guest_agent_up_event);
+    signal_init(&guest_agent_reset_event);
+
+    /* there's a reason for this madness */
+    p->tx_timer = qemu_new_timer(rt_clock, qmp_proxy_write_handler, p);
+    p->tx_timer_interval = 10;
+    p->tx = g_string_new("");
+    p->chr = chr;
+    json_message_parser_init(&p->parser, qmp_proxy_process_event);
+    QTAILQ_INIT(&p->requests);
+
+    return p;
+}
+
+void qmp_proxy_close(QmpProxy *p)
+{
+    qmp_proxy_cancel_all(p);
+    g_string_free(p->tx, TRUE);
+    qemu_free(p);
+}
diff --git a/vl.c b/vl.c
index 3fdc7cc..e8c49ef 100644
--- a/vl.c
+++ b/vl.c
@@ -231,6 +231,7 @@ int ctrl_grab = 0;
 unsigned int nb_prom_envs = 0;
 const char *prom_envs[MAX_PROM_ENVS];
 int boot_menu;
+QmpProxy *qmp_proxy_default;
 
 ShutdownEvent qemu_shutdown_event;
 ResetEvent qemu_reset_event;
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v2 10/17] qmp proxy: add qmp_proxy chardev

[Michael Roth]

This allows qemu to be started with guest agent support via:

qemu -chardev qmp_proxy,id=qmp_proxy -device ...,chardev=qmp_proxy

Writes to the guest agent are buffered, with deferred work handled by a
timer. Writes from the guest agent to host/proxy are passed directly
into a JSON streamer object/buffer.

The chardev is intended for use with virtio-serial or isa-serial
channels. Other uses may be possible with appropriate changes to guest
agent.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qemu-char.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index d301925..27b1252 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2276,6 +2276,60 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
 }
 
 /***********************************************************/
+/* QMP host->guest proxy chardev */
+#include "qmp-proxy-core.h"
+#include "json-streamer.h"
+
+extern QmpProxy *qmp_proxy_default;
+
+static int qmp_proxy_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
+{
+    QmpProxy *p = chr->opaque;
+
+    qmp_proxy_read(p, buf, len);
+
+    return len;
+}
+
+static void qmp_proxy_chr_close(CharDriverState *chr)
+{
+    QmpProxy *p = chr->opaque;
+
+    qmp_proxy_default = NULL;
+    qmp_proxy_close(p);
+    qemu_chr_event(chr, CHR_EVENT_CLOSED);
+}
+
+static CharDriverState *qemu_chr_open_qmp_proxy(QemuOpts *opts)
+{
+    CharDriverState *chr = qemu_mallocz(sizeof(CharDriverState));
+    QmpProxy *p;
+
+    /* initialize the qmp guest proxy */
+    if (qmp_proxy_default) {
+        fprintf(stderr, "error, multiple qmp guest proxies are not allowed\n");
+        goto err;
+    }
+    p = qmp_proxy_default = qmp_proxy_new(chr);
+    if (p == NULL) {
+        fprintf(stderr, "error initializing qmp guest proxy\n");
+        goto err;
+    }
+
+    chr->opaque = p;
+    chr->chr_write = qmp_proxy_chr_write;
+    chr->chr_close = qmp_proxy_chr_close;
+    qemu_chr_generic_open(chr);
+
+    return chr;
+err:
+    if (chr) {
+        qemu_free(chr);
+    }
+    return NULL;
+}
+
+/***********************************************************/
 /* Memory chardev */
 typedef struct {
     size_t outbuf_size;
@@ -2495,6 +2549,7 @@ static const struct {
     || defined(__FreeBSD_kernel__)
     { .name = "parport",   .open = qemu_chr_open_pp },
 #endif
+    { .name = "qmp_proxy", .open = qemu_chr_open_qmp_proxy },
 };
 
 CharDriverState *qemu_chr_open_opts(QemuOpts *opts,
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v2 11/17] qmp proxy: build QEMU with qmp proxy

[Michael Roth]

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 Makefile.objs |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/Makefile.objs b/Makefile.objs
index df7e670..f143bd8 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -103,6 +103,7 @@ common-obj-y += block-migration.o
 common-obj-y += pflib.o
 common-obj-y += qmp-marshal.o qmp-marshal-types.o
 common-obj-y += qmp-marshal-types-core.o qmp-core.o
+common-obj-y += qmp-proxy.o
 common-obj-y += hmp.o
 
 common-obj-$(CONFIG_BRLAPI) += baum.o
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v2 12/17] guest agent: worker thread class

[Michael Roth]

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/guest-agent-worker.c |  173 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 173 insertions(+), 0 deletions(-)
 create mode 100644 qga/guest-agent-worker.c

diff --git a/qga/guest-agent-worker.c b/qga/guest-agent-worker.c
new file mode 100644
index 0000000..e3295da
--- /dev/null
+++ b/qga/guest-agent-worker.c
@@ -0,0 +1,173 @@
+/*
+ * QEMU Guest Agent worker thread interfaces
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Michael Roth      <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include <glib.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <stdbool.h>
+#include <pthread.h>
+#include <errno.h>
+#include <string.h>
+#include "guest-agent.h"
+#include "../error.h"
+
+struct GAWorker {
+    pthread_t thread;
+    ga_worker_func execute;
+    pthread_mutex_t input_mutex;
+    pthread_cond_t input_avail_cond;
+    void *input;
+    bool input_avail;
+    pthread_mutex_t output_mutex;
+    pthread_cond_t output_avail_cond;
+    void *output;
+    Error *output_error;
+    bool output_avail;
+};
+
+static void *worker_run(void *worker_p)
+{
+    GAWorker *worker = worker_p;
+    Error *err;
+    void *input, *output;
+
+    while (1) {
+        /* wait for input */
+        pthread_mutex_lock(&worker->input_mutex);
+        while (!worker->input_avail) {
+            pthread_cond_wait(&worker->input_avail_cond, &worker->input_mutex);
+        }
+        input = worker->input;
+        worker->input_avail = false;
+        pthread_mutex_unlock(&worker->input_mutex);
+
+        /* process input. input points to shared data, so if we ever add
+         * asynchronous dispatch, we'll need to copy the input instead
+         */
+        worker->execute(input, &output, &err);
+
+        /* signal waiters */
+        pthread_mutex_lock(&worker->output_mutex);
+        worker->output = output;
+        worker->output_error = err;
+        worker->output_avail = true;
+        pthread_cond_signal(&worker->output_avail_cond);
+        pthread_mutex_unlock(&worker->output_mutex);
+    }
+
+    return NULL;
+}
+
+static void ga_worker_set_input(GAWorker *worker, void *input)
+{
+    pthread_mutex_lock(&worker->input_mutex);
+
+    /* provide input for thread, and signal it */
+    worker->input = input;
+    worker->input_avail = true;
+    pthread_cond_signal(&worker->input_avail_cond);
+
+    pthread_mutex_unlock(&worker->input_mutex);
+}
+
+static bool ga_worker_get_output(GAWorker *worker, void **output, int timeout)
+{
+    struct timespec ts;
+    GTimeVal tv;
+    bool timed_out = false;
+    int ret;
+
+    pthread_mutex_lock(&worker->output_mutex);
+
+    while (!worker->output_avail) {
+        if (timeout > 0) {
+            g_get_current_time(&tv);
+            g_time_val_add(&tv, timeout * 1000);
+            ts.tv_sec = tv.tv_sec;
+            ts.tv_nsec = tv.tv_usec * 1000;
+            ret = pthread_cond_timedwait(&worker->output_avail_cond,
+                                         &worker->output_mutex, &ts);
+            if (ret == ETIMEDOUT) {
+                timed_out = true;
+                goto out;
+            }
+        } else {
+            ret = pthread_cond_wait(&worker->output_avail_cond,
+                                    &worker->output_mutex);
+        }
+    }
+
+    /* handle output from thread */
+    worker->output_avail = false;
+    *output = worker->output;
+
+out:
+    pthread_mutex_unlock(&worker->output_mutex);
+    return timed_out;
+}
+
+bool ga_worker_dispatch(GAWorker *worker, void *input, void *output,
+                        int timeout, Error **errp)
+{
+    ga_worker_set_input(worker, input);
+    return ga_worker_get_output(worker, output, timeout);
+}
+
+static void ga_worker_start(GAWorker *worker)
+{
+    int ret;
+
+    pthread_cond_init(&worker->input_avail_cond, NULL);
+    pthread_cond_init(&worker->output_avail_cond, NULL);
+    pthread_mutex_init(&worker->input_mutex, NULL);
+    pthread_mutex_init(&worker->output_mutex, NULL);
+    worker->output_avail = false;
+    worker->input_avail = false;
+
+    ret = pthread_create(&worker->thread, NULL, worker_run, worker);
+    if (ret == -1) {
+        g_error("error: %s", strerror(errno));
+    }
+}
+
+static void ga_worker_stop(GAWorker *worker)
+{
+    int ret;
+    void *status;
+
+    ret = pthread_cancel(worker->thread);
+    if (ret == -1) { 
+        g_error("pthread_cancel() failed: %s", strerror(errno));
+    }
+
+    ret = pthread_join(worker->thread, &status);
+    if (ret == -1) { 
+        g_error("pthread_join() failed: %s", strerror(errno));
+    }
+    /* TODO: should *_destroy pthread data structures here */
+}
+
+GAWorker *ga_worker_new(ga_worker_func func)
+{
+    GAWorker *worker = g_malloc0(sizeof(GAWorker));
+
+    g_assert(func);
+    worker->execute = func;
+    ga_worker_start(worker);
+
+    return worker;
+}
+
+void ga_worker_cleanup(GAWorker *worker)
+{
+    ga_worker_stop(worker);
+    g_free(worker);
+}
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v2 13/17] guest agent: command state class

[Michael Roth]

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/guest-agent-command-state.c |   73 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 73 insertions(+), 0 deletions(-)
 create mode 100644 qga/guest-agent-command-state.c

diff --git a/qga/guest-agent-command-state.c b/qga/guest-agent-command-state.c
new file mode 100644
index 0000000..997355e
--- /dev/null
+++ b/qga/guest-agent-command-state.c
@@ -0,0 +1,73 @@
+/*
+ * QEMU Guest Agent command state interfaces
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Michael Roth      <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include <glib.h>
+#include "guest-agent.h"
+
+struct GACommandState {
+    GSList *groups;
+};
+
+typedef struct GACommandGroup {
+    void (*init)(void);
+    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));
+    cg->init = init;
+    cg->cleanup = cleanup;
+    cs->groups = g_slist_append(cs->groups, cg);
+}
+
+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;
+}
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v2 14/17] guest agent: core marshal/dispatch interfaces

[Michael Roth]

These are basically a stripped-down, guest-side analogue to what's in
qmp_core.c: definitions used by qmp-gen.py-generated marshalling code to
handle dispatch and a registration of command->function mappings (minus
all the bits related to Qmp sessions/server/events).

As a result of that, there is a little bit of code duplication here.
It wouldn't be difficult to make the common bits shareable, but there
isn't much.

The guest agent will use these interfaces to handle dispatch/execution
of requests it gets from the proxy.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/guest-agent-core.c |  139 ++++++++++++++++++++++++++++++++++++++++++++++++
 qga/guest-agent-core.h |   40 ++++++++++++++
 2 files changed, 179 insertions(+), 0 deletions(-)
 create mode 100644 qga/guest-agent-core.c
 create mode 100644 qga/guest-agent-core.h

diff --git a/qga/guest-agent-core.c b/qga/guest-agent-core.c
new file mode 100644
index 0000000..55498b1
--- /dev/null
+++ b/qga/guest-agent-core.c
@@ -0,0 +1,139 @@
+/*
+ * QEMU Guest Agent core functions
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Adam Litke        <aglitke@linux.vnet.ibm.com>
+ *  Michael Roth      <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "guest-agent-core.h"
+
+typedef enum QmpCommandType {
+    QCT_NORMAL,
+    QCT_ASYNC,
+} QmpCommandType;
+
+typedef struct QmpCommand {
+    const char *name;
+    QmpCommandType type;
+    QmpCommandFunc *fn;
+    QmpAsyncCommandFunc *afn;
+    QTAILQ_ENTRY(QmpCommand) node;
+} QmpCommand;
+
+static QTAILQ_HEAD(, QmpCommand) qmp_commands =
+    QTAILQ_HEAD_INITIALIZER(qmp_commands);
+
+QmpMarshalState *qmp_state_get_mstate(QmpState *sess)
+{
+    return NULL;
+}
+
+void qga_register_command(const char *name, QmpCommandFunc *fn)
+{
+    QmpCommand *cmd = qemu_mallocz(sizeof(*cmd));
+
+    cmd->name = name;
+    cmd->type = QCT_NORMAL;
+    cmd->fn = fn;
+    QTAILQ_INSERT_TAIL(&qmp_commands, cmd, node);
+}
+
+static QmpCommand *qmp_find_command(const char *name)
+{
+    QmpCommand *i;
+
+    QTAILQ_FOREACH(i, &qmp_commands, node) {
+        if (strcmp(i->name, name) == 0) {
+            return i;
+        }
+    }
+    return NULL;
+}
+
+static QObject *qga_dispatch_err(QObject *request, Error **errp)
+{
+    const char *command;
+    QDict *args, *dict;
+    QmpCommand *cmd;
+    QObject *ret = NULL;
+
+    if (qobject_type(request) != QTYPE_QDICT) {
+        error_set(errp, QERR_JSON_PARSE_ERROR, "request is not a dictionary");
+        goto out;
+    }
+
+    dict = qobject_to_qdict(request);
+    if (!qdict_haskey(dict, "execute")) {
+        error_set(errp, QERR_JSON_PARSE_ERROR, "no execute key");
+        goto out;
+    }
+
+    command = qdict_get_str(dict, "execute");
+    cmd = qmp_find_command(command);
+    if (cmd == NULL) {
+        error_set(errp, QERR_COMMAND_NOT_FOUND, command);
+        goto out;
+    }
+
+    if (!qdict_haskey(dict, "arguments")) {
+        args = qdict_new();
+    } else {
+        args = qdict_get_qdict(dict, "arguments");
+        QINCREF(args);
+    }
+
+    switch (cmd->type) {
+    case QCT_NORMAL:
+        cmd->fn(NULL, args, &ret, errp);
+        if (ret == NULL) {
+            ret = QOBJECT(qdict_new());
+        }
+        break;
+    case QCT_ASYNC: {
+        QmpCommandState *s = qemu_mallocz(sizeof(*s));
+        /* FIXME save async commands and do something
+         * smart if disconnect occurs before completion
+         */
+        s->tag = NULL;
+        if (qdict_haskey(dict, "tag")) {
+            s->tag = qdict_get(dict, "tag");
+            qobject_incref(s->tag);
+        }
+        cmd->afn(args, errp, s);
+        ret = NULL;
+    }
+        break;
+    }
+
+    QDECREF(args);
+
+out:
+    return ret;
+}
+
+QObject *qga_dispatch(QObject *request, Error **errp)
+{
+    Error *err = NULL;
+    QObject *ret;
+    QDict *rsp;
+
+    ret = qga_dispatch_err(request, &err);
+
+    rsp = qdict_new();
+    if (err) {
+        qdict_put_obj(rsp, "error", error_get_qobject(err));
+        error_free(err);
+    } else if (ret) {
+        qdict_put_obj(rsp, "return", ret);
+    } else {
+        QDECREF(rsp);
+        return NULL;
+    }
+
+    return QOBJECT(rsp);
+}
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
new file mode 100644
index 0000000..51ae798
--- /dev/null
+++ b/qga/guest-agent-core.h
@@ -0,0 +1,40 @@
+/*
+ * QEMU Guest Agent core declarations
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Adam Litke        <aglitke@linux.vnet.ibm.com>
+ *  Michael Roth      <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "qmp-core.h"
+#include "qapi-types-core.h"
+#include "qmp-marshal-types-core.h"
+#include "qemu-common.h"
+#include "qdict.h"
+
+#define QGA_VERSION "1.0"
+
+typedef struct GAState GAState;
+typedef struct GACommandState GACommandState;
+typedef struct GAWorker GAWorker;
+typedef void (*ga_worker_func)(void *input, void *output, Error **errp);
+
+QObject *qga_dispatch(QObject *obj, Error **errp);
+void qga_register_command(const char *name, QmpCommandFunc *fn);
+void qga_init_marshal(void);
+void ga_command_state_init(GAState *s, GACommandState *cs);
+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);
+GAWorker *ga_worker_new(ga_worker_func func);
+void ga_worker_cleanup(GAWorker *worker);
+bool ga_worker_dispatch(GAWorker *worker, void *input, void *output,
+                        int timeout, Error **errp);
+int ga_get_timeout(GAState *s);
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v2 15/17] guest agent: qemu-ga daemon

[Michael Roth]

This is the actual guest daemon, it listens for requests over a
virtio-serial/isa-serial/unix socket channel and routes them through
to dispatch routines, and writes the results back to the channel in
a manner similar to QMP.

A shorthand invocation:

  qemu-ga -d

Is equivalent to:

  qemu-ga -c virtio-serial -p /dev/virtio-ports/org.qemu.guest_agent \
          -p /var/run/qemu-guest-agent.pid -d

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qemu-ga.c |  711 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 711 insertions(+), 0 deletions(-)
 create mode 100644 qemu-ga.c

diff --git a/qemu-ga.c b/qemu-ga.c
new file mode 100644
index 0000000..800d16b
--- /dev/null
+++ b/qemu-ga.c
@@ -0,0 +1,711 @@
+/*
+ * QEMU Guest Agent
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Adam Litke        <aglitke@linux.vnet.ibm.com>
+ *  Michael Roth      <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include <stdlib.h>
+#include <stdio.h>
+#include <stdbool.h>
+#include <glib.h>
+#include <gio/gio.h>
+#include <getopt.h>
+#include <termios.h>
+#include <syslog.h>
+#include "qemu_socket.h"
+#include "json-streamer.h"
+#include "json-parser.h"
+#include "qga/guest-agent.h"
+
+#define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent"
+#define QGA_PIDFILE_DEFAULT "/var/run/qemu-va.pid"
+#define QGA_BAUDRATE_DEFAULT B38400 /* for isa-serial channels */
+#define QGA_TIMEOUT_DEFAULT 30*1000 /* ms */
+
+struct GAState {
+    bool active;
+    int session_id;
+    const char *proxy_path;
+    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;
+    GAWorker *worker;
+    int timeout_ms;
+    GLogLevelFlags log_level;
+    FILE *log_file;
+};
+
+static void usage(const char *cmd)
+{
+    printf(
+"Usage: %s -c <channel_opts>\n"
+"QEMU Guest Agent %s\n"
+"\n"
+"  -c, --channel     channel method: one of unix-connect, virtio-serial, or\n"
+"                    isa-serial (virtio-serial is the default)\n"
+"  -p, --path        channel path (%s is the default for virtio-serial)\n"
+"  -l, --logfile     set logfile path, logs to stderr by default\n"
+"  -f, --pidfile     specify pidfile (default is %s)\n"
+"  -v, --verbose     log extra debugging information\n"
+"  -V, --version     print version information and exit\n"
+"  -d, --daemonize   become a daemon\n"
+"  -h, --help        display this help and exit\n"
+"\n"
+"Report bugs to <mdroth@linux.vnet.ibm.com>\n"
+    , cmd, QGA_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT);
+}
+
+static void conn_channel_close(GAState *s);
+
+static const char *ga_log_level_str(GLogLevelFlags level)
+{
+    switch (level & G_LOG_LEVEL_MASK) {
+        case G_LOG_LEVEL_ERROR:     return "error";
+        case G_LOG_LEVEL_CRITICAL:  return "critical";
+        case G_LOG_LEVEL_WARNING:   return "warning";
+        case G_LOG_LEVEL_MESSAGE:   return "message";
+        case G_LOG_LEVEL_INFO:      return "info";
+        case G_LOG_LEVEL_DEBUG:     return "debug";
+        default:                    return "user";
+    }
+}
+
+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);
+
+    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);
+    }
+}
+
+int ga_get_timeout(GAState *s)
+{
+    return s->timeout_ms;
+}
+
+static void become_daemon(const char *pidfile)
+{
+    pid_t pid, sid;
+    int pidfd;
+    char *pidstr = NULL;
+
+    pid = fork();
+    if (pid < 0) {
+        exit(EXIT_FAILURE);
+    }
+    if (pid > 0) {
+        exit(EXIT_SUCCESS);
+    }
+
+    pidfd = open(pidfile, O_CREAT|O_RDWR, S_IRUSR|S_IWUSR);
+    if (!pidfd || lockf(pidfd, F_TLOCK, 0)) {
+        g_error("Cannot lock pid file");
+    }
+
+    if (ftruncate(pidfd, 0) || lseek(pidfd, 0, SEEK_SET)) {
+        g_critical("Cannot truncate pid file");
+        goto fail;
+    }
+    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;
+    }
+
+    umask(0);
+    sid = setsid();
+    if (sid < 0) {
+        goto fail;
+    }
+    if ((chdir("/")) < 0) {
+        goto fail;
+    }
+
+    close(STDIN_FILENO);
+    close(STDOUT_FILENO);
+    close(STDERR_FILENO);
+    return;
+
+fail:
+    if (pidstr) {
+        free(pidstr);
+    }
+    unlink(pidfile);
+    g_error("failed to daemonize");
+}
+
+static int conn_channel_send_payload(GIOChannel *channel, QObject *payload)
+{
+    gsize count, written = 0;
+    int ret;
+    const char *buf;
+    QString *payload_qstr;
+    GIOStatus status;
+    GError *err = NULL;
+
+    if (!payload || !channel) {
+        ret = -EINVAL;
+        goto out;
+    }
+
+    payload_qstr = qobject_to_json(payload);
+    if (!payload_qstr) {
+        ret = -EINVAL;
+        goto out;
+    }
+
+    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 sending payload: %s", err->message);
+            ret = err->code;
+            goto out_free;
+        }
+        if (status == G_IO_STATUS_NORMAL) {
+            count -= written;
+        } else if (status == G_IO_STATUS_ERROR || status == G_IO_STATUS_EOF) {
+            ret = -EPIPE;
+            goto out_free;
+        }
+    }
+
+    status = g_io_channel_write_chars(channel, (char *)"\n", 1, &written, &err);
+    if (err != NULL) {
+        g_warning("error sending newline: %s", err->message);
+        ret = err->code;
+        goto out_free;
+    } else if (status == G_IO_STATUS_ERROR || status == G_IO_STATUS_EOF) {
+        ret = -EPIPE;
+        goto out_free;
+    }
+
+    g_io_channel_flush(channel, &err);
+    if (err != NULL) {
+        g_warning("error flushing payload: %s", err->message);
+        ret = err->code;
+        goto out_free;
+    }
+
+    ret = 0;
+
+out_free:
+    QDECREF(payload_qstr);
+out:
+    return ret;
+}
+
+/* when inactive, we keep reading channel, but ignore all non-control messages
+ * until channel is active again (i.e. negotiation is complete)
+ */
+static void set_channel_active(GAState *s, bool active)
+{
+    g_debug("channel active state: %d", active);
+    s->active = active;
+}
+
+static void send_guest_up(GAState *s)
+{
+    QDict *payload = qdict_new();
+    int ret;
+
+    qdict_put_obj(payload, "_control_event",
+                  QOBJECT(qstring_from_str("guest_up")));
+
+    ret = conn_channel_send_payload(s->conn_channel, QOBJECT(payload));
+    if (ret) {
+        g_warning("failed to send guest init, resetting connection");
+        conn_channel_close(s);
+    }
+}
+
+/* host started channel negotiation, process/acknowledge it */
+static void send_guest_ack(GAState *s, int host_sid)
+{
+    QDict *payload = qdict_new();
+    int ret;
+
+    g_debug("[re]negotiating connection");
+
+    g_assert(s && s->conn_channel);
+    set_channel_active(s, false);
+    s->session_id = g_random_int_range(1, G_MAXINT32);
+
+    qdict_put_obj(payload, "_control_event",
+                  QOBJECT(qstring_from_str("guest_ack")));
+    qdict_put_obj(payload, "_control_arg_guest_sid",
+                  QOBJECT(qint_from_int(s->session_id)));
+    qdict_put_obj(payload, "_control_arg_host_sid",
+                  QOBJECT(qint_from_int(host_sid)));
+
+    ret = conn_channel_send_payload(s->conn_channel, QOBJECT(payload));
+    if (ret) {
+        g_warning("failed to send guest init, resetting connection");
+        conn_channel_close(s);
+    }
+}
+
+/* process transport-level events */
+static void process_control_event(GAState *s, const QDict *qdict)
+{
+    const char *cmd;
+    int sid;
+
+    g_assert(s && qdict);
+    cmd = qdict_get_try_str(qdict, "_control_event");
+    if (!cmd) {
+        g_warning("received NULL transport event");
+    } else if (strcmp(cmd, "host_init") == 0) {
+        sid = qdict_get_try_int(qdict, "_control_arg_host_sid", 0);
+        /* host started negotiation, acknowledge it */
+        send_guest_ack(s, sid);
+        set_channel_active(s, false);
+    } else if (strcmp(cmd, "host_ack") == 0) {
+        /* host attempting to complete negotiation, process it */
+        sid = qdict_get_try_int(qdict, "_control_arg_guest_sid", 0);
+        if (sid != s->session_id) {
+            g_warning("received host ack for incorrect session id");
+        } else {
+            set_channel_active(s, true);
+        }
+    } else {
+        g_debug("recieved unknown control event: %s", cmd);
+    }
+}
+
+static void process_command_worker(void *input, void *output, Error **errp)
+{
+    QDict *req = input;
+    QObject **rsp = output;
+
+    g_assert(req && rsp);
+    *rsp = qga_dispatch(QOBJECT(req), errp);
+}
+
+static void process_command(GAState *s, QDict *req)
+{
+    QObject *rsp = NULL;
+    Error *err = NULL;
+    bool timeout;
+    int ret;
+
+    g_assert(req);
+    g_debug("processing command");
+    timeout = ga_worker_dispatch(s->worker, req, &rsp, s->timeout_ms, &err);
+    if (timeout) {
+        g_warning("command timed out");
+    } else if (rsp) {
+        if (err) {
+            g_warning("command failed: %s", error_get_pretty(err));
+        }
+        g_debug("response: %s", qstring_get_str(qobject_to_json(rsp)));
+        ret = conn_channel_send_payload(s->conn_channel, rsp);
+        if (ret) {
+            g_warning("error sending payload: %s", strerror(ret));
+        }
+        qobject_decref(rsp);
+    } else {
+        g_warning("error getting response");
+    }
+}
+
+/* handle requests/control events coming in over the channel */
+static void process_event(JSONMessageParser *parser, QList *tokens)
+{
+    GAState *s = container_of(parser, GAState, parser);
+    QObject *obj;
+    QDict *qdict;
+    Error *err = NULL;
+
+    g_assert(s && parser);
+
+    g_debug("process_event: called");
+    obj = json_parser_parse_err(tokens, NULL, &err);
+    if (!obj || qobject_type(obj) != QTYPE_QDICT) {
+        g_warning("failed to parse event");
+        return;
+    } else {
+        g_debug("parse successful");
+        qdict = qobject_to_qdict(obj);
+        g_assert(qdict);
+    }
+
+    /* check for transport-only commands/events */
+    if (qdict_haskey(qdict, "_control_event")) {
+        process_control_event(s, qdict);
+        goto out_free;
+    }
+
+    /* ignore any non-control-related events/objects */
+    if (!s->active) {
+        g_debug("ignoring non-control event, still awaiting host ack");
+        goto out_free;
+    }
+
+    /* handle host->guest commands */
+    if (qdict_haskey(qdict, "execute")) {
+        process_command(s, qdict);
+    } else {
+        g_warning("unrecognized payload format, ignoring");
+    }
+
+out_free:
+    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);
+        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");
+        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);
+        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);
+        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);
+            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);
+        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:
+    /* reset negotiated state. we'll either send another init upon reconnect
+     * to our socket, or the host will send an init to tell us it has
+     * re-opened the chardev, at which point we'll send another init
+     */
+    set_channel_active(s, false);
+}
+
+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");
+        }
+        send_guest_up(s);
+    } 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);
+    }
+
+    qga_init_marshal();
+
+    s = g_malloc0(sizeof(GAState));
+    s->session_id = 0;
+    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->worker = ga_worker_new(process_command_worker);
+    s->timeout_ms = QGA_TIMEOUT_DEFAULT;
+    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);
+
+    set_channel_active(s, false);
+    init_guest_agent(s);
+
+    g_main_loop_run(s->main_loop);
+
+    ga_command_state_cleanup_all(s->command_state);
+    ga_worker_cleanup(s->worker);
+
+    return 0;
+}
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v2 16/17] guest agent: add guest agent RPCs/commands

[Michael Roth]

This adds the initial set of QMP/QAPI commands provided by the guest
agent:

guest-ping
guest-file-open
guest-file-read
guest-file-write
guest-file-seek
guest-file-close

The input/output specification for these commands are documented in the
schema.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qapi-schema.json           |  133 ++++++++++++++++++++-
 qga/guest-agent-commands.c |  284 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 415 insertions(+), 2 deletions(-)
 create mode 100644 qga/guest-agent-commands.c

diff --git a/qapi-schema.json b/qapi-schema.json
index 5292938..e2f209d 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1387,10 +1387,139 @@
 { 'enum': 'VirtioNetTxStrategy', 'data': ['bh', 'timer'] }
 { 'type': 'GuestInfo', 'data': {'*name': 'str', '*pid': 'int'} }
 
+##
+# @guest-ping:
+#
+# Ping the guest agent, a non-error return implies success
+#
+# Since: 0.15.0
+##
 { 'command': 'guest-ping' }
-{ 'command': 'guest-view-file', 'data': { 'filename': 'str' },
-  'returns': 'str' }
 
+##
+# @guest-info:
+#
+# Get some information about the guest agent.
+#
+# Since: 0.15.0
+##
+{ 'type': 'GuestAgentInfo', 'data': {'version': 'str', 'timeout_ms': 'int'} }
+{ 'command': 'guest-info',
+  'returns': 'GuestAgentInfo' }
+
+##
+# @guest-shutdown:
+#
+# Initiate guest-activated shutdown
+#
+# @shutdown_mode: "halt", "powerdown", or "reboot"
+#
+# Returns: Nothing on success
+#
+# Since: 0.15.0
+##
+{ 'command': 'guest-shutdown', 'data': { 'shutdown_mode': 'str' } }
+
+##
+# @guest-file-open:
+#
+# Open a file in the guest and retrieve a file handle for it
+#
+# @filename: Full path to the file in the guest to open.
+#
+# @mode: #optional open mode, as per fopen(), "r" is the default.
+#
+# Returns: Guest file handle on success.
+#          If @filename cannot be opened, OpenFileFailed
+#
+# Since: 0.15.0
+##
+{ 'command': 'guest-file-open',
+  'data':    { 'filename': 'str', 'mode': 'str' },
+  'returns': 'int' }
+
+##
+# @guest-file-read:
+#
+# Read from an open file in the guest
+#
+# @filehandle: filehandle returned by guest-file-open
+#
+# @count: maximum number of bytes to read
+#
+# Returns: GuestFileRead on success.
+#          If @filehandle cannot be found, OpenFileFailed
+#
+# Since: 0.15.0
+##
+{ 'type': 'GuestFileRead',
+  'data': { 'count': 'int', 'buf': 'str', 'eof': 'bool' } }
+
+{ 'command': 'guest-file-read',
+  'data':    { 'filehandle': 'int', 'count': 'int' },
+  'returns': 'GuestFileRead' }
+
+##
+# @guest-file-write:
+#
+# Write to an open file in the guest
+#
+# @filehandle: filehandle returned by guest-file-open
+#
+# @data_b64: base64-encoded string representing data to be written
+#
+# @count: bytes to write (actual bytes, after b64-decode)
+#
+# Returns: GuestFileWrite on success.
+#          If @filehandle cannot be found, OpenFileFailed
+#
+# Since: 0.15.0
+##
+{ 'type': 'GuestFileWrite',
+  'data': { 'count': 'int', 'eof': 'bool' } }
+{ 'command': 'guest-file-write',
+  'data':    { 'filehandle': 'int', 'data_b64': 'str', 'count': 'int' },
+  'returns': 'GuestFileWrite' }
+
+##
+# @guest-file-seek:
+#
+# Seek to a position in the file, as with fseek(), and return the
+# current file position afterward. Also encapsulates ftell()'s
+# functionality, just Set offset=0, whence=SEEK_CUR.
+#
+# @filehandle: filehandle returned by guest-file-open
+#
+# @offset: bytes to skip over in the file stream
+#
+# @whence: SEEK_SET, SEEK_CUR, or SEEK_END, as with fseek()
+#
+# Returns: GuestFileSeek on success.
+#          If @filename cannot be opened, OpenFileFailed
+#
+# Since: 0.15.0
+##
+{ 'type': 'GuestFileSeek',
+  'data': { 'position': 'int', 'eof': 'bool' } }
+
+{ 'command': 'guest-file-seek',
+  'data':    { 'filehandle': 'int', 'offset': 'int', 'whence': 'int' },
+  'returns': 'GuestFileSeek' }
+
+##
+# @guest-file-close:
+#
+# Close an open file in the guest
+#
+# @filehandle: filehandle returned by guest-file-open
+#
+# Returns: Nothing on success.
+#          If @filename cannot be opened, OpenFileFailed
+#
+# Since: 0.15.0
+##
+{ 'command': 'guest-file-close',
+  'data': { 'filehandle': 'int' } }
 
 { 'type': 'ProbeProtocol', 'data': { 'unsafe': 'bool', 'filename': 'str' } }
 
diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
new file mode 100644
index 0000000..843ef36
--- /dev/null
+++ b/qga/guest-agent-commands.c
@@ -0,0 +1,284 @@
+/*
+ * QEMU Guest Agent commands
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Michael Roth      <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include <glib.h>
+#include "guest-agent.h"
+
+static bool enable_syslog = true;
+static GAState *ga_state;
+
+#define SLOG(msg, ...) do { \
+    if (enable_syslog) { \
+        g_log("syslog", G_LOG_LEVEL_INFO, msg, ## __VA_ARGS__); \
+    } \
+} while(0)
+
+void qga_guest_ping(Error **err)
+{
+    SLOG("guest-ping called");
+}
+
+struct GuestAgentInfo *qga_guest_info(Error **err)
+{
+    GuestAgentInfo *info = g_malloc0(sizeof(GuestInfo));
+
+    info->version = g_strdup(QGA_VERSION);
+    info->timeout_ms = ga_get_timeout(ga_state);
+
+    return info;
+}
+
+void qga_guest_shutdown(const char *shutdown_mode, Error **err)
+{
+    int ret;
+    const char *shutdown_flag;
+
+    SLOG("guest-shutdown called, shutdown_mode: %s", shutdown_mode);
+
+    if (strcmp(shutdown_mode, "halt") == 0) {
+        shutdown_flag = "-H";
+    } else if (strcmp(shutdown_mode, "powerdown") == 0) {
+        shutdown_flag = "-P";
+    } else if (strcmp(shutdown_mode, "reboot") == 0) {
+        shutdown_flag = "-r";
+    } else {
+        ret = -EINVAL;
+        error_set(err, QERR_INVALID_PARAMETER_VALUE, "shutdown_mode",
+                  "halt|powerdown|reboot");
+        return;
+    }
+
+    ret = fork();
+    if (ret == 0) {
+        /* child, start the shutdown */
+        setsid();
+        fclose(stdin);
+        fclose(stdout);
+        fclose(stderr);
+
+        sleep(5);
+        ret = execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
+                    "hypervisor initiated shutdown", (char*)NULL);
+        exit(!!ret);
+    } else if (ret < 0) {
+        error_set(err, QERR_UNDEFINED_ERROR);
+    }
+}
+
+typedef struct GuestFileHandle {
+    uint64_t id;
+    FILE *fh;
+} GuestFileHandle;
+
+static struct {
+    GSList *filehandles;
+    uint64_t last_id;
+} guest_file_state;
+
+static void guest_file_init(void)
+{
+    guest_file_state.filehandles = NULL;
+    guest_file_state.last_id = 0;
+}
+
+static void guest_file_cleanup(void)
+{
+    /* TODO: cleanup old array, close out any open filehandles */
+    guest_file_state.filehandles = NULL;
+    guest_file_state.last_id = 0;
+}
+
+static int64_t guest_file_handle_add(FILE *fh)
+{
+    GuestFileHandle *gfh;
+
+    gfh = g_malloc(sizeof(GuestFileHandle));
+    gfh->id = guest_file_state.last_id++;
+    gfh->fh = fh;
+    guest_file_state.filehandles = g_slist_append(guest_file_state.filehandles,
+                                                  gfh);
+    return gfh->id;
+}
+
+static gint guest_file_handle_match(gconstpointer elem, gconstpointer id_p)
+{
+    const uint64_t *id = id_p;
+    const GuestFileHandle *gfh = elem;
+
+    g_assert(gfh);
+    return (gfh->id != *id);
+}
+
+static FILE *guest_file_handle_find(int64_t id)
+{
+    GSList *elem = g_slist_find_custom(guest_file_state.filehandles, &id,
+                                       guest_file_handle_match);
+    GuestFileHandle *gfh;
+
+    if (elem) {
+        g_assert(elem->data);
+        gfh = elem->data;
+        return gfh->fh;
+    }
+
+    return NULL;
+}
+
+static void guest_file_handle_remove(int64_t id)
+{
+    GSList *elem = g_slist_find_custom(guest_file_state.filehandles, &id,
+                                       guest_file_handle_match);
+    gpointer data = elem->data;
+
+    if (!data) {
+        return;
+    }
+    guest_file_state.filehandles = g_slist_remove(guest_file_state.filehandles,
+                                                  data);
+    g_free(data);
+}
+
+int64_t qga_guest_file_open(const char *filename, const char *mode, Error **err)
+{
+    FILE *fh;
+    int fd, ret;
+    int64_t id = -1;
+
+    SLOG("guest-file-open called, filename: %s, mode: %s", filename, mode);
+    fh = fopen(filename, mode);
+    if (!fh) {
+        error_set(err, QERR_OPEN_FILE_FAILED, filename);
+        goto out;
+    }
+
+    /* set fd non-blocking to avoid common use cases (like reading from a
+     * named pipe) from hanging the agent
+     */
+    fd = fileno(fh);
+    ret = fcntl(fd, F_GETFL);
+    ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK);
+    if (ret == -1) {
+        error_set(err, QERR_OPEN_FILE_FAILED, filename);
+        fclose(fh);
+        goto out;
+    }
+
+    id = guest_file_handle_add(fh);
+    SLOG("guest-file-open, filehandle: %ld", id);
+out:
+    return id;
+}
+
+struct GuestFileRead *qga_guest_file_read(int64_t filehandle, int64_t count,
+                                          Error **err)
+{
+    GuestFileRead *read_data;
+    guchar *buf;
+    FILE *fh = guest_file_handle_find(filehandle);
+    size_t read_count;
+
+    if (!fh) {
+        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
+        return NULL;
+    }
+
+    read_data = g_malloc0(sizeof(GuestFileRead));
+    buf = g_malloc(count);
+
+    read_count = fread(buf, 1, count, fh);
+    buf[read_count] = 0;
+    read_data->count = read_count;
+    read_data->eof = feof(fh);
+    if (read_count) {
+        read_data->buf = g_base64_encode(buf, read_count);
+    }
+    g_free(buf);
+    /* clear error and eof. error is generally due to EAGAIN from non-blocking
+     * mode, and no real way to differenitate from a real error since we only
+     * get boolean error flag from ferror()
+     */
+    clearerr(fh);
+
+    return read_data;
+}
+
+GuestFileWrite *qga_guest_file_write(int64_t filehandle, const char *data_b64,
+                                     int64_t count, Error **err)
+{
+    GuestFileWrite *write_data;
+    guchar *data;
+    gsize data_len;
+    int write_count;
+    FILE *fh = guest_file_handle_find(filehandle);
+
+    if (!fh) {
+        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
+        return NULL;
+    }
+
+    write_data = g_malloc0(sizeof(GuestFileWrite));
+    data = g_base64_decode(data_b64, &data_len);
+    write_count = fwrite(data, 1, MIN(count, data_len), fh);
+    write_data->count = write_count;
+    write_data->eof = feof(fh);
+    g_free(data);
+    clearerr(fh);
+
+    return write_data;
+}
+
+struct GuestFileSeek *qga_guest_file_seek(int64_t filehandle, int64_t offset,
+                                          int64_t whence, Error **err)
+{
+    GuestFileSeek *seek_data;
+    FILE *fh = guest_file_handle_find(filehandle);
+    int ret;
+
+    if (!fh) {
+        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
+        return NULL;
+    }
+
+    seek_data = g_malloc0(sizeof(GuestFileRead));
+    ret = fseek(fh, offset, whence);
+    if (ret == -1) {
+        error_set(err, QERR_UNDEFINED_ERROR);
+        g_free(seek_data);
+        return NULL;
+    }
+    seek_data->position = ftell(fh);
+    seek_data->eof = feof(fh);
+    clearerr(fh);
+
+    return seek_data;
+}
+
+void qga_guest_file_close(int64_t filehandle, Error **err)
+{
+    FILE *fh = guest_file_handle_find(filehandle);
+
+    SLOG("guest-file-close called, filehandle: %ld", filehandle);
+    if (!fh) {
+        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
+        return;
+    }
+
+    fclose(fh);
+    guest_file_handle_remove(filehandle);
+}
+
+/* register init/cleanup routines for stateful command groups */
+void ga_command_state_init(GAState *s, GACommandState *cs)
+{
+    ga_state = s;
+    ga_command_state_add(cs, &guest_file_init, &guest_file_cleanup);
+}
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v2 17/17] guest agent: build qemu-ga, add QEMU-wide gio dep

[Michael Roth]

This allows us to build qemu-ga with "make qemu-ga". It pulls in the
qemu-tools deps, but does not currently build by default. This may
change to avoid bitrot and help with host-side-only unit tests.

This also pulls in gio dependences for all of qemu, currently we only
pull in gthread. In general this brings in gio, gmodule, and
gobject.

Can limit this to only the guest agent, but it's expected that all of
these will be needed as we start relying more on glib throughout qemu,
so leaving for now.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 Makefile  |   15 +++++++++------
 configure |    6 +++---
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index d510779..a7c1503 100644
--- a/Makefile
+++ b/Makefile
@@ -173,10 +173,10 @@ libqmp.h: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qmp-gen.py
 libqmp.c: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qmp-gen.py
 	$(call quiet-command,python $(SRC_PATH)/qmp-gen.py --lib-body $@ < $<, "  GEN   $@")
 
-guest-agent.h: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qmp-gen.py
+qga/guest-agent.h: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qmp-gen.py
 	$(call quiet-command,python $(SRC_PATH)/qmp-gen.py --guest-header $@ < $<, "  GEN   $@")
 
-guest-agent.c: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qmp-gen.py
+qga/guest-agent.c: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qmp-gen.py
 	$(call quiet-command,python $(SRC_PATH)/qmp-gen.py --guest-body $@ < $<, "  GEN   $@")
 
 qdev-marshal.h: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qmp-gen.py
@@ -206,6 +206,7 @@ libqmp.o: libqmp.c libqmp.h qapi-types.h
 qdev-marshal.o: qdev-marshal.c qdev-marshal.h qapi-types.h
 qcfg-marshal.o: qcfg-marshal.c qcfg-marshal.h qapi-types.h
 qcfg-opts.o: qcfg-opts.c qcfg-opts.h qcfg-marshal.h qapi-types.h
+qemu-ga.o: qga/guest-agent.c qga/guest-agent.h qmp-marshal-types.c qmp-marshal-types.h
 
 version.o: $(SRC_PATH)/version.rc config-host.mak
 	$(call quiet-command,$(WINDRES) -I. -o $@ $<,"  RC    $(TARGET_DIR)$@")
@@ -214,7 +215,9 @@ version-obj-$(CONFIG_WIN32) += version.o
 ######################################################################
 
 qemu-img.o: qemu-img-cmds.h
-qemu-img.o qemu-tool.o qemu-nbd.o qemu-io.o cmd.o: $(GENERATED_HEADERS)
+qemu-img.o qemu-tool.o qemu-nbd.o qemu-io.o cmd.o qemu-ga.o: $(GENERATED_HEADERS)
+
+qemu-ga$(EXESUF): qemu-ga.o qemu-tool.o qemu-error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o qemu-sockets.o qmp-marshal-types-core.o qmp-marshal-types.o qga/guest-agent.o qga/guest-agent-core.o qga/guest-agent-commands.o qga/guest-agent-command-state.o qga/guest-agent-worker.o
 
 qemu-img$(EXESUF): qemu-img.o qemu-tool.o qemu-error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o
 
@@ -276,8 +279,8 @@ clean:
 # avoid old build problems by removing potentially incorrect old files
 	rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h gen-op-arm.h
 	rm -f qemu-options.def
-	rm -f *.o *.d *.a $(TOOLS) TAGS cscope.* *.pod *~ */*~
-	rm -f slirp/*.o slirp/*.d audio/*.o audio/*.d block/*.o block/*.d net/*.o net/*.d fsdev/*.o fsdev/*.d ui/*.o ui/*.d
+	rm -f *.o *.d *.a $(TOOLS) qemu-ga TAGS cscope.* *.pod *~ */*~
+	rm -f slirp/*.o slirp/*.d audio/*.o audio/*.d block/*.o block/*.d net/*.o net/*.d fsdev/*.o fsdev/*.d ui/*.o ui/*.d qga/*.o qga/*.d
 	rm -f qemu-img-cmds.h
 	rm -f trace.c trace.h trace.c-timestamp trace.h-timestamp
 	rm -f trace-dtrace.dtrace trace-dtrace.dtrace-timestamp
@@ -463,4 +466,4 @@ tarbin:
 	$(mandir)/man8/qemu-nbd.8
 
 # Include automatically generated dependency files
--include $(wildcard *.d audio/*.d slirp/*.d block/*.d net/*.d ui/*.d qapi/*.d)
+-include $(wildcard *.d audio/*.d slirp/*.d block/*.d net/*.d ui/*.d qapi/*.d qga/*.d)
diff --git a/configure b/configure
index d99deb2..bfea356 100755
--- a/configure
+++ b/configure
@@ -1659,9 +1659,9 @@ fi
 
 ##########################################
 # glib support probe
-if $pkg_config --modversion gthread-2.0 > /dev/null 2>&1 ; then
-    glib_cflags=`$pkg_config --cflags gthread-2.0 2>/dev/null`
-    glib_libs=`$pkg_config --libs gthread-2.0 2>/dev/null`
+if $pkg_config --modversion gthread-2.0 gio-2.0 > /dev/null 2>&1 ; then
+    glib_cflags=`$pkg_config --cflags gthread-2.0 gio-2.0 2>/dev/null`
+    glib_libs=`$pkg_config --libs gthread-2.0 gio-2.0 2>/dev/null`
     libs_softmmu="$glib_libs $libs_softmmu"
     libs_tools="$glib_libs $libs_tools"
 else
-- 
1.7.0.4

Re: [Qemu-devel] [RFC][PATCH v2 08/17] qapi: fix Error usage in qemu-sockets.c

[Jes Sorensen]

On 04/18/11 17:02, Michael Roth wrote:
> Fix spurious errors due to not initializing Error pointer to NULL before
> checking for errors.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  qemu-sockets.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index dc8beeb..e709e5f 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -630,7 +630,7 @@ int unix_connect(const char *path)
>  {
>      QemuOpts *opts;
>      int sock;
> -    Error *err;
> +    Error *err = NULL;
>  
>      opts = qemu_opts_create(&dummy_opts, NULL, 0, &err);
>      if (err) {

This one really should go into the tree asap, even if the rest of the
virt agent patches are still pending.

Reviewed-by: Jes Sorensen <Jes.Sorensen@redhat.com>

Re: [Qemu-devel] [RFC][PATCH v2 09/17] qmp proxy: core code for proxying qmp requests to guest

[Jes Sorensen]

On 04/18/11 17:02, Michael Roth wrote:
> diff --git a/qmp-core.c b/qmp-core.c
> index 9f3d182..dab50a1 100644
> --- a/qmp-core.c
> +++ b/qmp-core.c
> @@ -937,7 +937,15 @@ void qmp_async_complete_command(QmpCommandState *cmd, QObject *retval, Error *er
>      qemu_free(cmd);
>  }
>  
> +extern QmpProxy *qmp_proxy_default;

Please put this in a header file.

> +static void qmp_proxy_process_control_event(QmpProxy *p, const QDict *evt)
> +{
> +    const char *cmd;
> +    int host_sid, guest_sid;
> +
> +    cmd = qdict_get_try_str(evt, "_control_event");
> +    if (!cmd) {
> +        fprintf(stderr, "received NULL control event\n");
> +    } else if (strcmp(cmd, "guest_ack") == 0) {
> +        host_sid = qdict_get_try_int(evt, "_control_arg_host_sid", 0);
> +        if (!host_sid) {
> +            fprintf(stderr, "invalid guest_ack: wrong host sid\n");
> +            return;
> +        }
> +        /* guest responded to host_init, return a host_ack */
> +        /* reset outstanding requests, then send an ack with the
> +         * session id they passed us
> +         */
> +        guest_sid = qdict_get_try_int(evt, "_control_arg_guest_sid", 0);

I am wondering if it would make sense to put these arguments in a header
file as #define's to make sure you don't have to chase down a typo on
one side at some point? Just an idea, dunno if it is worth it.

Cheers,
Jes

Re: [Qemu-devel] [RFC][PATCH v2 12/17] guest agent: worker thread class

[Jes Sorensen]

On 04/18/11 17:02, Michael Roth wrote:
> diff --git a/qga/guest-agent-worker.c b/qga/guest-agent-worker.c
> new file mode 100644
> index 0000000..e3295da
> --- /dev/null
> +++ b/qga/guest-agent-worker.c
> @@ -0,0 +1,173 @@
> +/*
> + * QEMU Guest Agent worker thread interfaces
> + *
> + * Copyright IBM Corp. 2011
> + *
> + * Authors:
> + *  Michael Roth      <mdroth@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include <glib.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <stdbool.h>
> +#include <pthread.h>
> +#include <errno.h>
> +#include <string.h>
> +#include "guest-agent.h"
> +#include "../error.h"

Oh dear! do not do that please! Fix the Makefile to include the
appropriate path.

> +struct GAWorker {
> +    pthread_t thread;
> +    ga_worker_func execute;
> +    pthread_mutex_t input_mutex;
> +    pthread_cond_t input_avail_cond;
> +    void *input;
> +    bool input_avail;
> +    pthread_mutex_t output_mutex;
> +    pthread_cond_t output_avail_cond;

You really should use QemuMutex and friends here.

> +    void *output;
> +    Error *output_error;
> +    bool output_avail;
> +};
> +
> +static void *worker_run(void *worker_p)
> +{
> +    GAWorker *worker = worker_p;
> +    Error *err;
> +    void *input, *output;
> +
> +    while (1) {
> +        /* wait for input */
> +        pthread_mutex_lock(&worker->input_mutex);

qemu_mutex_lock()

> +        while (!worker->input_avail) {
> +            pthread_cond_wait(&worker->input_avail_cond, &worker->input_mutex);
> +        }

again

> +        input = worker->input;
> +        worker->input_avail = false;
> +        pthread_mutex_unlock(&worker->input_mutex);

and again.... I'll stop. Basically there really should be no references
to pthread_*

Jes

Re: [Qemu-devel] [RFC][PATCH v2 15/17] guest agent: qemu-ga daemon

[Jes Sorensen]

On 04/18/11 17:02, Michael Roth wrote:
> +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";
> +    }

Urgh!

No two statements on the same line please!

Jes

Re: [Qemu-devel] [RFC][PATCH v2 00/11] QEMU Guest Agent: QMP-based host/guest communication (virtagent)

[Jes Sorensen]

On 04/18/11 17:02, Michael Roth wrote:
> These apply on top of Anthony's glib tree, commit 03d5927deb5e6baebaade1b4c8ff2428a85e125c currently, and can also be obtained from:
> git://repo.or.cz/qemu/mdroth.git qga_v2
> 
> Patches 1-8 are general json/QAPI-related fixes. Anthony, please consider pulling these into your glib tree. The json fix-ups may need further evaluation, but I'm confident they're at least an improvement. The QAPI ones are mostly trivial fix-ups.
> 
> Changes since V1:
> 
> - Added guest agent worker thread to execute RPCs in the guest. With this in place we have a reliable timeout mechanism for hung commands, currently set at 30 seconds.
> - Add framework for registering init/cleanup routines for stateful RPCs to clean up after themselves after a timeout.
> - Added the following RPCs: guest-file-{open,close,read,write,seek}, guest-shutdown, guest-info, and removed stubs for guest-view-file (now deprecated)
> - Added GUEST_AGENT_UP/GUEST_AGENT_DOWN QMP events
> - Switched to a TCP-style host-initiated 3-way handshake for channel negotiation, this simplifies client negotiation/interaction over the wire
> - Added configurable log level/log file/pid file options for guest agent
> - Various fixes for bugs/memory leaks and checkpatch.pl fixups
> 
> ISSUES/TODOS:
> 
> - Fix QMP proxy handling of error responses sent by guest agent, currently ignored
> - Add unit tests for guest agent wire protocol
> - Add unit tests for QMP interfaces
> - Add host-side timeout mechanism for async QMP commands
> - Return error for guest commands if guest up event has not yet been recieved
> - Make QMP param names more consistent between related commands
> - Clean up logging
> 

Hi,

I had a look through this patchset and generally it looks pretty good.
There were a few nits, and I ignored all the python gibberish to avoid
getting a headache.

Did you do anything with the fsfreeze patches, or were they dropped in
the migration to qapi?

Cheers,
Jes

Re: [Qemu-devel] [RFC][PATCH v2 09/17] qmp proxy: core code for proxying qmp requests to guest

[Michael Roth]

On 04/21/2011 03:30 AM, Jes Sorensen wrote:
> On 04/18/11 17:02, Michael Roth wrote:
>> diff --git a/qmp-core.c b/qmp-core.c
>> index 9f3d182..dab50a1 100644
>> --- a/qmp-core.c
>> +++ b/qmp-core.c
>> @@ -937,7 +937,15 @@ void qmp_async_complete_command(QmpCommandState *cmd, QObject *retval, Error *er
>>       qemu_free(cmd);
>>   }
>>
>> +extern QmpProxy *qmp_proxy_default;
>
> Please put this in a header file.
>
>> +static void qmp_proxy_process_control_event(QmpProxy *p, const QDict *evt)
>> +{
>> +    const char *cmd;
>> +    int host_sid, guest_sid;
>> +
>> +    cmd = qdict_get_try_str(evt, "_control_event");
>> +    if (!cmd) {
>> +        fprintf(stderr, "received NULL control event\n");
>> +    } else if (strcmp(cmd, "guest_ack") == 0) {
>> +        host_sid = qdict_get_try_int(evt, "_control_arg_host_sid", 0);
>> +        if (!host_sid) {
>> +            fprintf(stderr, "invalid guest_ack: wrong host sid\n");
>> +            return;
>> +        }
>> +        /* guest responded to host_init, return a host_ack */
>> +        /* reset outstanding requests, then send an ack with the
>> +         * session id they passed us
>> +         */
>> +        guest_sid = qdict_get_try_int(evt, "_control_arg_guest_sid", 0);
>
> I am wondering if it would make sense to put these arguments in a header
> file as #define's to make sure you don't have to chase down a typo on
> one side at some point? Just an idea, dunno if it is worth it.

That's probably a good idea.

>
> Cheers,
> Jes

Re: [Qemu-devel] [RFC][PATCH v2 12/17] guest agent: worker thread class

[Michael Roth]

On 04/21/2011 03:44 AM, Jes Sorensen wrote:
> On 04/18/11 17:02, Michael Roth wrote:
>> diff --git a/qga/guest-agent-worker.c b/qga/guest-agent-worker.c
>> new file mode 100644
>> index 0000000..e3295da
>> --- /dev/null
>> +++ b/qga/guest-agent-worker.c
>> @@ -0,0 +1,173 @@
>> +/*
>> + * QEMU Guest Agent worker thread interfaces
>> + *
>> + * Copyright IBM Corp. 2011
>> + *
>> + * Authors:
>> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +#include<glib.h>
>> +#include<stdlib.h>
>> +#include<stdio.h>
>> +#include<stdbool.h>
>> +#include<pthread.h>
>> +#include<errno.h>
>> +#include<string.h>
>> +#include "guest-agent.h"
>> +#include "../error.h"
>
> Oh dear! do not do that please! Fix the Makefile to include the
> appropriate path.
>
>> +struct GAWorker {
>> +    pthread_t thread;
>> +    ga_worker_func execute;
>> +    pthread_mutex_t input_mutex;
>> +    pthread_cond_t input_avail_cond;
>> +    void *input;
>> +    bool input_avail;
>> +    pthread_mutex_t output_mutex;
>> +    pthread_cond_t output_avail_cond;
>
> You really should use QemuMutex and friends here.
>
>> +    void *output;
>> +    Error *output_error;
>> +    bool output_avail;
>> +};
>> +
>> +static void *worker_run(void *worker_p)
>> +{
>> +    GAWorker *worker = worker_p;
>> +    Error *err;
>> +    void *input, *output;
>> +
>> +    while (1) {
>> +        /* wait for input */
>> +        pthread_mutex_lock(&worker->input_mutex);
>
> qemu_mutex_lock()
>
>> +        while (!worker->input_avail) {
>> +            pthread_cond_wait(&worker->input_avail_cond,&worker->input_mutex);
>> +        }
>
> again
>
>> +        input = worker->input;
>> +        worker->input_avail = false;
>> +        pthread_mutex_unlock(&worker->input_mutex);
>
> and again.... I'll stop. Basically there really should be no references
> to pthread_*

This is on the guest side of things where I'm trying to use GLib 
wherever possible to keep things somewhat portable: logging/list 
utilities/io events/etc. And I *really* wanted to use GThreads here, but 
the problem is that GThread does not have any sane means to kill off a 
thread when a timeout occurs: there's no analogue to pthread_cancel(), 
and to use signals you need to break the abstraction to get the 
underlying pid. The new QemuThread stuff is using GThread underneath the 
covers so same limitation there.

pthreads provides these things and is fairly portable however, so I 
opted to make it an explicit dependency on the guest side. So 
glib+pthreads are the current dependencies.

>
> Jes

Re: [Qemu-devel] [RFC][PATCH v2 12/17] guest agent: worker thread class

[Jes Sorensen]

On 04/21/11 15:15, Michael Roth wrote:
> On 04/21/2011 03:44 AM, Jes Sorensen wrote:
>> and again.... I'll stop. Basically there really should be no references
>> to pthread_*
> 
> This is on the guest side of things where I'm trying to use GLib
> wherever possible to keep things somewhat portable: logging/list
> utilities/io events/etc. And I *really* wanted to use GThreads here, but
> the problem is that GThread does not have any sane means to kill off a
> thread when a timeout occurs: there's no analogue to pthread_cancel(),
> and to use signals you need to break the abstraction to get the
> underlying pid. The new QemuThread stuff is using GThread underneath the
> covers so same limitation there.
> 
> pthreads provides these things and is fairly portable however, so I
> opted to make it an explicit dependency on the guest side. So
> glib+pthreads are the current dependencies.

That is really unfortunate - is there no way around it? It really would
be ideal if we could build the guest relying on QemuThreads for portability.

Either way, please fix the include issue.

Jes

Re: [Qemu-devel] [RFC][PATCH v2 15/17] guest agent: qemu-ga daemon

[Michael Roth]

On 04/21/2011 03:50 AM, Jes Sorensen wrote:
> On 04/18/11 17:02, Michael Roth wrote:
>> +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";
>> +    }
>
> Urgh!
>
> No two statements on the same line please!

Darn, I was hoping my surplus on coding style points for actually 
indenting my case statements would make up for that :)

>
> Jes

Re: [Qemu-devel] [RFC][PATCH v2 00/11] QEMU Guest Agent: QMP-based host/guest communication (virtagent)

[Michael Roth]

On 04/21/2011 04:46 AM, Jes Sorensen wrote:
> On 04/18/11 17:02, Michael Roth wrote:
>> These apply on top of Anthony's glib tree, commit 03d5927deb5e6baebaade1b4c8ff2428a85e125c currently, and can also be obtained from:
>> git://repo.or.cz/qemu/mdroth.git qga_v2
>>
>> Patches 1-8 are general json/QAPI-related fixes. Anthony, please consider pulling these into your glib tree. The json fix-ups may need further evaluation, but I'm confident they're at least an improvement. The QAPI ones are mostly trivial fix-ups.
>>
>> Changes since V1:
>>
>> - Added guest agent worker thread to execute RPCs in the guest. With this in place we have a reliable timeout mechanism for hung commands, currently set at 30 seconds.
>> - Add framework for registering init/cleanup routines for stateful RPCs to clean up after themselves after a timeout.
>> - Added the following RPCs: guest-file-{open,close,read,write,seek}, guest-shutdown, guest-info, and removed stubs for guest-view-file (now deprecated)
>> - Added GUEST_AGENT_UP/GUEST_AGENT_DOWN QMP events
>> - Switched to a TCP-style host-initiated 3-way handshake for channel negotiation, this simplifies client negotiation/interaction over the wire
>> - Added configurable log level/log file/pid file options for guest agent
>> - Various fixes for bugs/memory leaks and checkpatch.pl fixups
>>
>> ISSUES/TODOS:
>>
>> - Fix QMP proxy handling of error responses sent by guest agent, currently ignored
>> - Add unit tests for guest agent wire protocol
>> - Add unit tests for QMP interfaces
>> - Add host-side timeout mechanism for async QMP commands
>> - Return error for guest commands if guest up event has not yet been recieved
>> - Make QMP param names more consistent between related commands
>> - Clean up logging
>>
>
> Hi,
>
> I had a look through this patchset and generally it looks pretty good.
> There were a few nits, and I ignored all the python gibberish to avoid
> getting a headache.
>
> Did you do anything with the fsfreeze patches, or were they dropped in
> the migration to qapi?

They were pending some changes required on the agent side that weren't 
really addressed/doable until this patchset, namely:

1) server-side timeout mechanism to recover from RPCs that can hang 
indefinitely or take a really long time (fsfreeze, fopen, etc), 
currently it's 30 seconds, may need to bump it to 60 for fsfreeze, or 
potentially add an RPC to change the server-side timeout
2) a simple way to temporarily turn off logging so agent doesn't 
deadlock itself
3) a way to register a cleanup handler when a timeout occurs.
4) disabling RPCs where proper accounting/logging is required 
(guest-open-file, guest-shutdown, etc)

#4 isn't implemented...I think this could be done fairly in-evasively 
with something like:

Response important_rpc():
   if (!ga_log("syslog", LEVEL_CRITICAL, "important stuff happening"))
     return ERROR_LOGGING_CURRENTLY_DISABLED
   ...

bool ga_log(log_domain, level, msg):
   if (log_domain == "syslog")
     if (!logging_enabled && is_critical(log_level))
       return False;
     syslog(msg, ...)
   else
     if (logging_enabled)
       normallog(msg, ...)
   return True

With that I think we could actually drop the fsfreeze stuff in. Thoughts?

>
> Cheers,
> Jes
>

Re: [Qemu-devel] [RFC][PATCH v2 00/11] QEMU Guest Agent: QMP-based host/guest communication (virtagent)

[Jes Sorensen]

On 04/18/11 17:02, Michael Roth wrote:
> These apply on top of Anthony's glib tree, commit 03d5927deb5e6baebaade1b4c8ff2428a85e125c currently, and can also be obtained from:
> git://repo.or.cz/qemu/mdroth.git qga_v2
> 
> Patches 1-8 are general json/QAPI-related fixes. Anthony, please consider pulling these into your glib tree. The json fix-ups may need further evaluation, but I'm confident they're at least an improvement. The QAPI ones are mostly trivial fix-ups.
> 
> Changes since V1:
> 
> - Added guest agent worker thread to execute RPCs in the guest. With this in place we have a reliable timeout mechanism for hung commands, currently set at 30 seconds.
> - Add framework for registering init/cleanup routines for stateful RPCs to clean up after themselves after a timeout.
> - Added the following RPCs: guest-file-{open,close,read,write,seek}, guest-shutdown, guest-info, and removed stubs for guest-view-file (now deprecated)
> - Added GUEST_AGENT_UP/GUEST_AGENT_DOWN QMP events
> - Switched to a TCP-style host-initiated 3-way handshake for channel negotiation, this simplifies client negotiation/interaction over the wire
> - Added configurable log level/log file/pid file options for guest agent
> - Various fixes for bugs/memory leaks and checkpatch.pl fixups
> 
> ISSUES/TODOS:
> 
> - Fix QMP proxy handling of error responses sent by guest agent, currently ignored
> - Add unit tests for guest agent wire protocol
> - Add unit tests for QMP interfaces
> - Add host-side timeout mechanism for async QMP commands
> - Return error for guest commands if guest up event has not yet been recieved
> - Make QMP param names more consistent between related commands
> - Clean up logging

Michael,

One thing I cannot seem to figure out with this tree - the agent
commands do not seem to show up in the monitor? What am I missing?

Cheers,
Jes

Re: [Qemu-devel] [RFC][PATCH v2 00/11] QEMU Guest Agent: QMP-based host/guest communication (virtagent)

[Michael Roth]

On 04/21/2011 09:10 AM, Jes Sorensen wrote:
> On 04/18/11 17:02, Michael Roth wrote:
>> These apply on top of Anthony's glib tree, commit 03d5927deb5e6baebaade1b4c8ff2428a85e125c currently, and can also be obtained from:
>> git://repo.or.cz/qemu/mdroth.git qga_v2
>>
>> Patches 1-8 are general json/QAPI-related fixes. Anthony, please consider pulling these into your glib tree. The json fix-ups may need further evaluation, but I'm confident they're at least an improvement. The QAPI ones are mostly trivial fix-ups.
>>
>> Changes since V1:
>>
>> - Added guest agent worker thread to execute RPCs in the guest. With this in place we have a reliable timeout mechanism for hung commands, currently set at 30 seconds.
>> - Add framework for registering init/cleanup routines for stateful RPCs to clean up after themselves after a timeout.
>> - Added the following RPCs: guest-file-{open,close,read,write,seek}, guest-shutdown, guest-info, and removed stubs for guest-view-file (now deprecated)
>> - Added GUEST_AGENT_UP/GUEST_AGENT_DOWN QMP events
>> - Switched to a TCP-style host-initiated 3-way handshake for channel negotiation, this simplifies client negotiation/interaction over the wire
>> - Added configurable log level/log file/pid file options for guest agent
>> - Various fixes for bugs/memory leaks and checkpatch.pl fixups
>>
>> ISSUES/TODOS:
>>
>> - Fix QMP proxy handling of error responses sent by guest agent, currently ignored
>> - Add unit tests for guest agent wire protocol
>> - Add unit tests for QMP interfaces
>> - Add host-side timeout mechanism for async QMP commands
>> - Return error for guest commands if guest up event has not yet been recieved
>> - Make QMP param names more consistent between related commands
>> - Clean up logging
>
> Michael,
>
> One thing I cannot seem to figure out with this tree - the agent
> commands do not seem to show up in the monitor? What am I missing?

Hmm, for some reason this email never hit my inbox.

You mean with the human monitor? Currently, with these new patches, 
we're QMP only. And most of the command names/semantics have changed as 
well. The qapi-schema.json changes in patch 16 have the new prototypes, 
and the 0 patch has some usage examples.

>
> Cheers,
> Jes
>
>

Re: [Qemu-devel] [RFC][PATCH v2 15/17] guest agent: qemu-ga daemon

[Ian Molton]

On Thu, 2011-04-21 at 08:21 -0500, Michael Roth wrote:
> >> +    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";
> >> +    }
> >
> > Urgh!
> >
> > No two statements on the same line please!

Always wondered what the logic for this one is. IMHO the above is FAR
neater than splitting it to near double its height.

What kind of coding error does splitting this out aim to prevent?
missing break; / return; statements? Because I dont see how it achieves
that...

Re: [Qemu-devel] [RFC][PATCH v2 15/17] guest agent: qemu-ga daemon

[Jes Sorensen]

On 04/22/11 11:23, Ian Molton wrote:
> On Thu, 2011-04-21 at 08:21 -0500, Michael Roth wrote:
>>>> +    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";
>>>> +    }
>>>
>>> Urgh!
>>>
>>> No two statements on the same line please!
> 
> Always wondered what the logic for this one is. IMHO the above is FAR
> neater than splitting it to near double its height.
> 
> What kind of coding error does splitting this out aim to prevent?
> missing break; / return; statements? Because I dont see how it achieves
> that...

Hiding things you miss when reading the code, it's a classic for people
to do if(foo) bleh(); on the same line, and whoever reads the code will
expect the action on the next line, especially if foo is a long complex
statement.

It's one of these 'just don't do it, it bites you in the end' things.

Jes

Re: [Qemu-devel] [RFC][PATCH v2 15/17] guest agent: qemu-ga daemon

[Ian Molton]

On Fri, 2011-04-22 at 13:51 +0200, Jes Sorensen wrote:
> > What kind of coding error does splitting this out aim to prevent?
> > missing break; / return; statements? Because I dont see how it
> achieves
> > that...
> 
> Hiding things you miss when reading the code, it's a classic for
> people
> to do if(foo) bleh(); on the same line, and whoever reads the code
> will
> expect the action on the next line, especially if foo is a long
> complex
> statement.
> 
> It's one of these 'just don't do it, it bites you in the end' things. 

Meh. I dont see it that way...

Sure, if it was one line out of 20 written that way, it would be weird,
but as is, its just part of a block of identical lines.

I dont really see a parallel with the if() statement either since the
condition in the switch() case isnt on the same line as such. I must
admit that I only write one-liner if statements if the condition is
short though.

-Ian

Re: [Qemu-devel] [RFC][PATCH v2 00/11] QEMU Guest Agent: QMP-based host/guest communication (virtagent)

[Jes Sorensen]

On 04/21/11 22:58, Michael Roth wrote:
> On 04/21/2011 09:10 AM, Jes Sorensen wrote:
>> On 04/18/11 17:02, Michael Roth wrote:
>> One thing I cannot seem to figure out with this tree - the agent
>> commands do not seem to show up in the monitor? What am I missing?
> 
> Hmm, for some reason this email never hit my inbox.
> 
> You mean with the human monitor? Currently, with these new patches,
> we're QMP only. And most of the command names/semantics have changed as
> well. The qapi-schema.json changes in patch 16 have the new prototypes,
> and the 0 patch has some usage examples.

Hi Michael,

Yeah it was the conclusion I came to on Thursday when I was working on
porting the freeze patches over. After fighting the json %#$%#$%#$ I
ended up with something I couldn't test in the end :(

Any plans to add human monitor support in the near future?

Cheers,
Jes

Re: [Qemu-devel] [RFC][PATCH v2 09/17] qmp proxy: core code for proxying qmp requests to guest

[Stefan Hajnoczi]

On Mon, Apr 18, 2011 at 4:02 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> +static int qmp_proxy_cancel_request(QmpProxy *p, QmpProxyRequest *r)
> +{
> +    if (r && r->cb) {
> +        r->cb(r->opaque, NULL, NULL);
> +    }
> +
> +    return 0;
> +}
> +
> +static int qmp_proxy_cancel_all(QmpProxy *p)
> +{
> +    QmpProxyRequest *r, *tmp;
> +    QTAILQ_FOREACH_SAFE(r, &p->requests, entry, tmp) {
> +        qmp_proxy_cancel_request(p, r);
> +        QTAILQ_REMOVE(&p->requests, r, entry);
> +    }
> +
> +    return 0;
> +}

qmp_proxy_cancel_all() will remove requests from the list.
qmp_proxy_cancel_request() will not remove it from the list.  This
could cause confusion in the future if someone adds a call to
qmp_proxy_cancel_request() without realizing that it will not remove
the request from the list.  The two function's names are similar, it
would be nice if they acted the same way.

> +static void qmp_proxy_process_event(JSONMessageParser *parser, QList *tokens)
> +{
> +    QmpProxy *p = container_of(parser, QmpProxy, parser);
> +    QmpProxyRequest *r;
> +    QObject *obj;
> +    QDict *qdict;
> +    Error *err = NULL;
> +
> +    fprintf(stderr, "qmp proxy: called\n");
> +    obj = json_parser_parse_err(tokens, NULL, &err);
> +    if (!obj) {
> +        fprintf(stderr, "qmp proxy: failed to parse\n");
> +        return;
> +    } else {
> +        fprintf(stderr, "qmp proxy: parse successful\n");
> +        qdict = qobject_to_qdict(obj);
> +    }
> +
> +    if (qdict_haskey(qdict, "_control_event")) {
> +        /* handle transport-level control event */
> +        qmp_proxy_process_control_event(p, qdict);
> +    } else if (qdict_haskey(qdict, "return")) {
> +        /* handle proxied qmp command response */
> +        fprintf(stderr, "received return\n");
> +        r = QTAILQ_FIRST(&p->requests);
> +        if (!r) {
> +            fprintf(stderr, "received return, but no request queued\n");

QDECREF(qdict)?

> +            return;
> +        }
> +        /* XXX: can't assume type here */
> +        fprintf(stderr, "recieved response for cmd: %s\nreturn: %s\n",
> +                r->name, qstring_get_str(qobject_to_json(QOBJECT(qdict))));
> +        r->cb(r->opaque, qdict_get(qdict, "return"), NULL);
> +        QTAILQ_REMOVE(&p->requests, r, entry);
> +        qemu_free(r);
> +        fprintf(stderr, "done handling response\n");
> +    } else {
> +        fprintf(stderr, "received invalid payload format\n");
> +    }
> +
> +    QDECREF(qdict);
> +}
> +void qmp_proxy_send_request(QmpProxy *p, const char *name,
> +                            const QDict *args, Error **errp,
> +                            QmpGuestCompletionFunc *cb, void *opaque)
> +{
> +    QmpProxyRequest *r = qemu_mallocz(sizeof(QmpProxyRequest));
> +    QDict *payload = qdict_new();
> +    QString *json;
> +
> +    /* TODO: don't really need to hold on to name/args after encoding */
> +    r->name = name;
> +    r->args = args;
> +    r->cb = cb;
> +    r->opaque = opaque;
> +
> +    qdict_put_obj(payload, "execute", QOBJECT(qstring_from_str(r->name)));
> +    /* TODO: casting a const so we can add it to our dictionary. bad. */
> +    qdict_put_obj(payload, "arguments", QOBJECT((QDict *)args));
> +
> +    json = qobject_to_json(QOBJECT((QDict *)payload));
> +    if (!json) {
> +        goto out_bad;
> +    }
> +
> +    QTAILQ_INSERT_TAIL(&p->requests, r, entry);
> +    g_string_append(p->tx, qstring_get_str(json));
> +    QDECREF(json);
> +    qmp_proxy_write(p);
> +    return;
> +
> +out_bad:
> +    cb(opaque, NULL, NULL);
> +    qemu_free(r);

Need to free payload?

> +}
> +
> +QmpProxy *qmp_proxy_new(CharDriverState *chr)
> +{
> +    QmpProxy *p = qemu_mallocz(sizeof(QmpProxy));
> +
> +    signal_init(&guest_agent_up_event);
> +    signal_init(&guest_agent_reset_event);
> +
> +    /* there's a reason for this madness */

Helpful comment :)

> +    p->tx_timer = qemu_new_timer(rt_clock, qmp_proxy_write_handler, p);
> +    p->tx_timer_interval = 10;
> +    p->tx = g_string_new("");
> +    p->chr = chr;
> +    json_message_parser_init(&p->parser, qmp_proxy_process_event);
> +    QTAILQ_INIT(&p->requests);
> +
> +    return p;
> +}
> +
> +void qmp_proxy_close(QmpProxy *p)
> +{
> +    qmp_proxy_cancel_all(p);
> +    g_string_free(p->tx, TRUE);

Free tx_timer?

> +    qemu_free(p);
> +}

Re: [Qemu-devel] [RFC][PATCH v2 15/17] guest agent: qemu-ga daemon

[Jes Sorensen]

On 04/25/11 14:27, Ian Molton wrote:
> On Fri, 2011-04-22 at 13:51 +0200, Jes Sorensen wrote:
>> Hiding things you miss when reading the code, it's a classic for 
>> people to do if(foo) bleh(); on the same line, and whoever reads
>> the code will expect the action on the next line, especially if foo
>> is a long complex statement.
>>
>> It's one of these 'just don't do it, it bites you in the end' things. 
> 
> Meh. I dont see it that way...
> 
> Sure, if it was one line out of 20 written that way, it would be weird,
> but as is, its just part of a block of identical lines.

It is a matter of consistency, we allow it in one place, we suddenly
have it all over. The moment someone wants to add a slightly more
complex case to such a switch statement it is all down the drain. It is
way better to stay consistent across the board.

> I dont really see a parallel with the if() statement either since the
> condition in the switch() case isnt on the same line as such. I must
> admit that I only write one-liner if statements if the condition is
> short though.

Writing one-liner if() statements is inherently broken, or you could
call it the Perl syndrome. Write-once, read-never.....

Jes

Re: [Qemu-devel] [RFC][PATCH v2 00/11] QEMU Guest Agent: QMP-based host/guest communication (virtagent)

[Michael Roth]

On 04/26/2011 01:57 AM, Jes Sorensen wrote:
> On 04/21/11 22:58, Michael Roth wrote:
>> On 04/21/2011 09:10 AM, Jes Sorensen wrote:
>>> On 04/18/11 17:02, Michael Roth wrote:
>>> One thing I cannot seem to figure out with this tree - the agent
>>> commands do not seem to show up in the monitor? What am I missing?
>>
>> Hmm, for some reason this email never hit my inbox.
>>
>> You mean with the human monitor? Currently, with these new patches,
>> we're QMP only. And most of the command names/semantics have changed as
>> well. The qapi-schema.json changes in patch 16 have the new prototypes,
>> and the 0 patch has some usage examples.
>
> Hi Michael,
>
> Yeah it was the conclusion I came to on Thursday when I was working on
> porting the freeze patches over. After fighting the json %#$%#$%#$ I
> ended up with something I couldn't test in the end :(

I actually worked on getting most of the fsfreeze ported over yesterday, 
were they any major changes from the last set of patches other than the 
porting work? If so, feel free to send the patches my way and I'll hack 
on those a bit, otherwise I plan to have the fsfreeze stuff included in 
the next re-spin later this week.

>
> Any plans to add human monitor support in the near future?

The main target will be QMP for the initial patches. But ideally the HMP 
commands we add will have a pretty close correspondence with QMP.

>
> Cheers,
> Jes
>

Re: [Qemu-devel] [RFC][PATCH v2 00/11] QEMU Guest Agent: QMP-based host/guest communication (virtagent)

[Jes Sorensen]

On 04/26/11 16:27, Michael Roth wrote:
> On 04/26/2011 01:57 AM, Jes Sorensen wrote:
>> Yeah it was the conclusion I came to on Thursday when I was working on
>> porting the freeze patches over. After fighting the json %#$%#$%#$ I
>> ended up with something I couldn't test in the end :(
> 
> I actually worked on getting most of the fsfreeze ported over yesterday,
> were they any major changes from the last set of patches other than the
> porting work? If so, feel free to send the patches my way and I'll hack
> on those a bit, otherwise I plan to have the fsfreeze stuff included in
> the next re-spin later this week.

I'll try and post them later today.

>> Any plans to add human monitor support in the near future?
> 
> The main target will be QMP for the initial patches. But ideally the HMP
> commands we add will have a pretty close correspondence with QMP.

That is unfortunate, QMP is really user unfriendly :(

Cheers,
Jes

Re: [Qemu-devel] [RFC][PATCH v2 09/17] qmp proxy: core code for proxying qmp requests to guest

[Michael Roth]

On 04/26/2011 08:21 AM, Stefan Hajnoczi wrote:
> On Mon, Apr 18, 2011 at 4:02 PM, Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>> +static int qmp_proxy_cancel_request(QmpProxy *p, QmpProxyRequest *r)
>> +{
>> +    if (r&&  r->cb) {
>> +        r->cb(r->opaque, NULL, NULL);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int qmp_proxy_cancel_all(QmpProxy *p)
>> +{
>> +    QmpProxyRequest *r, *tmp;
>> +    QTAILQ_FOREACH_SAFE(r,&p->requests, entry, tmp) {
>> +        qmp_proxy_cancel_request(p, r);
>> +        QTAILQ_REMOVE(&p->requests, r, entry);
>> +    }
>> +
>> +    return 0;
>> +}
>
> qmp_proxy_cancel_all() will remove requests from the list.
> qmp_proxy_cancel_request() will not remove it from the list.  This
> could cause confusion in the future if someone adds a call to
> qmp_proxy_cancel_request() without realizing that it will not remove
> the request from the list.  The two function's names are similar, it
> would be nice if they acted the same way.
>
>> +static void qmp_proxy_process_event(JSONMessageParser *parser, QList *tokens)
>> +{
>> +    QmpProxy *p = container_of(parser, QmpProxy, parser);
>> +    QmpProxyRequest *r;
>> +    QObject *obj;
>> +    QDict *qdict;
>> +    Error *err = NULL;
>> +
>> +    fprintf(stderr, "qmp proxy: called\n");
>> +    obj = json_parser_parse_err(tokens, NULL,&err);
>> +    if (!obj) {
>> +        fprintf(stderr, "qmp proxy: failed to parse\n");
>> +        return;
>> +    } else {
>> +        fprintf(stderr, "qmp proxy: parse successful\n");
>> +        qdict = qobject_to_qdict(obj);
>> +    }
>> +
>> +    if (qdict_haskey(qdict, "_control_event")) {
>> +        /* handle transport-level control event */
>> +        qmp_proxy_process_control_event(p, qdict);
>> +    } else if (qdict_haskey(qdict, "return")) {
>> +        /* handle proxied qmp command response */
>> +        fprintf(stderr, "received return\n");
>> +        r = QTAILQ_FIRST(&p->requests);
>> +        if (!r) {
>> +            fprintf(stderr, "received return, but no request queued\n");
>
> QDECREF(qdict)?
>
>> +            return;
>> +        }
>> +        /* XXX: can't assume type here */
>> +        fprintf(stderr, "recieved response for cmd: %s\nreturn: %s\n",
>> +                r->name, qstring_get_str(qobject_to_json(QOBJECT(qdict))));
>> +        r->cb(r->opaque, qdict_get(qdict, "return"), NULL);
>> +        QTAILQ_REMOVE(&p->requests, r, entry);
>> +        qemu_free(r);
>> +        fprintf(stderr, "done handling response\n");
>> +    } else {
>> +        fprintf(stderr, "received invalid payload format\n");
>> +    }
>> +
>> +    QDECREF(qdict);
>> +}
>> +void qmp_proxy_send_request(QmpProxy *p, const char *name,
>> +                            const QDict *args, Error **errp,
>> +                            QmpGuestCompletionFunc *cb, void *opaque)
>> +{
>> +    QmpProxyRequest *r = qemu_mallocz(sizeof(QmpProxyRequest));
>> +    QDict *payload = qdict_new();
>> +    QString *json;
>> +
>> +    /* TODO: don't really need to hold on to name/args after encoding */
>> +    r->name = name;
>> +    r->args = args;
>> +    r->cb = cb;
>> +    r->opaque = opaque;
>> +
>> +    qdict_put_obj(payload, "execute", QOBJECT(qstring_from_str(r->name)));
>> +    /* TODO: casting a const so we can add it to our dictionary. bad. */
>> +    qdict_put_obj(payload, "arguments", QOBJECT((QDict *)args));
>> +
>> +    json = qobject_to_json(QOBJECT((QDict *)payload));
>> +    if (!json) {
>> +        goto out_bad;
>> +    }
>> +
>> +    QTAILQ_INSERT_TAIL(&p->requests, r, entry);
>> +    g_string_append(p->tx, qstring_get_str(json));
>> +    QDECREF(json);
>> +    qmp_proxy_write(p);
>> +    return;
>> +
>> +out_bad:
>> +    cb(opaque, NULL, NULL);
>> +    qemu_free(r);
>
> Need to free payload?
>
>> +}
>> +
>> +QmpProxy *qmp_proxy_new(CharDriverState *chr)
>> +{
>> +    QmpProxy *p = qemu_mallocz(sizeof(QmpProxy));
>> +
>> +    signal_init(&guest_agent_up_event);
>> +    signal_init(&guest_agent_reset_event);
>> +
>> +    /* there's a reason for this madness */
>
> Helpful comment :)
>
>> +    p->tx_timer = qemu_new_timer(rt_clock, qmp_proxy_write_handler, p);
>> +    p->tx_timer_interval = 10;
>> +    p->tx = g_string_new("");
>> +    p->chr = chr;
>> +    json_message_parser_init(&p->parser, qmp_proxy_process_event);
>> +    QTAILQ_INIT(&p->requests);
>> +
>> +    return p;
>> +}
>> +
>> +void qmp_proxy_close(QmpProxy *p)
>> +{
>> +    qmp_proxy_cancel_all(p);
>> +    g_string_free(p->tx, TRUE);
>
> Free tx_timer?
>
>> +    qemu_free(p);
>> +}

All good catches/suggestions, thanks.

Re: [Qemu-devel] [RFC][PATCH v2 00/11] QEMU Guest Agent: QMP-based host/guest communication (virtagent)

[Jes Sorensen]

On 04/21/11 15:55, Michael Roth wrote:
>> Did you do anything with the fsfreeze patches, or were they dropped in
>> the migration to qapi?
> 
> They were pending some changes required on the agent side that weren't
> really addressed/doable until this patchset, namely:
> 
> 1) server-side timeout mechanism to recover from RPCs that can hang
> indefinitely or take a really long time (fsfreeze, fopen, etc),
> currently it's 30 seconds, may need to bump it to 60 for fsfreeze, or
> potentially add an RPC to change the server-side timeout
> 2) a simple way to temporarily turn off logging so agent doesn't
> deadlock itself
> 3) a way to register a cleanup handler when a timeout occurs.
> 4) disabling RPCs where proper accounting/logging is required
> (guest-open-file, guest-shutdown, etc)
> 
> #4 isn't implemented...I think this could be done fairly in-evasively
> with something like:
> 
> Response important_rpc():
>   if (!ga_log("syslog", LEVEL_CRITICAL, "important stuff happening"))
>     return ERROR_LOGGING_CURRENTLY_DISABLED

Either that, or maybe simply disable the full command while the freeze
is in progress? I fear we're more likely to miss a case of checking for
logging than we are to miss command disabling?

It should still be very non evasive, maybe just a flag in the struct
declaring the functions marking it as logging-required and if the
no-logging flag is set, the command is made to wait, or return -EAGAIN

> 
> bool ga_log(log_domain, level, msg):
>   if (log_domain == "syslog")
>     if (!logging_enabled && is_critical(log_level))
>       return False;
>     syslog(msg, ...)
>   else
>     if (logging_enabled)
>       normallog(msg, ...)
>   return True
> 
> With that I think we could actually drop the fsfreeze stuff in. Thoughts?

IMHO it is better to disable the commands rather than just logging, but
either way should allow it to drop in.

Sorry for the late reply, been a bit swamped here.

Cheers,
Jes

Re: [Qemu-devel] [RFC][PATCH v2 00/11] QEMU Guest Agent: QMP-based host/guest communication (virtagent)

[Michael Roth]

On 05/03/2011 07:51 AM, Jes Sorensen wrote:
> On 04/21/11 15:55, Michael Roth wrote:
>>> Did you do anything with the fsfreeze patches, or were they dropped in
>>> the migration to qapi?
>>
>> They were pending some changes required on the agent side that weren't
>> really addressed/doable until this patchset, namely:
>>
>> 1) server-side timeout mechanism to recover from RPCs that can hang
>> indefinitely or take a really long time (fsfreeze, fopen, etc),
>> currently it's 30 seconds, may need to bump it to 60 for fsfreeze, or
>> potentially add an RPC to change the server-side timeout
>> 2) a simple way to temporarily turn off logging so agent doesn't
>> deadlock itself
>> 3) a way to register a cleanup handler when a timeout occurs.
>> 4) disabling RPCs where proper accounting/logging is required
>> (guest-open-file, guest-shutdown, etc)
>>
>> #4 isn't implemented...I think this could be done fairly in-evasively
>> with something like:
>>
>> Response important_rpc():
>>    if (!ga_log("syslog", LEVEL_CRITICAL, "important stuff happening"))
>>      return ERROR_LOGGING_CURRENTLY_DISABLED
>
> Either that, or maybe simply disable the full command while the freeze
> is in progress? I fear we're more likely to miss a case of checking for
> logging than we are to miss command disabling?
>
> It should still be very non evasive, maybe just a flag in the struct
> declaring the functions marking it as logging-required and if the
> no-logging flag is set, the command is made to wait, or return -EAGAIN
>

Yup when I actually starting dropping it in I realized this was a much 
better approach. Although, for now I just added something like "if 
(!logging_enabled) { error_set(QERR_GA_LOGGING_DISABLED); return }" to 
the start of functions where logging is considered critical, which will 
result in the user getting an error message about logging so it's not 
too much of a surprise to them.

The actual dispatch code closely mirrors Anthony's dispatch stuff for 
QMP so I was hesitant to try to modify it to handle this automatically, 
since it would require some changes to how the schema parsing/handling 
is done (would probably need to add a "requires_logging" flag in the 
schema). Wouldn't take much though. Either way, should be a clean 
conversion if we decide to go that route.

>>
>> bool ga_log(log_domain, level, msg):
>>    if (log_domain == "syslog")
>>      if (!logging_enabled&&  is_critical(log_level))
>>        return False;
>>      syslog(msg, ...)
>>    else
>>      if (logging_enabled)
>>        normallog(msg, ...)
>>    return True
>>
>> With that I think we could actually drop the fsfreeze stuff in. Thoughts?
>
> IMHO it is better to disable the commands rather than just logging, but
> either way should allow it to drop in.

Kinda agree, but logging seems to be the real dependency. With the 
server-side timeouts now in place even doing stuff like fopen/fwrite is 
permitted (it would just timeout if it blocked too long). It's the 
logging stuff that we don't really have a way to recover from, because 
it's not run in a thread we can just nuke after a certain amount of time.

Even when we're not frozen, we can't guarantee an fopen/fwrite/fread 
will succeed, so failures shouldn't be too much of a surprise since they 
need to be handled anyway. And determining whether or not a command 
should be marked as executable during a freeze is somewhat nebulous 
(fopen might work for read-only access, but hang for write access when 
O_CREATE is set, fwrite might succeed if it doesn't require a flush, 
etc), plus internal things like logging need to be taken into account.

So, for now at least I think it's a reasonable way to do it.

>
> Sorry for the late reply, been a bit swamped here.

No problem I have your patches in my tree now. They still need a little 
bit of love and testing but I should be able to get them out on the list 
shortly.

>
> Cheers,
> Jes

Re: [Qemu-devel] [RFC][PATCH v2 00/11] QEMU Guest Agent: QMP-based host/guest communication (virtagent)

[Jes Sorensen]

On 05/03/11 15:53, Michael Roth wrote:
>>
>> IMHO it is better to disable the commands rather than just logging, but
>> either way should allow it to drop in.
> 
> Kinda agree, but logging seems to be the real dependency. With the
> server-side timeouts now in place even doing stuff like fopen/fwrite is
> permitted (it would just timeout if it blocked too long). It's the
> logging stuff that we don't really have a way to recover from, because
> it's not run in a thread we can just nuke after a certain amount of time.
> 
> Even when we're not frozen, we can't guarantee an fopen/fwrite/fread
> will succeed, so failures shouldn't be too much of a surprise since they
> need to be handled anyway. And determining whether or not a command
> should be marked as executable during a freeze is somewhat nebulous
> (fopen might work for read-only access, but hang for write access when
> O_CREATE is set, fwrite might succeed if it doesn't require a flush,
> etc), plus internal things like logging need to be taken into account.
> 
> So, for now at least I think it's a reasonable way to do it.

Please be very careful with any fwrite() calls - it's not just logging.
Any writes to the frozen file systems will result in the caller being
put to sleep until the file system is unfrozen. It won't timeout, and
the agent will be stuck hanging in that call.

It's fun playing with the fsfreeze stuff on your desktop system - doing
it in an xterm has 'interesting' effects..... :)

This is why I prefer the 'disable function' rather 'disable logging'
approach.

>> Sorry for the late reply, been a bit swamped here.
> 
> No problem I have your patches in my tree now. They still need a little
> bit of love and testing but I should be able to get them out on the list
> shortly.

Sounds great!

Cheers,
Jes

Re: [Qemu-devel] [RFC][PATCH v2 00/11] QEMU Guest Agent: QMP-based host/guest communication (virtagent)

[Michael Roth]

On 05/03/2011 09:12 AM, Jes Sorensen wrote:
> On 05/03/11 15:53, Michael Roth wrote:
>>>
>>> IMHO it is better to disable the commands rather than just logging, but
>>> either way should allow it to drop in.
>>
>> Kinda agree, but logging seems to be the real dependency. With the
>> server-side timeouts now in place even doing stuff like fopen/fwrite is
>> permitted (it would just timeout if it blocked too long). It's the
>> logging stuff that we don't really have a way to recover from, because
>> it's not run in a thread we can just nuke after a certain amount of time.
>>
>> Even when we're not frozen, we can't guarantee an fopen/fwrite/fread
>> will succeed, so failures shouldn't be too much of a surprise since they
>> need to be handled anyway. And determining whether or not a command
>> should be marked as executable during a freeze is somewhat nebulous
>> (fopen might work for read-only access, but hang for write access when
>> O_CREATE is set, fwrite might succeed if it doesn't require a flush,
>> etc), plus internal things like logging need to be taken into account.
>>
>> So, for now at least I think it's a reasonable way to do it.
>
> Please be very careful with any fwrite() calls - it's not just logging.
> Any writes to the frozen file systems will result in the caller being
> put to sleep until the file system is unfrozen. It won't timeout, and
> the agent will be stuck hanging in that call.
>
> It's fun playing with the fsfreeze stuff on your desktop system - doing
> it in an xterm has 'interesting' effects..... :)
>
> This is why I prefer the 'disable function' rather 'disable logging'
> approach.
>

I'll review the code to make sure, but what I mean is that the actual 
daemon only ever does disk i/o when it's doing logging. All the RPCs are 
handled in a separate worker thread, so if they do something fubar we 
should still be able to recover (the timeouts are done by 
pthread_cancel()'ing the worker thread). So things should be recoverable 
so long as we don't log when frozen. RPCs succeeding or failing we don't 
care too much about, since they'll just result in a nice "command timed 
out" response to the user eventually.

Checking logging_enabled is more to ensure we don't let the user exploit 
the fs_freeze side-effects to do sensitive stuff under the guest owner's 
nose (not that we couldn't anyway from host, but it may important for 
piece of mind about have an externally accessible/privileged agent in 
your guest).

works_under_fsfreeze is harder to determine beforehand, unless maybe you 
flag any command that did i/o other than logging...but there are 
actually use cases where that would be too restrictive. If they're using 
fwrite to pipe output through a character device, for instance, or 
twiddling knobs in sysfs. Better to keep the same semantics as you'd 
have when "physically" on the machine, when possible.

Will need to do some good testing though, fsfreeze is definitely a 
strange state to be in :)

>>> Sorry for the late reply, been a bit swamped here.
>>
>> No problem I have your patches in my tree now. They still need a little
>> bit of love and testing but I should be able to get them out on the list
>> shortly.
>
> Sounds great!
>
> Cheers,
> Jes