Patchwork [05/18] Monitor: Drop is_async_return()

login
register
mail settings
Submitter Luiz Capitulino
Date Sept. 16, 2010, 8:20 p.m.
Message ID <1284668464-15981-6-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/64998/
State New
Headers show

Comments

Luiz Capitulino - Sept. 16, 2010, 8:20 p.m.
If I understood it correcty, the is_async_return() logic was only
used to prevent QMP from issuing duplicated success responses
for asynchronous handlers.

However, QMP doesn't use do_info() anymore so this is dead logic
and (hopefully) can be safely dropped.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |   25 +------------------------
 1 files changed, 1 insertions(+), 24 deletions(-)
Markus Armbruster - Sept. 29, 2010, 11:21 a.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> If I understood it correcty, the is_async_return() logic was only
> used to prevent QMP from issuing duplicated success responses
> for asynchronous handlers.
>
> However, QMP doesn't use do_info() anymore so this is dead logic
> and (hopefully) can be safely dropped.

Looks like it.

Does anybody understand this async monitor stuff?  It's used only for
"info balloon" and "balloon".  It keeps getting in the way.  I'm sorely
tempted to rip it out and reimplement it after we're done refactoring
the monitor.
Luiz Capitulino - Sept. 29, 2010, 1:28 p.m.
On Wed, 29 Sep 2010 13:21:08 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > If I understood it correcty, the is_async_return() logic was only
> > used to prevent QMP from issuing duplicated success responses
> > for asynchronous handlers.
> >
> > However, QMP doesn't use do_info() anymore so this is dead logic
> > and (hopefully) can be safely dropped.
> 
> Looks like it.
> 
> Does anybody understand this async monitor stuff?  

I guess I do, but every now and then it surprises me someway.

> It's used only for
> "info balloon" and "balloon".  It keeps getting in the way.  I'm sorely
> tempted to rip it out and reimplement it after we're done refactoring
> the monitor.

I've considered doing that, but then I realized that it's actually a good
interface with two serious problems:

  1. It's half done. A true async interface would provide a way to list and
     cancel requests (at the API level and in QMP/HMP)

  2. It got too complex, and that's so because of the current state of the
     Monitor code

