diff mbox

[2/7] monitor: Handle new and old style handlers

Message ID 1253136760-3614-3-git-send-email-lcapitulino@redhat.com
State Superseded
Headers show

Commit Message

Luiz Capitulino Sept. 16, 2009, 9:32 p.m. UTC 
This commit changes monitor_handle_command() to support old style
and new style handlers.

New style handlers are protocol independent, they return their
data to the Monitor, which in turn decides how to print them
(ie. user protocol vs. machine protocol).

Converted handlers will use the 'user_print' member of 'mon_cmd_t'
to define its user protocol function, which will be called to print
data in the user protocol format.

Handlers which don't have 'user_print' defined are not converted.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |   32 ++++++++++++++++++++++++++------
 1 files changed, 26 insertions(+), 6 deletions(-)

Comments

Paolo Bonzini Sept. 17, 2009, 6:39 a.m. UTC | #1 
On 09/16/2009 11:32 PM, Luiz Capitulino wrote:
> +    return (cmd->user_print == NULL ? 0 : 1);

cmd->user_print != NULL.

I would actually inline it at the use point.  Also, calling a function 
with an extra argument is fine, so while keeping the void* handler you 
can do:

+        QObject *data = NULL;
+        int (*handler_new)(Monitor *mon,
+                           const QDict *params, QObject **ret_data);

+        handler_new = cmd->handler;
+        handler_new(mon, qdict, &data);

+        if (cmd->user_print)
+                cmd->user_print(mon, data);
+        if (data)
+                qobject_decref(data);

If you do not like calling the function with the "wrong" number of 
arguments, you can mass-convert the functions to the new prototype new 
and leave anyway user_print == NULL.

What are the plans for the return code of handler_new?

Paolo
Luiz Capitulino Sept. 17, 2009, 1:18 p.m. UTC | #2 
On Thu, 17 Sep 2009 08:39:25 +0200
Paolo Bonzini <bonzini@gnu.org> wrote:

> On 09/16/2009 11:32 PM, Luiz Capitulino wrote:
> > +    return (cmd->user_print == NULL ? 0 : 1);
> 
> cmd->user_print != NULL.

 Right.

> I would actually inline it at the use point.  Also, calling a function 
> with an extra argument is fine, so while keeping the void* handler you 
> can do:
> 
> +        QObject *data = NULL;
> +        int (*handler_new)(Monitor *mon,
> +                           const QDict *params, QObject **ret_data);
> 
> +        handler_new = cmd->handler;
> +        handler_new(mon, qdict, &data);
> 
> +        if (cmd->user_print)
> +                cmd->user_print(mon, data);
> +        if (data)
> +                qobject_decref(data);
> 
> If you do not like calling the function with the "wrong" number of 
> arguments, you can mass-convert the functions to the new prototype new 
> and leave anyway user_print == NULL.

 The problem is that the monitor_handler_ported() branch can
have additional code in the future (eg. protocol emission code),
so different branches makes things easier to understand.

> What are the plans for the return code of handler_new?

 The protocol emission code will use it to emit 'error' or
'success' messages.
Paolo Bonzini Sept. 17, 2009, 1:28 p.m. UTC | #3 
>   The problem is that the monitor_handler_ported() branch can
> have additional code in the future (eg. protocol emission code),
> so different branches makes things easier to understand.

Well, you don't know... It may end up in different functions, like

void print_cmd_result_user (Monitor *mon, MonitorCmd *cmd, int result,
			    QObject *data)
{
	if (result)
		...
	else if (cmd->user_print)
		cmd->user_print (mon, data);
}

void print_cmd_result_json (Monitor *mon, MonitorCmd *cmd, int result,
			    QObject *data)
{
	... make qdict with result and data ...
	qdict_print_json (mon->something, output_dict);
	qobject_decref (output_dict);
}

or it may end up not being a branch (but an indirect function call) for 
example:

	result = handler_new (mon, qdict, &data);
	mon->print_cmd_result (mon, cmd, result, data);
	if (data)
		qobject_decref (data);

I'd stay with the simplest thing for now.  I'm not going to argue, 
especially if I'm overridden by other reviewers, of course.

Paolo

Paolo
Luiz Capitulino Sept. 17, 2009, 5:16 p.m. UTC | #4 
On Thu, 17 Sep 2009 15:28:32 +0200
Paolo Bonzini <bonzini@gnu.org> wrote:

> 
> >   The problem is that the monitor_handler_ported() branch can
> > have additional code in the future (eg. protocol emission code),
> > so different branches makes things easier to understand.
> 
> Well, you don't know... It may end up in different functions, like

 Actually, I already have this code and will post shortly (for review,
not for inclusion).
diff mbox

Patch

diff --git a/monitor.c b/monitor.c
index fe604d8..17754fb 100644
--- a/monitor.c
+++ b/monitor.c
@@ -210,6 +210,11 @@  static int monitor_fprintf(FILE *stream, const char *fmt, ...)
     return 0;
 }
 
+static int monitor_handler_ported(const mon_cmd_t *cmd)
+{
+    return (cmd->user_print == NULL ? 0 : 1);
+}
+
 static int compare_cmd(const char *name, const char *list)
 {
     const char *p, *pstart;
@@ -3041,17 +3046,32 @@  static void monitor_handle_command(Monitor *mon, const char *cmdline)
     qdict = qdict_new();
 
     cmd = monitor_parse_command(mon, cmdline, qdict);
-    if (cmd) {
-        void (*handler)(Monitor *mon, const QDict *qdict);
+    if (!cmd)
+        goto out;
+
+    qemu_errors_to_mon(mon);
 
-        qemu_errors_to_mon(mon);
+    if (monitor_handler_ported(cmd)) {
+        QObject *data = NULL;
+        int (*handler_new)(Monitor *mon,
+                           const QDict *params, QObject **ret_data);
 
-        handler = cmd->handler;
-        handler(mon, qdict);
+        handler_new = cmd->handler;
+        handler_new(mon, qdict, &data);
 
-        qemu_errors_to_previous();
+        cmd->user_print(mon, data);
+
+        if (data)
+            qobject_decref(data);
+    } else {
+        void (*handler_old)(Monitor *mon, const QDict *qdict);
+        handler_old = cmd->handler;
+        handler_old(mon, qdict);
     }
 
+   qemu_errors_to_previous();
+
+out:
     QDECREF(qdict);
 }