Re: [Qemu-devel] [PATCH v2] vhost-user: define conventions for vhost-user backends

[Daniel P . Berrangé]

On Tue, Oct 16, 2018 at 02:47:12PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Oct 12, 2018 at 1:56 PM Daniel P. Berrangé <address@hidden> wrote:
> >
> > On Fri, Oct 12, 2018 at 01:43:39PM +0400, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Thu, Oct 11, 2018 at 7:49 PM Daniel P. Berrangé <address@hidden> wrote:
> > > >
> > > > Adding Markus since we're talking about new CLI argument and capability
> > > > reporting standards.
> > > >
> > > > On Fri, Sep 14, 2018 at 05:52:30PM +0400, Marc-André Lureau wrote:
> > > > > +Backend program conventions
> > > > > +---------------------------
> > > > > +
> > > > > +vhost-user backends provide various services and they may need to be
> > > > > +configured manually depending on the use case. However, it is a good
> > > > > +idea to follow the conventions listed here when possible. Users, QEMU
> > > > > +or libvirt, can then rely on some common behaviour to avoid
> > > > > +heterogenous configuration and management of the backend program and
> > > > > +facilitate interoperability.
> > > > > +
> > > > > +In order to be discoverable, default vhost-user backends should be
> > > > > +located under "/usr/libexec", and be named "vhost-user-$device" where
> > > > > +"$device" is the device name in lower-case following the name listed
> > > > > +in the Linux virtio_ids.h header (ex: the VIRTIO_ID_RPROC_SERIAL
> > > > > +backend would be named "vhost-user-rproc-serial").
> > > > > +
> > > > > +Mechanisms to list, and to select among alternatives implementations
> > > > > +or modify the default backend are not described at this point (a
> > > > > +distribution may use update-alternatives, for example, to list and to
> > > > > +pick a different default backend).
> > > >
> > > > I don't think that update-alternatives is a good thing as it presumes
> > > > that each host only needs a single preferred impl at a time.
> > > >
> > > > I think we need to be able to discover all impls for a given device
> > > > type.
> > >
> > > That was left for a future improvement. I don't think it's a good idea
> > > to tackle problems we don't have yet.
> >
> > I think the need to support multiple alternatives will arrive pretty
> > much immediately, as it has already been raised in previous threads
> > about vhost-user. It is counterproductive to implement something we
> 
> At this point, there are no alternative implementation of the
> vhost-user-gpu or vhost-user-input backends (and no other device has
> been implemented to follow this spec).

We can't assume that will remain true. The very notion of the
capabilities reporting for optional features indicates that
we're expecting alternate impls with varying feature sets.

> > know is not satisfactory when we can easily provide an approach
> > which is extensible for no significant extra work upfront. Having
> > it aligned with the approach we do for firmware makes it even more
> > compelling, as we get a consistent story across QEMU instead of
> > two completely different approaches for the same concept.
> 
> That's not incompatible with doing it as a future improvement.

It is different from the POV of libvirt, as it implies two different
ways to identifying which is the preferred / default vhost. I don't
see a compelling reason to provide an initial impl that is intentionally
not flexible enough to cope with future needs.

> > > > > +The backend program must not daemonize itself, but it may be
> > > > > +daemonized by the management layer. It may also have a restricted
> > > > > +access to the system.
> > > > > +
> > > > > +File descriptors 0, 1 and 2 will exist, and have regular
> > > > > +stdin/stdout/stderr usage (they may be redirected to /dev/null by the
> > > > > +management layer, or to a log handler).
> > > > > +
> > > > > +The backend program must end (as quickly and cleanly as possible) 
> > > > > when
> > > > > +the SIGTERM signal is received. Eventually, it may be SIGKILL by the
> > > > > +management layer after a few seconds.
> > > > > +
> > > > > +The following command line options have an expected behaviour. They
> > > > > +are mandatory, unless explicitly said differently:
> > > > > +
> > > > > +* --socket-path=PATH
> > > > > +
> > > > > +This option specify the location of the vhost-user Unix domain 
> > > > > socket.
> > > > > +It is incompatible with --fd.
> > > > > +
> > > > > +* --fd=FDNUM
> > > > > +
> > > > > +When this argument is given, the backend program is started with the
> > > > > +vhost-user socket as file descriptor FDNUM. It is incompatible with
> > > > > +--socket-path.
> > > > > +
> > > > > +* --print-capabilities
> > > > > +
> > > > > +Output to stdout a line-seperated list of backend capabilities, and
> > > > > +then exit successfully. Other options and arguments should be 
> > > > > ignored,
> > > > > +and the backend program should not perform its normal function.
> > > >
> > > > This is going to repeat the mistakes we've had with every other
> > > > binary in QEMU. A "simple" flag list or args sounds appealing,
> > > > but we've always been burnt by it in the medium-long term, which
> > > > is why we created QAPI.
> > >
> > > isn't libvirt using a list of strings/symbols as well for the
> > > capabilities? To me it sounds fairly easy to extend this way.
> > >
> > > > If we're doing to have any capabilities reporting, we should
> > > > model it in QAPI schema, so any '--print-capabilities' arg
> > > > should print a JSON doc following the documented schema.
> > >
> > > perhaps we could have --print-json-capabilities later, if needed.
> >
> > A QAPI schema can still start off as reporting a simple list of
> > features flags.
> > The key point is to provide a data format that is extensible
> > right from the start. For QEMU that means QAPI, instead of
> > inventing yet another custom data format, that we'd have to
> > fix later when we find a flat list of strings is insufficient.
> 
> 
> Changing --print-capabilites from:
> feature-a
> feature-b
> 
> to a json form:
> {
>   "features": [
>     "feature-a",
>     "feature-b"
>   ]
> }
> 
> Would that be extensible enough?

As long as the JSON format is associated with a QAPI schema so we
have a clear specification that we're following as illustrated in
this example

  https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02459.html

> > > > While talking about QAPI, I think this is an opportunity to
> > > > also avoid the problems of CLI arg values becoming more
> > > > complex than just scalars. eg
> > > >
> > > >   --socket-path=PATH
> > > >
> > > > may inevitably grow more options  - eg to perhaps say whether
> > > > to use it in listen or connect mode. Or to indicate a reconnect
> > > > timeout. etc
> > >
> > > Yes, that would be new capabilities symbols. That doesn't require json to 
> > > me.
> >
> > It just reinvents the chardev unix socket syntax, but in a
> > different adhoc manner, which is repeating the mistake we have
> > made time & again in QEMU. Using QAPI we can directly accept
> > the ChardevSocket syntax we already know about. Instead of
> > having --socket-path and --fd and documenting that they are
> > mutually exclusive ChardevSocket QAPI schema provides that
> > representation in a well defined format which is discoverable
> > and QEMU and mgmt apps already understand.
> 
> That would require external vhost-user backends to implement QAPI/json
> parsing. Is this necessary for a vhost-user backend? I doubt it.

They could, but would not be required, to implement QAPI/json parser.

The QAPI schema defines a standard for how to model & interpret the
non-scalar values for command line arguments. An external impl would
need to ensure that whatever parsing it does for CLI args is semantically
compatible with the parsing rules defined by the QEMU QAPI schema we
define to ensure interoperability of its impl.

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 :|