[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 06/12] qga: process_event() simplification and l
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH 06/12] qga: process_event() simplification and leak fix |
Date: |
Tue, 17 Jul 2018 11:27:45 +0200 |
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
[Qemu-devel] [PATCH 08/12] json-lexer: make it safe to call multiple times, Marc-André Lureau, 2018/07/06
[Qemu-devel] [PATCH 09/12] tests: add a few qemu-qmp tests, Marc-André Lureau, 2018/07/06
[Qemu-devel] [PATCH 10/12] tests: add a qmp success-response test, Marc-André Lureau, 2018/07/06