During vhost-user reconnect functionality testing the following assert
was hit:
  qemu-system-x86_64: chardev/char-socket.c:125:
  qemu_chr_socket_restart_timer: Assertion `!s->reconnect_timer' failed.
  Aborted (core dumped)
This is observed only if the connection is closed by the vhost-user-blk
daemon during the initialization routine. In this case the
tcp_chr_disconnect_locked() routine is called twice. First time it is
called in the tcp_chr_write() routine, after getting the SIGPIPE signal.
Second time it is called when vhost_user_blk_connect() routine return
error. In general it looks correct, because the initialization routine
can return error in many cases.
The tcp_chr_disconnect_locked() routine could be fixed. The timer will
be restarted only if the close event is emitted.

Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
---
 chardev/char-socket.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index c128cca..83ca4d9 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -476,7 +476,7 @@ static void update_disconnected_filename(SocketChardev *s)
 static void tcp_chr_disconnect_locked(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
-    bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED;
+    bool was_connected = s->state == TCP_CHARDEV_STATE_CONNECTED;
 
     tcp_chr_free_connection(chr);
 
@@ -485,11 +485,11 @@ static void tcp_chr_disconnect_locked(Chardev *chr)
                                               chr, NULL, chr->gcontext);
     }
     update_disconnected_filename(s);
-    if (emit_close) {
+    if (was_connected) {
         qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
-    }
-    if (s->reconnect_time) {
-        qemu_chr_socket_restart_timer(chr);
+        if (s->reconnect_time) {
+            qemu_chr_socket_restart_timer(chr);
+        }
     }
 }
 
-- 
2.7.4

Hi

On Thu, Apr 23, 2020 at 8:41 PM Dima Stepanov <dimastep@yandex-team.ru> wrote:
>
> During vhost-user reconnect functionality testing the following assert
> was hit:
>   qemu-system-x86_64: chardev/char-socket.c:125:
>   qemu_chr_socket_restart_timer: Assertion `!s->reconnect_timer' failed.
>   Aborted (core dumped)

That looks related to "[PATCH 3/4] char-socket: avoid double call
tcp_chr_free_connection"

> This is observed only if the connection is closed by the vhost-user-blk
> daemon during the initialization routine. In this case the
> tcp_chr_disconnect_locked() routine is called twice. First time it is
> called in the tcp_chr_write() routine, after getting the SIGPIPE signal.
> Second time it is called when vhost_user_blk_connect() routine return
> error. In general it looks correct, because the initialization routine
> can return error in many cases.
> The tcp_chr_disconnect_locked() routine could be fixed. The timer will
> be restarted only if the close event is emitted.
>
> Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
> ---
>  chardev/char-socket.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index c128cca..83ca4d9 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -476,7 +476,7 @@ static void update_disconnected_filename(SocketChardev *s)
>  static void tcp_chr_disconnect_locked(Chardev *chr)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
> -    bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED;
> +    bool was_connected = s->state == TCP_CHARDEV_STATE_CONNECTED;
>
>      tcp_chr_free_connection(chr);
>
> @@ -485,11 +485,11 @@ static void tcp_chr_disconnect_locked(Chardev *chr)
>                                                chr, NULL, chr->gcontext);
>      }
>      update_disconnected_filename(s);
> -    if (emit_close) {
> +    if (was_connected) {
>          qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> -    }
> -    if (s->reconnect_time) {
> -        qemu_chr_socket_restart_timer(chr);
> +        if (s->reconnect_time) {
> +            qemu_chr_socket_restart_timer(chr);
> +        }
>      }
>  }
>
> --
> 2.7.4
>
>


-- 
Marc-André Lureau

This patch is trying to fix the same issue with me.
However, our fix is different.

I think that check the s->reconnect_timer is better.

Thanks,
Feng Li

Marc-André Lureau <marcandre.lureau@gmail.com> 于2020年4月24日周五 上午3:16写道：


