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?
> 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.
Regardless:
Reviewed-by: Markus Armbruster <arm...@redhat.com>
