On 8/13/20 11:29 AM, Kevin Wolf wrote:
We want to have a common set of commands for all types of block exports.
Currently, this is only NBD, but we're going to add more types.
This patch adds the basic BlockExport and BlockExportDriver structs and
a QMP command block-export-add that creates a new export based on the
given BlockExportOptions.
qmp_nbd_server_add() becomes a wrapper around qmp_block_export_add().
Signed-off-by: Kevin Wolf <kw...@redhat.com>
---
Seeing if I can spot anything beyond Max's fine points:
+++ b/qapi/block-export.json
@@ -170,3 +170,12 @@
'nbd': 'BlockExportOptionsNbd'
} }
+##
+# @block-export-add:
+#
+# Creates a new block export.
+#
+# Since: 5.2
+##
+{ 'command': 'block-export-add',
+ 'data': 'BlockExportOptions', 'boxed': true }
So if I read patch 3 correctly, the difference between nbd-server-add
and block-export-add is that the latter includes a "type":"nbd" member.
(If we ever play with allowing a default discriminator value in QAPI, we
could declare the default for "type" to be NBD, and then the two would
be identical - except that since you are adding a new command designed
to extend to more than just nbd, I don't think keeping nbd as default
makes sense)
+++ b/block/export/export.c
+void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
+{
+ BlockExportOptions export = {
+ .type = BLOCK_EXPORT_TYPE_NBD,
+ .u.nbd = *arg,
+ };
+ qmp_block_export_add(&export, errp);
+}
And indeed, this matches that analysis.
@@ -217,6 +220,8 @@ void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error
**errp)
out:
aio_context_release(aio_context);
+ /* TODO Remove the cast: Move to server.c which can access fields of exp */
+ return (BlockExport*) exp;
Should this use container_of()? Ah, the TODO says you want to, but
can't because exp is an opaque type in this file...
}
void qmp_nbd_server_remove(const char *name,
diff --git a/nbd/server.c b/nbd/server.c
index bee2ef8bd1..774325dbe5 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -18,6 +18,8 @@
*/
#include "qemu/osdep.h"
+
+#include "block/export.h"
#include "qapi/error.h"
#include "qemu/queue.h"
#include "trace.h"
@@ -80,6 +82,7 @@ struct NBDRequestData {
};
struct NBDExport {
+ BlockExport common;
...but at least the cast is accurate.
Overall, the idea looks sane. I'm happy if you want to move
blockdev-nbd.c out of the top level.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