>
> Hi
>
> On Thu, Apr 23, 2020 at 8:41 PM Dima Stepanov <dimastep@yandex-team.ru> wrote:
> >
> > During vhost-user reconnect functionality testing the following assert
> > was hit:
> >   qemu-system-x86_64: chardev/char-socket.c:125:
> >   qemu_chr_socket_restart_timer: Assertion `!s->reconnect_timer' failed.
> >   Aborted (core dumped)
>
> That looks related to "[PATCH 3/4] char-socket: avoid double call
> tcp_chr_free_connection"
>
> > This is observed only if the connection is closed by the vhost-user-blk
> > daemon during the initialization routine. In this case the
> > tcp_chr_disconnect_locked() routine is called twice. First time it is
> > called in the tcp_chr_write() routine, after getting the SIGPIPE signal.
> > Second time it is called when vhost_user_blk_connect() routine return
> > error. In general it looks correct, because the initialization routine
> > can return error in many cases.
> > The tcp_chr_disconnect_locked() routine could be fixed. The timer will
> > be restarted only if the close event is emitted.
> >
> > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
> > ---
> >  chardev/char-socket.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index c128cca..83ca4d9 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -476,7 +476,7 @@ static void update_disconnected_filename(SocketChardev *s)
> >  static void tcp_chr_disconnect_locked(Chardev *chr)
> >  {
> >      SocketChardev *s = SOCKET_CHARDEV(chr);
> > -    bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED;
> > +    bool was_connected = s->state == TCP_CHARDEV_STATE_CONNECTED;
> >
> >      tcp_chr_free_connection(chr);
> >
> > @@ -485,11 +485,11 @@ static void tcp_chr_disconnect_locked(Chardev *chr)
> >                                                chr, NULL, chr->gcontext);
> >      }
> >      update_disconnected_filename(s);
> > -    if (emit_close) {
> > +    if (was_connected) {
> >          qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> > -    }
> > -    if (s->reconnect_time) {
> > -        qemu_chr_socket_restart_timer(chr);
> > +        if (s->reconnect_time) {
> > +            qemu_chr_socket_restart_timer(chr);
> > +        }
> >      }
> >  }
> >
> > --
> > 2.7.4
> >
> >
>
>
> --
> Marc-André Lureau

-- 
The SmartX email address is only for business purpose. Any sent message 
that is not related to the business is not authorized or permitted by 
SmartX.
本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.

On Sun, Apr 26, 2020 at 03:26:58PM +0800, Li Feng wrote:
> This patch is trying to fix the same issue with me.
> However, our fix is different.
> 
> I think that check the s->reconnect_timer is better.

I also thought about your solution:
  - if (s->reconnect_time) {
  + if (s->reconnect_time && !s->reconnect_timer) {
But was afraid of possible side effects. Since Marc-André approved your
fix, i'm also good with your approach. In this case i'll remove this
patch from the v2 patchset.

Thanks for handling it!

> 
> Thanks,
> Feng Li
> 
> Marc-André Lureau <marcandre.lureau@gmail.com> 于2020年4月24日周五 上午3:16写道：
> 
> 
> >
> > Hi
> >
> > On Thu, Apr 23, 2020 at 8:41 PM Dima Stepanov <dimastep@yandex-team.ru> wrote:
> > >
> > > During vhost-user reconnect functionality testing the following assert
> > > was hit:
> > >   qemu-system-x86_64: chardev/char-socket.c:125:
> > >   qemu_chr_socket_restart_timer: Assertion `!s->reconnect_timer' failed.
> > >   Aborted (core dumped)
> >
> > That looks related to "[PATCH 3/4] char-socket: avoid double call
> > tcp_chr_free_connection"
> >
> > > This is observed only if the connection is closed by the vhost-user-blk
> > > daemon during the initialization routine. In this case the
> > > tcp_chr_disconnect_locked() routine is called twice. First time it is
> > > called in the tcp_chr_write() routine, after getting the SIGPIPE signal.
> > > Second time it is called when vhost_user_blk_connect() routine return
> > > error. In general it looks correct, because the initialization routine
> > > can return error in many cases.
> > > The tcp_chr_disconnect_locked() routine could be fixed. The timer will
> > > be restarted only if the close event is emitted.
> > >
> > > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
> > > ---
> > >  chardev/char-socket.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > index c128cca..83ca4d9 100644
> > > --- a/chardev/char-socket.c
> > > +++ b/chardev/char-socket.c
> > > @@ -476,7 +476,7 @@ static void update_disconnected_filename(SocketChardev *s)
> > >  static void tcp_chr_disconnect_locked(Chardev *chr)
> > >  {
> > >      SocketChardev *s = SOCKET_CHARDEV(chr);
> > > -    bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED;
> > > +    bool was_connected = s->state == TCP_CHARDEV_STATE_CONNECTED;
> > >
> > >      tcp_chr_free_connection(chr);
> > >
> > > @@ -485,11 +485,11 @@ static void tcp_chr_disconnect_locked(Chardev *chr)
> > >                                                chr, NULL, chr->gcontext);
> > >      }
> > >      update_disconnected_filename(s);
> > > -    if (emit_close) {
> > > +    if (was_connected) {
> > >          qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> > > -    }
> > > -    if (s->reconnect_time) {
> > > -        qemu_chr_socket_restart_timer(chr);
> > > +        if (s->reconnect_time) {
> > > +            qemu_chr_socket_restart_timer(chr);
> > > +        }
> > >      }
> > >  }
> > >
> > > --
> > > 2.7.4
> > >
> > >
> >
> >
> > --
> > Marc-André Lureau
> 
> -- 
> The SmartX email address is only for business purpose. Any sent message 
> that is not related to the business is not authorized or permitted by 
> SmartX.
> 本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
> 
>