Re: [RFC PATCH 08/18] qemu-storage-daemon: Add --export option
Kevin Wolf writes: > Am 06.11.2019 um 14:11 hat Max Reitz geschrieben: >> On 17.10.19 15:01, Kevin Wolf wrote: >> > Add a --export option to qemu-storage-daemon to export a block node. For >> > now, only NBD exports are implemented. Apart from the 'type' option >> > (which is the implied key), it maps the arguments for nbd-server-add to >> > the command line. Example: >> > >> > --export nbd,device=disk,name=test-export,writable=on >> > >> > Signed-off-by: Kevin Wolf >> > --- >> > qapi/block.json | 27 +++ >> > qemu-storage-daemon.c | 31 +++ >> > 2 files changed, 58 insertions(+) >> >> Would it be better to collect the BlockExports in a list and work on it >> after all arguments have been parsed? As it is, it’s important that >> users define block devices and things like NBD servers before --export. >> Yes, I know, that’s exactly how it works with qemu, but is that really >> the best way? > > It's actually not how QEMU works generally. QEMU collects things in > QemuOptsLists and then tries to interpret them in the right order. Of > course, we never get the order actually right, which results in constant > reshuffling of the order of initialisations in vl.c. > > It also means that vl.c (!) has a list of -object types that need to be > created early so that other backends can make use of them, and of those > types that actually depend on a backend already being present (see > object_create_initial() for details). > > I think it's much cleaner to simply use the order in the command line > instead of adding magic that tries to resolve (and fails at actually > resolving) all the dependencies. Seconded. The "process arguments strictly left to right" strategy is also visible in PATCH 02 and 05. > I seem to remember that this was in > fact one of the things Markus keeps mentioning he would change if he > were to rewrite the QEMU command line parser from scratch without > compatibility requirements. E.g. Message-ID: <87poomg77m@dusky.pond.sub.org> https://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg00163.html
Re: [RFC PATCH 08/18] qemu-storage-daemon: Add --export option
On 06.11.19 14:34, Kevin Wolf wrote: > Am 06.11.2019 um 14:11 hat Max Reitz geschrieben: >> On 17.10.19 15:01, Kevin Wolf wrote: >>> Add a --export option to qemu-storage-daemon to export a block node. For >>> now, only NBD exports are implemented. Apart from the 'type' option >>> (which is the implied key), it maps the arguments for nbd-server-add to >>> the command line. Example: >>> >>> --export nbd,device=disk,name=test-export,writable=on >>> >>> Signed-off-by: Kevin Wolf >>> --- >>> qapi/block.json | 27 +++ >>> qemu-storage-daemon.c | 31 +++ >>> 2 files changed, 58 insertions(+) >> >> Would it be better to collect the BlockExports in a list and work on it >> after all arguments have been parsed? As it is, it’s important that >> users define block devices and things like NBD servers before --export. >> Yes, I know, that’s exactly how it works with qemu, but is that really >> the best way? > > It's actually not how QEMU works generally. QEMU collects things in > QemuOptsLists and then tries to interpret them in the right order. Of > course, we never get the order actually right, which results in constant > reshuffling of the order of initialisations in vl.c. > > It also means that vl.c (!) has a list of -object types that need to be > created early so that other backends can make use of them, and of those > types that actually depend on a backend already being present (see > object_create_initial() for details). > > I think it's much cleaner to simply use the order in the command line > instead of adding magic that tries to resolve (and fails at actually > resolving) all the dependencies. I seem to remember that this was in > fact one of the things Markus keeps mentioning he would change if he > were to rewrite the QEMU command line parser from scratch without > compatibility requirements. OK. Max signature.asc Description: OpenPGP digital signature
Re: [RFC PATCH 08/18] qemu-storage-daemon: Add --export option
Am 06.11.2019 um 14:11 hat Max Reitz geschrieben: > On 17.10.19 15:01, Kevin Wolf wrote: > > Add a --export option to qemu-storage-daemon to export a block node. For > > now, only NBD exports are implemented. Apart from the 'type' option > > (which is the implied key), it maps the arguments for nbd-server-add to > > the command line. Example: > > > > --export nbd,device=disk,name=test-export,writable=on > > > > Signed-off-by: Kevin Wolf > > --- > > qapi/block.json | 27 +++ > > qemu-storage-daemon.c | 31 +++ > > 2 files changed, 58 insertions(+) > > Would it be better to collect the BlockExports in a list and work on it > after all arguments have been parsed? As it is, it’s important that > users define block devices and things like NBD servers before --export. > Yes, I know, that’s exactly how it works with qemu, but is that really > the best way? It's actually not how QEMU works generally. QEMU collects things in QemuOptsLists and then tries to interpret them in the right order. Of course, we never get the order actually right, which results in constant reshuffling of the order of initialisations in vl.c. It also means that vl.c (!) has a list of -object types that need to be created early so that other backends can make use of them, and of those types that actually depend on a backend already being present (see object_create_initial() for details). I think it's much cleaner to simply use the order in the command line instead of adding magic that tries to resolve (and fails at actually resolving) all the dependencies. I seem to remember that this was in fact one of the things Markus keeps mentioning he would change if he were to rewrite the QEMU command line parser from scratch without compatibility requirements. Kevin signature.asc Description: PGP signature
Re: [RFC PATCH 08/18] qemu-storage-daemon: Add --export option
On 17.10.19 15:01, Kevin Wolf wrote: > Add a --export option to qemu-storage-daemon to export a block node. For > now, only NBD exports are implemented. Apart from the 'type' option > (which is the implied key), it maps the arguments for nbd-server-add to > the command line. Example: > > --export nbd,device=disk,name=test-export,writable=on > > Signed-off-by: Kevin Wolf > --- > qapi/block.json | 27 +++ > qemu-storage-daemon.c | 31 +++ > 2 files changed, 58 insertions(+) Would it be better to collect the BlockExports in a list and work on it after all arguments have been parsed? As it is, it’s important that users define block devices and things like NBD servers before --export. Yes, I know, that’s exactly how it works with qemu, but is that really the best way? Max signature.asc Description: OpenPGP digital signature
[RFC PATCH 08/18] qemu-storage-daemon: Add --export option
Add a --export option to qemu-storage-daemon to export a block node. For
now, only NBD exports are implemented. Apart from the 'type' option
(which is the implied key), it maps the arguments for nbd-server-add to
the command line. Example:
--export nbd,device=disk,name=test-export,writable=on
Signed-off-by: Kevin Wolf
---
qapi/block.json | 27 +++
qemu-storage-daemon.c | 31 +++
2 files changed, 58 insertions(+)
diff --git a/qapi/block.json b/qapi/block.json
index 567f116875..d9b1f16fbf 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -481,3 +481,30 @@
{ 'event': 'QUORUM_REPORT_BAD',
'data': { 'type': 'QuorumOpType', '*error': 'str', 'node-name': 'str',
'sector-num': 'int', 'sectors-count': 'int' } }
+
+##
+# @BlockExportType:
+#
+# An enumeration of block export types
+#
+# @nbd: NBD export
+#
+# Since: 4.2
+##
+{ 'enum': 'BlockExportType',
+ 'data': [ 'nbd' ] }
+
+##
+# @BlockExport:
+#
+# Describes a block export, i.e. how single node should be exported on an
+# external interface.
+#
+# Since: 4.2
+##
+{ 'union': 'BlockExport',
+ 'base': { 'type': 'BlockExportType' },
+ 'discriminator': 'type',
+ 'data': {
+ 'nbd': 'BlockExportNbd'
+ } }
diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
index 51882452f3..9e5f474fd0 100644
--- a/qemu-storage-daemon.c
+++ b/qemu-storage-daemon.c
@@ -67,6 +67,11 @@ static void help(void)
" [,driver specific parameters...]\n"
" configure a block backend\n"
"\n"
+" --export [type=]nbd,device=[,name=]\n"
+" [,writable=on|off][,bitmap=]\n"
+" export the specified block node over NBD\n"
+" (requires --nbd-server)\n"
+"\n"
" --nbd-server addr.type=inet,addr.host=,addr.port=\n"
" [,tls-creds=][,tls-authz=]\n"
" --nbd-server addr.type=unix,addr.path=\n"
@@ -84,6 +89,7 @@ enum {
OPTION_OBJECT = 256,
OPTION_BLOCKDEV,
OPTION_NBD_SERVER,
+OPTION_EXPORT,
};
static QemuOptsList qemu_object_opts = {
@@ -95,6 +101,17 @@ static QemuOptsList qemu_object_opts = {
},
};
+static void init_export(BlockExport *export, Error **errp)
+{
+switch (export->type) {
+case BLOCK_EXPORT_TYPE_NBD:
+qmp_nbd_server_add(>u.nbd, errp);
+break;
+default:
+g_assert_not_reached();
+}
+}
+
static int process_options(int argc, char *argv[], Error **errp)
{
int c;
@@ -106,6 +123,7 @@ static int process_options(int argc, char *argv[], Error
**errp)
{"object", required_argument, 0, OPTION_OBJECT},
{"blockdev", required_argument, 0, OPTION_BLOCKDEV},
{"nbd-server", required_argument, 0, OPTION_NBD_SERVER},
+{"export", required_argument, 0, OPTION_EXPORT},
{"version", no_argument, 0, 'V'},
{"trace", required_argument, NULL, 'T'},
{0, 0, 0, 0}
@@ -176,6 +194,19 @@ static int process_options(int argc, char *argv[], Error
**errp)
qapi_free_NbdServerOptions(options);
break;
}
+case OPTION_EXPORT:
+{
+Visitor *v;
+BlockExport *export;
+
+v = qobject_input_visitor_new_str(optarg, "type",
_fatal);
+visit_type_BlockExport(v, NULL, , _fatal);
+visit_free(v);
+
+init_export(export, _fatal);
+qapi_free_BlockExport(export);
+break;
+}
}
}
if (optind != argc) {
--
2.20.1
