Re: [Qemu-devel] [PATCH v2 7/9] chardev: tcp: postpone async connection setup

[Peter Xu]

On Thu, Aug 16, 2018 at 08:27:40PM +0200, Marc-André Lureau wrote:
> Hi
> On Thu, Aug 16, 2018 at 7:49 PM Marc-André Lureau
> <address@hidden> wrote:
> >
> > Hi
> > On Tue, Mar 6, 2018 at 6:41 AM Peter Xu <address@hidden> wrote:
> > >
> > > This patch allows the socket chardev async connection be setup with
> > > non-default gcontext.  We do it by postponing the setup to machine done,
> > > since until then we can know which context we should run the async
> > > operation on.
> > >
> > > Reviewed-by: Paolo Bonzini <address@hidden>
> > > Signed-off-by: Peter Xu <address@hidden>
> > > ---
> > >  chardev/char-socket.c | 17 ++++++++++++++---
> > >  1 file changed, 14 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > index 1ce5adad9a..165612845a 100644
> > > --- a/chardev/char-socket.c
> > > +++ b/chardev/char-socket.c
> > > @@ -1004,9 +1004,8 @@ static void qmp_chardev_open_socket(Chardev *chr,
> > >          s->reconnect_time = reconnect;
> > >      }
> > >
> > > -    if (s->reconnect_time) {
> > > -        tcp_chr_connect_async(chr);
> > > -    } else {
> > > +    /* If reconnect_time is set, will do that in chr_machine_done. */
> > > +    if (!s->reconnect_time) {
> > >          if (s->is_listen) {
> > >              char *name;
> > >              s->listener = qio_net_listener_new();
> > > @@ -1138,6 +1137,17 @@ char_socket_get_connected(Object *obj, Error 
> > > **errp)
> > >      return s->connected;
> > >  }
> > >
> > > +static int tcp_chr_machine_done_hook(Chardev *chr)
> > > +{
> > > +    SocketChardev *s = SOCKET_CHARDEV(chr);
> > > +
> > > +    if (s->reconnect_time) {
> > > +        tcp_chr_connect_async(chr);
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> >
> > This patch broke /vhost-user/reconnect test, since it is using
> > reconnect chardev sockets under a test program, where no machine done
> > hook exist.
> >
> 
> What about chardev created after machine done?

Hmm, so binding that with machine done might be problematic...

I'm thinking whether we can do that in ->chr_update_read_handler()
instead.

Btw, is there a short answer on why we disabled the reconnection test
in vhost-user-test?  Could I just verify the problem using:

QTEST_VHOST_USER_FIXME=1 QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 
./tests/vhost-user-test

?  Or any easy way to reproduce/verify the problem?

Regards,

-- 
Peter Xu