Re: [Qemu-devel] [PATCH v3 07/38] json-parser: always set an error if return NULL

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Let's make json_parser_parse_err() suck less, and simplify caller
> error handling.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  monitor.c             | 4 ----
>  qobject/json-parser.c | 7 ++++++-
>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 5889a32231..e9d0c4d172 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4053,10 +4053,6 @@ static void handle_qmp_command(JSONMessageParser 
> *parser, GQueue *tokens)
>      QMPRequest *req_obj;
>  
>      req = json_parser_parse_err(tokens, NULL, &err);
> -    if (!req && !err) {
> -        /* json_parser_parse_err() sucks: can fail without setting @err */
> -        error_setg(&err, QERR_JSON_PARSING);
> -    }
>      if (err) {
>          goto err;
>      }
> diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> index 769b960c9f..c39cd8e4d7 100644
> --- a/qobject/json-parser.c
> +++ b/qobject/json-parser.c
> @@ -591,7 +591,12 @@ QObject *json_parser_parse_err(GQueue *tokens, va_list 
> *ap, Error **errp)
>  
>      result = parse_value(ctxt, ap);
>  
> -    error_propagate(errp, ctxt->err);
> +    if (!result && !ctxt->err) {
> +        /* TODO: improve error reporting */
> +        error_setg(errp, "Failed to parse JSON");
> +    } else {
> +        error_propagate(errp, ctxt->err);
> +    }
>  
>      parser_context_free(ctxt);

I'd be tempted to put more colorful language in that comment ;)

You update just one caller.  Let's review the other ones:

* qga/main.c process_event()

    qdict = qobject_to(QDict, json_parser_parse_err(tokens, NULL, &err));
    if (err || !qdict) {
        qobject_unref(qdict);
        qdict = qdict_new();
        if (!err) {
            g_warning("failed to parse event: unknown error");
            error_setg(&err, QERR_JSON_PARSING);
        } else {
            g_warning("failed to parse event: %s", error_get_pretty(err));
        }
        qdict_put_obj(qdict, "error", qmp_build_error_object(err));
        error_free(err);
    }

  The if (!err) conditional is now dead.  Please drop it.

* qobject/json-parser.c json_parser_parse()

  Ignores the error.  Okay.

* qobject/qjson.c qobject_from_jsonv() via parse_json()

  - qobject_from_json()

    ~ block.c parse_json_filename()

        options_obj = qobject_from_json(filename, errp);
        if (!options_obj) {
            /* Work around qobject_from_json() lossage TODO fix that */
            if (errp && !*errp) {
                error_setg(errp, "Could not parse the JSON options");
                return NULL;
            }
            error_prepend(errp, "Could not parse the JSON options: ");
            return NULL;
        }

      The work-around is now dead.  Please drop it.

    ~ More callers, please review them.

  - qobject_from_jsonf()

        va_start(ap, string);
        obj = qobject_from_jsonv(string, &ap, &error_abort);
        va_end(ap);

        assert(obj != NULL);

    Now dies in error_handle_fatal() instead of the assertion.  Okay.

  - tests/libqtest.c qmp_fd_sendv()

    Passes &error_abort, does nothing when qobject_from_jsonv() returns
    null.  Your fix makes this catch invalid JSON instead of silently
    ignoring it.  Good, but please mention it in the commit message.

  - tests/test-qobject-input-visitor.c
    visitor_input_test_init_internal()

    Like qobject_from_jsonf().  Okay.