Re: docs: Update vhost-user spec regarding backend program conventions

[Marc-André Lureau]

Hi

On Fri, Feb 14, 2020 at 2:24 PM Boeuf, Sebastien
<address@hidden> 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
> > <address@hidden> wrote:
> > > From c073d528b8cd7082832fd1825dc33dd65b305aa2 Mon Sep 17 00:00:00
> > > 2001
> > > From: Sebastien Boeuf <address@hidden>
> > > 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 <address@hidden>
> > > Signed-off-by: Stefan Hajnoczi <address@hidden>
> > > ---
> > >  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.