1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH v2] qemu: agent: Avoid agentError when closing the QEMU agent

Wang Yechao posted 1 patch 5 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1538127368-2305-1-git-send-email-wang.yechao255@zte.com.cn
There is a newer version of this series
src/qemu/qemu_agent.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
[libvirt] [PATCH v2] qemu: agent: Avoid agentError when closing the QEMU agent
Posted by Wang Yechao 5 years, 7 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: agent: Avoid agentError when closing the QEMU agent
Posted by Michal Privoznik 5 years, 6 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt][PATCH v2] qemu: agent: Avoid agentError when closing the QEMU agent
Posted by wang.yechao255@zte.com.cn 5 years, 6 months ago 
> 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--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: agent: Avoid agentError when closing the QEMU agent
Posted by John Ferlan 5 years, 6 months ago 

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);
>      }
>  
>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt][PATCH v2] qemu: agent: Avoid agentError when closing the QEMU agent
Posted by wang.yechao255@zte.com.cn 5 years, 6 months ago 
> 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--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.