[PATCH 2/2] qemu: remove redundant needReply argument of qemuAgentCommand
Michal Privoznik
mprivozn at redhat.com
Wed Mar 11 09:38:20 UTC 2020
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 at virtuozzo.com>
> ---
> 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
More information about the libvir-list
mailing list