On 11.03.2020 12:38, Michal Privoznik wrote:
> 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@xxxxxxxxxxxxx>
>> ---
>> 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.
This debug line is not correct. I reflected it in the commit message.
I actually get this line during debug session when agent monitor is closed
during VM shutdown on serial event and not shutdown event. Serial event
just comes first.
>
>> + 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?
The whole point of the patch is in this line)
Yeah, you are talking of success paths. But on error paths mon->await_event is not cleared currently so next command which need reply
can be finish on await event without needReply which lead to SIGSGEV. This is what patch [1] fixes by introducing needReply which don't need to be cleared
as it is on stack.
I guess scenario is next given the bug [2] mentioned in [1]:
- vm shutdown command is sent to GA
- it is finished with timeout (60 s), mon->await_event is not cleared
- guest ping is sent
- guest shutdowned and ping command get awakend by this event as mon->await_event is set (bug)
So needReply argument is helpful but it is cleaner just to reset mon->await_event properly.
[1] qemu: make sure agent returns error when required data are missing
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1058149
Nikolay
>
>> 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
>
