Hi On Tue, Jul 17, 2018 at 7:53 AM, Markus Armbruster <arm...@redhat.com> wrote: > Marc-André Lureau <marcandre.lur...@redhat.com> writes: > >> json_parser_parse_err() may return something else than a QDict, in >> which case we loose the object. Let's keep track of the original >> object to avoid leaks. > > Should this leak fix go into 3.0?
It has been there for a while, but it could be fixed for 3.0 indeed. > >> When an error occurs, "qdict" contains the response, but we still >> check the "execute" key there. > > Harmless. > >> Untangle a bit this code, by having a >> clear error path. > > Untangling might make sense anyway. Let's see. > >> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> >> --- >> qga/main.c | 50 +++++++++++++++++++++++++------------------------- >> 1 file changed, 25 insertions(+), 25 deletions(-) >> >> diff --git a/qga/main.c b/qga/main.c >> index 537cc0e162..0784761605 100644 >> --- a/qga/main.c >> +++ b/qga/main.c >> @@ -600,6 +600,7 @@ static void process_command(GAState *s, QDict *req) >> static void process_event(JSONMessageParser *parser, GQueue *tokens) >> { >> GAState *s = container_of(parser, GAState, parser); >> + QObject *obj; >> QDict *qdict; >> Error *err = NULL; >> int ret; >> @@ -607,35 +608,34 @@ static void process_event(JSONMessageParser *parser, >> GQueue *tokens) >> g_assert(s && parser); >> >> g_debug("process_event: called"); >> - qdict = qobject_to(QDict, json_parser_parse_err(tokens, NULL, &err)); >> - if (err || !qdict) { >> - qobject_unref(qdict); >> - 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 = qmp_error_response(err); >> + obj = json_parser_parse_err(tokens, NULL, &err); >> + if (err) { >> + goto err; >> } >> - >> - /* handle host->guest commands */ >> - if (qdict_haskey(qdict, "execute")) { >> - process_command(s, qdict); >> - } else { >> - if (!qdict_haskey(qdict, "error")) { >> - qobject_unref(qdict); >> - g_warning("unrecognized payload format"); >> - error_setg(&err, QERR_UNSUPPORTED); >> - qdict = qmp_error_response(err); >> - } >> - ret = send_response(s, qdict); >> - if (ret < 0) { >> - g_warning("error sending error response: %s", strerror(-ret)); >> - } >> + qdict = qobject_to(QDict, obj); >> + if (!qdict) { >> + error_setg(&err, QERR_JSON_PARSING); >> + goto err; >> + } >> + if (!qdict_haskey(qdict, "execute")) { >> + g_warning("unrecognized payload format"); >> + error_setg(&err, QERR_UNSUPPORTED); >> + goto err; >> } >> >> + process_command(s, qdict); >> + qobject_unref(obj); >> + return; >> + >> +err: >> + g_warning("failed to parse event: %s", error_get_pretty(err)); >> + qdict = qmp_error_response(err); >> + ret = send_response(s, qdict); >> + if (ret < 0) { >> + g_warning("error sending error response: %s", strerror(-ret)); >> + } >> qobject_unref(qdict); >> + qobject_unref(obj); >> } >> >> /* false return signals GAChannel to close the current client connection */ > > Control flow is much improved. Took me a minute to convince myself the > reference counting is okay: qdict is a weak reference before qdict = > qmp_error_response(), and becomes strong there. Suggest to use a new > variable @err_rsp for the latter purpose. Yes, the code is further improved in patch 11. > > Regardless: > Reviewed-by: Markus Armbruster <arm...@redhat.com> > thanks -- Marc-André Lureau