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

[Marc-André Lureau]

Hi

On Tue, Jul 17, 2018 at 7:53 AM, Markus Armbruster <address@hidden> wrote:
> 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?

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 <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.

Yes, the code is further improved in patch 11.

>
> Regardless:
> Reviewed-by: Markus Armbruster <address@hidden>
>

thanks


-- 
Marc-André Lureau