[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: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API |
Date: |
Thu, 1 Oct 2020 16:25:15 +0100 |
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?
Yes.
Stefan
signature.asc
Description: PGP signature
- Re: [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API,
Stefan Hajnoczi <=