Patchwork [28/50] error: Let converted handlers print in human monitor

login
register
mail settings
Submitter Markus Armbruster
Date March 4, 2010, 3:56 p.m.
Message ID <1267718231-13303-29-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/46958/
State New
Headers show

Comments

Markus Armbruster - March 4, 2010, 3:56 p.m.
While fully converted handlers are not supposed to print anything when
running in a QMP monitor, they are free to print in a human monitor.
For instance, device_add (not yet converted) prints help, and will
continue to do so after conversion.

Moreover, utility functions converted to QError should remain usable
from unconverted handlers.

Two problems:

* handler_audit() complains when a converted handler prints.  Limit
  that to QMP monitors.

* With QMP, handlers need to pass the error object by way of
  monitor_set_error().  However, we do that both for QMP and for the
  human monitor.  The human monitor prints the error object after the
  handler returns.  If the handler prints anything else, that output
  "overtakes" the error message.

  Limit use of monitor_set_error() to QMP monitors.  Update
  handler_audit() accordingly.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c    |   80 ++++++++++++++++++++++++++-------------------------------
 qemu-error.c |    2 +-
 2 files changed, 38 insertions(+), 44 deletions(-)
Luiz Capitulino - March 4, 2010, 8:50 p.m.
On Thu,  4 Mar 2010 16:56:49 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> While fully converted handlers are not supposed to print anything when
> running in a QMP monitor, they are free to print in a human monitor.

 I disagree.

 One of the key decisions behind the new Monitor design is that handlers
are part of common code. User printing is output-specific and should not
be done by handlers.

 One of the problems with this layer violation is that the consumer of this
data might not be what you have assumed. For example, it has been suggested
that we could move the qemu shell out of qemu in the future. In this case,
this kind of information _could_ be useful.

 Another important problem is that free printing like this is error-prone
as it's not at all clear what could be printed and there's no way to catch
mistakes mechanically. Not to mention that this is for sure going to used
by those who want to ignore QMP completely.

 I'm ok with shallow conversion, provided that handler_audit() is there to
warn us about the job to be done.
Markus Armbruster - March 4, 2010, 9:06 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu,  4 Mar 2010 16:56:49 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> While fully converted handlers are not supposed to print anything when
>> running in a QMP monitor, they are free to print in a human monitor.
>
>  I disagree.
>
>  One of the key decisions behind the new Monitor design is that handlers
> are part of common code. User printing is output-specific and should not
> be done by handlers.
>
>  One of the problems with this layer violation is that the consumer of this
> data might not be what you have assumed. For example, it has been suggested
> that we could move the qemu shell out of qemu in the future. In this case,
> this kind of information _could_ be useful.

How do you intend to solve the problem of printing help then?

The perfect is the enemy of the good.

>  Another important problem is that free printing like this is error-prone
> as it's not at all clear what could be printed and there's no way to catch
> mistakes mechanically. Not to mention that this is for sure going to used
> by those who want to ignore QMP completely.

This paragraph I don't get.

>  I'm ok with shallow conversion, provided that handler_audit() is there to
> warn us about the job to be done.
Luiz Capitulino - March 4, 2010, 9:14 p.m.
On Thu, 04 Mar 2010 22:06:42 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Thu,  4 Mar 2010 16:56:49 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> While fully converted handlers are not supposed to print anything when
> >> running in a QMP monitor, they are free to print in a human monitor.
> >
> >  I disagree.
> >
> >  One of the key decisions behind the new Monitor design is that handlers
> > are part of common code. User printing is output-specific and should not
> > be done by handlers.
> >
> >  One of the problems with this layer violation is that the consumer of this
> > data might not be what you have assumed. For example, it has been suggested
> > that we could move the qemu shell out of qemu in the future. In this case,
> > this kind of information _could_ be useful.
> 
> How do you intend to solve the problem of printing help then?

 Doesn't a shallow conversion takes care of it for now?

> The perfect is the enemy of the good.
> 
> >  Another important problem is that free printing like this is error-prone
> > as it's not at all clear what could be printed and there's no way to catch
> > mistakes mechanically. Not to mention that this is for sure going to used
> > by those who want to ignore QMP completely.
> 
> This paragraph I don't get.

 If something that should be available under qmp is printed by using
monitor_printf(), we loose big time.
Luiz Capitulino - March 5, 2010, 3:43 p.m.
On Thu, 4 Mar 2010 17:50:20 -0300
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Thu,  4 Mar 2010 16:56:49 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
> 
> > While fully converted handlers are not supposed to print anything when
> > running in a QMP monitor, they are free to print in a human monitor.
> 
>  I disagree.

 I've talked to Markus by irc about this one and he convinced me that
this is the best solution for the immediate term.

 Actually, I found out that only error is printed in the human monitor
(through qerror_report()) so this is not as serious as I thought at first.

 So, I won't nack it and the bigger mid-term discussion we should have
is whether or not it's ok to mix qerror_report(), error_printf() &
friends in handlers.
Markus Armbruster - March 5, 2010, 4:43 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 4 Mar 2010 17:50:20 -0300
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
>
>> On Thu,  4 Mar 2010 16:56:49 +0100
>> Markus Armbruster <armbru@redhat.com> wrote:
>> 
>> > While fully converted handlers are not supposed to print anything when
>> > running in a QMP monitor, they are free to print in a human monitor.
>> 
>>  I disagree.
>
>  I've talked to Markus by irc about this one and he convinced me that
> this is the best solution for the immediate term.
>
>  Actually, I found out that only error is printed in the human monitor
> (through qerror_report()) so this is not as serious as I thought at first.

In case I confused not just you: the patch does not affect QMP monitors
at all.

