Stefan Hajnoczi <stefa...@redhat.com> writes: > On Wed, Sep 30, 2020 at 04:33:18PM +0200, Markus Armbruster wrote: >> Stefan Hajnoczi <stefa...@redhat.com> writes: >> >> > On Wed, Sep 30, 2020 at 07:28:58AM +0200, Markus Armbruster wrote: >> >> Stefan Hajnoczi <stefa...@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 <stefa...@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 <arm...@redhat.com>