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

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

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?