[v2,13/20] monitor: Limit QError use to command handlers

The previous commits narrowed use of QError to handle_qmp_command()
and its helpers monitor_protocol_emitter(), build_qmp_error_dict().
Narrow it further to just the command handler call: instead of
converting Error to QError throughout handle_qmp_command(), convert
the QError gotten from the command handler to Error, and switch the
helpers from QError to Error.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 monitor.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

On Tue, 26 May 2015 17:20:48 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> The previous commits narrowed use of QError to handle_qmp_command()
> and its helpers monitor_protocol_emitter(), build_qmp_error_dict().
> Narrow it further to just the command handler call: instead of
> converting Error to QError throughout handle_qmp_command(), convert
> the QError gotten from the command handler to Error, and switch the
> helpers from QError to Error.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  monitor.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index f7e8fdf..1ed8462 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -391,19 +391,19 @@ static void monitor_json_emitter(Monitor *mon, const QObject *data)
>      QDECREF(json);
>  }
>  
> -static QDict *build_qmp_error_dict(const QError *err)
> +static QDict *build_qmp_error_dict(Error *err)
>  {
>      QObject *obj;
>  
> -    obj = qobject_from_jsonf("{ 'error': { 'class': %s, 'desc': %p } }",
> -                             ErrorClass_lookup[err->err_class],
> -                             qerror_human(err));
> +    obj = qobject_from_jsonf("{ 'error': { 'class': %s, 'desc': %s } }",
> +                             ErrorClass_lookup[error_get_class(err)],
> +                             error_get_pretty(err));
>  
>      return qobject_to_qdict(obj);
>  }
>  
>  static void monitor_protocol_emitter(Monitor *mon, QObject *data,
> -                                     QError *err)
> +                                     Error *err)
>  {
>      QDict *qmp;
>  
> @@ -4982,13 +4982,12 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>      obj = json_parser_parse(tokens, NULL);
>      if (!obj) {
>          // FIXME: should be triggered in json_parser_parse()
> -        qerror_report(QERR_JSON_PARSING);
> +        error_set(&local_err, QERR_JSON_PARSING);
>          goto err_out;
>      }
>  
>      input = qmp_check_input_obj(obj, &local_err);
>      if (!input) {
> -        qerror_report_err(local_err);
>          qobject_decref(obj);
>          goto err_out;
>      }
> @@ -5000,8 +4999,8 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>      trace_handle_qmp_command(mon, cmd_name);
>      cmd = qmp_find_cmd(cmd_name);
>      if (!cmd) {
> -        qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND,
> -                      "The command %s has not been found", cmd_name);
> +        error_set(&local_err, ERROR_CLASS_COMMAND_NOT_FOUND,
> +                  "The command %s has not been found", cmd_name);
>          goto err_out;
>      }
>      if (invalid_qmp_mode(mon, cmd)) {
> @@ -5018,7 +5017,6 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>  
>      qmp_check_client_args(cmd, args, &local_err);
>      if (local_err) {
> -        qerror_report_err(local_err);
>          goto err_out;
>      }
>  
> @@ -5026,12 +5024,16 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>          /* Command failed... */
>          if (!mon->error) {
>              /* ... without setting an error, so make one up */
> -            qerror_report(QERR_UNDEFINED_ERROR);
> +            error_set(&local_err, QERR_UNDEFINED_ERROR);
>          }
>      }
> +    if (mon->error) {
> +        error_set(&local_err, mon->error->err_class, "%s",
> +                  mon->error->err_msg);
> +    }
>  
>  err_out:
> -    monitor_protocol_emitter(mon, data, mon->error);
> +    monitor_protocol_emitter(mon, data, local_err);

This breaks error reporting from invalid_qmp_mode(). The end result
is that every command succeeds in capability negotiation mode and
qmp_capabilities never fails (even in command mode).

There are two simple ways to fix it: just propagate mon->error to
local_err when invalid_qmp_mode() fails, or change invalid_qmp_mode()
to take an Error object (preferable).

