Hi
On Tue, Jul 17, 2018 at 9:06 AM, Markus Armbruster <arm...@redhat.com> wrote:
> Marc-André Lureau <marcandre.lur...@redhat.com> 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 <marcandre.lur...@redhat.com>
>> ---
>> 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