I wish I had explained this better.  Let me retry: the patch provides
for partially converted handlers.  Without it, CONFIG_DEBUG_MONITOR
cries bloody murder, and converted errors get delayed until after all
output from the not-yet-converted parts of the handler.

For instance, it takes me four steps to fully convert do_device_add().
I want all the partially converted intermediate versions to work.

Even more compelling is the case of utility functions: I want to be able
to convert a function to QError without having to convert all the
handlers using it to QError / QObject in the same commit, because doing
that would result in an unreviewable hairball of a patch.

>  So, I won't nack it and the bigger mid-term discussion we should have
> is whether or not it's ok to mix qerror_report(), error_printf() &
> friends in handlers.

I think mixing is both unavoidable and harmless in intermediate
conversion steps.

Whether we want to allow stuff like error_printf_unless_qmp() in the
final state is a separate and valid question.  error_printf_unless_qmp()
solved the problem at hand for me, but of course I'm open to other
ideas.  I don't think delaying this series could help with that, though.
Luiz Capitulino - March 8, 2010, 1:41 p.m.
On Fri, 05 Mar 2010 17:43:40 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >
> >  So, I won't nack it and the bigger mid-term discussion we should have
> > is whether or not it's ok to mix qerror_report(), error_printf() &
> > friends in handlers.
> 
> I think mixing is both unavoidable and harmless in intermediate
> conversion steps.

 I agree.

Patch

diff --git a/monitor.c b/monitor.c
index 3580d37..dfeb9db 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3873,13 +3873,6 @@  void monitor_set_error(Monitor *mon, QError *qerror)
     }
 }
 
-static void monitor_print_error(Monitor *mon)
-{
-    qerror_print(mon->error);
-    QDECREF(mon->error);
-    mon->error = NULL;
-}
-
 static int is_async_return(const QObject *data)
 {
     if (data && qobject_type(data) == QTYPE_QDICT) {
@@ -3891,45 +3884,49 @@  static int is_async_return(const QObject *data)
 
 static void handler_audit(Monitor *mon, const mon_cmd_t *cmd, int ret)
 {
-    if (ret && !monitor_has_error(mon)) {
-        /*
-         * If it returns failure, it must have passed on error.
-         *
-         * Action: Report an internal error to the client if in QMP.
-         */
-        if (monitor_ctrl_mode(mon)) {
+    if (monitor_ctrl_mode(mon)) {
+        if (ret && !monitor_has_error(mon)) {
+            /*
+             * If it returns failure, it must have passed on error.
+             *
+             * Action: Report an internal error to the client if in QMP.
+             */
             qerror_report(QERR_UNDEFINED_ERROR);
+            MON_DEBUG("command '%s' returned failure but did not pass an error\n",
+                      cmd->name);
         }
-        MON_DEBUG("command '%s' returned failure but did not pass an error\n",
-                  cmd->name);
-    }
 
 #ifdef CONFIG_DEBUG_MONITOR
-    if (!ret && monitor_has_error(mon)) {
-        /*
-         * If it returns success, it must not have passed an error.
-         *
-         * Action: Report the passed error to the client.
-         */
-        MON_DEBUG("command '%s' returned success but passed an error\n",
-                  cmd->name);
-    }
+        if (!ret && monitor_has_error(mon)) {
+            /*
+             * If it returns success, it must not have passed an error.
+             *
+             * Action: Report the passed error to the client.
+             */
+            MON_DEBUG("command '%s' returned success but passed an error\n",
+                      cmd->name);
+        }
 
-    if (mon_print_count_get(mon) > 0 && strcmp(cmd->name, "info") != 0) {
-        /*
-         * Handlers should not call Monitor print functions.
-         *
-         * Action: Ignore them in QMP.
-         *
-         * (XXX: we don't check any 'info' or 'query' command here
-         * because the user print function _is_ called by do_info(), hence
-         * we will trigger this check. This problem will go away when we
-         * make 'query' commands real and kill do_info())
-         */
-        MON_DEBUG("command '%s' called print functions %d time(s)\n",
-                  cmd->name, mon_print_count_get(mon));
-    }
+        if (mon_print_count_get(mon) > 0 && strcmp(cmd->name, "info") != 0) {
+            /*
+             * Handlers should not call Monitor print functions.
+             *
+             * Action: Ignore them in QMP.
+             *
+             * (XXX: we don't check any 'info' or 'query' command here
+             * because the user print function _is_ called by do_info(), hence
+             * we will trigger this check. This problem will go away when we
+             * make 'query' commands real and kill do_info())
+             */
+            MON_DEBUG("command '%s' called print functions %d time(s)\n",
+                      cmd->name, mon_print_count_get(mon));
+        }
 #endif
+    } else {
+        assert(!monitor_has_error(mon));
+        QDECREF(mon->error);
+        mon->error = NULL;
+    }
 }
 
 static void monitor_call_handler(Monitor *mon, const mon_cmd_t *cmd,
@@ -3982,9 +3979,6 @@  static void handle_user_command(Monitor *mon, const char *cmdline)
         cmd->mhandler.cmd(mon, qdict);
     }
 
-    if (monitor_has_error(mon))
-        monitor_print_error(mon);
-
 out:
     QDECREF(qdict);
 }
diff --git a/qemu-error.c b/qemu-error.c
index 5be6bea..a8c178b 100644
--- a/qemu-error.c
+++ b/qemu-error.c
@@ -207,7 +207,7 @@  void qerror_report_internal(const char *file, int linenr, const char *func,
     qerror = qerror_from_info(file, linenr, func, fmt, &va);
     va_end(va);
 
-    if (cur_mon) {
+    if (monitor_cur_is_qmp()) {
         monitor_set_error(cur_mon, qerror);
     } else {
         qerror_print(qerror);