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

[Qemu-devel] [RFC][PATCH v1 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_v1

The QGA-specific patches are in pretty rough shape, and there are some outstanding issues that I'll note below. I just wanted to put the general approach out there for consideration. Patch-level comments/review are still much-appreciated though.

However, patches 1-5 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 trivial fix-ups.

ISSUES/TODOS:

 - QMP's async callbacks expect the command results to be de-marshalled beforehand. This is completely infeasible to attempt outside of the code generator, so this is a big area that needs to be addressed. So for now, only the 'guest-ping' command works, since it has no return value. The dummy "guest-view-file" command will cause an error to be reported to the client.
 - qemu-ga guest daemon is currently not working over virtio-serial or isa-serial. This is probably an issue with how I'm using glib's io channel interfaces, still investigating. This means the only way to currently test is by invocing qemu-ga with "-c unix-listen -p <sockpath>", then doing something like `socat /dev/ttyS0,raw,echo=0 unix-connect:<sockpath>`.
 - guest-view-file is a stub, and will be broken out into an open/read/close set of RPCs, possibly with a high-level interface built around those.

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,path=/tmp/qmp-proxy2.sock,server,nowait,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-ping"}
  {"return": {}}

__

 Makefile               |    5 +-
 Makefile.objs          |    1 +
 configure              |    6 +-
 guest-agent-commands.c |   24 +++
 guest-agent-core.c     |  143 +++++++++++++
 guest-agent-core.h     |   21 ++
 json-lexer.c           |   22 ++-
 json-lexer.h           |    1 +
 json-parser.c          |    6 +-
 json-streamer.c        |   35 ++-
 qemu-char.c            |   46 +++++
 qemu-ga.c              |  522 ++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-core.c             |   13 +-
 qmp-core.h             |    7 +-
 qmp-gen.py             |    2 +-
 qmp-proxy-core.h       |   20 ++
 qmp-proxy.c            |  335 +++++++++++++++++++++++++++++++
 vl.c                   |    1 +
 18 files changed, 1181 insertions(+), 29 deletions(-)

[Qemu-devel] [RFC][PATCH v1 01/12] 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(0xFF) to an error state here, since it's an invalid
UTF-8 character. QMP guest proxy/agent use this 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 |   22 ++++++++++++++++++----
 json-lexer.h |    1 +
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/json-lexer.c b/json-lexer.c
index 3462c89..21aa03a 100644
--- a/json-lexer.c
+++ b/json-lexer.c
@@ -105,7 +105,7 @@ static const uint8_t json_lexer[][256] =  {
         ['u'] = IN_DQ_UCODE0,
     },
     [IN_DQ_STRING] = {
-        [1 ... 0xFF] = IN_DQ_STRING,
+        [1 ... 0xFE] = IN_DQ_STRING,
         ['\\'] = IN_DQ_STRING_ESCAPE,
         ['"'] = JSON_STRING,
     },
@@ -144,7 +144,7 @@ static const uint8_t json_lexer[][256] =  {
         ['u'] = IN_SQ_UCODE0,
     },
     [IN_SQ_STRING] = {
-        [1 ... 0xFF] = IN_SQ_STRING,
+        [1 ... 0xFE] = IN_SQ_STRING,
         ['\\'] = IN_SQ_STRING_ESCAPE,
         ['\''] = JSON_STRING,
     },
@@ -305,10 +305,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 +349,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;
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] Re: [RFC][PATCH v1 01/12] json-lexer: make lexer error-recovery more deterministic

[Anthony Liguori]

On 03/25/2011 02:47 PM, Michael Roth wrote:
> 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(0xFF) to an error state here, since it's an invalid
> UTF-8 character. QMP guest proxy/agent use this 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 |   22 ++++++++++++++++++----
>   json-lexer.h |    1 +
>   2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/json-lexer.c b/json-lexer.c
> index 3462c89..21aa03a 100644
> --- a/json-lexer.c
> +++ b/json-lexer.c
> @@ -105,7 +105,7 @@ static const uint8_t json_lexer[][256] =  {
>           ['u'] = IN_DQ_UCODE0,
>       },
>       [IN_DQ_STRING] = {
> -        [1 ... 0xFF] = IN_DQ_STRING,
> +        [1 ... 0xFE] = IN_DQ_STRING,

We also need to exclude 192, 193, 245-254 as these are all invalid bytes 
in a UTF-8 sequence.  See http://en.wikipedia.org/wiki/UTF-8#Codepage_layout

We probably ought to actually handle UTF-8 extend byte sequences in the 
lexer but we can keep this as a future exercise.

>           ['\\'] = IN_DQ_STRING_ESCAPE,
>           ['"'] = JSON_STRING,
>       },
> @@ -144,7 +144,7 @@ static const uint8_t json_lexer[][256] =  {
>           ['u'] = IN_SQ_UCODE0,
>       },
>       [IN_SQ_STRING] = {
> -        [1 ... 0xFF] = IN_SQ_STRING,
> +        [1 ... 0xFE] = IN_SQ_STRING,
>           ['\\'] = IN_SQ_STRING_ESCAPE,
>           ['\''] = JSON_STRING,
>       },
> @@ -305,10 +305,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 +349,6 @@ int json_lexer_feed(JSONLexer *lexer, const char *buffer, size_t size)
>
>       for (i = 0; i<  size; i++) {
>           int err;
> -

This whitespace change slipped in FWIW.

Regards,

Anthony Liguori

>           err = json_lexer_feed_char(lexer, buffer[i]);
>           if (err<  0) {
>               return err;
> 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;

[Qemu-devel] [RFC][PATCH v1 02/12] 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..659e3f0 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 v1 03/12] 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 v1 04/12] 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 v1 05/12] 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] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks

[Anthony Liguori]

On 03/25/2011 02:47 PM, Michael Roth wrote:
> 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()));

Luiz, I know we decided to return empty dicts because it lets us extend 
things better, but did we want to rule out the use of a 'null' return 
value entirely?

For a command like this, I can't imagine ever wanting to extend the 
return value...

Regards,

Anthony Liguori

>       }
>       if (cmd->tag) {
>           qdict_put_obj(rsp, "tag", cmd->tag);

[Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks

[Luiz Capitulino]

On Fri, 25 Mar 2011 16:22:16 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> On 03/25/2011 02:47 PM, Michael Roth wrote:
> > 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()));
> 
> Luiz, I know we decided to return empty dicts because it lets us extend 
> things better, but did we want to rule out the use of a 'null' return 
> value entirely?

For asynchronous commands you mean? No we didn't.

*iirc*, what happens today is that no command using this api is truly async,
for two reasons. First, changing from sync to async can break clients (that
happened to query-balloon). Second, although I can't remember the exact
details, the api that exists in the tree today is limited.

But for a new thing, like QAPI, having different semantics for async commands
seems the right thing to be done (ie. delaying the response).

> 
> For a command like this, I can't imagine ever wanting to extend the 
> return value...
> 
> Regards,
> 
> Anthony Liguori
> 
> >       }
> >       if (cmd->tag) {
> >           qdict_put_obj(rsp, "tag", cmd->tag);
> 

[Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks

[Anthony Liguori]

On 03/28/2011 11:47 AM, Luiz Capitulino wrote:
> On Fri, 25 Mar 2011 16:22:16 -0500
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> On 03/25/2011 02:47 PM, Michael Roth wrote:
>>> 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()));
>> Luiz, I know we decided to return empty dicts because it lets us extend
>> things better, but did we want to rule out the use of a 'null' return
>> value entirely?
> For asynchronous commands you mean? No we didn't.

No, nothing to do with asynchronous commands.  Just in general.

The question is, is it legal for a command to return 'null'.  It's 
certain valid JSON, but is it valid QMP?

Regards,

Anthony Liguori

> *iirc*, what happens today is that no command using this api is truly async,
> for two reasons. First, changing from sync to async can break clients (that
> happened to query-balloon). Second, although I can't remember the exact
> details, the api that exists in the tree today is limited.
>
> But for a new thing, like QAPI, having different semantics for async commands
> seems the right thing to be done (ie. delaying the response).



>> For a command like this, I can't imagine ever wanting to extend the
>> return value...
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>>        }
>>>        if (cmd->tag) {
>>>            qdict_put_obj(rsp, "tag", cmd->tag);

[Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks

[Luiz Capitulino]

On Mon, 28 Mar 2011 12:01:16 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> On 03/28/2011 11:47 AM, Luiz Capitulino wrote:
> > On Fri, 25 Mar 2011 16:22:16 -0500
> > Anthony Liguori<aliguori@us.ibm.com>  wrote:
> >
> >> On 03/25/2011 02:47 PM, Michael Roth wrote:
> >>> 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()));
> >> Luiz, I know we decided to return empty dicts because it lets us extend
> >> things better, but did we want to rule out the use of a 'null' return
> >> value entirely?
> > For asynchronous commands you mean? No we didn't.
> 
> No, nothing to do with asynchronous commands.  Just in general.
> 
> The question is, is it legal for a command to return 'null'.  It's 
> certain valid JSON, but is it valid QMP?

No, it's not valid.

> 
> Regards,
> 
> Anthony Liguori
> 
> > *iirc*, what happens today is that no command using this api is truly async,
> > for two reasons. First, changing from sync to async can break clients (that
> > happened to query-balloon). Second, although I can't remember the exact
> > details, the api that exists in the tree today is limited.
> >
> > But for a new thing, like QAPI, having different semantics for async commands
> > seems the right thing to be done (ie. delaying the response).
> 
> 
> 
> >> For a command like this, I can't imagine ever wanting to extend the
> >> return value...
> >>
> >> Regards,
> >>
> >> Anthony Liguori
> >>
> >>>        }
> >>>        if (cmd->tag) {
> >>>            qdict_put_obj(rsp, "tag", cmd->tag);
> 

Re: [Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks

[Anthony Liguori]

On 03/28/2011 12:06 PM, Luiz Capitulino wrote:
> On Mon, 28 Mar 2011 12:01:16 -0500
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> On 03/28/2011 11:47 AM, Luiz Capitulino wrote:
>>> On Fri, 25 Mar 2011 16:22:16 -0500
>>> Anthony Liguori<aliguori@us.ibm.com>   wrote:
>>>
>>>> On 03/25/2011 02:47 PM, Michael Roth wrote:
>>>>> 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()));
>>>> Luiz, I know we decided to return empty dicts because it lets us extend
>>>> things better, but did we want to rule out the use of a 'null' return
>>>> value entirely?
>>> For asynchronous commands you mean? No we didn't.
>> No, nothing to do with asynchronous commands.  Just in general.
>>
>> The question is, is it legal for a command to return 'null'.  It's
>> certain valid JSON, but is it valid QMP?
> No, it's not valid.

