Hi On Fri, Feb 14, 2020 at 2:24 PM Boeuf, Sebastien <sebastien.bo...@intel.com> wrote: > > Hi Marc-Andre, > > On Tue, 2020-02-11 at 22:24 +0100, Marc-André Lureau wrote: > > Hi > > > > On Tue, Feb 11, 2020 at 4:24 PM Boeuf, Sebastien > > <sebastien.bo...@intel.com> wrote: > > > From c073d528b8cd7082832fd1825dc33dd65b305aa2 Mon Sep 17 00:00:00 > > > 2001 > > > From: Sebastien Boeuf <sebastien.bo...@intel.com> > > > Date: Tue, 11 Feb 2020 16:01:22 +0100 > > > Subject: [PATCH] docs: Update vhost-user spec regarding backend > > > program > > > conventions > > > > > > The vhost-user specification is not clearly stating the expected > > > behavior from a backend program whenever the client disconnects. > > > > > > This patch addresses the issue by defining the default behavior and > > > proposing an alternative through a command line option. > > > > > > By default, a backend program will have to keep listening even if > > > the > > > client disconnects, unless told otherwise through the newly > > > introduced > > > option --exit-on-disconnect. > > > > > > Signed-off-by: Sebastien Boeuf <sebastien.bo...@intel.com> > > > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com> > > > --- > > > docs/interop/vhost-user.rst | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost- > > > user.rst > > > index 5f8b3a456b..da9a1ebc4c 100644 > > > --- a/docs/interop/vhost-user.rst > > > +++ b/docs/interop/vhost-user.rst > > > @@ -1323,6 +1323,10 @@ The backend program must end (as quickly and > > > cleanly as possible) when > > > the SIGTERM signal is received. Eventually, it may receive SIGKILL > > > by > > > the management layer after a few seconds. > > > > > > +By default, the backend program continues running after the client > > > +disconnects. It accepts only 1 connection at a time on each UNIX > > > domain > > > +socket. > > > > I don't think that's the most common behaviour. libvhost-user will > > panic() on disconnect in general, unless the error/exit is handled > > gracefully by the backend. > > It's not the default behavior from libvhost-user, but that's exactly > something I'd like to see changing. This should be the normal case if > you think about a standard client/server connection, where the server > would simply listen again after the client disconnects.
I disagree, a "normal" lifecycle is a single connection & instance per device. Having the backend handle multiple connections is needlessly more complicated. You need to correctly handle multiple states, flushed anything private between connections etc. It should be optional. > > > > The most common case is to have 1-1 relation between device/qemu > > instance and backend. > > Yes this part is fine, but that's not a reason why the backend should > terminates. It is simpler to ensure it is reset correctly. > > > > > Why not restart the backend for another instance? > > Because you need some management tool to do so. And I think that by > default it could be interesting to have the least amount of extra > management involved. The management layer should be involved if any side crashes or restart anyway. > > > > > > + > > > The following command line options have an expected behaviour. > > > They > > > are mandatory, unless explicitly said differently: > > > > > > @@ -1337,6 +1341,12 @@ are mandatory, unless explicitly said > > > differently: > > > vhost-user socket as file descriptor FDNUM. It is incompatible > > > with > > > --socket-path. > > > > > > +--exit-on-disconnect > > > + > > > + When this option is provided, the backend program must terminate > > > when > > > + the client disconnects. This can be used to keep the backend > > > program's > > > + lifetime synchronized with its client process. > > > > This section list options that are mandatory. It's probably a bit > > late > > to add more mandatory options (I regret already some of them) > > The spec states "They are mandatory, unless explicitly said > differently", and in this case I'm explicitely saying "When this option > is provided", which means if it's not provided it's fine and we can > ignore the fact it's not there. Ah ok, I think we can guard it with a capability too. > > > > > Do we need to specify the behaviour on client disconnect? Can't we > > leave that to the backend and management layer to decide? > > My goal here is to make the spec a bit less loose. I know libvhost-user > is the de-facto implementation but we cannot just assume everything out > of the libvhost-user implementation, especially since there is a > dedicated spec. That's the reason why I thought it'd be nice to have > the backend behavior well defined in the spec. Sure, the goal of the spec is to have basic interoperability between implementations, not to follow whatever libvhost-user is currently doing. > The point is, relying on the current definition, there's not enough > information to make sure a VMM will properly interface with a vhost- > user backend. I don't know why having the backend handle multiple connections would help with that. Having undefined behaviour for things that should not happen in normal circumstances seems acceptable. Having QEMU (or the /master/) restart is currently undefined, because it's not considered a simple/normal situation: the vhost-user spec doesn't say much about it, does it? I'd prefer to keep things simple and have 1-1 device-backend instance relationship by default.
