[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 11/13] block/export: convert vhost-user-blk server to bloc
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API |
Date: |
Wed, 30 Sep 2020 16:33:18 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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?
> 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' ] }
> 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?
- [PATCH v2 04/13] util/vhost-user-server: drop unnecessary watch deletion, (continued)
- [PATCH v2 04/13] util/vhost-user-server: drop unnecessary watch deletion, Stefan Hajnoczi, 2020/09/24
- [PATCH v2 05/13] block/export: consolidate request structs into VuBlockReq, Stefan Hajnoczi, 2020/09/24
- [PATCH v2 06/13] util/vhost-user-server: drop unused DevicePanicNotifier, Stefan Hajnoczi, 2020/09/24
- [PATCH v2 07/13] util/vhost-user-server: fix memory leak in vu_message_read(), Stefan Hajnoczi, 2020/09/24
- [PATCH v2 08/13] util/vhost-user-server: check EOF when reading payload, Stefan Hajnoczi, 2020/09/24
- [PATCH v2 09/13] util/vhost-user-server: rework vu_client_trip() coroutine lifecycle, Stefan Hajnoczi, 2020/09/24
- [PATCH v2 10/13] block/export: report flush errors, Stefan Hajnoczi, 2020/09/24
- [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API, Stefan Hajnoczi, 2020/09/24
- [PATCH v2 13/13] util/vhost-user-server: use static library in meson.build, Stefan Hajnoczi, 2020/09/24
- [PATCH v2 12/13] util/vhost-user-server: move header to include/, Stefan Hajnoczi, 2020/09/24