[libvirt] [PATCH 0/2] Revert to blocking behavior of qemuAgentCommand
Commit 05447e3af44ec153314ff97cd611330d9b4b5730 made changes to qemuAgentCommand so that an arbitrary timeout can be passed to it. However, it did so partially in a very confusing way and partially in a totally wrong way. Jiri Denemark (2): qemu: Remove redundant parameter from qemuAgentSend qemu: Revert to blocking behavior of qemuAgentCommand src/qemu/qemu_agent.c | 46 ++++++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 22 deletions(-) -- 1.7.12
The @timeout parameter of qemuAgentSend is both redundant and confusing. This patch should not result in any functional changes. --- src/qemu/qemu_agent.c | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index aaff9fc..6a7a0b4 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -832,19 +832,22 @@ void qemuAgentClose(qemuAgentPtr mon) virObjectUnref(mon); } -#define QEMU_AGENT_WAIT_TIME (1000ull * 5) +#define QEMU_AGENT_WAIT_TIME 5 /** * qemuAgentSend: * @mon: Monitor * @msg: Message - * @timeout: use timeout? - * @seconds: timeout seconds. if VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT and - * @timeout is true, use default value. + * @seconds: number of seconds to wait for the result, it can be either + * -2, -1, 0 or positive. * - * Send @msg to agent @mon. - * Wait max QEMU_AGENT_WAIT_TIME for agent - * to reply. + * Send @msg to agent @mon. If @seconds is equal to + * VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK(-2), this function will block forever + * waiting for the result. The value of + * VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT(-1) means use default timeout value + * and VIR_DOMAIN_QEMU_AGENT_COMMAND_NOWAIT(0) makes this this function return + * immediately without waiting. Any positive value means the number of seconds + * to wait for the result. * * Returns: 0 on success, * -2 on timeout, @@ -852,11 +855,10 @@ void qemuAgentClose(qemuAgentPtr mon) */ static int qemuAgentSend(qemuAgentPtr mon, qemuAgentMessagePtr msg, - bool timeout, int seconds) { int ret = -1; - unsigned long long now, then = 0; + unsigned long long then = 0; /* Check whether qemu quit unexpectedly */ if (mon->lastError.code != VIR_ERR_OK) { @@ -866,21 +868,21 @@ static int qemuAgentSend(qemuAgentPtr mon, return -1; } - if (timeout) { + if (seconds > VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) { + unsigned long long now; if (virTimeMillisNow(&now) < 0) return -1; - if (!(seconds >= 0 || seconds == VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT)) - return -1; - then = now + (seconds == VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT ? - QEMU_AGENT_WAIT_TIME : seconds * 1000ull); + if (seconds == VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT) + seconds = QEMU_AGENT_WAIT_TIME; + then = now + seconds * 1000ull; } mon->msg = msg; qemuAgentUpdateWatch(mon); while (!mon->msg->finished) { - if ((timeout && virCondWaitUntil(&mon->notify, &mon->lock, then) < 0) || - (!timeout && virCondWait(&mon->notify, &mon->lock) < 0)) { + if ((then && virCondWaitUntil(&mon->notify, &mon->lock, then) < 0) || + (!then && virCondWait(&mon->notify, &mon->lock) < 0)) { if (errno == ETIMEDOUT) { virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s", _("Guest agent not available for now")); @@ -945,7 +947,7 @@ qemuAgentGuestSync(qemuAgentPtr mon) VIR_DEBUG("Sending guest-sync command with ID: %llu", id); - send_ret = qemuAgentSend(mon, &sync_msg, true, + send_ret = qemuAgentSend(mon, &sync_msg, VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT); VIR_DEBUG("qemuAgentSend returned: %d", send_ret); @@ -1015,7 +1017,7 @@ qemuAgentCommand(qemuAgentPtr mon, VIR_DEBUG("Send command '%s' for write, seconds = %d", cmdstr, seconds); - ret = qemuAgentSend(mon, &msg, seconds < -1 ? false : true, seconds); + ret = qemuAgentSend(mon, &msg, seconds); VIR_DEBUG("Receive command reply ret=%d rxObject=%p", ret, msg.rxObject); -- 1.7.12
On 30.08.2012 15:12, Jiri Denemark wrote:
The @timeout parameter of qemuAgentSend is both redundant and confusing. This patch should not result in any functional changes. --- src/qemu/qemu_agent.c | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index aaff9fc..6a7a0b4 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -832,19 +832,22 @@ void qemuAgentClose(qemuAgentPtr mon) virObjectUnref(mon); }
-#define QEMU_AGENT_WAIT_TIME (1000ull * 5) +#define QEMU_AGENT_WAIT_TIME 5
/** * qemuAgentSend: * @mon: Monitor * @msg: Message - * @timeout: use timeout? - * @seconds: timeout seconds. if VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT and - * @timeout is true, use default value. + * @seconds: number of seconds to wait for the result, it can be either + * -2, -1, 0 or positive. * - * Send @msg to agent @mon. - * Wait max QEMU_AGENT_WAIT_TIME for agent - * to reply. + * Send @msg to agent @mon. If @seconds is equal to + * VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK(-2), this function will block forever + * waiting for the result. The value of + * VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT(-1) means use default timeout value + * and VIR_DOMAIN_QEMU_AGENT_COMMAND_NOWAIT(0) makes this this function return + * immediately without waiting. Any positive value means the number of seconds + * to wait for the result. * * Returns: 0 on success, * -2 on timeout, @@ -852,11 +855,10 @@ void qemuAgentClose(qemuAgentPtr mon) */ static int qemuAgentSend(qemuAgentPtr mon, qemuAgentMessagePtr msg, - bool timeout, int seconds) { int ret = -1; - unsigned long long now, then = 0; + unsigned long long then = 0;
/* Check whether qemu quit unexpectedly */ if (mon->lastError.code != VIR_ERR_OK) { @@ -866,21 +868,21 @@ static int qemuAgentSend(qemuAgentPtr mon, return -1; }
- if (timeout) { + if (seconds > VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) { + unsigned long long now; if (virTimeMillisNow(&now) < 0) return -1; - if (!(seconds >= 0 || seconds == VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT)) - return -1; - then = now + (seconds == VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT ? - QEMU_AGENT_WAIT_TIME : seconds * 1000ull); + if (seconds == VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT) + seconds = QEMU_AGENT_WAIT_TIME; + then = now + seconds * 1000ull; }
mon->msg = msg; qemuAgentUpdateWatch(mon);
while (!mon->msg->finished) { - if ((timeout && virCondWaitUntil(&mon->notify, &mon->lock, then) < 0) || - (!timeout && virCondWait(&mon->notify, &mon->lock) < 0)) { + if ((then && virCondWaitUntil(&mon->notify, &mon->lock, then) < 0) || + (!then && virCondWait(&mon->notify, &mon->lock) < 0)) { if (errno == ETIMEDOUT) { virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s", _("Guest agent not available for now")); @@ -945,7 +947,7 @@ qemuAgentGuestSync(qemuAgentPtr mon)
VIR_DEBUG("Sending guest-sync command with ID: %llu", id);
- send_ret = qemuAgentSend(mon, &sync_msg, true, + send_ret = qemuAgentSend(mon, &sync_msg, VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT);
VIR_DEBUG("qemuAgentSend returned: %d", send_ret); @@ -1015,7 +1017,7 @@ qemuAgentCommand(qemuAgentPtr mon,
VIR_DEBUG("Send command '%s' for write, seconds = %d", cmdstr, seconds);
- ret = qemuAgentSend(mon, &msg, seconds < -1 ? false : true, seconds); + ret = qemuAgentSend(mon, &msg, seconds);
VIR_DEBUG("Receive command reply ret=%d rxObject=%p", ret, msg.rxObject);
ACK Michal
Before commit 05447e3af44ec153314ff97cd611330d9b4b5730, qemuAgentCommand blocked until it got a reply or appropriate event. When new parameter was added to qemuAgentCommand in the above commit, all existing callers of it were updated in a wrong way changing them from blocking to 5-seconds timeout. --- src/qemu/qemu_agent.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 6a7a0b4..51e60d2 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1296,7 +1296,7 @@ int qemuAgentShutdown(qemuAgentPtr mon, mon->await_event = QEMU_AGENT_EVENT_SHUTDOWN; ret = qemuAgentCommand(mon, cmd, &reply, - VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT); + VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK); if (reply && ret == 0) ret = qemuAgentCheckError(cmd, reply); @@ -1329,7 +1329,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon) return -1; if (qemuAgentCommand(mon, cmd, &reply, - VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT) < 0 || + VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0 || qemuAgentCheckError(cmd, reply) < 0) goto cleanup; @@ -1367,7 +1367,7 @@ int qemuAgentFSThaw(qemuAgentPtr mon) return -1; if (qemuAgentCommand(mon, cmd, &reply, - VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT) < 0 || + VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0 || qemuAgentCheckError(cmd, reply) < 0) goto cleanup; @@ -1405,7 +1405,7 @@ qemuAgentSuspend(qemuAgentPtr mon, mon->await_event = QEMU_AGENT_EVENT_SUSPEND; ret = qemuAgentCommand(mon, cmd, &reply, - VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT); + VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK); if (reply && ret == 0) ret = qemuAgentCheckError(cmd, reply); -- 1.7.12
On 30.08.2012 15:12, Jiri Denemark wrote:
Before commit 05447e3af44ec153314ff97cd611330d9b4b5730, qemuAgentCommand blocked until it got a reply or appropriate event. When new parameter was added to qemuAgentCommand in the above commit, all existing callers of it were updated in a wrong way changing them from blocking to 5-seconds timeout. --- src/qemu/qemu_agent.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 6a7a0b4..51e60d2 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1296,7 +1296,7 @@ int qemuAgentShutdown(qemuAgentPtr mon,
mon->await_event = QEMU_AGENT_EVENT_SHUTDOWN; ret = qemuAgentCommand(mon, cmd, &reply, - VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT); + VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK);
if (reply && ret == 0) ret = qemuAgentCheckError(cmd, reply); @@ -1329,7 +1329,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon) return -1;
if (qemuAgentCommand(mon, cmd, &reply, - VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT) < 0 || + VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0 || qemuAgentCheckError(cmd, reply) < 0) goto cleanup;
@@ -1367,7 +1367,7 @@ int qemuAgentFSThaw(qemuAgentPtr mon) return -1;
if (qemuAgentCommand(mon, cmd, &reply, - VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT) < 0 || + VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0 || qemuAgentCheckError(cmd, reply) < 0) goto cleanup;
@@ -1405,7 +1405,7 @@ qemuAgentSuspend(qemuAgentPtr mon,
mon->await_event = QEMU_AGENT_EVENT_SUSPEND; ret = qemuAgentCommand(mon, cmd, &reply, - VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT); + VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK);
if (reply && ret == 0) ret = qemuAgentCheckError(cmd, reply);
ACK Michal
On Thu, Aug 30, 2012 at 03:12:44PM +0200, Jiri Denemark wrote:
Commit 05447e3af44ec153314ff97cd611330d9b4b5730 made changes to qemuAgentCommand so that an arbitrary timeout can be passed to it. However, it did so partially in a very confusing way and partially in a totally wrong way.
Jiri Denemark (2): qemu: Remove redundant parameter from qemuAgentSend qemu: Revert to blocking behavior of qemuAgentCommand
src/qemu/qemu_agent.c | 46 ++++++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 22 deletions(-)
ACK to the set. The core reason is that the new functionality of adding arbitrary guest agent command support should not be done at the detriment of existing functionality, in that case we get an error when using guest agent based shutdown https://bugzilla.redhat.com/show_bug.cgi?id=853002 Let's try to get the new API fixed for the next release then Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, Aug 30, 2012 at 21:58:12 +0800, Daniel Veillard wrote:
On Thu, Aug 30, 2012 at 03:12:44PM +0200, Jiri Denemark wrote:
Commit 05447e3af44ec153314ff97cd611330d9b4b5730 made changes to qemuAgentCommand so that an arbitrary timeout can be passed to it. However, it did so partially in a very confusing way and partially in a totally wrong way.
Jiri Denemark (2): qemu: Remove redundant parameter from qemuAgentSend qemu: Revert to blocking behavior of qemuAgentCommand
src/qemu/qemu_agent.c | 46 ++++++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 22 deletions(-)
ACK to the set. The core reason is that the new functionality of adding arbitrary guest agent command support should not be done at the detriment of existing functionality, in that case we get an error when using guest agent based shutdown
https://bugzilla.redhat.com/show_bug.cgi?id=853002
Let's try to get the new API fixed for the next release then
Thanks guys, I pushed this series. Jirka
participants (3)
-
Daniel Veillard -
Jiri Denemark -
Michal Privoznik