Do we have a reason for this?

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks

[Luiz Capitulino]

On Mon, 28 Mar 2011 12:19:53 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 03/28/2011 12:06 PM, Luiz Capitulino wrote:
> > On Mon, 28 Mar 2011 12:01:16 -0500
> > Anthony Liguori<aliguori@us.ibm.com>  wrote:
> >
> >> On 03/28/2011 11:47 AM, Luiz Capitulino wrote:
> >>> On Fri, 25 Mar 2011 16:22:16 -0500
> >>> Anthony Liguori<aliguori@us.ibm.com>   wrote:
> >>>
> >>>> On 03/25/2011 02:47 PM, Michael Roth wrote:
> >>>>> 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()));
> >>>> Luiz, I know we decided to return empty dicts because it lets us extend
> >>>> things better, but did we want to rule out the use of a 'null' return
> >>>> value entirely?
> >>> For asynchronous commands you mean? No we didn't.
> >> No, nothing to do with asynchronous commands.  Just in general.
> >>
> >> The question is, is it legal for a command to return 'null'.  It's
> >> certain valid JSON, but is it valid QMP?
> > No, it's not valid.
> 
> Do we have a reason for this?

We had to make a choice. We chose the current 'return' response. Iirc, one of
my first suggestions was "{ 'return': 'null' }" but you refused to have a 'null'
object, our parser doesn't even support it afaik.

But what's the problem with the current format?

Re: [Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks

[Anthony Liguori]

On 03/28/2011 12:27 PM, Luiz Capitulino wrote:
>
> We had to make a choice. We chose the current 'return' response. Iirc, one of
> my first suggestions was "{ 'return': 'null' }"

It would be:

{ 'return': null }

That's the valid JSON version.

>   but you refused to have a 'null'
> object, our parser doesn't even support it afaik.

It doesn't but that's because there isn't a QNone.

> But what's the problem with the current format?

Nothing, I was mostly curious as I only vaguely remember this discussion 
previously.

Regards,

Anthony Liguori

[Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks

[Michael Roth]

On 03/28/2011 11:47 AM, Luiz Capitulino wrote:
> On Fri, 25 Mar 2011 16:22:16 -0500
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> On 03/25/2011 02:47 PM, Michael Roth wrote:
>>> 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()));
>>
>> Luiz, I know we decided to return empty dicts because it lets us extend
>> things better, but did we want to rule out the use of a 'null' return
>> value entirely?
>
> For asynchronous commands you mean? No we didn't.
>
> *iirc*, what happens today is that no command using this api is truly async,
> for two reasons. First, changing from sync to async can break clients (that
> happened to query-balloon). Second, although I can't remember the exact
> details, the api that exists in the tree today is limited.
>
> But for a new thing, like QAPI, having different semantics for async commands
> seems the right thing to be done (ie. delaying the response).
>
>>
>> For a command like this, I can't imagine ever wanting to extend the
>> return value...

I think this is another topic, but also one we should hash out a bit better.

Currently the plan is that the C API not expose asynchronicity, 
underneath the covers the library will issue the request, then do a 
blocking read for the response. So the API call will block till 
completion, and no other command's will be serviced through the same 
underlying session until it is completed or cancelled.

For the JSON-based clients, the behavior is different. You have an 
optional tag you can pass in for an async command, and after issuing 
one, you can immediately begin issuing other async or non-async 
commands. As a result, the responses you receive will not necessarily be 
in FIFO order.

The upside to this is you can implement async commands on the client 
side without using separate threads, and can exploit some level of 
concurrency being able to do work within a session while a slower 
host->guest command completes. The downsides are that:

1) There is some inconsistency between this and the C API semantics.
2) The "optional" tags are basically required tags, at least for async 
commands, unless the client side does something to force synchronicity.

One option would be to disable the QMP session's read handler once a 
JSON object is received, and not re-enable it until we send the 
response. This would enforce FIFO-ordering. It might also add reduce the 
potential for a client being able to blow up our TX buffers by issuing 
lots of requests and not handling the responses in a timely enough 
manner (have seen this just from piping responses to stdout).

Thoughts?

Re: [Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks

[Anthony Liguori]

On 03/28/2011 12:59 PM, Michael Roth wrote:
>>>
>>> For a command like this, I can't imagine ever wanting to extend the
>>> return value...
>
>
> I think this is another topic, but also one we should hash out a bit 
> better.
>
> Currently the plan is that the C API not expose asynchronicity, 
> underneath the covers the library will issue the request, then do a 
> blocking read for the response. So the API call will block till 
> completion, and no other command's will be serviced through the same 
> underlying session until it is completed or cancelled.

No, that's just the patches as they stand today.

The medium term goal is to have twin APIs--one API that is synchronous 
and another that is asynchronous.  The asynchronous version will mirror 
how asynchronous commands are dispatched within QEMU.  That is, for 
query-block, you'll have:

typedef void (*QueryBlockFunc)(void *opaque, BlockInfo *retval, Error *err);

void libqmp_async_query_block(QmpSession *sess, Error **errp, 
QueryBlockFunc *cb, void *opaque);

The challenge with async library commands is that you need to think 
carefully about how you interact with the socket.  You can make a glib 
mainloop be a prerequisite, or you can have a set of function pointers 
that let you implement your own main loop.

But since there isn't a pressing need for this in the short term, it's 
easier to just delay this.

> For the JSON-based clients, the behavior is different. You have an 
> optional tag you can pass in for an async command, and after issuing 
> one, you can immediately begin issuing other async or non-async 
> commands. As a result, the responses you receive will not necessarily 
> be in FIFO order.

There is no such thing as a "JSON-based client".  QMP is not self 
describing enough to implement this with a pure transport client.  
Another language implementation (which I think you mean) would still 
need to solve the same problem as libqmp.

>
> The upside to this is you can implement async commands on the client 
> side without using separate threads, and can exploit some level of 
> concurrency being able to do work within a session while a slower 
> host->guest command completes. The downsides are that:
>
> 1) There is some inconsistency between this and the C API semantics.

You still need to pass a continuation to implement an event-based 
interface regardless of the language you're using.  The function 
pointer/opaque parameter is a continuation in C.

> 2) The "optional" tags are basically required tags, at least for async 
> commands, unless the client side does something to force synchronicity.
>
> One option would be to disable the QMP session's read handler once a 
> JSON object is received, and not re-enable it until we send the 
> response. This would enforce FIFO-ordering. It might also add reduce 
> the potential for a client being able to blow up our TX buffers by 
> issuing lots of requests and not handling the responses in a timely 
> enough manner (have seen this just from piping responses to stdout).

No, we're mixing up wire semantics with client/server semantics.

These are all completely different things.

The wire semantics are:

1) All commands are tagged.  Untagged commands have an implicit tag 
(let's refer to it as the psuedo-tag).

2) Until a command is completed, a tag cannot be reused.  This is also 
true for the psuedo-tag.

3) There is no guarantee about command completion order.

4) If a client happens to use the same tag for all commands, the client 
ends up enforcing a completion order because the server is only ever 
processing one command at a time.

The server semantics are:

1) All tags are preserved including the psuedo-tag.  This is required by 
the protocol.

2) Most commands are implemented by immediately dispatching a function 
and then computing the return value and immediately putting on the 
socket buffer.

3) Some commands are implemented by delaying the computation of the 
return value.  When this happens (which is an indeterminate amount of 
time later), the data will be put on the socket buffer.

4) Which commands are handled by (2) vs. (3) are transparent to the client.

The (current) client semantics are:

1) All commands are tagged with the psuedo-tag which enforces that only 
one command can be in flight at a time.  In the future, this interface 
will support threading and use different tags such that two threads can 
be used to send simultaneous commands.

2) A second interface will be implemented that provides an event-based 
interface whereas each command is passed a continuation.  Commands will 
use different tags to support this interface.

3) The reason to have both interfaces is to support the two most common 
models of concurrency, event-based concurrency and thread based concurrency.

Notice that I said nothing about 'C' in the above.  It's equally true to 
a C or Python client.

Regards,

Anthony Liguori

> Thoughts?
>

