Re: [Qemu-devel] [PATCH 00/24] vhost-user reconnect fixes

[Michael S. Tsirkin]

On Tue, Jun 21, 2016 at 12:02:28PM +0200, address@hidden wrote:
> From: Marc-André Lureau <address@hidden>
> 
> Hi,
> 
> Since 'vhost-user: simple reconnection support' was merged, it is
> possible to disconnect and reconnect a vhost-user backend. However,
> many code paths in qemu may trigger assert() when the backend is
> disconnected.
> 
> Some assert() could simply be replaced by error_report() or silently
> fail since they are recoverable cases. Some missing error checks can
> also help prevent later issues. In many cases, the code assumes
> get_vhost_net() will be non-NULL after a succesful connection, so I
> changed it to stay after a disconnect until the new connection comes
> (as suggested by Michael). There are also code paths that are wrong,
> see "don't assume opaque is a fd" patch for an example.

OK so if it's ok for read or write to fail, then I think
the callback should just return success.
This will make sure backends such as vhost net in kernel
where failure indicates an internal bug are handled
correctly, while vhost user ignores errors and attempts
to go on.

> Since there is feature checks on reconnection, qemu should wait for
> the initial connection feature negotiation to complete. The test added
> demonstrates this.


So on disconnect, we keep the guest going? But then when connect is
attempted, then we for some reason block guest until negotiation
is done? Sounds rather strange to me.

> For convenience, the series is also available on:
> https://github.com/elmarco/qemu, branch vhost-user-reconnect
> 
> Marc-André Lureau (24):
>   misc: indentation
>   vhost-user: minor simplification
>   qemu-char: check socket is actually connected
>   vhost-user: check qemu_chr_fe_set_msgfds() return value
>   vhost: change some assert() for error_report() or silent fail
>   vhost-user: check vhost_user_write() return value
>   vhost: use error_report() instead of fprintf(stderr,...)
>   vhost-user: return a read error
>   vhost: make vhost_log_put() idempotent
>   vhost: call vhost_log_put() on cleanup
>   vhost: add vhost device only after all success
>   vhost: make vhost_dev_cleanup() idempotent
>   vhost-net: always call vhost_dev_cleanup() on failure
>   vhost: don't assume opaque is a fd, use backend cleanup
>   vhost: fix calling vhost_dev_cleanup() after vhost_dev_init()
>   vhost-user: keep vhost_net after a disconnection
>   Revert "vhost-net: do not crash if backend is not present"
>   get_vhost_net() should be != null after vhost_user_init
>   vhost-net: success if backend has no ops->vhost_migration_done
>   vhost: add assert() to check runtime behaviour
>   char: add chr_wait_connected callback
>   char: add and use tcp_chr_wait_connected
>   vhost-user: wait until link is up
>   tests: add /vhost-user/connect-fail test
> 
>  hw/net/vhost_net.c      |  19 +++-----
>  hw/virtio/vhost-user.c  |  54 +++++++++++++++--------
>  hw/virtio/vhost.c       | 114 
> +++++++++++++++++++++++++++---------------------
>  include/sysemu/char.h   |   8 ++++
>  net/tap.c               |   1 +
>  net/vhost-user.c        |  36 ++++++++-------
>  qemu-char.c             |  82 ++++++++++++++++++++++++----------
>  tests/vhost-user-test.c |  37 ++++++++++++++++
>  8 files changed, 233 insertions(+), 118 deletions(-)
> 
> -- 
> 2.7.4