Re: [Qemu-devel] [PATCH v2 00/16] chardev: refactoring & many bugfixes related tcp_chr_wait_connected

[Marc-André Lureau]

Hi

On Wed, Feb 6, 2019 at 2:56 PM Daniel P. Berrangé <address@hidden> wrote:
>
> On Wed, Feb 06, 2019 at 02:45:22PM +0100, Marc-André Lureau wrote:
> > Hi
> > On Wed, Jan 23, 2019 at 6:27 PM Daniel P. Berrangé <address@hidden> wrote:
> > >
> > > This is a followup to
> > >
> > >   v1: https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg03344.html
> > >
> > > This series comes out of a discussion between myself & Yongji Xie in:
> > >
> > >   https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg01881.html
> > >
> > > I eventually understood that the problem faced was that
> > > tcp_chr_wait_connected was racing with the background connection attempt
> > > previously started, causing two connections to be established. This
> > > broke because some vhost user servers only allow a single connection.
> > >
> > > After messing around with the code alot the final solution was in fact
> > > very easy. We simply have to delay the first background connection
> > > attempt until the main loop is running. It will then automatically
> > > turn into a no-op if tcp_chr_wait_connected has been run. This is
> > > dealt with in the last patch in this series
> > >
> > > I believe this should solve the problem Yongji Xie faced, and thus not
> > > require us to add support for "nowait" option with client sockets at
> > > all. The reconnect=1 option effectively already implements nowait
> > > semantics, and now plays nicely with tcp_chr_wait_connected.
> > >
> > > In investigating this I found various other bugs that needed fixing and
> > > identified some useful refactoring to simplify / clarify the code, hence
> > > this very long series.
> > >
> > > This series passes make check-unit & check-qtest (for x86_64 target).
> > >
> > > I did a test with tests/vhost-user-bridge too, which was in fact already
> > > broken for the same reason Yongji Xie found - it only accepts a single
> > > connection. This series fixes use of reconnect=1 with vhost-user-bridge.
> > >
> > > Changed in v2:
> > >
> > >  - Rewrite the way we synchronize in tcp_chr_wait_connected again
> > >    to cope with chardev+device hotplug scenario
> > >  - Add extensive unit test coverage
> > >  - Resolve conflicts with git master
> > >
> > > Daniel P. Berrangé (16):
> > >   io: store reference to thread information in the QIOTask struct
> > >   io: add qio_task_wait_thread to join with a background thread
> > >   chardev: fix validation of options for QMP created chardevs
> > >   chardev: forbid 'reconnect' option with server sockets
> > >   chardev: forbid 'wait' option with client sockets
> > >   chardev: remove many local variables in qemu_chr_parse_socket
> > >   chardev: ensure qemu_chr_parse_compat reports missing driver error
> > >   chardev: remove unused 'sioc' variable & cleanup paths
> > >   chardev: split tcp_chr_wait_connected into two methods
> > >   chardev: split up qmp_chardev_open_socket connection code
> > >   chardev: use a state machine for socket connection state
> > >   chardev: honour the reconnect setting in tcp_chr_wait_connected
> > >   chardev: disallow TLS/telnet/websocket with tcp_chr_wait_connected
> > >   chardev: fix race with client connections in tcp_chr_wait_connected
> > >   tests: expand coverage of socket chardev test
> > >   chardev: ensure termios is fully initialized
> > >
> > >  chardev/char-serial.c          |   2 +-
> > >  chardev/char-socket.c          | 487 ++++++++++++++++++-------
> > >  chardev/char.c                 |   2 +
> > >  include/io/task.h              |  29 +-
> > >  io/task.c                      |  88 +++--
> > >  io/trace-events                |   2 +
> > >  tests/ivshmem-test.c           |   2 +-
> > >  tests/libqtest.c               |   4 +-
> > >  tests/test-char.c              | 643 ++++++++++++++++++++++++---------
> > >  tests/test-filter-redirector.c |   4 +-
> > >  10 files changed, 928 insertions(+), 335 deletions(-)
> > >
> >
> > With the fix squashed, I am queuing this series.
> > Reviewed-by: Marc-André Lureau <address@hidden>
>
> FYI in case you didn't notice, the fix i posted for patch 1, will
> invalidate a small part of patch 2 where I had fixed the same
> problem without realizing/remembering
>
> I pushed my updated branch to if you want to compare:
>
>   https://github.com/berrange/qemu/commits/chr-sock

Thanks, I fixed the trivial conflict on rebase.
I also replaced setenv/unsetenv with glib equivalent to fix mingw build.

>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>


-- 
Marc-André Lureau