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

[Marc-André Lureau]

Hi

On Tue, Jul 17, 2018 at 9:06 AM, Markus Armbruster <address@hidden> wrote:
> Marc-André Lureau <address@hidden> writes:
>
>> Let's make json_parser_parse_err() suck less, and simplify caller
>> error handling.
>
> Missing:
>
>    * monitor.c handle_qmp_command(): drop workaround
>
>>  * qga/main.c process_event() doesn't need further changes after
>>    previous cleanup.
>
> "Doesn't need further changes" yes, but I think it could use one.
> Consider:
>
>     obj = json_parser_parse_err(tokens, NULL, &err);
>     if (err) {
>         goto err;
>     }
>     qdict = qobject_to(QDict, obj);
>     if (!qdict) {
>         error_setg(&err, QERR_JSON_PARSING);
>         goto err;
>     }
>
> Before this patch, we get to the error_setg() when
> json_parser_parse_err() fails without setting an error, and when it
> succeeds but returns anything but a dictionary.  QERR_JSON_PARSING is
> appropriate for the first case, but not the second.
>
> This patch removes the first case.  Please improve the error message.
>

ok, replaced with

error_setg(&err, "Input must be a JSON object");

>>  * qobject/json-parser.c json_parser_parse()
>>    Ignores the error.
>
> Stupid wrapper that's used exactly once, in libqtest.c.  Let's use
> json_parser_parse_err() there, and drop the wrapper.  Let's rename
> json_parser_parse_err() to json_parser_parse() then.
>
>>  * qobject/qjson.c qobject_from_jsonv() via parse_json()
>>   - qobject_from_json()
>>     ~ block.c parse_json_filename() - removed workaround
>>     ~ block/rbd.c - abort (generated json - should never fail)
>>     ~ qapi/qobject-input-visitor.c - removed workaround
>>     ~ tests/check-qjson.c - abort, ok
>
> To be precise, we pass &error_abort and either assert or crash when it
> returns null.  Okay.  Same for other instances of "abort, ok" below.
>
> There are a few instances that don't abort.  First one when !utf8_out:
>
>         obj = qobject_from_json(json_in, utf8_out ? &error_abort : NULL);
>         if (utf8_out) {
>             str = qobject_to(QString, obj);
>             g_assert(str);
>             g_assert_cmpstr(qstring_get_str(str), ==, utf8_out);
>         } else {
>             g_assert(!obj);
>         }
>         qobject_unref(obj);
>
> It ignores the error.  Okay.
>
> Next one:
>
>     static void unterminated_string(void)
>     {
>         Error *err = NULL;
>         QObject *obj = qobject_from_json("\"abc", &err);
>         g_assert(!err);             /* BUG */
>         g_assert(obj == NULL);
>     }
>
> This patch should fix the BUG.  We'll see at [*] below why it doens't.
>
>>     ~ tests/test-visitor-serialization.c - abort, ok
>>
>>   - qobject_from_jsonf()
>
> This is called qobject_from_jsonf_nofail() since commit f3e9cc9f7a1, and
> became the obvious wrapper around qobject_from_vjsonf_nofail() in commit
> 74670550ee0.  Please mention both new names.

Hmm this is not upstream yet :)

> I guess the commit message needs more updates for these recent changes
> below, but I'm glossing over that now, along with much of the patch,
> because I think I've found something more serious, explained at the end
> of the patch.
>
>>     Now dies in error_handle_fatal() instead of the assertion.
>
> Which assertion?  Ah, the one in qobject_from_vjsonf_nofail().
>
> The assertion is now dead, isn't it?

not upstream at least

>
>>     Only used in tests, ok.
>>
>>   - tests/test-qobject-input-visitor.c
>>   - tests/libqtest.c qmp_fd_sendv()
>>     Passes &error_abort, does nothing when qobject_from_jsonv() returns
>>     null.  The fix makes this catch invalid JSON instead of silently
>>     ignoring it.
>>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> ---
>>  block.c                      | 5 -----
>>  monitor.c                    | 4 ----
>>  qapi/qobject-input-visitor.c | 5 -----
>>  qobject/json-parser.c        | 8 +++++++-
>>  4 files changed, 7 insertions(+), 15 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index ac8b3a3511..9046d66465 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1478,11 +1478,6 @@ static QDict *parse_json_filename(const char 
>> *filename, Error **errp)
>>
>>      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;
>>      }
>> diff --git a/monitor.c b/monitor.c
>> index ec40a80d81..a3efe96d1d 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -4112,10 +4112,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);
>> -    }
>>
>>      qdict = qobject_to(QDict, req);
>>      if (qdict) {
>> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
>> index da57f4cc24..3e88b27f9e 100644
>> --- a/qapi/qobject-input-visitor.c
>> +++ b/qapi/qobject-input-visitor.c
>> @@ -725,11 +725,6 @@ Visitor *qobject_input_visitor_new_str(const char *str,
>>      if (is_json) {
>>          obj = qobject_from_json(str, errp);
>>          if (!obj) {
>> -            /* Work around qobject_from_json() lossage TODO fix that */
>> -            if (errp && !*errp) {
>> -                error_setg(errp, "JSON parse error");
>> -                return NULL;
>> -            }
>>              return NULL;
>>          }
>>          args = qobject_to(QDict, obj);
>> diff --git a/qobject/json-parser.c b/qobject/json-parser.c
>> index a5aa790d62..82c3167629 100644
>> --- a/qobject/json-parser.c
>> +++ b/qobject/json-parser.c
>> @@ -24,6 +24,7 @@
>>  #include "qapi/qmp/json-parser.h"
>>  #include "qapi/qmp/json-lexer.h"
>>  #include "qapi/qmp/json-streamer.h"
>> +#include "qapi/qmp/qerror.h"
>>
>>  typedef struct JSONParserContext
>>  {
>> @@ -591,7 +592,12 @@ QObject *json_parser_parse_err(GQueue *tokens, va_list 
>> *ap, Error **errp)
>    QObject *json_parser_parse_err(GQueue *tokens, va_list *ap, Error **errp)
>    {
>        JSONParserContext *ctxt = parser_context_new(tokens);
>        QObject *result;
>
>        if (!ctxt) {
>            return NULL;
>
> [*] Still returns null without setting an error.  Two cases lumped into
> one: "no tokens due to empty input (not an error)", and "no tokens due
> to lexical error".  This is not the spot to distinguish the two, it
> needs to be done in its callers.  I figure the sane contract for

I tried to tackle that in the next iteration, but it's harder than it
looks like somehow ;)

> json_parser_parse_err() would be
>
> * If @tokens is null, return null.
> * Else if @tokens parse okay, return the parse tree.
> * Else set an error and return null.
>
> But then "always set an error if return NULL" is not possible.  Ideas?

That's okay, I'll document the function that way


>
>        }
>>
>>      result = parse_value(ctxt, ap);
>>
>> -    error_propagate(errp, ctxt->err);
>> +    if (!result && !ctxt->err) {
>> +        /* TODO: improve error reporting */
>> +        error_setg(errp, QERR_JSON_PARSING);
>> +    } else {
>> +        error_propagate(errp, ctxt->err);
>> +    }
>>
>>      parser_context_free(ctxt);
>



-- 
Marc-André Lureau