Re: [Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks

[Michael Roth]

On 03/28/2011 01:27 PM, Anthony Liguori wrote:
> On 03/28/2011 12:59 PM, Michael Roth wrote:
>>>>
>>>> For a command like this, I can't imagine ever wanting to extend the
>>>> return value...
>>
>>
>> I think this is another topic, but also one we should hash out a bit
>> better.
>>
>> Currently the plan is that the C API not expose asynchronicity,
>> underneath the covers the library will issue the request, then do a
>> blocking read for the response. So the API call will block till
>> completion, and no other command's will be serviced through the same
>> underlying session until it is completed or cancelled.
>
> No, that's just the patches as they stand today.
>
> The medium term goal is to have twin APIs--one API that is synchronous
> and another that is asynchronous. The asynchronous version will mirror
> how asynchronous commands are dispatched within QEMU. That is, for
> query-block, you'll have:
>
> typedef void (*QueryBlockFunc)(void *opaque, BlockInfo *retval, Error
> *err);
>
> void libqmp_async_query_block(QmpSession *sess, Error **errp,
> QueryBlockFunc *cb, void *opaque);
>
> The challenge with async library commands is that you need to think
> carefully about how you interact with the socket. You can make a glib
> mainloop be a prerequisite, or you can have a set of function pointers
> that let you implement your own main loop.
>
> But since there isn't a pressing need for this in the short term, it's
> easier to just delay this.
>
>> For the JSON-based clients, the behavior is different. You have an
>> optional tag you can pass in for an async command, and after issuing
>> one, you can immediately begin issuing other async or non-async
>> commands. As a result, the responses you receive will not necessarily
>> be in FIFO order.
>
> There is no such thing as a "JSON-based client". QMP is not self
> describing enough to implement this with a pure transport client.
> Another language implementation (which I think you mean) would still
> need to solve the same problem as libqmp.
>

By JSON-based I mean interacting directly with the QMP server socket 
via, say, `socat unix-connect:/tmp/qmp.sock readline`. I think this is 
what you're describing as being the wire protocol.

What I mean is that the wire protocol currently supports more than the 
libqmp C API does in terms of being able to handle multiple in-flight 
requests.

My thought was simply that if the C API provided by libqmp (and other 
language bindings) would always be synchronous, then the wire protocol, 
and the server-side handling of it, could potentially be simplified by 
enforcing FIFO ordering and not needing to handle multiple in-flight 
requests.

But if the long-term goal is to provide for asynchronous APIs then that 
probably doesn't make any sense.

>>
>> The upside to this is you can implement async commands on the client
>> side without using separate threads, and can exploit some level of
>> concurrency being able to do work within a session while a slower
>> host->guest command completes. The downsides are that:
>>
>> 1) There is some inconsistency between this and the C API semantics.
>
> You still need to pass a continuation to implement an event-based
> interface regardless of the language you're using. The function
> pointer/opaque parameter is a continuation in C.
>
>> 2) The "optional" tags are basically required tags, at least for async
>> commands, unless the client side does something to force synchronicity.
>>
>> One option would be to disable the QMP session's read handler once a
>> JSON object is received, and not re-enable it until we send the
>> response. This would enforce FIFO-ordering. It might also add reduce
>> the potential for a client being able to blow up our TX buffers by
>> issuing lots of requests and not handling the responses in a timely
>> enough manner (have seen this just from piping responses to stdout).
>
> No, we're mixing up wire semantics with client/server semantics.
>
> These are all completely different things.
>
> The wire semantics are:
>
> 1) All commands are tagged. Untagged commands have an implicit tag
> (let's refer to it as the psuedo-tag).
>
> 2) Until a command is completed, a tag cannot be reused. This is also
> true for the psuedo-tag.
>
> 3) There is no guarantee about command completion order.
>
> 4) If a client happens to use the same tag for all commands, the client
> ends up enforcing a completion order because the server is only ever
> processing one command at a time.

Is this supposed to be the current behavior? In early testing I noticed 
that not including a tag, and issuing an async command that never 
completed, still allowed for me to get responses for subsequent, 
tagless/pseudo-tagged requests.

>
> The server semantics are:
>
> 1) All tags are preserved including the psuedo-tag. This is required by
> the protocol.
>
> 2) Most commands are implemented by immediately dispatching a function
> and then computing the return value and immediately putting on the
> socket buffer.
>
> 3) Some commands are implemented by delaying the computation of the
> return value. When this happens (which is an indeterminate amount of
> time later), the data will be put on the socket buffer.
>
> 4) Which commands are handled by (2) vs. (3) are transparent to the client.
>
> The (current) client semantics are:
>
> 1) All commands are tagged with the psuedo-tag which enforces that only
> one command can be in flight at a time. In the future, this interface
> will support threading and use different tags such that two threads can
> be used to send simultaneous commands.
>
> 2) A second interface will be implemented that provides an event-based
> interface whereas each command is passed a continuation. Commands will
> use different tags to support this interface.
>
> 3) The reason to have both interfaces is to support the two most common
> models of concurrency, event-based concurrency and thread based
> concurrency.
>
> Notice that I said nothing about 'C' in the above. It's equally true to
> a C or Python client.

This clears things up a lot for me, thanks.

>
> Regards,
>
> Anthony Liguori
>
>> Thoughts?
>>
>

Re: [Qemu-devel] Re: [RFC][PATCH v1 05/12] qapi: fix handling for null-return async callbacks

[Anthony Liguori]

On 03/28/2011 03:42 PM, Michael Roth wrote:
>
> Is this supposed to be the current behavior? In early testing I 
> noticed that not including a tag, and issuing an async command that 
> never completed, still allowed for me to get responses for subsequent, 
> tagless/pseudo-tagged requests.

I don't think the server enforces tag-reuse today as that's really 
something a client should be checking for.  It certainly doesn't hurt 
for the server to complain if a client sends a tag that's already in use 
though.

Regards,

Anthony Liguori

[Qemu-devel] [RFC][PATCH v1 06/12] qmp proxy: build qemu with guest proxy dependency

[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 v1 07/12] 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.

Currently the class requires a path to a listening socket that either
corresponds to the chardev that the guest agent is communicating
through, or a local socket so we can communicate with a host-side
"guest" agent for testing purposes.

A subsequent patch will introduce a new chardev that sets up the
socket chardev and initializes the QmpProxy instance to abstract this
away from the user. Unifying this with local "guest" agent support may
not be feasible, so another command-line option may be needed support
host-side-only testing.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qmp-core.c       |    8 ++
 qmp-core.h       |    7 +-
 qmp-proxy-core.h |   20 ++++
 qmp-proxy.c      |  335 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 vl.c             |    1 +
 5 files changed, 365 insertions(+), 6 deletions(-)
 create mode 100644 qmp-proxy-core.h
 create mode 100644 qmp-proxy.c

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..47ac85d
--- /dev/null
+++ b/qmp-proxy-core.h
@@ -0,0 +1,20 @@
+#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(const char *channel_path);
+void qmp_proxy_close(QmpProxy *p);
+
+#endif
diff --git a/qmp-proxy.c b/qmp-proxy.c
new file mode 100644
index 0000000..eaa6e6e
--- /dev/null
+++ b/qmp-proxy.c
@@ -0,0 +1,335 @@
+/*
+ * 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 "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;
+
+typedef struct QmpProxyWriteState {
+    QmpProxyRequest *current_request;
+    const char *buf;
+    size_t size;
+    size_t pos;
+    bool use_sentinel;
+} QmpProxyWriteState;
+
+typedef struct QmpProxyReadState {
+    char *buf;
+    size_t size;
+    size_t pos;
+} QmpProxyReadState;
+
+struct QmpProxy {
+    int fd;
+    const char *path;
+    QmpProxyWriteState write_state;
+    QmpProxyReadState read_state;
+    JSONMessageParser parser;
+    QTAILQ_HEAD(, QmpProxyRequest) sent_requests;
+    QTAILQ_HEAD(, QmpProxyRequest) queued_requests;
+    QString *xport_event;
+    QString *xport_event_sending;
+};
+
+static void qmp_proxy_read_handler(void *opaque);
+static void qmp_proxy_write_handler(void *opaque);
+
+static int qmp_proxy_cancel_request(QmpProxy *p, QmpProxyRequest *r)
+{
+    if (r->name) {
+        if (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->queued_requests, entry, tmp) {
+        qmp_proxy_cancel_request(p, r);
+        QTAILQ_REMOVE(&p->queued_requests, r, entry);
+    }
+    QTAILQ_FOREACH_SAFE(r, &p->sent_requests, entry, tmp) {
+        qmp_proxy_cancel_request(p, r);
+        QTAILQ_REMOVE(&p->queued_requests, r, entry);
+    }
+
+    return 0;
+}
+
+static void qmp_proxy_send_host_ack(QmpProxy *p, int session_id)
+{
+    QDict *evt = qdict_new();
+
+    /* only the last ack matters, nuke any outstanding ones. need to rethink
+     * this approach if a host->guest reset event is added
+     */
+    if (p->xport_event) {
+        QDECREF(p->xport_event);
+    }
+
+    qdict_put_obj(evt, "_xport_event", QOBJECT(qstring_from_str("host_ack")));
+    qdict_put_obj(evt, "_xport_arg_sid", QOBJECT(qint_from_int(session_id)));
+
+    p->xport_event = qobject_to_json(QOBJECT(evt));
+
+    qemu_set_fd_handler(p->fd, qmp_proxy_read_handler,
+                        qmp_proxy_write_handler, p);
+}
+
+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;
+    const char *cmd;
+    int session_id;
+
+    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);
+    }
+
+    /* check for transport-only commands/events */
+    if (qdict_haskey(qdict, "_xport_event")) {
+        cmd = qdict_get_try_str(qdict, "_xport_event");
+        if (cmd && strcmp(cmd, "guest_init") == 0) {
+            /* reset outstanding requests, then send an ack with the
+             * session id they passed us
+             */
+            session_id = qdict_get_try_int(qdict, "_xport_arg_sid", 0);
+            if (!session_id) {
+                fprintf(stderr, "received invalid guest_init event\n");
+            }
+            qmp_proxy_cancel_all(p);
+            qmp_proxy_send_host_ack(p, session_id);
+
+            return;
+        }
+    } else if (qdict_haskey(qdict, "return")) {
+        fprintf(stderr, "received return\n");
+        r = QTAILQ_FIRST(&p->sent_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))));
+        /* TODO: we don't know what kind of qtype the return value is, what
+         * really need is for the qmp async callbacks to handle demarshalling
+         * for us, for now we just pass the whole response up the stack, which
+         * means everything except commands with no return value, like
+         * guest-ping, will result in errors reported to the client
+         */ 
+        r->cb(r->opaque, QOBJECT(qdict), NULL);
+        QTAILQ_REMOVE(&p->sent_requests, r, entry);
+        fprintf(stderr, "done handling response\n");
+    } else {
+        fprintf(stderr, "received invalid payload format\n");
+    }
+}
+
+static void qmp_proxy_read_handler(void *opaque)
+{
+    QmpProxy *p = opaque;
+    char buf[4096];
+    int ret;
+
+    do {
+        ret = read(p->fd, buf, 4096);
+        if (ret == -1) {
+            if (errno != EAGAIN && errno != EINTR) {
+                fprintf(stderr, "qmp proxy: error reading request: %s",
+                        strerror(errno));
+            }
+            return;
+        } else if (ret == 0) {
+            /* TODO: is this recoverable? should only happen for hot-unplug
+             * in the chardev case, but for testing via a local guest agent
+             * we may want to do some special handling...
+             */
+            fprintf(stderr, "qmp proxy: connection closed unexpectedly");
+            qmp_proxy_cancel_all(p);
+            qemu_set_fd_handler(p->fd, NULL, NULL, p);
+            return;
+        }
+        buf[ret] = 0;
+        json_message_parser_feed(&p->parser, (char *)buf, ret);
+    } while (ret > 0);
+}
+
+static void qmp_proxy_write_handler(void *opaque)
+{
+    QmpProxy *p = opaque;
+    QmpProxyWriteState s = p->write_state;
+    QmpProxyRequest *r;
+    char sentinel = QMP_SENTINEL;
+    int ret;
+
+send_another:
+    if (p->xport_event) {
+        s.current_request = NULL;
+        if (p->xport_event_sending) {
+            QDECREF(p->xport_event_sending);
+        }
+        p->xport_event_sending = p->xport_event;
+        p->xport_event = NULL;
+        s.buf = qstring_get_str(p->xport_event_sending);
+        s.pos = 0;
+        s.size = strlen(s.buf);
+        s.use_sentinel = true;
+    } else if (!s.current_request) {
+        r = QTAILQ_FIRST(&p->queued_requests);
+        if (r == NULL) {
+            /* no more requests to send for now */
+            qemu_set_fd_handler(p->fd, qmp_proxy_read_handler, NULL, p);
+            return;
+        }
+        s.current_request = r;
+        s.buf = qstring_get_str(s.current_request->json);
+        s.pos = 0;
+        s.size = strlen(s.buf);
+        s.use_sentinel = false;
+    }
+
+    while (s.pos < s.size) {
+        if (s.use_sentinel) {
+            ret = write(p->fd, &sentinel, 1);
+        } else {
+            ret = write(p->fd, s.buf + s.pos, s.size - s.pos);
+        }
+        if (ret == -1) {
+            if (errno != EAGAIN && errno != EINTR) {
+                fprintf(stderr, "qmp proxy: error sending payload");
+            }
+            return;
+        } else if (ret == 0) {
+            /* TODO: is this recoverable? should only happen for hot-unplug
+             * in the chardev case, but for testing via a local guest agent
+             * we may want to do some special handling...
+             */
+            fprintf(stderr, "qmp proxy: connection closed unexpectedly");
+            qmp_proxy_cancel_all(p);
+            qemu_set_fd_handler(p->fd, NULL, NULL, p);
+            return;
+        }
+        if (s.use_sentinel) {
+            s.use_sentinel = false;
+        } else {
+            s.pos += ret;
+        }
+    }
+
+    /* done with this request. send another if there is one */
+    if (s.current_request) {
+        QTAILQ_REMOVE(&p->queued_requests, s.current_request, entry);
+        QTAILQ_INSERT_TAIL(&p->sent_requests, s.current_request, entry);
+        s.current_request = NULL;
+    } else if (p->xport_event) {
+        QDECREF(p->xport_event);
+        p->xport_event = NULL;
+    }
+    s.use_sentinel = false;
+    goto send_another;
+}
+
+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();
+
+    /* 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;
+    QTAILQ_INSERT_TAIL(&p->queued_requests, r, entry);
+
+    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));
+
+    r->json = qobject_to_json(QOBJECT((QDict *)payload));
+    if (!r->json) {
+        QDECREF(r->json);
+        goto out_bad;
+    }
+
+    qemu_set_fd_handler(p->fd, qmp_proxy_read_handler,
+                        qmp_proxy_write_handler, p);
+out_bad:
+    return;
+}
+
+QmpProxy *qmp_proxy_new(const char *channel_path)
+{
+    QmpProxy *p = qemu_mallocz(sizeof(QmpProxy));
+    QemuOpts *opts;
+    int fd;
+
+    /* connect to guest agent channel */
+    opts = qemu_opts_create(qemu_find_opts("chardev", NULL), NULL, 0, NULL);
+    qemu_opt_set(opts, "path", channel_path, NULL);
+    fd = unix_connect_opts(opts);
+    if (fd == -1) {
+        qemu_opts_del(opts);
+        fprintf(stderr, "qmp proxy: error opening channel: %s",
+                strerror(errno));
+        return NULL;
+    }
+    qemu_opts_del(opts);
+    socket_set_nonblock(fd);
+
+    p->fd = fd;
+    json_message_parser_init(&p->parser, qmp_proxy_process_event);
+    QTAILQ_INIT(&p->queued_requests);
+    QTAILQ_INIT(&p->sent_requests);
+    qemu_set_fd_handler(p->fd, qmp_proxy_read_handler, NULL, p);
+
+    return p;
+}
+
+void qmp_proxy_close(QmpProxy *p)
+{
+    qmp_proxy_cancel_all(p);
+    close(p->fd);
+    unlink(p->path);
+    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] Re: [RFC][PATCH v1 07/12] qmp proxy: core code for proxying qmp requests to guest

