Re: [Qemu-devel] [PATCH 06/12] qga: process_event() simplification and leak fix

[Markus Armbruster]

Marc-André Lureau <address@hidden> 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 <address@hidden>
> ---
>  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 <address@hidden>