My optimistic side is telling me that both problems can be solved during
the refactoring process: Monitor code will be simplified and the current
async interface could be used as the basis for the internal QMP API (which
should be async-only, but that's another discussion).
Anthony Liguori - Sept. 29, 2010, 3:11 p.m.
On 09/29/2010 08:28 AM, Luiz Capitulino wrote:
> On Wed, 29 Sep 2010 13:21:08 +0200
> Markus Armbruster<armbru@redhat.com>  wrote:
>
>    
>> Luiz Capitulino<lcapitulino@redhat.com>  writes:
>>
>>      
>>> If I understood it correcty, the is_async_return() logic was only
>>> used to prevent QMP from issuing duplicated success responses
>>> for asynchronous handlers.
>>>
>>> However, QMP doesn't use do_info() anymore so this is dead logic
>>> and (hopefully) can be safely dropped.
>>>        
>> Looks like it.
>>
>> Does anybody understand this async monitor stuff?
>>      
> I guess I do, but every now and then it surprises me someway.
>
>    
>> It's used only for
>> "info balloon" and "balloon".  It keeps getting in the way.  I'm sorely
>> tempted to rip it out and reimplement it after we're done refactoring
>> the monitor.
>>      
> I've considered doing that, but then I realized that it's actually a good
> interface with two serious problems:
>
>    1. It's half done. A true async interface would provide a way to list and
>       cancel requests (at the API level and in QMP/HMP)
>
>    2. It got too complex, and that's so because of the current state of the
>       Monitor code
>
> My optimistic side is telling me that both problems can be solved during
> the refactoring process: Monitor code will be simplified and the current
> async interface could be used as the basis for the internal QMP API (which
> should be async-only, but that's another discussion).
>    

I think one of the ugly bits of QMP as a protocol is capabilities 
negotiation because it implies that QMP has stateful sessions.

Capabilities doesn't have to imply a stateful session but some of the 
things we've discussed in the past would (like event notification masking).

When you look at other RPCs like XML-RPC, REST, or even JSON-RPC over 
HTTP, in order to be used over HTTP it's important that the actual RPC 
session has no state because HTTP is stateless.

The reason this discussion matters is because today we have a Monitor 
object that all commands operate on and could potentially use to store 
state.

An alternative model would be to do away with the Monitor object used by 
the commands which means the commands have no way to store session 
state.  The human monitor needs state because it's stateful but it's not 
clear that we need to do this with QMP.

I think the vast majority of the complexity of async is based on the 
fact that we have a session object and we make the session object 
globally available.  I don't think this is really something we've ever 
considered with QMP though.

Regards,

Anthony Liguori

Regards,

Anthony Liguori
Anthony Liguori - Sept. 29, 2010, 3:11 p.m.
On 09/29/2010 08:28 AM, Luiz Capitulino wrote:
> On Wed, 29 Sep 2010 13:21:08 +0200
> Markus Armbruster<armbru@redhat.com>  wrote:
>
>    
>> Luiz Capitulino<lcapitulino@redhat.com>  writes:
>>
>>      
>>> If I understood it correcty, the is_async_return() logic was only
>>> used to prevent QMP from issuing duplicated success responses
>>> for asynchronous handlers.
>>>
>>> However, QMP doesn't use do_info() anymore so this is dead logic
>>> and (hopefully) can be safely dropped.
>>>        
>> Looks like it.
>>
>> Does anybody understand this async monitor stuff?
>>      
> I guess I do, but every now and then it surprises me someway.
>
>    
>> It's used only for
>> "info balloon" and "balloon".  It keeps getting in the way.  I'm sorely
>> tempted to rip it out and reimplement it after we're done refactoring
>> the monitor.
>>      
> I've considered doing that, but then I realized that it's actually a good
> interface with two serious problems:
>
>    1. It's half done. A true async interface would provide a way to list and
>       cancel requests (at the API level and in QMP/HMP)
>
>    2. It got too complex, and that's so because of the current state of the
>       Monitor code
>
> My optimistic side is telling me that both problems can be solved during
> the refactoring process: Monitor code will be simplified and the current
> async interface could be used as the basis for the internal QMP API (which
> should be async-only, but that's another discussion).
>    

I think one of the ugly bits of QMP as a protocol is capabilities 
negotiation because it implies that QMP has stateful sessions.

Capabilities doesn't have to imply a stateful session but some of the 
things we've discussed in the past would (like event notification masking).

When you look at other RPCs like XML-RPC, REST, or even JSON-RPC over 
HTTP, in order to be used over HTTP it's important that the actual RPC 
session has no state because HTTP is stateless.

The reason this discussion matters is because today we have a Monitor 
object that all commands operate on and could potentially use to store 
state.

An alternative model would be to do away with the Monitor object used by 
the commands which means the commands have no way to store session 
state.  The human monitor needs state because it's stateful but it's not 
clear that we need to do this with QMP.

I think the vast majority of the complexity of async is based on the 
fact that we have a session object and we make the session object 
globally available.  I don't think this is really something we've ever 
considered with QMP though.

Regards,

Anthony Liguori

Regards,

Anthony Liguori

Patch

diff --git a/monitor.c b/monitor.c
index b76e227..1d52362 100644
--- a/monitor.c
+++ b/monitor.c
@@ -653,12 +653,6 @@  static int do_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
 
     if (monitor_handler_is_async(cmd)) {
         user_async_info_handler(mon, cmd);
-        /*
-         * Indicate that this command is asynchronous and will not return any
-         * data (not even empty).  Instead, the data will be returned via a
-         * completion callback.
-         */
-        *ret_data = qobject_from_jsonf("{ '__mon_async': 'return' }");
     } else if (monitor_handler_ported(cmd)) {
         QObject *info_data = NULL;
 
@@ -3717,15 +3711,6 @@  void monitor_set_error(Monitor *mon, QError *qerror)
     }
 }
 
-static int is_async_return(const QObject *data)
-{
-    if (data && qobject_type(data) == QTYPE_QDICT) {
-        return qdict_haskey(qobject_to_qdict(data), "__mon_async");
-    }
-
-    return 0;
-}
-
 static void handler_audit(Monitor *mon, const mon_cmd_t *cmd, int ret)
 {
     if (monitor_ctrl_mode(mon)) {
@@ -3784,15 +3769,7 @@  static void monitor_call_handler(Monitor *mon, const mon_cmd_t *cmd,
     ret = cmd->mhandler.cmd_new(mon, params, &data);
     handler_audit(mon, cmd, ret);
 
-    if (is_async_return(data)) {
-        /*
-         * Asynchronous commands have no initial return data but they can
-         * generate errors.  Data is returned via the async completion handler.
-         */
-        if (monitor_ctrl_mode(mon) && monitor_has_error(mon)) {
-            monitor_protocol_emitter(mon, NULL);
-        }
-    } else if (monitor_ctrl_mode(mon)) {
+    if (monitor_ctrl_mode(mon)) {
         /* Monitor Protocol */
         monitor_protocol_emitter(mon, data);
     } else {