[PATCH 0/2] ui/vdagent: Fix two bugs about disconnect event handling

[PATCH 0/2] ui/vdagent: Fix two bugs about disconnect event handling

[tugy]

From: Guoyi Tu <tugy@chinatelecom.cn>

and resource leak

Guoyi Tu (2):
  ui/vdagent: call vdagent_disconnect() when agent connection is lost
  ui/vdagent: Unregister input handler of mouse during finalization

 ui/vdagent.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

-- 
2.27.0

[PATCH 1/2] ui/vdagent: call vdagent_disconnect() when agent connection is lost

[tugy]

From: Guoyi Tu <tugy@chinatelecom.cn>

when the agent connection is lost, the input handler of the mouse
doesn't deactivate, which results in unresponsive mouse events in
VNC windows.

To fix this issue, call vdagent_disconnect() to reset the state
each time the frontend disconncect

Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
Signed-off-by: dengpengcheng <dengpc12@chinatelecom.cn>
---
 ui/vdagent.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ui/vdagent.c b/ui/vdagent.c
index 8a651492f0..386dc5abe0 100644
--- a/ui/vdagent.c
+++ b/ui/vdagent.c
@@ -870,8 +870,11 @@ static void vdagent_disconnect(VDAgentChardev *vd)
 
 static void vdagent_chr_set_fe_open(struct Chardev *chr, int fe_open)
 {
+    VDAgentChardev *vd = QEMU_VDAGENT_CHARDEV(chr);
+
     if (!fe_open) {
         trace_vdagent_close();
+        vdagent_disconnect(vd);
         /* To reset_serial, we CLOSED our side. Make sure the other end knows we
          * are ready again. */
         qemu_chr_be_event(chr, CHR_EVENT_OPENED);
@@ -922,7 +925,6 @@ static void vdagent_chr_fini(Object *obj)
     VDAgentChardev *vd = QEMU_VDAGENT_CHARDEV(obj);
 
     migrate_del_blocker(vd->migration_blocker);
-    vdagent_disconnect(vd);
     buffer_free(&vd->outbuf);
     error_free(vd->migration_blocker);
 }
-- 
2.27.0

[PATCH 2/2] ui/vdagent: Unregister input handler of mouse during finalization

[tugy]

From: Guoyi Tu <tugy@chinatelecom.cn>

Input handler resource should be released when
VDAgentChardev object finalize

Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
Signed-off-by: dengpengcheng <dengpc12@chinatelecom.cn>
---
 ui/vdagent.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ui/vdagent.c b/ui/vdagent.c
index 386dc5abe0..4c9b3b7ba8 100644
--- a/ui/vdagent.c
+++ b/ui/vdagent.c
@@ -924,6 +924,9 @@ static void vdagent_chr_fini(Object *obj)
 {
     VDAgentChardev *vd = QEMU_VDAGENT_CHARDEV(obj);
 
+    if (vd->mouse_hs) {
+        qemu_input_handler_unregister(vd->mouse_hs);
+    }
     migrate_del_blocker(vd->migration_blocker);
     buffer_free(&vd->outbuf);
     error_free(vd->migration_blocker);
-- 
2.27.0

Re: [PATCH 1/2] ui/vdagent: call vdagent_disconnect() when agent connection is lost

[Marc-André Lureau]

Hi

On Thu, Aug 17, 2023 at 3:32 PM <tugy@chinatelecom.cn> wrote:
>
> From: Guoyi Tu <tugy@chinatelecom.cn>
>
> when the agent connection is lost, the input handler of the mouse
> doesn't deactivate, which results in unresponsive mouse events in
> VNC windows.
>
> To fix this issue, call vdagent_disconnect() to reset the state
> each time the frontend disconncect
>
> Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
> Signed-off-by: dengpengcheng <dengpc12@chinatelecom.cn>
> ---
>  ui/vdagent.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/ui/vdagent.c b/ui/vdagent.c
> index 8a651492f0..386dc5abe0 100644
> --- a/ui/vdagent.c
> +++ b/ui/vdagent.c
> @@ -870,8 +870,11 @@ static void vdagent_disconnect(VDAgentChardev *vd)
>
>  static void vdagent_chr_set_fe_open(struct Chardev *chr, int fe_open)
>  {
> +    VDAgentChardev *vd = QEMU_VDAGENT_CHARDEV(chr);
> +
>      if (!fe_open) {
>          trace_vdagent_close();
> +        vdagent_disconnect(vd);
>          /* To reset_serial, we CLOSED our side. Make sure the other end knows we
>           * are ready again. */
>          qemu_chr_be_event(chr, CHR_EVENT_OPENED);
> @@ -922,7 +925,6 @@ static void vdagent_chr_fini(Object *obj)
>      VDAgentChardev *vd = QEMU_VDAGENT_CHARDEV(obj);
>
>      migrate_del_blocker(vd->migration_blocker);
> -    vdagent_disconnect(vd);

why remove this cleanup ? (the function seems safe to call multiple
times, if it is the case during finalize)

>      buffer_free(&vd->outbuf);
>      error_free(vd->migration_blocker);
>  }
> --
> 2.27.0
>
>


-- 
Marc-André Lureau

Re: [PATCH 2/2] ui/vdagent: Unregister input handler of mouse during finalization

[Marc-André Lureau]

On Thu, Aug 17, 2023 at 3:33 PM <tugy@chinatelecom.cn> wrote:
>
> From: Guoyi Tu <tugy@chinatelecom.cn>
>
> Input handler resource should be released when
> VDAgentChardev object finalize
>
> Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
> Signed-off-by: dengpengcheng <dengpc12@chinatelecom.cn>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  ui/vdagent.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/ui/vdagent.c b/ui/vdagent.c
> index 386dc5abe0..4c9b3b7ba8 100644
> --- a/ui/vdagent.c
> +++ b/ui/vdagent.c
> @@ -924,6 +924,9 @@ static void vdagent_chr_fini(Object *obj)
>  {
>      VDAgentChardev *vd = QEMU_VDAGENT_CHARDEV(obj);
>
> +    if (vd->mouse_hs) {
> +        qemu_input_handler_unregister(vd->mouse_hs);
> +    }
>      migrate_del_blocker(vd->migration_blocker);
>      buffer_free(&vd->outbuf);
>      error_free(vd->migration_blocker);
> --
> 2.27.0
>
>


-- 
Marc-André Lureau

Re: [PATCH 1/2] ui/vdagent: call vdagent_disconnect() when agent connection is lost

[Guoyi Tu]

On 2023/8/17 20:49, 【外部账号】Marc-André Lureau wrote:
> Hi
> 
> On Thu, Aug 17, 2023 at 3:32 PM <tugy@chinatelecom.cn> wrote:
>>
>> From: Guoyi Tu <tugy@chinatelecom.cn>
>>
>> when the agent connection is lost, the input handler of the mouse
>> doesn't deactivate, which results in unresponsive mouse events in
>> VNC windows.
>>
>> To fix this issue, call vdagent_disconnect() to reset the state
>> each time the frontend disconncect
>>
>> Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
>> Signed-off-by: dengpengcheng <dengpc12@chinatelecom.cn>
>> ---
>>   ui/vdagent.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/ui/vdagent.c b/ui/vdagent.c
>> index 8a651492f0..386dc5abe0 100644
>> --- a/ui/vdagent.c
>> +++ b/ui/vdagent.c
>> @@ -870,8 +870,11 @@ static void vdagent_disconnect(VDAgentChardev *vd)
>>
>>   static void vdagent_chr_set_fe_open(struct Chardev *chr, int fe_open)
>>   {
>> +    VDAgentChardev *vd = QEMU_VDAGENT_CHARDEV(chr);
>> +
>>       if (!fe_open) {
>>           trace_vdagent_close();
>> +        vdagent_disconnect(vd);
>>           /* To reset_serial, we CLOSED our side. Make sure the other end knows we
>>            * are ready again. */
>>           qemu_chr_be_event(chr, CHR_EVENT_OPENED);
>> @@ -922,7 +925,6 @@ static void vdagent_chr_fini(Object *obj)
>>       VDAgentChardev *vd = QEMU_VDAGENT_CHARDEV(obj);
>>
>>       migrate_del_blocker(vd->migration_blocker);
>> -    vdagent_disconnect(vd);
> 
> why remove this cleanup ? (the function seems safe to call multiple
> times, if it is the case during finalize)
> 
Before the instance finalize, the connection will be closed and the
vdagent_chr_set_fe_open() will be called with be_open set to false,
which in turn call vdagent_disconnect to clean up resources.

>>       buffer_free(&vd->outbuf);
>>       error_free(vd->migration_blocker);
>>   }
>> --
>> 2.27.0
>>
>>
> 
> 

Re: [PATCH 1/2] ui/vdagent: call vdagent_disconnect() when agent connection is lost

[Marc-André Lureau]

Hi

On Thu, Aug 17, 2023 at 5:10 PM Guoyi Tu <tugy@chinatelecom.cn> wrote:
>
>
>
> On 2023/8/17 20:49, 【外部账号】Marc-André Lureau wrote:
> > Hi
> >
> > On Thu, Aug 17, 2023 at 3:32 PM <tugy@chinatelecom.cn> wrote:
> >>
> >> From: Guoyi Tu <tugy@chinatelecom.cn>
> >>
> >> when the agent connection is lost, the input handler of the mouse
> >> doesn't deactivate, which results in unresponsive mouse events in
> >> VNC windows.
> >>
> >> To fix this issue, call vdagent_disconnect() to reset the state
> >> each time the frontend disconncect
> >>
> >> Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
> >> Signed-off-by: dengpengcheng <dengpc12@chinatelecom.cn>
> >> ---
> >>   ui/vdagent.c | 4 +++-
> >>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/ui/vdagent.c b/ui/vdagent.c
> >> index 8a651492f0..386dc5abe0 100644
> >> --- a/ui/vdagent.c
> >> +++ b/ui/vdagent.c
> >> @@ -870,8 +870,11 @@ static void vdagent_disconnect(VDAgentChardev *vd)
> >>
> >>   static void vdagent_chr_set_fe_open(struct Chardev *chr, int fe_open)
> >>   {
> >> +    VDAgentChardev *vd = QEMU_VDAGENT_CHARDEV(chr);
> >> +
> >>       if (!fe_open) {
> >>           trace_vdagent_close();
> >> +        vdagent_disconnect(vd);
> >>           /* To reset_serial, we CLOSED our side. Make sure the other end knows we
> >>            * are ready again. */
> >>           qemu_chr_be_event(chr, CHR_EVENT_OPENED);
> >> @@ -922,7 +925,6 @@ static void vdagent_chr_fini(Object *obj)
> >>       VDAgentChardev *vd = QEMU_VDAGENT_CHARDEV(obj);
> >>
> >>       migrate_del_blocker(vd->migration_blocker);
> >> -    vdagent_disconnect(vd);
> >
> > why remove this cleanup ? (the function seems safe to call multiple
> > times, if it is the case during finalize)
> >
> Before the instance finalize, the connection will be closed and the
> vdagent_chr_set_fe_open() will be called with be_open set to false,
> which in turn call vdagent_disconnect to clean up resources.

Let's leave it, as it's more straightforward and safe, even if it's
called two times in some code paths.



-- 
Marc-André Lureau

Re: [PATCH 1/2] ui/vdagent: call vdagent_disconnect() when agent connection is lost

[Guoyi Tu]

On 2023/8/17 21:36, 【外部账号】Marc-André Lureau wrote:
> Hi
> 
> On Thu, Aug 17, 2023 at 5:10 PM Guoyi Tu <tugy@chinatelecom.cn> wrote:
>>
>>
>>
>> On 2023/8/17 20:49, 【外部账号】Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Thu, Aug 17, 2023 at 3:32 PM <tugy@chinatelecom.cn> wrote:
>>>>
>>>> From: Guoyi Tu <tugy@chinatelecom.cn>
>>>>
>>>> when the agent connection is lost, the input handler of the mouse
>>>> doesn't deactivate, which results in unresponsive mouse events in
>>>> VNC windows.
>>>>
>>>> To fix this issue, call vdagent_disconnect() to reset the state
>>>> each time the frontend disconncect
>>>>
>>>> Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
>>>> Signed-off-by: dengpengcheng <dengpc12@chinatelecom.cn>
>>>> ---
>>>>    ui/vdagent.c | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/ui/vdagent.c b/ui/vdagent.c
>>>> index 8a651492f0..386dc5abe0 100644
>>>> --- a/ui/vdagent.c
>>>> +++ b/ui/vdagent.c
>>>> @@ -870,8 +870,11 @@ static void vdagent_disconnect(VDAgentChardev *vd)
>>>>
>>>>    static void vdagent_chr_set_fe_open(struct Chardev *chr, int fe_open)
>>>>    {
>>>> +    VDAgentChardev *vd = QEMU_VDAGENT_CHARDEV(chr);
>>>> +
>>>>        if (!fe_open) {
>>>>            trace_vdagent_close();
>>>> +        vdagent_disconnect(vd);
>>>>            /* To reset_serial, we CLOSED our side. Make sure the other end knows we
>>>>             * are ready again. */
>>>>            qemu_chr_be_event(chr, CHR_EVENT_OPENED);
>>>> @@ -922,7 +925,6 @@ static void vdagent_chr_fini(Object *obj)
>>>>        VDAgentChardev *vd = QEMU_VDAGENT_CHARDEV(obj);
>>>>
>>>>        migrate_del_blocker(vd->migration_blocker);
>>>> -    vdagent_disconnect(vd);
>>>
>>> why remove this cleanup ? (the function seems safe to call multiple
>>> times, if it is the case during finalize)
>>>
>> Before the instance finalize, the connection will be closed and the
>> vdagent_chr_set_fe_open() will be called with be_open set to false,
>> which in turn call vdagent_disconnect to clean up resources.
> 
> Let's leave it, as it's more straightforward and safe, even if it's
> called two times in some code paths.
> 
Okay, it sounds reasonable, i will send another patches