Re: [PATCH 2/2] qemu: remove redundant needReply argument of qemuAgentCommand

On 3/5/20 3:47 PM, Nikolay Shirokovskiy wrote: > needReply added in [1] looks redundant. Indeed it is set to false only > when mon->await_event is set too (the only exception qemuAgentFSTrim > which is mistaken). > > However it fixes the issue when qemuAgentCommand exits on error path and > mon->await_event is not reset. Let's instead reset mon->await_event properly. > > Also remove "Woken up by event" debug message as it can be misleading. > We can get it also if monitor is closed due to serial changed event > currently. Anyway both qemuAgentClose and qemuAgentNotifyEvent log > itself. > > [1] qemu: make sure agent returns error when required data are missing > > Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; > --- >   src/qemu/qemu_agent.c | 47 ++++++++++++++++++++----------------------- >   1 file changed, 22 insertions(+), 25 deletions(-) > > diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c > index 2de53efb7a..605c9f563e 100644 > --- a/src/qemu/qemu_agent.c > +++ b/src/qemu/qemu_agent.c > @@ -1112,7 +1112,6 @@ static int >   qemuAgentCommand(qemuAgentPtr mon, >                    virJSONValuePtr cmd, >                    virJSONValuePtr *reply, > -                 bool needReply, >                    int seconds) >   { >       int ret = -1; > @@ -1121,17 +1120,16 @@ qemuAgentCommand(qemuAgentPtr mon, >       int await_event = mon->await_event; >         *reply = NULL; > +    memset(&msg, 0, sizeof(msg)); >         if (!mon->running) { >           virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s", >                          _("Guest agent disappeared while executing command")); > -        return -1; > +        goto cleanup; >       } >         if (qemuAgentGuestSync(mon) < 0) > -        return -1; > - > -    memset(&msg, 0, sizeof(msg)); > +        goto cleanup; >         if (!(cmdstr = virJSONValueToString(cmd, false))) >           goto cleanup; > @@ -1149,9 +1147,7 @@ qemuAgentCommand(qemuAgentPtr mon, >           /* If we haven't obtained any reply but we wait for an >            * event, then don't report this as error */ >           if (!msg.rxObject) { > -            if (await_event && !needReply) { > -                VIR_DEBUG("Woken up by event %d", await_event); > -            } else { Please keep this debug printing. It doesn't hurt anything and has potential of helping us to debug.

> +            if (!await_event) { >                   if (mon->running) >                       virReportError(VIR_ERR_INTERNAL_ERROR, "%s", >                                      _("Missing monitor reply object")); > @@ -1169,6 +1165,7 @@ qemuAgentCommand(qemuAgentPtr mon, >    cleanup: >       VIR_FREE(cmdstr); >       VIR_FREE(msg.txBuffer); > +    mon->await_event = QEMU_AGENT_EVENT_NONE; This is the part I don't like about this patch. The rest looks fine. Why is this needed? Either the mon->await_event is not set by the caller, or if set the the event handler will clear it out by calling qemuAgentNotifyEvent(). Or am I missing something?

>         return ret; >   } > @@ -1269,7 +1266,7 @@ int qemuAgentShutdown(qemuAgentPtr mon, >           mon->await_event = QEMU_AGENT_EVENT_RESET; >       else >           mon->await_event = QEMU_AGENT_EVENT_SHUTDOWN; > -    ret = qemuAgentCommand(mon, cmd, &reply, false, > +    ret = qemuAgentCommand(mon, cmd, &reply, >                              VIR_DOMAIN_QEMU_AGENT_COMMAND_SHUTDOWN); >         virJSONValueFree(cmd); > @@ -1312,7 +1309,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints, >       if (!cmd) >           goto cleanup; >   -    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0) > +    if (qemuAgentCommand(mon, cmd, &reply, mon->timeout) < 0) >           goto cleanup; >         if (virJSONValueObjectGetNumberInt(reply, "return", &ret) < 0) { > @@ -1349,7 +1346,7 @@ int qemuAgentFSThaw(qemuAgentPtr mon) >       if (!cmd) >           return -1; >   -    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0) > +    if (qemuAgentCommand(mon, cmd, &reply, mon->timeout) < 0) >           goto cleanup; >         if (virJSONValueObjectGetNumberInt(reply, "return", &ret) < 0) { > @@ -1386,7 +1383,7 @@ qemuAgentSuspend(qemuAgentPtr mon, >           return -1; >         mon->await_event = QEMU_AGENT_EVENT_SUSPEND; > -    ret = qemuAgentCommand(mon, cmd, &reply, false, mon->timeout); > +    ret = qemuAgentCommand(mon, cmd, &reply, mon->timeout); >         virJSONValueFree(cmd); >       virJSONValueFree(reply); > @@ -1415,7 +1412,7 @@ qemuAgentArbitraryCommand(qemuAgentPtr mon, >       if (!(cmd = virJSONValueFromString(cmd_str))) >           goto cleanup; >   -    if ((ret = qemuAgentCommand(mon, cmd, &reply, true, timeout)) < 0) > +    if ((ret = qemuAgentCommand(mon, cmd, &reply, timeout)) < 0) >           goto cleanup; >         if (!(*result = virJSONValueToString(reply, false))) > @@ -1442,7 +1439,7 @@ qemuAgentFSTrim(qemuAgentPtr mon, >       if (!cmd) >           return ret; >   -    ret = qemuAgentCommand(mon, cmd, &reply, false, mon->timeout); > +    ret = qemuAgentCommand(mon, cmd, &reply, mon->timeout); Oh this is fun. This 'false' you are removing should have been true as the command does have a reply that we need to wait for. >         virJSONValueFree(cmd); >       virJSONValueFree(reply); Michal