Re: [RFC PATCH v1 3/7] char-socket: initialize reconnect timer only if close is emitted

[Dima Stepanov]

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 <address@hidden> 于2020年4月24日周五 上午3:16写道：
> 
> 
> >
> > Hi
> >
> > On Thu, Apr 23, 2020 at 8:41 PM Dima Stepanov <address@hidden> 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 <address@hidden>
> > > ---
> > >  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）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
> 
>