[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 00/16] chardev: refactoring & many bugfixes r
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH v2 00/16] chardev: refactoring & many bugfixes related tcp_chr_wait_connected |
Date: |
Wed, 6 Feb 2019 14:45:22 +0100 |
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>
> --
> 2.20.1
>
- Re: [Qemu-devel] [PATCH v2 00/16] chardev: refactoring & many bugfixes related tcp_chr_wait_connected,
Marc-André Lureau <=