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 <marcandre.lur...@gmail.com> writes:
> Hi
>
> On Tue, Jul 17, 2018 at 7:53 AM, Markus Armbruster <arm...@redhat.com> wrote:
>> 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?
>
> 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 <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.
>
> 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 <arm...@redhat.com>
>>
>
> thanks