[Anthony Liguori]

On 03/25/2011 02:47 PM, Michael Roth wrote:
> 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.
>
> Currently the class requires a path to a listening socket that either
> corresponds to the chardev that the guest agent is communicating
> through, or a local socket so we can communicate with a host-side
> "guest" agent for testing purposes.
>
> A subsequent patch will introduce a new chardev that sets up the
> socket chardev and initializes the QmpProxy instance to abstract this
> away from the user. Unifying this with local "guest" agent support may
> not be feasible, so another command-line option may be needed support
> host-side-only testing.
>
> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
> ---
>   qmp-core.c       |    8 ++
>   qmp-core.h       |    7 +-
>   qmp-proxy-core.h |   20 ++++
>   qmp-proxy.c      |  335 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   vl.c             |    1 +
>   5 files changed, 365 insertions(+), 6 deletions(-)
>   create mode 100644 qmp-proxy-core.h
>   create mode 100644 qmp-proxy.c
>
> 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..47ac85d
> --- /dev/null
> +++ b/qmp-proxy-core.h
> @@ -0,0 +1,20 @@
> +#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(const char *channel_path);
> +void qmp_proxy_close(QmpProxy *p);
> +
> +#endif
> diff --git a/qmp-proxy.c b/qmp-proxy.c
> new file mode 100644
> index 0000000..eaa6e6e
> --- /dev/null
> +++ b/qmp-proxy.c
> @@ -0,0 +1,335 @@
> +/*
> + * 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 "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;
> +
> +typedef struct QmpProxyWriteState {
> +    QmpProxyRequest *current_request;
> +    const char *buf;
> +    size_t size;
> +    size_t pos;
> +    bool use_sentinel;
> +} QmpProxyWriteState;
> +
> +typedef struct QmpProxyReadState {
> +    char *buf;
> +    size_t size;
> +    size_t pos;
> +} QmpProxyReadState;

I suspect you should use GStrings for both the rx and tx buffers.  See 
the QmpUnixSession for an example.

> +struct QmpProxy {
> +    int fd;
> +    const char *path;
> +    QmpProxyWriteState write_state;
> +    QmpProxyReadState read_state;
> +    JSONMessageParser parser;
> +    QTAILQ_HEAD(, QmpProxyRequest) sent_requests;
> +    QTAILQ_HEAD(, QmpProxyRequest) queued_requests;
> +    QString *xport_event;
> +    QString *xport_event_sending;
> +};
> +
> +static void qmp_proxy_read_handler(void *opaque);
> +static void qmp_proxy_write_handler(void *opaque);
> +
> +static int qmp_proxy_cancel_request(QmpProxy *p, QmpProxyRequest *r)
> +{
> +    if (r->name) {
> +        if (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->queued_requests, entry, tmp) {
> +        qmp_proxy_cancel_request(p, r);
> +        QTAILQ_REMOVE(&p->queued_requests, r, entry);
> +    }
> +    QTAILQ_FOREACH_SAFE(r,&p->sent_requests, entry, tmp) {
> +        qmp_proxy_cancel_request(p, r);
> +        QTAILQ_REMOVE(&p->queued_requests, r, entry);
> +    }
> +
> +    return 0;
> +}
> +
> +static void qmp_proxy_send_host_ack(QmpProxy *p, int session_id)
> +{
> +    QDict *evt = qdict_new();
> +
> +    /* only the last ack matters, nuke any outstanding ones. need to rethink
> +     * this approach if a host->guest reset event is added
> +     */
> +    if (p->xport_event) {
> +        QDECREF(p->xport_event);
> +    }
> +
> +    qdict_put_obj(evt, "_xport_event", QOBJECT(qstring_from_str("host_ack")));
> +    qdict_put_obj(evt, "_xport_arg_sid", QOBJECT(qint_from_int(session_id)));

I don't quite follow what this is doing.

