[RFC,08/18] qemu-storage-daemon: Add --export option
diff mbox series

Message ID 20191017130204.16131-9-kwolf@redhat.com
State New
Headers show
Series
  • Add qemu-storage-daemon
Related show

Commit Message

Kevin Wolf Oct. 17, 2019, 1:01 p.m. UTC
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 <kwolf@redhat.com>
---
 qapi/block.json       | 27 +++++++++++++++++++++++++++
 qemu-storage-daemon.c | 31 +++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

Comments

Max Reitz Nov. 6, 2019, 1:11 p.m. UTC | #1
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 <kwolf@redhat.com>
> ---
>  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
Kevin Wolf Nov. 6, 2019, 1:34 p.m. UTC | #2
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 <kwolf@redhat.com>
> > ---
> >  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
Max Reitz Nov. 6, 2019, 1:39 p.m. UTC | #3
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 <kwolf@redhat.com>
>>> ---
>>>  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
Markus Armbruster Nov. 8, 2019, 3:57 p.m. UTC | #4
Kevin Wolf <kwolf@redhat.com> 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 <kwolf@redhat.com>
>> > ---
>> >  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.fsf@dusky.pond.sub.org>
https://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg00163.html

Patch
diff mbox series

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=<node-name>[,name=<export-name>]\n"
+"           [,writable=on|off][,bitmap=<name>]\n"
+"                         export the specified block node over NBD\n"
+"                         (requires --nbd-server)\n"
+"\n"
 "  --nbd-server addr.type=inet,addr.host=<host>,addr.port=<port>\n"
 "               [,tls-creds=<id>][,tls-authz=<id>]\n"
 "  --nbd-server addr.type=unix,addr.path=<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(&export->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", &error_fatal);
+                visit_type_BlockExport(v, NULL, &export, &error_fatal);
+                visit_free(v);
+
+                init_export(export, &error_fatal);
+                qapi_free_BlockExport(export);
+                break;
+            }
         }
     }
     if (optind != argc) {