10:47 p.m.
The second patch is unrelated through.
Nikolay Shirokovskiy (2):
qemu: agent: sync once if qemu has serial port event
qemu: remove redundant needReply argument of qemuAgentCommand
src/qemu/qemu_agent.c | 60 ++++++++++++++++++++----------------
src/qemu/qemu_agent.h | 3 +-
src/qemu/qemu_process.c | 3 +-
tests/qemumonitortestutils.c | 3 +-
4 files changed, 40 insertions(+), 29 deletions(-)
--
2.23.0
10:47 p.m.
New subject: [PATCH 1/2] qemu: agent: sync once if qemu has serial port event
Sync was introduced in [1] to check for ga presence. This
check is racy but in the era before serial events are available
there was not better solution I guess.
In case we have the events the sync function is different. It allows us
to flush stateless ga channel from remnants of previous communications.
But we need to do it only once. Until we get timeout on issued command
channel state is ok.
[1] qemu_agent: Issue guest-sync prior to every command
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/qemu/qemu_agent.c | 13 ++++++++++++-
src/qemu/qemu_agent.h | 3 ++-
src/qemu/qemu_process.c | 3 ++-
tests/qemumonitortestutils.c | 3 ++-
4 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index cd25ef6cd3..2de53efb7a 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -104,6 +104,8 @@ struct _qemuAgent {
int watch;
bool running;
+ bool singleSync;
+ bool inSync;
virDomainObjPtr vm;
@@ -679,7 +681,8 @@ qemuAgentIO(int watch, int fd, int events, void *opaque)
qemuAgentPtr
qemuAgentOpen(virDomainObjPtr vm,
const virDomainChrSourceDef *config,
- qemuAgentCallbacksPtr cb)
+ qemuAgentCallbacksPtr cb,
+ bool singleSync)
{
qemuAgentPtr mon;
@@ -705,6 +708,7 @@ qemuAgentOpen(virDomainObjPtr vm,
}
mon->vm = vm;
mon->cb = cb;
+ mon->singleSync = singleSync;
switch (config->type) {
case VIR_DOMAIN_CHR_TYPE_UNIX:
@@ -869,6 +873,7 @@ static int qemuAgentSend(qemuAgentPtr mon,
_("Unable to wait on agent monitor "
"condition"));
}
+ mon->inSync = false;
goto cleanup;
}
}
@@ -910,6 +915,9 @@ qemuAgentGuestSync(qemuAgentPtr mon)
qemuAgentMessage sync_msg;
int timeout = VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT;
+ if (mon->singleSync && mon->inSync)
+ return 0;
+
/* if user specified a custom agent timeout that is lower than the
* default timeout, use the shorter timeout instead */
if ((mon->timeout >= 0) && (mon->timeout < timeout))
@@ -955,6 +963,9 @@ qemuAgentGuestSync(qemuAgentPtr mon)
}
}
+ if (mon->singleSync)
+ mon->inSync = true;
+
ret = 0;
cleanup:
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index 5656fe60ff..f9f75cee25 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -41,7 +41,8 @@ struct _qemuAgentCallbacks {
qemuAgentPtr qemuAgentOpen(virDomainObjPtr vm,
const virDomainChrSourceDef *config,
- qemuAgentCallbacksPtr cb);
+ qemuAgentCallbacksPtr cb,
+ bool singleSync);
void qemuAgentClose(qemuAgentPtr mon);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 36fbffd7b8..a910afc307 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -237,7 +237,8 @@ qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm)
agent = qemuAgentOpen(vm,
config->source,
- &agentCallbacks);
+ &agentCallbacks,
+ virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VSERPORT_CHANGE));
virObjectLock(vm);
diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c
index 00f5b49439..c3e4d86dd6 100644
--- a/tests/qemumonitortestutils.c
+++ b/tests/qemumonitortestutils.c
@@ -1393,7 +1393,8 @@ qemuMonitorTestNewAgent(virDomainXMLOptionPtr xmlopt)
if (!(test->agent = qemuAgentOpen(test->vm,
&src,
- &qemuMonitorTestAgentCallbacks)))
+ &qemuMonitorTestAgentCallbacks,
+ false)))
goto error;
virObjectLock(test->agent);
--
2.23.0
5:38 p.m.
New subject: [PATCH 1/2] qemu: agent: sync once if qemu has serial port event
On 3/5/20 3:47 PM, Nikolay Shirokovskiy wrote:
Sync was introduced in [1] to check for ga presence. This
check is racy but in the era before serial events are available
there was not better solution I guess.
Can you elaborate more on the raciness? There should never be more than
one thread talking on the agent monitor.
In case we have the events the sync function is different. It allows us
to flush stateless ga channel from remnants of previous communications.
But we need to do it only once. Until we get timeout on issued command
channel state is ok.
[1] qemu_agent: Issue guest-sync prior to every command
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/qemu/qemu_agent.c | 13 ++++++++++++-
src/qemu/qemu_agent.h | 3 ++-
src/qemu/qemu_process.c | 3 ++-
tests/qemumonitortestutils.c | 3 ++-
4 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index cd25ef6cd3..2de53efb7a 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -104,6 +104,8 @@ struct _qemuAgent {
int watch;
bool running;
+ bool singleSync;
+ bool inSync;
I wonder if we can do this with just inSync and not have singleSync. I
mean, it looks like at all places where singleSync is used we have
access to qemuCaps so it should be as easy as:
qemuAgentOpen();
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VSERPORT_CHANGE)) {
if *qemuAgentGuestSync(mon) < 0)
goto error;
mon->inSync = true;
and then qemuagentGuestSync() would be a NOP if inSync is set. But it
would also not set it. But maybe I'm pushing it too far, it's just that
I'm confused by the name 'singleSync'. Probably 'hasSingleSync' would
be
better? The boolean reflect whether qemu has the VSERPORT_CHANGE
capability and thus libvirt knows when GA connects/disconnects.
'singleSync' just doesn't ring that bell at first.
Michal
7:12 p.m.
New subject: [PATCH 1/2] qemu: agent: sync once if qemu has serial port event
On 11.03.2020 12:38, Michal Privoznik wrote:
On 3/5/20 3:47 PM, Nikolay Shirokovskiy wrote:
> Sync was introduced in [1] to check for ga presence. This
> check is racy but in the era before serial events are available
> there was not better solution I guess.
Can you elaborate more on the raciness? There should never be more than one thread
talking on the agent monitor.
The race is in another dimension) So sync checks for GA presence, it only waits for 5s so
if
there is no GA the actual command with no timeout won't block. But GA can go down
right
after successful sync so command can block. This is true if there are no serial events
in the latter case agent monitor will be closed when GA go down and command will unblock.
>
> In case we have the events the sync function is different. It allows us
> to flush stateless ga channel from remnants of previous communications.
> But we need to do it only once. Until we get timeout on issued command
> channel state is ok.
>
> [1] qemu_agent: Issue guest-sync prior to every command
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
> ---
> src/qemu/qemu_agent.c | 13 ++++++++++++-
> src/qemu/qemu_agent.h | 3 ++-
> src/qemu/qemu_process.c | 3 ++-
> tests/qemumonitortestutils.c | 3 ++-
> 4 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index cd25ef6cd3..2de53efb7a 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -104,6 +104,8 @@ struct _qemuAgent {
> int watch;
> bool running;
> + bool singleSync;
> + bool inSync;
>
I wonder if we can do this with just inSync and not have singleSync. I mean, it looks
like at all places where singleSync is used we have access to qemuCaps so it should be as
easy as:
qemuAgentOpen();
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VSERPORT_CHANGE)) {
if *qemuAgentGuestSync(mon) < 0)
goto error;
mon->inSync = true;
and then qemuagentGuestSync() would be a NOP if inSync is set. But it would also not set
it. But maybe I'm pushing it too far, it's just that I'm confused by the name
'singleSync'. Probably 'hasSingleSync' would be better? The boolean
reflect whether qemu has the VSERPORT_CHANGE capability and thus libvirt knows when GA
connects/disconnects. 'singleSync' just doesn't ring that bell at first.
If we try to sync only on monitor opening then we can't sync if for example command
timeouts and
GA stays connect (thus no serial event and monitor won't be closed). With current
patch we just
try to sync when we need to send next command to GA.
I guess singleSync is better. It just reflects that we don't need to sync before
every
command (in order to avoid hangs) in case there is serial events. Strictly speaking
it is not correct to use sync to check for GA presence because of the race and as to
using it as a means to flush channel it works well without sereal events as well.
Nikolay
1:45 a.m.
New subject: [PATCH 1/2] qemu: agent: sync once if qemu has serial port event
On 3/11/20 12:12 PM, Nikolay Shirokovskiy wrote:
On 11.03.2020 12:38, Michal Privoznik wrote:
> On 3/5/20 3:47 PM, Nikolay Shirokovskiy wrote:
>> Sync was introduced in [1] to check for ga presence. This
>> check is racy but in the era before serial events are available
>> there was not better solution I guess.
>
> Can you elaborate more on the raciness? There should never be more than one thread
talking on the agent monitor.
The race is in another dimension) So sync checks for GA presence, it only waits for 5s so
if
there is no GA the actual command with no timeout won't block. But GA can go down
right
after successful sync so command can block. This is true if there are no serial events
in the latter case agent monitor will be closed when GA go down and command will
unblock.
>
>>
>> In case we have the events the sync function is different. It allows us
>> to flush stateless ga channel from remnants of previous communications.
>> But we need to do it only once. Until we get timeout on issued command
>> channel state is ok.
>>
>> [1] qemu_agent: Issue guest-sync prior to every command
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
>> ---
>> src/qemu/qemu_agent.c | 13 ++++++++++++-
>> src/qemu/qemu_agent.h | 3 ++-
>> src/qemu/qemu_process.c | 3 ++-
>> tests/qemumonitortestutils.c | 3 ++-
>> 4 files changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
>> index cd25ef6cd3..2de53efb7a 100644
>> --- a/src/qemu/qemu_agent.c
>> +++ b/src/qemu/qemu_agent.c
>> @@ -104,6 +104,8 @@ struct _qemuAgent {
>> int watch;
>> bool running;
>> + bool singleSync;
>> + bool inSync;
>>
>
> I wonder if we can do this with just inSync and not have singleSync. I mean, it looks
like at all places where singleSync is used we have access to qemuCaps so it should be as
easy as:
>
> qemuAgentOpen();
> if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VSERPORT_CHANGE)) {
> if *qemuAgentGuestSync(mon) < 0)
> goto error;
> mon->inSync = true;
>
>
> and then qemuagentGuestSync() would be a NOP if inSync is set. But it would also not
set it. But maybe I'm pushing it too far, it's just that I'm confused by the
name 'singleSync'. Probably 'hasSingleSync' would be better? The boolean
reflect whether qemu has the VSERPORT_CHANGE capability and thus libvirt knows when GA
connects/disconnects. 'singleSync' just doesn't ring that bell at first.
>
If we try to sync only on monitor opening then we can't sync if for example command
timeouts and
GA stays connect (thus no serial event and monitor won't be closed). With current
patch we just
try to sync when we need to send next command to GA.
I guess singleSync is better. It just reflects that we don't need to sync before
every
command (in order to avoid hangs) in case there is serial events. Strictly speaking
it is not correct to use sync to check for GA presence because of the race and as to
using it as a means to flush channel it works well without sereal events as well.
Nikolay
Alright, you've persuaded me on both.
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
and resolved the merge commit on both patches that appeared meanwhile
and pushed.
Michal
10:47 p.m.
New subject: [PATCH 2/2] qemu: remove redundant needReply argument of qemuAgentCommand
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>
---
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 {
+ 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;
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);
virJSONValueFree(cmd);
virJSONValueFree(reply);
@@ -1463,7 +1460,7 @@ qemuAgentGetVCPUs(qemuAgentPtr mon,
if (!(cmd = qemuAgentMakeCommand("guest-get-vcpus", NULL)))
return -1;
- if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
+ if (qemuAgentCommand(mon, cmd, &reply, mon->timeout) < 0)
goto cleanup;
if (!(data = virJSONValueObjectGetArray(reply, "return"))) {
@@ -1576,7 +1573,7 @@ qemuAgentSetVCPUsCommand(qemuAgentPtr mon,
NULL)))
goto cleanup;
- if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
+ if (qemuAgentCommand(mon, cmd, &reply, mon->timeout) < 0)
goto cleanup;
/* All negative values are invalid. Return of 0 is bogus since we wouldn't
@@ -1731,7 +1728,7 @@ qemuAgentGetHostname(qemuAgentPtr mon,
if (!cmd)
return ret;
- if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0) {
+ if (qemuAgentCommand(mon, cmd, &reply, mon->timeout) < 0) {
if (qemuAgentErrorCommandUnsupported(reply))
ret = -2;
goto cleanup;
@@ -1775,7 +1772,7 @@ qemuAgentGetTime(qemuAgentPtr mon,
if (!cmd)
return ret;
- if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
+ if (qemuAgentCommand(mon, cmd, &reply, mon->timeout) < 0)
goto cleanup;
if (virJSONValueObjectGetNumberUlong(reply, "return", &json_time) <
0) {
@@ -1840,7 +1837,7 @@ qemuAgentSetTime(qemuAgentPtr mon,
if (!cmd)
return ret;
- if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
+ if (qemuAgentCommand(mon, cmd, &reply, mon->timeout) < 0)
goto cleanup;
ret = 0;
@@ -1977,7 +1974,7 @@ qemuAgentGetFSInfo(qemuAgentPtr mon,
if (!cmd)
return ret;
- if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0) {
+ if (qemuAgentCommand(mon, cmd, &reply, mon->timeout) < 0) {
if (qemuAgentErrorCommandUnsupported(reply))
ret = -2;
goto cleanup;
@@ -2128,7 +2125,7 @@ qemuAgentGetInterfaces(qemuAgentPtr mon,
if (!(cmd = qemuAgentMakeCommand("guest-network-get-interfaces", NULL)))
goto cleanup;
- if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
+ if (qemuAgentCommand(mon, cmd, &reply, mon->timeout) < 0)
goto cleanup;
if (!(ret_array = virJSONValueObjectGet(reply, "return"))) {
@@ -2305,7 +2302,7 @@ qemuAgentSetUserPassword(qemuAgentPtr mon,
NULL)))
goto cleanup;
- if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
+ if (qemuAgentCommand(mon, cmd, &reply, mon->timeout) < 0)
goto cleanup;
ret = 0;
@@ -2336,7 +2333,7 @@ qemuAgentGetUsers(qemuAgentPtr mon,
if (!(cmd = qemuAgentMakeCommand("guest-get-users", NULL)))
return -1;
- if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0) {
+ if (qemuAgentCommand(mon, cmd, &reply, mon->timeout) < 0) {
if (qemuAgentErrorCommandUnsupported(reply))
return -2;
return -1;
@@ -2425,7 +2422,7 @@ qemuAgentGetOSInfo(qemuAgentPtr mon,
if (!(cmd = qemuAgentMakeCommand("guest-get-osinfo", NULL)))
return -1;
- if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0) {
+ if (qemuAgentCommand(mon, cmd, &reply, mon->timeout) < 0) {
if (qemuAgentErrorCommandUnsupported(reply))
return -2;
return -1;
@@ -2480,7 +2477,7 @@ qemuAgentGetTimezone(qemuAgentPtr mon,
if (!(cmd = qemuAgentMakeCommand("guest-get-timezone", NULL)))
return -1;
- if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0) {
+ if (qemuAgentCommand(mon, cmd, &reply, mon->timeout) < 0) {
if (qemuAgentErrorCommandUnsupported(reply))
return -2;
return -1;
--
2.23.0
5:38 p.m.
New subject: [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>
---
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
8:52 p.m.
New subject: [PATCH 2/2] qemu: remove redundant needReply argument of qemuAgentCommand
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(a)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.
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
1694
days inactive
1701
days old
7 comments
2 participants
participants (2)
-
Michal Privoznik -
Nikolay Shirokovskiy