> +    p->xport_event = qobject_to_json(QOBJECT(evt));
> +
> +    qemu_set_fd_handler(p->fd, qmp_proxy_read_handler,
> +                        qmp_proxy_write_handler, p);
> +}
> +
> +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;
> +    const char *cmd;
> +    int session_id;
> +
> +    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);
> +    }
> +
> +    /* check for transport-only commands/events */
> +    if (qdict_haskey(qdict, "_xport_event")) {
> +        cmd = qdict_get_try_str(qdict, "_xport_event");
> +        if (cmd&&  strcmp(cmd, "guest_init") == 0) {
> +            /* reset outstanding requests, then send an ack with the
> +             * session id they passed us
> +             */
> +            session_id = qdict_get_try_int(qdict, "_xport_arg_sid", 0);
> +            if (!session_id) {
> +                fprintf(stderr, "received invalid guest_init event\n");
> +            }
> +            qmp_proxy_cancel_all(p);
> +            qmp_proxy_send_host_ack(p, session_id);
> +
> +            return;
> +        }
> +    } else if (qdict_haskey(qdict, "return")) {
> +        fprintf(stderr, "received return\n");
> +        r = QTAILQ_FIRST(&p->sent_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))));
> +        /* TODO: we don't know what kind of qtype the return value is, what
> +         * really need is for the qmp async callbacks to handle demarshalling
> +         * for us, for now we just pass the whole response up the stack, which
> +         * means everything except commands with no return value, like
> +         * guest-ping, will result in errors reported to the client
> +         */
> +        r->cb(r->opaque, QOBJECT(qdict), NULL);
> +        QTAILQ_REMOVE(&p->sent_requests, r, entry);
> +        fprintf(stderr, "done handling response\n");
> +    } else {
> +        fprintf(stderr, "received invalid payload format\n");
> +    }
> +}
> +
> +static void qmp_proxy_read_handler(void *opaque)
> +{
> +    QmpProxy *p = opaque;
> +    char buf[4096];
> +    int ret;
> +
> +    do {
> +        ret = read(p->fd, buf, 4096);
> +        if (ret == -1) {
> +            if (errno != EAGAIN&&  errno != EINTR) {
> +                fprintf(stderr, "qmp proxy: error reading request: %s",
> +                        strerror(errno));
> +            }
> +            return;
> +        } else if (ret == 0) {
> +            /* TODO: is this recoverable? should only happen for hot-unplug
> +             * in the chardev case, but for testing via a local guest agent
> +             * we may want to do some special handling...
> +             */
> +            fprintf(stderr, "qmp proxy: connection closed unexpectedly");
> +            qmp_proxy_cancel_all(p);
> +            qemu_set_fd_handler(p->fd, NULL, NULL, p);
> +            return;
> +        }
> +        buf[ret] = 0;

You have a buffer overflow here.

> +        json_message_parser_feed(&p->parser, (char *)buf, ret);

cast is unnecessary.

> +    } while (ret>  0);
> +}
> +
> +static void qmp_proxy_write_handler(void *opaque)
> +{
> +    QmpProxy *p = opaque;
> +    QmpProxyWriteState s = p->write_state;
> +    QmpProxyRequest *r;
> +    char sentinel = QMP_SENTINEL;
> +    int ret;
> +
> +send_another:

Better to avoid being clever here and have a separate send_once function 
and then a loop() that calls send_once.

Regards,

Anthony Liguori

[Qemu-devel] Re: [RFC][PATCH v1 07/12] qmp proxy: core code for proxying qmp requests to guest

[Michael Roth]

On 03/25/2011 04:27 PM, Anthony Liguori wrote:
> On 03/25/2011 02:47 PM, Michael Roth wrote:
>> 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.
>>
>> Currently the class requires a path to a listening socket that either
>> corresponds to the chardev that the guest agent is communicating
>> through, or a local socket so we can communicate with a host-side
>> "guest" agent for testing purposes.
>>
>> A subsequent patch will introduce a new chardev that sets up the
>> socket chardev and initializes the QmpProxy instance to abstract this
>> away from the user. Unifying this with local "guest" agent support may
>> not be feasible, so another command-line option may be needed support
>> host-side-only testing.
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>> qmp-core.c | 8 ++
>> qmp-core.h | 7 +-
>> qmp-proxy-core.h | 20 ++++
>> qmp-proxy.c | 335 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> vl.c | 1 +
>> 5 files changed, 365 insertions(+), 6 deletions(-)
>> create mode 100644 qmp-proxy-core.h
>> create mode 100644 qmp-proxy.c
>>
>> 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..47ac85d
>> --- /dev/null
>> +++ b/qmp-proxy-core.h
>> @@ -0,0 +1,20 @@
>> +#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(const char *channel_path);
>> +void qmp_proxy_close(QmpProxy *p);
>> +
>> +#endif
>> diff --git a/qmp-proxy.c b/qmp-proxy.c
>> new file mode 100644
>> index 0000000..eaa6e6e
>> --- /dev/null
>> +++ b/qmp-proxy.c
>> @@ -0,0 +1,335 @@
>> +/*
>> + * 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 "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;
>> +
>> +typedef struct QmpProxyWriteState {
>> + QmpProxyRequest *current_request;
>> + const char *buf;
>> + size_t size;
>> + size_t pos;
>> + bool use_sentinel;
>> +} QmpProxyWriteState;
>> +
>> +typedef struct QmpProxyReadState {
>> + char *buf;
>> + size_t size;
>> + size_t pos;
>> +} QmpProxyReadState;
>
> I suspect you should use GStrings for both the rx and tx buffers. See
> the QmpUnixSession for an example.
>
>> +struct QmpProxy {
>> + int fd;
>> + const char *path;
>> + QmpProxyWriteState write_state;
>> + QmpProxyReadState read_state;
>> + JSONMessageParser parser;
>> + QTAILQ_HEAD(, QmpProxyRequest) sent_requests;
>> + QTAILQ_HEAD(, QmpProxyRequest) queued_requests;
>> + QString *xport_event;
>> + QString *xport_event_sending;
>> +};
>> +
>> +static void qmp_proxy_read_handler(void *opaque);
>> +static void qmp_proxy_write_handler(void *opaque);
>> +
>> +static int qmp_proxy_cancel_request(QmpProxy *p, QmpProxyRequest *r)
>> +{
>> + if (r->name) {
>> + if (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->queued_requests, entry, tmp) {
>> + qmp_proxy_cancel_request(p, r);
>> + QTAILQ_REMOVE(&p->queued_requests, r, entry);
>> + }
>> + QTAILQ_FOREACH_SAFE(r,&p->sent_requests, entry, tmp) {
>> + qmp_proxy_cancel_request(p, r);
>> + QTAILQ_REMOVE(&p->queued_requests, r, entry);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void qmp_proxy_send_host_ack(QmpProxy *p, int session_id)
>> +{
>> + QDict *evt = qdict_new();
>> +
>> + /* only the last ack matters, nuke any outstanding ones. need to
>> rethink
>> + * this approach if a host->guest reset event is added
>> + */
>> + if (p->xport_event) {
>> + QDECREF(p->xport_event);
>> + }
>> +
>> + qdict_put_obj(evt, "_xport_event",
>> QOBJECT(qstring_from_str("host_ack")));
>> + qdict_put_obj(evt, "_xport_arg_sid",
>> QOBJECT(qint_from_int(session_id)));
>
> I don't quite follow what this is doing.
>

That's for the session negotiation so we can reset state when the guest 
agent restarts. The sequence is:

guest -> host
{ "_xport_event": "guest_init", "_xport_arg_sid": <random session id> }

host -> guest
{ "_xport_event": "host_ack", "_xport_arg_sid": <received session id> }

Guest will ignore anything it gets until it sees an ack with the proper 
session id, host will cancel outstanding requests when it receives a 
guest init. If there's already an event waiting to be sent we clobber it 
since for the above exchange only the most recent event we sent matters.

We send them as json objects, but they get handled at transport level 
and terminate there.

Re: [Qemu-devel] Re: [RFC][PATCH v1 07/12] qmp proxy: core code for proxying qmp requests to guest

[Anthony Liguori]

On 03/25/2011 04:56 PM, Michael Roth wrote:
>> I don't quite follow what this is doing.
>>
>
>
> That's for the session negotiation so we can reset state when the 
> guest agent restarts. The sequence is:
>
> guest -> host
> { "_xport_event": "guest_init", "_xport_arg_sid": <random session id> }
>
> host -> guest
> { "_xport_event": "host_ack", "_xport_arg_sid": <received session id> }
>
> Guest will ignore anything it gets until it sees an ack with the 
> proper session id, host will cancel outstanding requests when it 
> receives a guest init. If there's already an event waiting to be sent 
> we clobber it since for the above exchange only the most recent event 
> we sent matters.
>
> We send them as json objects, but they get handled at transport level 
> and terminate there.

Doesn't an invalid UTF-8 sequence provide the same functionality?

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [RFC][PATCH v1 07/12] qmp proxy: core code for proxying qmp requests to guest

[Michael Roth]

On 03/28/2011 02:05 PM, Anthony Liguori wrote:
> On 03/25/2011 04:56 PM, Michael Roth wrote:
>>> I don't quite follow what this is doing.
>>>
>>
>>
>> That's for the session negotiation so we can reset state when the
>> guest agent restarts. The sequence is:
>>
>> guest -> host
>> { "_xport_event": "guest_init", "_xport_arg_sid": <random session id> }
>>
>> host -> guest
>> { "_xport_event": "host_ack", "_xport_arg_sid": <received session id> }
>>
>> Guest will ignore anything it gets until it sees an ack with the
>> proper session id, host will cancel outstanding requests when it
>> receives a guest init. If there's already an event waiting to be sent
>> we clobber it since for the above exchange only the most recent event
>> we sent matters.
>>
>> We send them as json objects, but they get handled at transport level
>> and terminate there.
>
> Doesn't an invalid UTF-8 sequence provide the same functionality?

We do use the invalid UTF-8 sequence here to some extent, but just to 
ensure the xport events are delivered intact. All xport events are 
preceded with 0xFF to make sure they delivered. After negotiation we 
assume everything is clear for standard json streaming.

We could use an invalid UTF-8 sequence by itself, but it would need to 
be paired with some mechanism to clear the channel, otherwise multiple 
guest agent restarts could result in an agent reading a "stale" ack from 
the host and proceeding to service canceled/invalid requests.

I think, at least with virtio-serial, a close() on the chardev would 
clear the channel...so that might work. That may not be the case with 
isa-serial though. Pairing the init/ack sequence with a session id just 
seemed a little more robust.

>
> Regards,
>
> Anthony Liguori
>
>

[Qemu-devel] [RFC][PATCH v1 08/12] qemu-char: add qmp_proxy chardev

[Michael Roth]

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

qemu -chardev qmp_proxy,path=<optional>,id=qmp_proxy \
      -device ...,chardev=qmp_proxy

It is essentially a wrapper for -chardev socket, which takes the extra
step of setting required defaults and initializing the qmp proxy by
passing the path argument along.

Not sure if this is the most elegant approach, but in terms of the
command-line invocation it seems to be the most consistent way to do
it.

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

diff --git a/qemu-char.c b/qemu-char.c
index d301925..6ff7698 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2275,6 +2275,51 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     return NULL;
 }
 
