[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>
---
 monitor.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

On 05/22/2015 05:36 AM, Markus Armbruster 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>
> ---
>  monitor.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 71ca03f..65ef400 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));

You're getting rid of one of three uses of '_jsonf.*%p' in the tree (two
in monitor.c, one in tests/check-qjson.c) [I didn't check for multi-line
instances where the format string was on a different line than the
function call].  Is it worth completely getting rid of the %p formatter,
and simplifying qobject/json-*.c accordingly, in a later patch?

[oh, and I hate that I could not find documentation on the format
strings accepted by qobject_from_jsonf; it took me forever to track down
that %i correlated to bool, when I was doing my series for "qobject" Use
'bool' for qbool]

> @@ -4984,13 +4984,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;

Is this FIXME still valid, or are we reaching the point where it could
be replaced by an assert(obj) in a later patch?

Reviewed-by: Eric Blake <eblake@redhat.com>

Eric Blake <eblake@redhat.com> writes:

> On 05/22/2015 05:36 AM, Markus Armbruster 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>
>> ---
>>  monitor.c | 26 ++++++++++++++------------
>>  1 file changed, 14 insertions(+), 12 deletions(-)
>> 
>> diff --git a/monitor.c b/monitor.c
>> index 71ca03f..65ef400 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));
>
> You're getting rid of one of three uses of '_jsonf.*%p' in the tree (two
> in monitor.c, one in tests/check-qjson.c) [I didn't check for multi-line
> instances where the format string was on a different line than the
> function call].  Is it worth completely getting rid of the %p formatter,
> and simplifying qobject/json-*.c accordingly, in a later patch?

No idea :)

Here, %p removal simply falls out of the QError -> Error conversion.  I
don't know how it's used elsewhere, and whether the uses are worth the
code backing it.

> [oh, and I hate that I could not find documentation on the format
> strings accepted by qobject_from_jsonf; it took me forever to track down
> that %i correlated to bool, when I was doing my series for "qobject" Use
> 'bool' for qbool]

Valid complaint.  Pretty much everything from that particular hacking
bout is underdocumented, but the undocumented conversion specifiers are
particularly egregious.

>> @@ -4984,13 +4984,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;
>
> Is this FIXME still valid, or are we reaching the point where it could
> be replaced by an assert(obj) in a later patch?

Yes, it's still valid.  Watch this:

    {"QMP": {"version": {"qemu": {"micro": 94, "minor": 2, "major": 2}, "package": ""}, "capabilities": []}}
    { "execute": "\xxxxxxxxx" }
    {"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
    {"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
    { "execute": "qmp_capabilities" }
    {"error": {"class": "GenericError", "desc": "Expected 'object' in QMP input"}}
    {"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
    {"error": {"class": "GenericError", "desc": "Expected 'object' in QMP input"}}
    {"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
    {"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
    {"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}

This does enter json-parser.c's

                        parse_error(ctxt, token,
                                    "invalid hex escape sequence in string");

and parse_error() duly creates an Error object with error_setg(), but
the object then gets lost somewhere on the way.

Also visible: multiple error replies, which is wrong, and an apparent
inability to recover from syntax error.

Not just the documentation sucks.  But then half-assed documentation has
always been a pretty reliable indicator of half-assed code.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!