>      qobject_decref(data);
>      QDECREF(mon->error);
>      mon->error = NULL;

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue, 26 May 2015 17:20:48 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> The previous commits narrowed use of QError to handle_qmp_command()
>> and its helpers monitor_protocol_emitter(), build_qmp_error_dict().
>> Narrow it further to just the command handler call: instead of
>> converting Error to QError throughout handle_qmp_command(), convert
>> the QError gotten from the command handler to Error, and switch the
>> helpers from QError to Error.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>  monitor.c | 26 ++++++++++++++------------
>>  1 file changed, 14 insertions(+), 12 deletions(-)
>> 
>> diff --git a/monitor.c b/monitor.c
>> index f7e8fdf..1ed8462 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -391,19 +391,19 @@ static void monitor_json_emitter(Monitor *mon, const QObject *data)
>>      QDECREF(json);
>>  }
>>  
>> -static QDict *build_qmp_error_dict(const QError *err)
>> +static QDict *build_qmp_error_dict(Error *err)
>>  {
>>      QObject *obj;
>>  
>> -    obj = qobject_from_jsonf("{ 'error': { 'class': %s, 'desc': %p } }",
>> -                             ErrorClass_lookup[err->err_class],
>> -                             qerror_human(err));
>> +    obj = qobject_from_jsonf("{ 'error': { 'class': %s, 'desc': %s } }",
>> +                             ErrorClass_lookup[error_get_class(err)],
>> +                             error_get_pretty(err));
>>  
>>      return qobject_to_qdict(obj);
>>  }
>>  
>>  static void monitor_protocol_emitter(Monitor *mon, QObject *data,
>> -                                     QError *err)
>> +                                     Error *err)
>>  {
>>      QDict *qmp;
>>  
>> @@ -4982,13 +4982,12 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>>      obj = json_parser_parse(tokens, NULL);
>>      if (!obj) {
>>          // FIXME: should be triggered in json_parser_parse()
>> -        qerror_report(QERR_JSON_PARSING);
>> +        error_set(&local_err, QERR_JSON_PARSING);
>>          goto err_out;
>>      }
>>  
>>      input = qmp_check_input_obj(obj, &local_err);
>>      if (!input) {
>> -        qerror_report_err(local_err);
>>          qobject_decref(obj);
>>          goto err_out;
>>      }
>> @@ -5000,8 +4999,8 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>>      trace_handle_qmp_command(mon, cmd_name);
>>      cmd = qmp_find_cmd(cmd_name);
>>      if (!cmd) {
>> -        qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND,
>> -                      "The command %s has not been found", cmd_name);
>> +        error_set(&local_err, ERROR_CLASS_COMMAND_NOT_FOUND,
>> +                  "The command %s has not been found", cmd_name);
>>          goto err_out;
>>      }
>>      if (invalid_qmp_mode(mon, cmd)) {
>> @@ -5018,7 +5017,6 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>>  
>>      qmp_check_client_args(cmd, args, &local_err);
>>      if (local_err) {
>> -        qerror_report_err(local_err);
>>          goto err_out;
>>      }
>>  
>> @@ -5026,12 +5024,16 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>>          /* Command failed... */
>>          if (!mon->error) {
>>              /* ... without setting an error, so make one up */
>> -            qerror_report(QERR_UNDEFINED_ERROR);
>> +            error_set(&local_err, QERR_UNDEFINED_ERROR);
>>          }
>>      }
>> +    if (mon->error) {
>> +        error_set(&local_err, mon->error->err_class, "%s",
>> +                  mon->error->err_msg);
>> +    }
>>  
>>  err_out:
>> -    monitor_protocol_emitter(mon, data, mon->error);
>> +    monitor_protocol_emitter(mon, data, local_err);
>
> This breaks error reporting from invalid_qmp_mode(). The end result
> is that every command succeeds in capability negotiation mode and
> qmp_capabilities never fails (even in command mode).

Oops.

> There are two simple ways to fix it: just propagate mon->error to
> local_err when invalid_qmp_mode() fails, or change invalid_qmp_mode()
> to take an Error object (preferable).

I'll do the latter.  Thanks!

>>      qobject_decref(data);
>>      QDECREF(mon->error);
>>      mon->error = NULL;