+#include "qmp-proxy-core.h"
+
+extern QmpProxy *qmp_proxy_default;
+
+static CharDriverState *qemu_chr_open_qmp_proxy(QemuOpts *opts)
+{
+    CharDriverState *chr;
+    QmpProxy *p;
+    const char *path;
+
+    /* revert to/enforce default socket chardev options for qmp proxy */
+    path = qemu_opt_get(opts, "path");
+    if (path == NULL) {
+        path = QMP_PROXY_PATH_DEFAULT;
+        qemu_opt_set_qerr(opts, "path", path);
+    }
+    /* required options for qmp proxy */
+    qemu_opt_set_qerr(opts, "server", "on");
+    qemu_opt_set_qerr(opts, "wait", "off");
+    qemu_opt_set_qerr(opts, "telnet", "off");
+
+    chr = qemu_chr_open_socket(opts);
+    if (chr == NULL) {
+        goto err;
+    }
+
+    /* initialize virtagent using the socket we just set up */
+    if (qmp_proxy_default) {
+        fprintf(stderr, "error, multiple qmp guest proxies are not allowed\n");
+    }
+    p = qmp_proxy_new(path);
+    if (p == NULL) {
+        fprintf(stderr, "error initializing qmp guest proxy\n");
+        goto err;
+    }
+    qmp_proxy_default = p;
+
+    return chr;
+err:
+    if (chr) {
+        qemu_free(chr);
+    }
+    return NULL;
+}
+
 /***********************************************************/
 /* Memory chardev */
 typedef struct {
@@ -2495,6 +2540,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] Re: [RFC][PATCH v1 08/12] qemu-char: add qmp_proxy chardev

[Anthony Liguori]

On 03/25/2011 02:47 PM, Michael Roth wrote:
> This allows qemu to be started with guest agent support via:
>
> qemu -chardev qmp_proxy,path=<optional>,id=qmp_proxy \
>        -device ...,chardev=qmp_proxy
>
> It is essentially a wrapper for -chardev socket, which takes the extra
> step of setting required defaults and initializing the qmp proxy by
> passing the path argument along.
>
> Not sure if this is the most elegant approach, but in terms of the
> command-line invocation it seems to be the most consistent way to do
> it.
>
> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
> ---
>   qemu-char.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 46 insertions(+), 0 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index d301925..6ff7698 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2275,6 +2275,51 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>       return NULL;
>   }
>
> +#include "qmp-proxy-core.h"
> +
> +extern QmpProxy *qmp_proxy_default;
> +
> +static CharDriverState *qemu_chr_open_qmp_proxy(QemuOpts *opts)
> +{
> +    CharDriverState *chr;
> +    QmpProxy *p;
> +    const char *path;
> +
> +    /* revert to/enforce default socket chardev options for qmp proxy */
> +    path = qemu_opt_get(opts, "path");
> +    if (path == NULL) {
> +        path = QMP_PROXY_PATH_DEFAULT;
> +        qemu_opt_set_qerr(opts, "path", path);
> +    }
> +    /* required options for qmp proxy */
> +    qemu_opt_set_qerr(opts, "server", "on");
> +    qemu_opt_set_qerr(opts, "wait", "off");
> +    qemu_opt_set_qerr(opts, "telnet", "off");

Why are these options required?

Regards,

Anthony Liguori

> +
> +    chr = qemu_chr_open_socket(opts);
> +    if (chr == NULL) {
> +        goto err;
> +    }
> +
> +    /* initialize virtagent using the socket we just set up */
> +    if (qmp_proxy_default) {
> +        fprintf(stderr, "error, multiple qmp guest proxies are not allowed\n");
> +    }
> +    p = qmp_proxy_new(path);
> +    if (p == NULL) {
> +        fprintf(stderr, "error initializing qmp guest proxy\n");
> +        goto err;
> +    }
> +    qmp_proxy_default = p;
> +
> +    return chr;
> +err:
> +    if (chr) {
> +        qemu_free(chr);
> +    }
> +    return NULL;
> +}
> +
>   /***********************************************************/
>   /* Memory chardev */
>   typedef struct {
> @@ -2495,6 +2540,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,

[Qemu-devel] Re: [RFC][PATCH v1 08/12] qemu-char: add qmp_proxy chardev

[Michael Roth]

On 03/25/2011 04:29 PM, Anthony Liguori wrote:
> On 03/25/2011 02:47 PM, Michael Roth wrote:
>> This allows qemu to be started with guest agent support via:
>>
>> qemu -chardev qmp_proxy,path=<optional>,id=qmp_proxy \
>> -device ...,chardev=qmp_proxy
>>
>> It is essentially a wrapper for -chardev socket, which takes the extra
>> step of setting required defaults and initializing the qmp proxy by
>> passing the path argument along.
>>
>> Not sure if this is the most elegant approach, but in terms of the
>> command-line invocation it seems to be the most consistent way to do
>> it.
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>> qemu-char.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 46 insertions(+), 0 deletions(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index d301925..6ff7698 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2275,6 +2275,51 @@ static CharDriverState
>> *qemu_chr_open_socket(QemuOpts *opts)
>> return NULL;
>> }
>>
>> +#include "qmp-proxy-core.h"
>> +
>> +extern QmpProxy *qmp_proxy_default;
>> +
>> +static CharDriverState *qemu_chr_open_qmp_proxy(QemuOpts *opts)
>> +{
>> + CharDriverState *chr;
>> + QmpProxy *p;
>> + const char *path;
>> +
>> + /* revert to/enforce default socket chardev options for qmp proxy */
>> + path = qemu_opt_get(opts, "path");
>> + if (path == NULL) {
>> + path = QMP_PROXY_PATH_DEFAULT;
>> + qemu_opt_set_qerr(opts, "path", path);
>> + }
>> + /* required options for qmp proxy */
>> + qemu_opt_set_qerr(opts, "server", "on");
>> + qemu_opt_set_qerr(opts, "wait", "off");
>> + qemu_opt_set_qerr(opts, "telnet", "off");
>
> Why are these options required?

The qmp_proxy_new() constructor expects a path to a socket it can 
connect() to. Not sure about telnet, but the other options are required 
for this. Well...server=on at least, wait=off needs to be set as well 
because that's the only way to have the socket chardev set the listening 
socket to non-block, since qmp_proxy_new() calls connect() on it before 
we return to the main I/O loop.

Although, we probably shouldn't just silently switch out explicitly set 
options; an error would probably be more appropriate here.

>
> Regards,
>
> Anthony Liguori
>

[Qemu-devel] Re: [RFC][PATCH v1 08/12] qemu-char: add qmp_proxy chardev

[Anthony Liguori]

On 03/25/2011 05:11 PM, Michael Roth wrote:
>> Why are these options required?
>
>
> The qmp_proxy_new() constructor expects a path to a socket it can 
> connect() to. Not sure about telnet, but the other options are 
> required for this. Well...server=on at least, wait=off needs to be set 
> as well because that's the only way to have the socket chardev set the 
> listening socket to non-block, since qmp_proxy_new() calls connect() 
> on it before we return to the main I/O loop.
>
> Although, we probably shouldn't just silently switch out explicitly 
> set options; an error would probably be more appropriate here.

You ought to make qmp_proxy_new() return a CharDriverState such that you 
can do:

qemu -device virtio-serial,chardev=foo -chardev guest-agent,id=foo

Regards,

Anthony Liguori

>>
>> Regards,
>>
>> Anthony Liguori
>>

[Qemu-devel] Re: [RFC][PATCH v1 08/12] qemu-char: add qmp_proxy chardev

[Michael Roth]

On 03/28/2011 12:45 PM, Anthony Liguori wrote:
> On 03/25/2011 05:11 PM, Michael Roth wrote:
>>> Why are these options required?
>>
>>
>> The qmp_proxy_new() constructor expects a path to a socket it can
>> connect() to. Not sure about telnet, but the other options are
>> required for this. Well...server=on at least, wait=off needs to be set
>> as well because that's the only way to have the socket chardev set the
>> listening socket to non-block, since qmp_proxy_new() calls connect()
>> on it before we return to the main I/O loop.
>>
>> Although, we probably shouldn't just silently switch out explicitly
>> set options; an error would probably be more appropriate here.
>
> You ought to make qmp_proxy_new() return a CharDriverState such that you
> can do:
>
> qemu -device virtio-serial,chardev=foo -chardev guest-agent,id=foo
>

That's what the current invocation looks like, but with regard to not 
relying on an intermediate socket between QmpProxy and individual qmp 
sessions, I think the main issue is:

- We expose the proxy via a call such a qmp_proxy_send_request(QDict 
request)
- This request can be arbitrarily large (not atm, but in the future 
perhaps with RPCs sent as qmp_send_file() they may potential by large)

So if we do this directly via a new char state rather than intermediate 
sock, we'd either:

1) Send the request in full via, say, 
qmp_proxy_send_request()->qemu_chr_read()->virtio_serial_write(), and 
assume the port/device can actually buffer it all in one shot. Or,

2) Send it in smaller chunks, via something that amounts to:

     can_read_bytes = virtio_serial_guest_ready()):
     virtio_serial_write(port, request, can_read_bytes)

    If anything is left over, we need to add something QEMU's main loop 
to handle some of the remaining bytes in the next main loop iteration. 
(With the intermediate socket, we achieve this by registering a write 
handler that selects on the intermediate socket and writes in small 
chunks until the buffer is empty, the socket chardev does all the work 
of checking for the backend device to be ready and not writing more than 
it should)

So, if we do 2), which I think is the only "correct" approach out of the 
2, to take the intermediate socket fd out of the equation, we either 
need to add a custom handler to QEMU's main loop, or register deferred 
work via something like qemu_bh_schedule_idle().

So I think that's the trade-off...we can:

- do what the patches currently do and complicate the plumbing by adding 
an intermediate socket (or perhaps even just a pipe), and simplify the 
logic of handling backlogged work via a select()-driven write handler, or

- remove the intermediate socket to simplify the plumbing, but 
complicate the back-end logic involved in sending requests to the guest 
by using a BH to clear our host->guest TX buffer

If using a BH to handle this work seems reasonable, then I'd be fine 
with attempting to implement this. If using a BH seems nasty, then I 
think the current intermediate socket/fd approach is the best alternative.

> Regards,
>
> Anthony Liguori
>

[Qemu-devel] [RFC][PATCH v1 09/12] 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>
---
 guest-agent-core.c |  143 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 guest-agent-core.h |   21 ++++++++
 2 files changed, 164 insertions(+), 0 deletions(-)
 create mode 100644 guest-agent-core.c
 create mode 100644 guest-agent-core.h

