| Message ID | 20180326150916.9602-8-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | RFC: monitor: add asynchronous command type | expand |
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > Let's make json_parser_parse_err() suck less, and simplify caller > error handling. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > 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.
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);
Let's make json_parser_parse_err() suck less, and simplify caller error handling. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- monitor.c | 4 ---- qobject/json-parser.c | 7 ++++++- 2 files changed, 6 insertions(+), 5 deletions(-)