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

[Markus Armbruster]

Mike, I got a bug fix for you to consider for 3.0.

Marc-André, there's one remark for you inline.

Marc-André Lureau <address@hidden> writes:

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

Looking... yes, it looks quite nice after PATCH 11.  Whether to make the
intermediate state after this patch a bit nicer just because we can is
Mike's call to make.

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