diff --git a/guest-agent-core.c b/guest-agent-core.c
new file mode 100644
index 0000000..6c3b031
--- /dev/null
+++ b/guest-agent-core.c
@@ -0,0 +1,143 @@
+/*
+ * 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->state = state;
+        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:
+    qobject_decref(request);
+
+    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/guest-agent-core.h b/guest-agent-core.h
new file mode 100644
index 0000000..18126b0
--- /dev/null
+++ b/guest-agent-core.h
@@ -0,0 +1,21 @@
+/*
+ * 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"
+
+QObject *qga_dispatch(QObject *obj, Error **errp);
+void qga_register_command(const char *name, QmpCommandFunc *fn);
+void qga_init_marshal(void);
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v1 10/12] 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.

This is currently horribly broken, only the unix-listen channel method
is working at the moment (likely due to mis-use of gio channel
interfaces), and the code is in overall rough shape.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qemu-ga.c |  522 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 522 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..435a1fc
--- /dev/null
+++ b/qemu-ga.c
@@ -0,0 +1,522 @@
+/*
+ * 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 "qemu_socket.h"
+#include "json-streamer.h"
+#include "json-parser.h"
+#include "guest-agent.h"
+
+#define QGA_VERSION "1.0"
+#define QGA_GUEST_PATH_VIRTIO_DEFAULT "/dev/virtio-ports/va"
+#define QGA_PIDFILE_DEFAULT "/var/run/qemu-va.pid"
+#define QGA_BAUDRATE_DEFAULT B38400 /* for isa-serial channels */
+
+bool verbose_enabled = false;
+
+typedef 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;
+} GAState;
+
+static void usage(const char *cmd)
+{
+    printf(
+"Usage: %s -c <channel_opts>\n"
+"QEMU virtagent guest agent %s\n"
+"\n"
+"  -c, --channel     channel method: one of unix-connect, virtio-serial, or\n"
+"                    isa-serial\n"
+"  -p, --path        channel path\n"
+"  -v, --verbose     display extra debugging information\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);
+}
+
+static void conn_channel_close(GAState *s);
+
+static void become_daemon(void)
+{
+    pid_t pid, sid;
+    int pidfd;
+    char *pidstr;
+
+    pid = fork();
+    if (pid < 0)
+        exit(EXIT_FAILURE);
+    if (pid > 0) {
+        exit(EXIT_SUCCESS);
+    }
+
+    pidfd = open(QGA_PIDFILE_DEFAULT, 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_error("Cannot truncate pid file");
+    if (asprintf(&pidstr, "%d", getpid()) == -1)
+        g_error("Cannot allocate memory");
+    if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr))
+        g_error("Failed to write pid file");
+    free(pidstr);
+
+    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:
+    unlink(QGA_PIDFILE_DEFAULT);
+    g_error("failed to daemonize");
+}
+
+static int conn_channel_send_payload(GIOChannel *channel, QObject *payload)
+{
+    gsize count, written=0;
+    const char *buf;
+    QString *payload_qstr;
+    GIOStatus status;
+    GError *err = NULL;
+
+    if (!payload || !channel) {
+        return -EINVAL;
+    }
+
+    payload_qstr = qobject_to_json(payload);
+    if (!payload_qstr) {
+        return -EINVAL;
+    }
+
+    buf = qstring_get_str(payload_qstr);
+    count = strlen(buf);
+
+    while (count) {
+        g_warning("sending, 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);
+            return err->code;
+        }
+        if (status == G_IO_STATUS_NORMAL) {
+            count -= written;
+        } else if (status == G_IO_STATUS_ERROR || status == G_IO_STATUS_EOF) {
+            return -EPIPE;
+        }
+    }
+    g_io_channel_flush(channel, &err);
+    if (err != NULL) {
+        g_warning("error flushing payload: %s", err->message);
+        return err->code;
+    }
+    return 0;
+}
+
+/* keep reading channel, but ignore all data until we see an appropriate
+ * ack from the host
+ */
+static void start_negotiation(GAState *s)
+{
+    QDict *payload = qdict_new();
+    int ret;
+
+    g_debug("[re]negotiating connection");
+
+    g_assert(s && s->conn_channel);
+    s->active = false;
+    s->session_id = g_random_int_range(1, G_MAXINT32);
+
+    qdict_put_obj(payload, "_xport_event",
+                  QOBJECT(qstring_from_str("guest_init")));
+    qdict_put_obj(payload, "_xport_arg_sid",
+                  QOBJECT(qint_from_int(s->session_id)));
+
+    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);
+    }
+}
+
+static void complete_negotiation(GAState *s)
+{
+    g_debug("connection negotiation complete");
+    s->active = true;
+}
+
+static void process_event(JSONMessageParser *parser, QList *tokens)
+{
+    GAState *s = container_of(parser, GAState, parser);
+    QObject *obj, *rsp;
+    QDict *qdict;
+    Error *err = NULL;
+    const char *cmd;
+    int session_id, ret;
+
+    g_assert(s && parser);
+
+    g_debug("process_event: called\n");
+    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);
+    }
+
+    /* check for transport-only commands/events */
+    if (qdict_haskey(qdict, "_xport_event")) {
+        cmd = qdict_get_try_str(qdict, "_xport_event");
+        if (cmd && strcmp(cmd, "host_ack") == 0) {
+            session_id = qdict_get_try_int(qdict, "_xport_arg_sid", 0);
+            if (session_id == s->session_id) {
+                /* host acknowledged us, begin normal operation */
+                complete_negotiation(s);
+            }
+            return;
+        }
+    }
+
+    /* ignore any non-xport-related events/objects */
+    if (!s->active) {
+        return;
+    }
+
+    if (qdict_haskey(qdict, "execute")) {
+        g_debug("received command");
+        rsp = qga_dispatch(QOBJECT(qdict), &err);
+        g_debug("done handling command");
+        if (err == NULL && rsp) {
+            g_debug("response:\n%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));
+                /* reset/renegotiate connection, since we may be in
+                 * unrecoverable state
+                 */
+                start_negotiation(s);
+            }
+            qobject_decref(rsp);
+        } else {
+            g_warning("error getting response");
+        }
+    } else {
+        g_warning("unrecognized payload format, ignoring");
+    }
+}
+
+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:
+        json_message_parser_feed(&s->parser, (char *)buf, (int)count);
+        g_warning("count: %d, data: %s", (int)count, buf);
+    case G_IO_STATUS_AGAIN:
+        return true;
+    case G_IO_STATUS_EOF:
+        g_debug("received EOF");
+        conn_channel_close(s);
+        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 && fd > 0 && !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, 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;
+    start_negotiation(s);
+    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_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);
+        return;
+    }
+    g_io_channel_shutdown(s->conn_channel, true, NULL);
+    g_io_channel_unref(s->conn_channel);
+    s->conn_channel = NULL;
+    s->conn_id = 0;
+}
+
+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_GUEST_PATH_VIRTIO_DEFAULT;
+    }
+
+    if (strcmp(s->method, "virtio-serial") == 0) {
+        fd = qemu_open(s->path, O_RDWR);
+        if (fd == -1) {
+            g_error("error opening channel: %s", strerror(errno));
+        }
+        ret = fcntl(fd, F_GETFL);
+        if (ret < 0) {
+            g_error("error getting channel flags: %s", strerror(errno));
+        }
+        ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK | O_ASYNC);
+        if (ret < 0) {
+            g_error("error setting channel flags: %s", strerror(errno));
+        }
+        ret = conn_channel_add(s, fd);
+        if (ret) {
+            g_error("error adding channel to main loop");
+        }
+    } else if (strcmp(s->method, "isa-serial") == 0) {
+        fd = qemu_open(s->path, O_RDWR | O_NOCTTY);
+        if (fd == -1) {
+            g_error("error opening channel: %s", strerror(errno));
+        }
+        tcgetattr(fd, &tio);
+        /* set up serial port for non-canonical, dumb byte streaming */
+        tio.c_iflag &= ~(IGNBRK | BRKINT | IGNPAR | PARMRK | INPCK | ISTRIP |
+                         INLCR | IGNCR | ICRNL | IXON | IXOFF | IXANY |
+                         IMAXBEL);
+        tio.c_oflag = 0;
+        tio.c_lflag = 0;
+        tio.c_cflag |= QGA_BAUDRATE_DEFAULT;
+        /* 1 available byte min or reads will block (we'll set non-blocking
+         * elsewhere, else we have to deal with read()=0 instead)
+         */
+        tio.c_cc[VMIN] = 1;
+        tio.c_cc[VTIME] = 0;
+        /* flush everything waiting for read/xmit, it's garbage at this point */
+        tcflush(fd, TCIFLUSH);
+        tcsetattr(fd, TCSANOW, &tio);
+        ret = conn_channel_add(s, fd);
+        if (ret) {
+            g_error("error adding channel to main loop");
+        }
+    } else if (strcmp(s->method, "unix-listen") == 0) {
+        fd = unix_listen(s->path, NULL, strlen(s->path));
+        if (fd <= 0) {
+            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:", *method = NULL, *path = NULL;
+    struct option lopt[] = {
+        { "help", 0, NULL, 'h' },
+        { "version", 0, NULL, 'V' },
+        { "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;
+    GAState *s;
+
+    g_type_init();
+
+    while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
+        switch (ch) {
+        case 'c':
+            method = optarg;
+            break;
+        case 'p':
+            path = optarg;
+            break;
+        case 'v':
+            /* TODO: set appropriate log level */
+            verbose_enabled = true;
+            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();
+    }
+
+    qga_init_marshal();
+    
+    s = g_malloc(sizeof(GAState));
+    s->active = false;
+    s->session_id = 0;
+    s->conn_id = 0;
+    s->conn_channel = NULL;
+    s->path = path;
+    s->method = method;
+
+    init_guest_agent(s);
+    g_main_loop_run(s->main_loop);
+
+    return 0;
+}
-- 
1.7.0.4

[Qemu-devel] Re: [RFC][PATCH v1 10/12] guest agent: qemu-ga daemon

[Jes Sorensen]

On 03/25/11 20:47, Michael Roth wrote:
> This is the actual guest daemon, it listens for requests over a
> virtio-serial/isa-serial/unix socket channel and routes them through
> to dispatch routines, and writes the results back to the channel in
> a manner similar to Qmp.
> 
> This is currently horribly broken, only the unix-listen channel method
> is working at the moment (likely due to mis-use of gio channel
> interfaces), and the code is in overall rough shape.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  qemu-ga.c |  522 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 522 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..435a1fc
> --- /dev/null
> +++ b/qemu-ga.c
> @@ -0,0 +1,522 @@
> +/*
> + * 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 "qemu_socket.h"
> +#include "json-streamer.h"
> +#include "json-parser.h"
> +#include "guest-agent.h"
> +
> +#define QGA_VERSION "1.0"
> +#define QGA_GUEST_PATH_VIRTIO_DEFAULT "/dev/virtio-ports/va"
> +#define QGA_PIDFILE_DEFAULT "/var/run/qemu-va.pid"
> +#define QGA_BAUDRATE_DEFAULT B38400 /* for isa-serial channels */
> +
> +bool verbose_enabled = false;
> +
> +typedef 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;
> +} GAState;
> +
> +static void usage(const char *cmd)
> +{
> +    printf(
> +"Usage: %s -c <channel_opts>\n"
> +"QEMU virtagent guest agent %s\n"
> +"\n"
> +"  -c, --channel     channel method: one of unix-connect, virtio-serial, or\n"
> +"                    isa-serial\n"
> +"  -p, --path        channel path\n"
> +"  -v, --verbose     display extra debugging information\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);
> +}
> +
> +static void conn_channel_close(GAState *s);
> +
> +static void become_daemon(void)
> +{
> +    pid_t pid, sid;
> +    int pidfd;
> +    char *pidstr;
> +
> +    pid = fork();
> +    if (pid < 0)
> +        exit(EXIT_FAILURE);

There's a pile of missing braces in this file - please go through it and
fix them before the next version.

Cheers,
Jes

[Qemu-devel] [RFC][PATCH v1 11/12] guest agent: guest-side command implementations

[Michael Roth]

This is where the actual commands/RPCs are defined. This patch is mostly
just to serve as an example, but guest-ping actually does everything it
needs to.

view_file will soon be replaced with a stateful open/read/close interface,
and shutdown will be ported over from virtagent soon as well.

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

diff --git a/guest-agent-commands.c b/guest-agent-commands.c
new file mode 100644
index 0000000..ca8a894
--- /dev/null
+++ b/guest-agent-commands.c
@@ -0,0 +1,24 @@
+/*
+ * 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 "guest-agent.h"
+
+void qga_guest_ping(Error **err)
+{
+}
+
+char *qga_guest_view_file(const char *filename, Error **err)
+{
+    char *test_response = qemu_mallocz(512);
+    strcpy(test_response, "this is some text");
+    return test_response;
+}
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v1 12/12] 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  |    5 ++++-
 configure |    6 +++---
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 6dc71a0..e8aa817 100644
--- a/Makefile
+++ b/Makefile
@@ -200,6 +200,7 @@ qcfg-opts.c: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qmp-gen.py
 qmp/schema.py: $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qmp-gen.py
 	$(call quiet-command,python $(SRC_PATH)/qmp-gen.py --pybinding $@ < $<, "  GEN   $@")
 
+guest-agent.o: guest-agent.c guest-agent.h
 qmp-marshal.o: qmp-marshal.c qmp.h qapi-types.h
 qapi-types.o: qapi-types.c qapi-types.h
 libqmp.o: libqmp.c libqmp.h qapi-types.h
@@ -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 guest-agent.o guest-agent-core.o qmp-marshal-types-core.o guest-agent-commands.c
 
 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
 
diff --git a/configure b/configure
index 1af6b44..608fa63 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

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

[Michael Roth]

On 03/25/2011 02:47 PM, 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_v1
>
> The QGA-specific patches are in pretty rough shape, and there are some outstanding issues that I'll note below. I just wanted to put the general approach out there for consideration. Patch-level comments/review are still much-appreciated though.
>
> However, patches 1-5 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 trivial fix-ups.
>
> ISSUES/TODOS:
>
>   - QMP's async callbacks expect the command results to be de-marshalled beforehand. This is completely infeasible to attempt outside of the code generator, so this is a big area that needs to be addressed. So for now, only the 'guest-ping' command works, since it has no return value. The dummy "guest-view-file" command will cause an error to be reported to the client.

Well, not completely de-marshalled. Basically just need a way to pull 
whatever is stored in the response qdict's "return" field out without 
specifying the QTYPE in advance... which exists in 
qdict.c:qdict_get_obj(), it's just not currently exposed to outside 
callers. Alternatively, the callback function can take in the entire 
response's qdict and pull the value out using knowledge from the schema. 
Will look into this more, but not nearly involved as I first thought.

>   - qemu-ga guest daemon is currently not working over virtio-serial or isa-serial. This is probably an issue with how I'm using glib's io channel interfaces, still investigating. This means the only way to currently test is by invocing qemu-ga with "-c unix-listen -p<sockpath>", then doing something like `socat /dev/ttyS0,raw,echo=0 unix-connect:<sockpath>`.
>   - guest-view-file is a stub, and will be broken out into an open/read/close set of RPCs, possibly with a high-level interface built around those.
>

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

[Anthony Liguori]

On 03/25/2011 03:42 PM, Michael Roth wrote:
> On 03/25/2011 02:47 PM, 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_v1
>>
>> The QGA-specific patches are in pretty rough shape, and there are 
>> some outstanding issues that I'll note below. I just wanted to put 
>> the general approach out there for consideration. Patch-level 
>> comments/review are still much-appreciated though.
>>
>> However, patches 1-5 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 trivial fix-ups.
>>
>> ISSUES/TODOS:
>>
>>   - QMP's async callbacks expect the command results to be 
>> de-marshalled beforehand. This is completely infeasible to attempt 
>> outside of the code generator, so this is a big area that needs to be 
>> addressed. So for now, only the 'guest-ping' command works, since it 
>> has no return value. The dummy "guest-view-file" command will cause 
>> an error to be reported to the client.
>
> Well, not completely de-marshalled. 

I don't follow.  Are you talking about async callbacks within QEMU?  
Because that should be totally transparent to you.  You'll receive a 
qobject and you can do whatever you need with it.

> Basically just need a way to pull whatever is stored in the response 
> qdict's "return" field out without specifying the QTYPE in advance... 
> which exists in qdict.c:qdict_get_obj(), it's just not currently 
> exposed to outside callers.

Just use qdict_get()--but I still don't understand what the problem is.

Regards,

Anthony Liguori


> Alternatively, the callback function can take in the entire response's 
> qdict and pull the value out using knowledge from the schema. Will 
> look into this more, but not nearly involved as I first thought.
>
>>   - qemu-ga guest daemon is currently not working over virtio-serial 
>> or isa-serial. This is probably an issue with how I'm using glib's io 
>> channel interfaces, still investigating. This means the only way to 
>> currently test is by invocing qemu-ga with "-c unix-listen 
>> -p<sockpath>", then doing something like `socat /dev/ttyS0,raw,echo=0 
>> unix-connect:<sockpath>`.
>>   - guest-view-file is a stub, and will be broken out into an 
>> open/read/close set of RPCs, possibly with a high-level interface 
>> built around those.
>>
>

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

[Michael Roth]

On 03/25/2011 05:03 PM, Anthony Liguori wrote:
> On 03/25/2011 03:42 PM, Michael Roth wrote:
>> On 03/25/2011 02:47 PM, 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_v1
>>>
>>> The QGA-specific patches are in pretty rough shape, and there are
>>> some outstanding issues that I'll note below. I just wanted to put
>>> the general approach out there for consideration. Patch-level
>>> comments/review are still much-appreciated though.
>>>
>>> However, patches 1-5 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 trivial fix-ups.
>>>
>>> ISSUES/TODOS:
>>>
>>> - QMP's async callbacks expect the command results to be
>>> de-marshalled beforehand. This is completely infeasible to attempt
>>> outside of the code generator, so this is a big area that needs to be
>>> addressed. So for now, only the 'guest-ping' command works, since it
>>> has no return value. The dummy "guest-view-file" command will cause
>>> an error to be reported to the client.
>>
>> Well, not completely de-marshalled.
>
> I don't follow. Are you talking about async callbacks within QEMU?
> Because that should be totally transparent to you. You'll receive a
> qobject and you can do whatever you need with it.
>

Heh, I was terribly confused when I initially noted that issue...must've 
been looking at a function further up the return chain and convinced 
myself something outside qmp was expected to de-marshal, but that was 
handled elsewhere in qmp-marshal.c

The follow-up was with regard to the much-lesser issue of dealing with 
something like:

static void qmp_guest_view_file_cb(void *qmp__opaque, QObject 
*qmp__retval, Error *qmp__err)

The generated code expects that to be a QObject of a certain type. On 
the QmpProxy side, we have a response from the guest in the form of a 
qdict, with the retval stored with the "return" key:

{ "return": <json/qobject of some type> }

So you need to pull <object of some type> out to pass to the callback. I 
was under the impression that there was no generic qdict_get(), and that 
you were expected to know the type in advance. But...

>> Basically just need a way to pull whatever is stored in the response
>> qdict's "return" field out without specifying the QTYPE in advance...
>> which exists in qdict.c:qdict_get_obj(), it's just not currently
>> exposed to outside callers.
>
> Just use qdict_get()--but I still don't understand what the problem is.

Argh! So that solves the "problem" completely. Thanks :)

>
> Regards,
>
> Anthony Liguori

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

[Anthony Liguori]

On 03/25/2011 05:36 PM, Michael Roth wrote:
>>> Basically just need a way to pull whatever is stored in the response
>>> qdict's "return" field out without specifying the QTYPE in advance...
>>> which exists in qdict.c:qdict_get_obj(), it's just not currently
>>> exposed to outside callers.
>>
>> Just use qdict_get()--but I still don't understand what the problem is.
>
> Argh! So that solves the "problem" completely. Thanks :)

Yeah, the asymmetry is confusing.  qdict_put() takes anything but a 
QObject yet qdict_get() returns a QObject.  qdict_put_obj() takes a 
QObject and qdict_get_obj() is static.

Regards,

Anthony Liguori

>
>>
>> Regards,
>>
>> Anthony Liguori
>