[libvirt] [PATCH v2] qemu: agent: Avoid agentError when closing the QEMU agent
After calling qemuAgentClose(), it is still possible for the QEMU Agent I/O event callback to get invoked. This will trigger an agent error because mon->fd has been set to -1 at this point. Then vm->privateData->agentError is always 'true' except that restart libvirtd or restart qemu-guest-agent process in guest. Silently ignore the case where mon->fd is -1, likewise for mon->watch being zero. Signed-off-by: Wang Yechao <wang.yechao255@zte.com.cn> --- v1 patch: https://www.redhat.com/archives/libvir-list/2018-September/msg01382.html Changes in v2: - do not set agentError, let agent state as disconnected instead of error. --- src/qemu/qemu_agent.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 97ad0e7..d842b0e 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -530,6 +530,9 @@ static void qemuAgentUpdateWatch(qemuAgentPtr mon) VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR; + if (!mon->watch) + return; + if (mon->lastError.code == VIR_ERR_OK) { events |= VIR_EVENT_HANDLE_READABLE; @@ -555,6 +558,12 @@ qemuAgentIO(int watch, int fd, int events, void *opaque) VIR_DEBUG("Agent %p I/O on watch %d fd %d events %d", mon, watch, fd, events); #endif + if (mon->fd == -1 || mon->watch == 0) { + virObjectUnlock(mon); + virObjectUnref(mon); + return; + } + if (mon->fd != fd || mon->watch != watch) { if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR)) eof = true; @@ -788,8 +797,10 @@ void qemuAgentClose(qemuAgentPtr mon) virObjectLock(mon); if (mon->fd >= 0) { - if (mon->watch) + if (mon->watch) { virEventRemoveHandle(mon->watch); + mon->watch = 0; + } VIR_FORCE_CLOSE(mon->fd); } -- 1.8.3.1
On 09/28/2018 11:36 AM, Wang Yechao wrote:
After calling qemuAgentClose(), it is still possible for the QEMU Agent I/O event callback to get invoked. This will trigger an agent error because mon->fd has been set to -1 at this point. Then vm->privateData->agentError is always 'true' except that restart libvirtd or restart qemu-guest-agent process in guest.
Silently ignore the case where mon->fd is -1, likewise for mon->watch being zero.
Signed-off-by: Wang Yechao <wang.yechao255@zte.com.cn> --- v1 patch: https://www.redhat.com/archives/libvir-list/2018-September/msg01382.html
Changes in v2: - do not set agentError, let agent state as disconnected instead of error. --- src/qemu/qemu_agent.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 97ad0e7..d842b0e 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -530,6 +530,9 @@ static void qemuAgentUpdateWatch(qemuAgentPtr mon) VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR;
+ if (!mon->watch) + return; + if (mon->lastError.code == VIR_ERR_OK) { events |= VIR_EVENT_HANDLE_READABLE;
@@ -555,6 +558,12 @@ qemuAgentIO(int watch, int fd, int events, void *opaque) VIR_DEBUG("Agent %p I/O on watch %d fd %d events %d", mon, watch, fd, events); #endif
+ if (mon->fd == -1 || mon->watch == 0) { + virObjectUnlock(mon); + virObjectUnref(mon); + return; + } +
Is this safe to do? What if there's a thread waiting for a message from the agent? Shouldn't @eof variable be set in this case so that appropriate code is run?
if (mon->fd != fd || mon->watch != watch) { if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR)) eof = true; @@ -788,8 +797,10 @@ void qemuAgentClose(qemuAgentPtr mon) virObjectLock(mon);
if (mon->fd >= 0) { - if (mon->watch) + if (mon->watch) { virEventRemoveHandle(mon->watch); + mon->watch = 0; + } VIR_FORCE_CLOSE(mon->fd); }
Michal
On 09/28/2018 11:36 AM, Wang Yechao wrote:
After calling qemuAgentClose(), it is still possible for the QEMU Agent I/O event callback to get invoked. This will trigger an agent error because mon->fd has been set to -1 at this point. Then vm->privateData->agentError is always 'true' except that restart libvirtd or restart qemu-guest-agent process in guest.
Silently ignore the case where mon->fd is -1, likewise for mon->watch being zero.
Signed-off-by: Wang Yechao <wang.yechao255@zte.com.cn> --- v1 patch: https://www.redhat.com/archives/libvir-list/2018-September/msg01382.html
Changes in v2: - do not set agentError, let agent state as disconnected instead of error. --- src/qemu/qemu_agent.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 97ad0e7..d842b0e 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -530,6 +530,9 @@ static void qemuAgentUpdateWatch(qemuAgentPtr mon) VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR;
+ if (!mon->watch) + return; + if (mon->lastError.code == VIR_ERR_OK) { events |= VIR_EVENT_HANDLE_READABLE;
@@ -555,6 +558,12 @@ qemuAgentIO(int watch, int fd, int events, void *opaque) VIR_DEBUG("Agent %p I/O on watch %d fd %d events %d", mon, watch, fd, events); #endif
+ if (mon->fd == -1 || mon->watch == 0) { + virObjectUnlock(mon); + virObjectUnref(mon); + return; + } +
Is this safe to do? What if there's a thread waiting for a message from the agent? Shouldn't @eof variable be set in this case so that appropriate code is run?
It is safe to do. The waiting thread is waked up when qemuAgentClose invoked, and it cannot get any message from the agent at this time. There is no need to set the @eof variable, because the EOF callback's job has been done when closing the agent.
if (mon->fd != fd || mon->watch != watch) { if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR)) eof = true; @@ -788,8 +797,10 @@ void qemuAgentClose(qemuAgentPtr mon) virObjectLock(mon);
if (mon->fd >= 0) { - if (mon->watch) + if (mon->watch) { virEventRemoveHandle(mon->watch); + mon->watch = 0; + } VIR_FORCE_CLOSE(mon->fd); }
Michal
--- Best wishes Wang Yechao
On 9/28/18 5:36 AM, Wang Yechao wrote:
After calling qemuAgentClose(), it is still possible for the QEMU Agent I/O event callback to get invoked. This will trigger an agent error because mon->fd has been set to -1 at this point. Then vm->privateData->agentError is always 'true' except that restart libvirtd or restart qemu-guest-agent process in guest.
Silently ignore the case where mon->fd is -1, likewise for mon->watch being zero.
Signed-off-by: Wang Yechao <wang.yechao255@zte.com.cn> --- v1 patch: https://www.redhat.com/archives/libvir-list/2018-September/msg01382.html
Changes in v2: - do not set agentError, let agent state as disconnected instead of error. --- src/qemu/qemu_agent.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
Since the v1 review referenced commit 89563efc which it seems you applied or liberally copied here, I think it would be a "good idea" to reference that commit in your commit message. Imitation is a form of flattery and so as is attribution. John
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 97ad0e7..d842b0e 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -530,6 +530,9 @@ static void qemuAgentUpdateWatch(qemuAgentPtr mon) VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR;
+ if (!mon->watch) + return; + if (mon->lastError.code == VIR_ERR_OK) { events |= VIR_EVENT_HANDLE_READABLE;
@@ -555,6 +558,12 @@ qemuAgentIO(int watch, int fd, int events, void *opaque) VIR_DEBUG("Agent %p I/O on watch %d fd %d events %d", mon, watch, fd, events); #endif
+ if (mon->fd == -1 || mon->watch == 0) { + virObjectUnlock(mon); + virObjectUnref(mon); + return; + } + if (mon->fd != fd || mon->watch != watch) { if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR)) eof = true; @@ -788,8 +797,10 @@ void qemuAgentClose(qemuAgentPtr mon) virObjectLock(mon);
if (mon->fd >= 0) { - if (mon->watch) + if (mon->watch) { virEventRemoveHandle(mon->watch); + mon->watch = 0; + } VIR_FORCE_CLOSE(mon->fd); }
On 9/28/18 5:36 AM, Wang Yechao wrote:
After calling qemuAgentClose(), it is still possible for the QEMU Agent I/O event callback to get invoked. This will trigger an agent error because mon->fd has been set to -1 at this point. Then vm->privateData->agentError is always 'true' except that restart libvirtd or restart qemu-guest-agent process in guest.
Silently ignore the case where mon->fd is -1, likewise for mon->watch being zero.
Signed-off-by: Wang Yechao <wang.yechao255@zte.com.cn> --- v1 patch: https://www.redhat.com/archives/libvir-list/2018-September/msg01382.html
Changes in v2: - do not set agentError, let agent state as disconnected instead of error. --- src/qemu/qemu_agent.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
Since the v1 review referenced commit 89563efc which it seems you applied or liberally copied here, I think it would be a "good idea" to reference that commit in your commit message. Imitation is a form of flattery and so as is attribution.
John
Thanks for your advice, I should reference commit 89563efc in my commit message.
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 97ad0e7..d842b0e 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -530,6 +530,9 @@ static void qemuAgentUpdateWatch(qemuAgentPtr mon) VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR;
+ if (!mon->watch) + return; + if (mon->lastError.code == VIR_ERR_OK) { events |= VIR_EVENT_HANDLE_READABLE;
@@ -555,6 +558,12 @@ qemuAgentIO(int watch, int fd, int events, void *opaque) VIR_DEBUG("Agent %p I/O on watch %d fd %d events %d", mon, watch, fd, events); #endif
+ if (mon->fd == -1 || mon->watch == 0) { + virObjectUnlock(mon); + virObjectUnref(mon); + return; + } + if (mon->fd != fd || mon->watch != watch) { if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR)) eof = true; @@ -788,8 +797,10 @@ void qemuAgentClose(qemuAgentPtr mon) virObjectLock(mon);
if (mon->fd >= 0) { - if (mon->watch) + if (mon->watch) { virEventRemoveHandle(mon->watch); + mon->watch = 0; + } VIR_FORCE_CLOSE(mon->fd); }
--- Best wishes Wang Yechao
participants (4)
-
John Ferlan -
Michal Privoznik -
Wang Yechao -
wang.yechao255＠zte.com.cn