Re: [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API

[Stefan Hajnoczi]

On Wed, Sep 30, 2020 at 07:28:58AM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > Use the new QAPI block exports API instead of defining our own QOM
> > objects.
> >
> > This is a large change because the lifecycle of VuBlockDev needs to
> > follow BlockExportDriver. QOM properties are replaced by QAPI options
> > objects.
> >
> > VuBlockDev is renamed VuBlkExport and contains a BlockExport field.
> > Several fields can be dropped since BlockExport already has equivalents.
> >
> > The file names and meson build integration will be adjusted in a future
> > patch. libvhost-user should probably be built as a static library that
> > is linked into QEMU instead of as a .c file that results in duplicate
> > compilation.
> >
> > The new command-line syntax is:
> >
> >   $ qemu-storage-daemon \
> >       --blockdev file,node-name=drive0,filename=test.img \
> >       --export 
> > vhost-user-blk,node-name=drive0,id=export0,unix-socket=/tmp/vhost-user-blk.sock
> >
> > Note that unix-socket is optional because we may wish to accept chardevs
> > too in the future.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > v2:
> >  * Replace str unix-socket with SocketAddress addr to match NBD and
> >    support file descriptor passing
> >  * Make addr mandatory [Markus]
> >  * Update vhost-user-blk-test.c to use --export syntax
> > ---
> >  qapi/block-export.json               |  21 +-
> >  block/export/vhost-user-blk-server.h |  23 +-
> >  block/export/export.c                |   8 +-
> >  block/export/vhost-user-blk-server.c | 452 +++++++--------------------
> >  tests/qtest/vhost-user-blk-test.c    |   2 +-
> >  util/vhost-user-server.c             |  10 +-
> >  block/export/meson.build             |   1 +
> >  block/meson.build                    |   1 -
> >  8 files changed, 158 insertions(+), 360 deletions(-)
> >
> > diff --git a/qapi/block-export.json b/qapi/block-export.json
> > index ace0d66e17..2e44625bb1 100644
> > --- a/qapi/block-export.json
> > +++ b/qapi/block-export.json
> > @@ -84,6 +84,21 @@
> >    'data': { '*name': 'str', '*description': 'str',
> >              '*bitmap': 'str' } }
> >  
> > +##
> > +# @BlockExportOptionsVhostUserBlk:
> > +#
> > +# A vhost-user-blk block export.
> > +#
> > +# @addr: The vhost-user socket on which to listen. Both 'unix' and 'fd'
> > +#        SocketAddress types are supported. Passed fds must be UNIX domain
> > +#        sockets.
> 
> "addr.type must be 'unix' or 'fd'" is not visible in introspection.
> Awkward.  Practical problem only if other addresses ever become
> available here.  Is that possible?

addr.type=fd itself has the same problem, because it is a file
descriptor without type information. Therefore the QMP client cannot
introspect which types of file descriptors can be passed.

Two ideas:

1. Introduce per-address family fd types (SocketAddrFdTcpInet,
   SocketAddrFdTcpInet6, SocketAddrFdUnix, etc) to express specific
   address families in the QAPI schema.

   Then use per-command unions to express the address families supported
   by specific commands. For example,
   BlockExportOptionsVhostUserBlkSocketAddr would only allow
   SocketAddrUnix and SocketAddrFdUnix. That way new address families
   can be supported in the future and introspection reports.

2. Use a side-channel (query-features, I think we discussed something
   like this a while back) to report features that cannot be
   introspected.

I think the added complexity for achieving full introspection is not
worth it. It becomes harder to define new QAPI commands, increases the
chance of errors, and is more tedious to program for clients/servers.

Accepting any SocketAddr seems reasonable to me since vhost-user
requires an address family that has file descriptor passing. Very few
address families support this feature and we don't expect to add new
ones often.

Stefan

signature.asc
Description: PGP signature