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

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Wed, Sep 30, 2020 at 04:33:18PM +0200, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>> 
>> > 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.
>> 
>> Yes, but if introspection could tell us which which values of addr.type
>> are valid, then a client should figure out the address families, as
>> follows.  Any valid value other than 'fd' corresponds to an address
>> family.  The set of values valid for addr.type therefore corresponds to
>> a set of address families.  The address families in that set are all
>> valid with 'fd', aren't they?
>
> Yes, in this case the address families in addr.type are the ones also
> supported by addr.type=fd.
>
>> > 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.
>> 
>> Awkward.  These types would have to differ structurally, or else they
>> are indistinguishable in introspection.
>>
>> > 2. Use a side-channel (query-features, I think we discussed something
>> >    like this a while back) to report features that cannot be
>> >    introspected.
>> 
>> We implemented this in the form of QAPI feature flags, visible in
>> introspection.  You could do something like
>> 
>>   'addr': { 'type': 'SocketAddress',
>>             'features': [ 'unix' ] }
>
> That is nice.
>
>> 
>> > 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.
>> 
>> Hence my question: is it possible that address families other than unix
>> become available here?
>> 
>> When that happens, we have an introspection problem of the sort we
>> common solve with a feature flag.
>> 
>> > 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.
>> 
>> Your answer appears to be "yes in theory, quite unlikely in practice".
>> Correct?

Keeping introspection "tight" would be nice, but since a real need for
"tight" here seems quite unlikely, it doesn't seem to be worth the
trouble.

Perhaps this argument could be worked into the commit message.  Up to
you.

Acked-by: Markus Armbruster <armbru@redhat.com>