[PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API
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
---
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.
+# @logical-block-size: Logical block size in bytes. Defaults to 512 bytes.
+#
+# Since: 5.2
+##
+{ 'struct': 'BlockExportOptionsVhostUserBlk',
+ 'data': { 'addr': 'SocketAddress', '*logical-block-size': 'size' } }
+
##
# @NbdServerAddOptions:
#
@@ -180,11 +195,12 @@
# An enumeration of block export types
#
# @nbd: NBD export
+# @vhost-user-blk: vhost-user-blk export (since 5.2)
#
# Since: 4.2
##
{ 'enum': 'BlockExportType',
- 'data': [ 'nbd' ] }
+ 'data': [ 'nbd', 'vhost-user-blk' ] }
##
# @BlockExportOptions:
@@ -213,7 +229,8 @@
'*writethrough': 'bool' },
'discriminator': 'type',
'data': {
- 'nbd': 'BlockExportOptionsNbd'
+ 'nbd': 'BlockExportOptionsNbd',
+ 'vhost-user-blk': 'BlockExportOptionsVhostUserBlk'
} }
##
diff --git a/block/export/vhost-user-blk-server.h
b/block/export/vhost-user-blk-server.h
index f06f37c4c8..fcf46fc8a5 100644
--- a/block/export/vhost-user-blk-server.h
+++ b/block/export/vhost-user-blk-server.h
@@ -10,27 +10,10 @@
#ifndef VHOST_USER_BLK_SERVER_H
#define VHOST_USER_BLK_SERVER_H
-#include "util/vhost-user-server.h"
-typedef struct VuBlockDev VuBlockDev;
-#define TYPE_VHOST_USER_BLK_SERVER "vhost-user-blk-server"
-#define VHOST_USER_BLK_SERVER(obj) \
- OBJECT_CHECK(VuBlockDev, obj, TYPE_VHOST_USER_BLK_SERVER)
+#include "block/export.h"
-/* vhost user block device */
-struct VuBlockDev {
-Object parent_obj;
-char *node_name;
-SocketAddress *addr;
-AioContext *ctx;
-VuServer vu_server;
-bool running;
-uint32_t blk_size;
-BlockBackend *backend;
-QIOChannelSocket *sioc;
-QTAILQ_ENTRY(VuBlockDev) next;
-struct virtio_blk_config blkcfg;
-bool writable;
-};
+/* For block/export/export.c */
+extern const BlockExportDriver blk_exp_vhost_user_blk;
#endif /* VHOST_USER_BLK_SERVER_H */
diff --git a/block/export/export.c b/block/export/export.c
index 87516d1e05..d0c7590911 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -17,6 +17,9 @@
#include "sysemu/block-backend.h"
#include "block/export.h"
#include "block/nbd.h"
+#if CONFIG_LINUX
+#include "block/export/vhost-user-blk-server.h"
+#endif
#include "qapi/error.h"
#include "qapi/qapi-commands-block-export.h"
#include "qapi/qapi-events-block-export.h"
@@ -24,6 +27,9 @@
static const BlockExportDriver *blk_exp_drivers[] = {
&blk_exp_nbd,
+#if CONFIG_LINUX
+&blk_exp_vhost_user_blk,
+#endif
};
/* Only accessed from the main thread */
@@ -123,7 +129,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error
**errp)
assert(drv->instance_size >= sizeof(BlockExport));
exp = g_malloc0(drv->instance_size);
*exp = (BlockExport) {
-.drv= &blk_exp_nbd,
+.drv= drv,
.refcount = 1,
.user_owned = true,
.id = g_strdup(export->id),
diff --git a/block/export/vh
Re: [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API
Stefan Hajnoczi 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
> ---
> 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?
> +# @logical-block-size: Logical block size in bytes. Defaults to 512 bytes.
> +#
> +# Since: 5.2
> +##
> +{ 'struct': 'BlockExportOptionsVhostUserBlk',
> + 'data': { 'addr': 'SocketAddress', '*logical-block-size': 'size' } }
> +
> ##
> # @NbdServerAddOptions:
> #
> @@ -180,11 +195,12 @@
> # An enumeration of block export types
> #
> # @nbd: NBD export
> +# @vhost-user-blk: vhost-user-blk export (since 5.2)
> #
> # Since: 4.2
> ##
> { 'enum': 'BlockExportType',
> - 'data': [ 'nbd' ] }
> + 'data': [ 'nbd', 'vhost-user-blk' ] }
>
> ##
> # @BlockExportOptions:
> @@ -213,7 +229,8 @@
> '*writethrough': 'bool' },
>'discriminator': 'type',
>'data': {
> - 'nbd': 'BlockExportOptionsNbd'
> + 'nbd': 'BlockExportOptionsNbd',
> + 'vhost-user-blk': 'BlockExportOptionsVhostUserBlk'
> } }
>
> ##
[...]
Re: [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API
On Wed, Sep 30, 2020 at 07:28:58AM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi 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
> > ---
> > 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
Re: [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API
Stefan Hajnoczi writes:
> On Wed, Sep 30, 2020 at 07:28:58AM +0200, Markus Armbruster wrote:
>> Stefan Hajnoczi 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
>> > ---
>> > 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?
Re: [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API
On Wed, Sep 30, 2020 at 04:33:18PM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi writes:
>
> > On Wed, Sep 30, 2020 at 07:28:58AM +0200, Markus Armbruster wrote:
> >> Stefan Hajnoczi 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
> >> > ---
> >> > 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 introspecti
Re: [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API
Stefan Hajnoczi writes:
> On Wed, Sep 30, 2020 at 04:33:18PM +0200, Markus Armbruster wrote:
>> Stefan Hajnoczi writes:
>>
>> > On Wed, Sep 30, 2020 at 07:28:58AM +0200, Markus Armbruster wrote:
>> >> Stefan Hajnoczi 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
>> >> > ---
>> >> > 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.
>>
Re: [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API
On Fri, Oct 02, 2020 at 07:20:48AM +0200, Markus Armbruster wrote: > Stefan Hajnoczi writes: > > On Wed, Sep 30, 2020 at 04:33:18PM +0200, Markus Armbruster wrote: > >> Stefan Hajnoczi writes: > >> > On Wed, Sep 30, 2020 at 07:28:58AM +0200, Markus Armbruster wrote: > >> >> Stefan Hajnoczi writes: > >> 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 Thanks, I will update the commit message when merging. signature.asc Description: PGP signature
