* [PATCH 01/29] nbd: Remove unused nbd_export_get_blockdev()
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
@ 2020-09-07 18:19 ` Kevin Wolf
2020-09-07 18:19 ` [PATCH 02/29] qapi: Create block-export module Kevin Wolf
` (28 subsequent siblings)
29 siblings, 0 replies; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
include/block/nbd.h | 2 --
nbd/server.c | 5 -----
2 files changed, 7 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 9bc3bfaeec..0451683d03 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -338,8 +338,6 @@ void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
void nbd_export_get(NBDExport *exp);
void nbd_export_put(NBDExport *exp);
-BlockBackend *nbd_export_get_blockdev(NBDExport *exp);
-
AioContext *nbd_export_aio_context(NBDExport *exp);
NBDExport *nbd_export_find(const char *name);
void nbd_export_close_all(void);
diff --git a/nbd/server.c b/nbd/server.c
index 982de67816..bd53f7baea 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1731,11 +1731,6 @@ void nbd_export_put(NBDExport *exp)
}
}
-BlockBackend *nbd_export_get_blockdev(NBDExport *exp)
-{
- return exp->blk;
-}
-
void nbd_export_close_all(void)
{
NBDExport *exp, *next;
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 02/29] qapi: Create block-export module
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
2020-09-07 18:19 ` [PATCH 01/29] nbd: Remove unused nbd_export_get_blockdev() Kevin Wolf
@ 2020-09-07 18:19 ` Kevin Wolf
2020-09-07 18:19 ` [PATCH 03/29] qapi: Rename BlockExport to BlockExportOptions Kevin Wolf
` (27 subsequent siblings)
29 siblings, 0 replies; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Move all block export related types and commands from block-core to the
new QAPI module block-export.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qapi/block-core.json | 166 -------------------------
qapi/block-export.json | 175 +++++++++++++++++++++++++++
qapi/qapi-schema.json | 1 +
include/block/nbd.h | 2 +-
block/monitor/block-hmp-cmds.c | 1 +
blockdev-nbd.c | 2 +-
storage-daemon/qemu-storage-daemon.c | 2 +-
qapi/meson.build | 4 +-
storage-daemon/qapi/qapi-schema.json | 1 +
9 files changed, 184 insertions(+), 170 deletions(-)
create mode 100644 qapi/block-export.json
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0345f6f2d2..41afd89a43 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5192,172 +5192,6 @@
'iothread': 'StrOrNull',
'*force': 'bool' } }
-##
-# @NbdServerOptions:
-#
-# @addr: Address on which to listen.
-# @tls-creds: ID of the TLS credentials object (since 2.6).
-# @tls-authz: ID of the QAuthZ authorization object used to validate
-# the client's x509 distinguished name. This object is
-# is only resolved at time of use, so can be deleted and
-# recreated on the fly while the NBD server is active.
-# If missing, it will default to denying access (since 4.0).
-#
-# Keep this type consistent with the nbd-server-start arguments. The only
-# intended difference is using SocketAddress instead of SocketAddressLegacy.
-#
-# Since: 4.2
-##
-{ 'struct': 'NbdServerOptions',
- 'data': { 'addr': 'SocketAddress',
- '*tls-creds': 'str',
- '*tls-authz': 'str'} }
-
-##
-# @nbd-server-start:
-#
-# Start an NBD server listening on the given host and port. Block
-# devices can then be exported using @nbd-server-add. The NBD
-# server will present them as named exports; for example, another
-# QEMU instance could refer to them as "nbd:HOST:PORT:exportname=NAME".
-#
-# @addr: Address on which to listen.
-# @tls-creds: ID of the TLS credentials object (since 2.6).
-# @tls-authz: ID of the QAuthZ authorization object used to validate
-# the client's x509 distinguished name. This object is
-# is only resolved at time of use, so can be deleted and
-# recreated on the fly while the NBD server is active.
-# If missing, it will default to denying access (since 4.0).
-#
-# Returns: error if the server is already running.
-#
-# Keep this type consistent with the NbdServerOptions type. The only intended
-# difference is using SocketAddressLegacy instead of SocketAddress.
-#
-# Since: 1.3.0
-##
-{ 'command': 'nbd-server-start',
- 'data': { 'addr': 'SocketAddressLegacy',
- '*tls-creds': 'str',
- '*tls-authz': 'str'} }
-
-##
-# @BlockExportNbd:
-#
-# An NBD block export.
-#
-# @device: The device name or node name of the node to be exported
-#
-# @name: Export name. If unspecified, the @device parameter is used as the
-# export name. (Since 2.12)
-#
-# @description: Free-form description of the export, up to 4096 bytes.
-# (Since 5.0)
-#
-# @writable: Whether clients should be able to write to the device via the
-# NBD connection (default false).
-#
-# @bitmap: Also export the dirty bitmap reachable from @device, so the
-# NBD client can use NBD_OPT_SET_META_CONTEXT with
-# "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0)
-#
-# Since: 5.0
-##
-{ 'struct': 'BlockExportNbd',
- 'data': {'device': 'str', '*name': 'str', '*description': 'str',
- '*writable': 'bool', '*bitmap': 'str' } }
-
-##
-# @nbd-server-add:
-#
-# Export a block node to QEMU's embedded NBD server.
-#
-# Returns: error if the server is not running, or export with the same name
-# already exists.
-#
-# Since: 1.3.0
-##
-{ 'command': 'nbd-server-add',
- 'data': 'BlockExportNbd', 'boxed': true }
-
-##
-# @NbdServerRemoveMode:
-#
-# Mode for removing an NBD export.
-#
-# @safe: Remove export if there are no existing connections, fail otherwise.
-#
-# @hard: Drop all connections immediately and remove export.
-#
-# Potential additional modes to be added in the future:
-#
-# hide: Just hide export from new clients, leave existing connections as is.
-# Remove export after all clients are disconnected.
-#
-# soft: Hide export from new clients, answer with ESHUTDOWN for all further
-# requests from existing clients.
-#
-# Since: 2.12
-##
-{'enum': 'NbdServerRemoveMode', 'data': ['safe', 'hard']}
-
-##
-# @nbd-server-remove:
-#
-# Remove NBD export by name.
-#
-# @name: Export name.
-#
-# @mode: Mode of command operation. See @NbdServerRemoveMode description.
-# Default is 'safe'.
-#
-# Returns: error if
-# - the server is not running
-# - export is not found
-# - mode is 'safe' and there are existing connections
-#
-# Since: 2.12
-##
-{ 'command': 'nbd-server-remove',
- 'data': {'name': 'str', '*mode': 'NbdServerRemoveMode'} }
-
-##
-# @nbd-server-stop:
-#
-# Stop QEMU's embedded NBD server, and unregister all devices previously
-# added via @nbd-server-add.
-#
-# Since: 1.3.0
-##
-{ 'command': 'nbd-server-stop' }
-
-##
-# @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'
- } }
-
##
# @QuorumOpType:
#
diff --git a/qapi/block-export.json b/qapi/block-export.json
new file mode 100644
index 0000000000..f038eb47aa
--- /dev/null
+++ b/qapi/block-export.json
@@ -0,0 +1,175 @@
+# -*- Mode: Python -*-
+# vim: filetype=python
+
+##
+# == Block device exports
+##
+
+{ 'include': 'sockets.json' }
+
+##
+# @NbdServerOptions:
+#
+# @addr: Address on which to listen.
+# @tls-creds: ID of the TLS credentials object (since 2.6).
+# @tls-authz: ID of the QAuthZ authorization object used to validate
+# the client's x509 distinguished name. This object is
+# is only resolved at time of use, so can be deleted and
+# recreated on the fly while the NBD server is active.
+# If missing, it will default to denying access (since 4.0).
+#
+# Keep this type consistent with the nbd-server-start arguments. The only
+# intended difference is using SocketAddress instead of SocketAddressLegacy.
+#
+# Since: 4.2
+##
+{ 'struct': 'NbdServerOptions',
+ 'data': { 'addr': 'SocketAddress',
+ '*tls-creds': 'str',
+ '*tls-authz': 'str'} }
+
+##
+# @nbd-server-start:
+#
+# Start an NBD server listening on the given host and port. Block
+# devices can then be exported using @nbd-server-add. The NBD
+# server will present them as named exports; for example, another
+# QEMU instance could refer to them as "nbd:HOST:PORT:exportname=NAME".
+#
+# @addr: Address on which to listen.
+# @tls-creds: ID of the TLS credentials object (since 2.6).
+# @tls-authz: ID of the QAuthZ authorization object used to validate
+# the client's x509 distinguished name. This object is
+# is only resolved at time of use, so can be deleted and
+# recreated on the fly while the NBD server is active.
+# If missing, it will default to denying access (since 4.0).
+#
+# Returns: error if the server is already running.
+#
+# Keep this type consistent with the NbdServerOptions type. The only intended
+# difference is using SocketAddressLegacy instead of SocketAddress.
+#
+# Since: 1.3.0
+##
+{ 'command': 'nbd-server-start',
+ 'data': { 'addr': 'SocketAddressLegacy',
+ '*tls-creds': 'str',
+ '*tls-authz': 'str'} }
+
+##
+# @BlockExportNbd:
+#
+# An NBD block export.
+#
+# @device: The device name or node name of the node to be exported
+#
+# @name: Export name. If unspecified, the @device parameter is used as the
+# export name. (Since 2.12)
+#
+# @description: Free-form description of the export, up to 4096 bytes.
+# (Since 5.0)
+#
+# @writable: Whether clients should be able to write to the device via the
+# NBD connection (default false).
+#
+# @bitmap: Also export the dirty bitmap reachable from @device, so the
+# NBD client can use NBD_OPT_SET_META_CONTEXT with
+# "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0)
+#
+# Since: 5.0
+##
+{ 'struct': 'BlockExportNbd',
+ 'data': {'device': 'str', '*name': 'str', '*description': 'str',
+ '*writable': 'bool', '*bitmap': 'str' } }
+
+##
+# @nbd-server-add:
+#
+# Export a block node to QEMU's embedded NBD server.
+#
+# Returns: error if the server is not running, or export with the same name
+# already exists.
+#
+# Since: 1.3.0
+##
+{ 'command': 'nbd-server-add',
+ 'data': 'BlockExportNbd', 'boxed': true }
+
+##
+# @NbdServerRemoveMode:
+#
+# Mode for removing an NBD export.
+#
+# @safe: Remove export if there are no existing connections, fail otherwise.
+#
+# @hard: Drop all connections immediately and remove export.
+#
+# Potential additional modes to be added in the future:
+#
+# hide: Just hide export from new clients, leave existing connections as is.
+# Remove export after all clients are disconnected.
+#
+# soft: Hide export from new clients, answer with ESHUTDOWN for all further
+# requests from existing clients.
+#
+# Since: 2.12
+##
+{'enum': 'NbdServerRemoveMode', 'data': ['safe', 'hard']}
+
+##
+# @nbd-server-remove:
+#
+# Remove NBD export by name.
+#
+# @name: Export name.
+#
+# @mode: Mode of command operation. See @NbdServerRemoveMode description.
+# Default is 'safe'.
+#
+# Returns: error if
+# - the server is not running
+# - export is not found
+# - mode is 'safe' and there are existing connections
+#
+# Since: 2.12
+##
+{ 'command': 'nbd-server-remove',
+ 'data': {'name': 'str', '*mode': 'NbdServerRemoveMode'} }
+
+##
+# @nbd-server-stop:
+#
+# Stop QEMU's embedded NBD server, and unregister all devices previously
+# added via @nbd-server-add.
+#
+# Since: 1.3.0
+##
+{ 'command': 'nbd-server-stop' }
+
+##
+# @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/qapi/qapi-schema.json b/qapi/qapi-schema.json
index f03ff91ceb..b78fc399a4 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -64,6 +64,7 @@
{ 'include': 'run-state.json' }
{ 'include': 'crypto.json' }
{ 'include': 'block.json' }
+{ 'include': 'block-export.json' }
{ 'include': 'char.json' }
{ 'include': 'dump.json' }
{ 'include': 'job.json' }
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 0451683d03..262f6da2ce 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -20,7 +20,7 @@
#ifndef NBD_H
#define NBD_H
-#include "qapi/qapi-types-block.h"
+#include "qapi/qapi-types-block-export.h"
#include "io/channel-socket.h"
#include "crypto/tlscreds.h"
#include "qapi/error.h"
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 4d3db5ed3c..6ce0f8f87c 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -40,6 +40,7 @@
#include "sysemu/block-backend.h"
#include "sysemu/blockdev.h"
#include "qapi/qapi-commands-block.h"
+#include "qapi/qapi-commands-block-export.h"
#include "qapi/qmp/qdict.h"
#include "qapi/error.h"
#include "qapi/qmp/qerror.h"
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 1a95d89f00..0f6b80c58f 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -14,7 +14,7 @@
#include "sysemu/block-backend.h"
#include "hw/block/block.h"
#include "qapi/error.h"
-#include "qapi/qapi-commands-block.h"
+#include "qapi/qapi-commands-block-export.h"
#include "block/nbd.h"
#include "io/channel-socket.h"
#include "io/net-listener.h"
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index 7e9b0e0d3f..ed9d2afcf3 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -35,8 +35,8 @@
#include "monitor/monitor-internal.h"
#include "qapi/error.h"
-#include "qapi/qapi-visit-block.h"
#include "qapi/qapi-visit-block-core.h"
+#include "qapi/qapi-visit-block-export.h"
#include "qapi/qapi-visit-control.h"
#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qstring.h"
diff --git a/qapi/meson.build b/qapi/meson.build
index 2b2872a41d..1ee19a07de 100644
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -16,8 +16,9 @@ util_ss.add(files(
qapi_all_modules = [
'audio',
'authz',
- 'block-core',
'block',
+ 'block-core',
+ 'block-export',
'char',
'common',
'control',
@@ -47,6 +48,7 @@ qapi_all_modules = [
qapi_storage_daemon_modules = [
'block-core',
+ 'block-export',
'char',
'common',
'control',
diff --git a/storage-daemon/qapi/qapi-schema.json b/storage-daemon/qapi/qapi-schema.json
index 6100d1f0c9..c6ad5ae1e3 100644
--- a/storage-daemon/qapi/qapi-schema.json
+++ b/storage-daemon/qapi/qapi-schema.json
@@ -16,6 +16,7 @@
{ 'include': '../../qapi/pragma.json' }
{ 'include': '../../qapi/block-core.json' }
+{ 'include': '../../qapi/block-export.json' }
{ 'include': '../../qapi/char.json' }
{ 'include': '../../qapi/common.json' }
{ 'include': '../../qapi/control.json' }
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 03/29] qapi: Rename BlockExport to BlockExportOptions
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
2020-09-07 18:19 ` [PATCH 01/29] nbd: Remove unused nbd_export_get_blockdev() Kevin Wolf
2020-09-07 18:19 ` [PATCH 02/29] qapi: Create block-export module Kevin Wolf
@ 2020-09-07 18:19 ` Kevin Wolf
2020-09-07 18:19 ` [PATCH 04/29] block/export: Add BlockExport infrastructure and block-export-add Kevin Wolf
` (26 subsequent siblings)
29 siblings, 0 replies; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
The name BlockExport will be used for the struct containing the runtime
state of block exports, so change the name of export creation options.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qapi/block-export.json | 12 ++++++------
block/monitor/block-hmp-cmds.c | 6 +++---
blockdev-nbd.c | 2 +-
storage-daemon/qemu-storage-daemon.c | 8 ++++----
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index f038eb47aa..799b7e50e7 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -57,7 +57,7 @@
'*tls-authz': 'str'} }
##
-# @BlockExportNbd:
+# @BlockExportOptionsNbd:
#
# An NBD block export.
#
@@ -78,7 +78,7 @@
#
# Since: 5.0
##
-{ 'struct': 'BlockExportNbd',
+{ 'struct': 'BlockExportOptionsNbd',
'data': {'device': 'str', '*name': 'str', '*description': 'str',
'*writable': 'bool', '*bitmap': 'str' } }
@@ -93,7 +93,7 @@
# Since: 1.3.0
##
{ 'command': 'nbd-server-add',
- 'data': 'BlockExportNbd', 'boxed': true }
+ 'data': 'BlockExportOptionsNbd', 'boxed': true }
##
# @NbdServerRemoveMode:
@@ -159,17 +159,17 @@
'data': [ 'nbd' ] }
##
-# @BlockExport:
+# @BlockExportOptions:
#
# Describes a block export, i.e. how single node should be exported on an
# external interface.
#
# Since: 4.2
##
-{ 'union': 'BlockExport',
+{ 'union': 'BlockExportOptions',
'base': { 'type': 'BlockExportType' },
'discriminator': 'type',
'data': {
- 'nbd': 'BlockExportNbd'
+ 'nbd': 'BlockExportOptionsNbd'
} }
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 6ce0f8f87c..fb632b1189 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -398,7 +398,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
Error *local_err = NULL;
BlockInfoList *block_list, *info;
SocketAddress *addr;
- BlockExportNbd export;
+ BlockExportOptionsNbd export;
if (writable && !all) {
error_setg(&local_err, "-w only valid together with -a");
@@ -431,7 +431,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
continue;
}
- export = (BlockExportNbd) {
+ export = (BlockExportOptionsNbd) {
.device = info->value->device,
.has_writable = true,
.writable = writable,
@@ -458,7 +458,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
bool writable = qdict_get_try_bool(qdict, "writable", false);
Error *local_err = NULL;
- BlockExportNbd export = {
+ BlockExportOptionsNbd export = {
.device = (char *) device,
.has_name = !!name,
.name = (char *) name,
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 0f6b80c58f..98ee1b6170 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -148,7 +148,7 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
qapi_free_SocketAddress(addr_flat);
}
-void qmp_nbd_server_add(BlockExportNbd *arg, Error **errp)
+void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
{
BlockDriverState *bs = NULL;
BlockBackend *on_eject_blk;
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index ed9d2afcf3..ed26097254 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -150,7 +150,7 @@ static void init_qmp_commands(void)
qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG);
}
-static void init_export(BlockExport *export, Error **errp)
+static void init_export(BlockExportOptions *export, Error **errp)
{
switch (export->type) {
case BLOCK_EXPORT_TYPE_NBD:
@@ -235,14 +235,14 @@ static void process_options(int argc, char *argv[])
case OPTION_EXPORT:
{
Visitor *v;
- BlockExport *export;
+ BlockExportOptions *export;
v = qobject_input_visitor_new_str(optarg, "type", &error_fatal);
- visit_type_BlockExport(v, NULL, &export, &error_fatal);
+ visit_type_BlockExportOptions(v, NULL, &export, &error_fatal);
visit_free(v);
init_export(export, &error_fatal);
- qapi_free_BlockExport(export);
+ qapi_free_BlockExportOptions(export);
break;
}
case OPTION_MONITOR:
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 04/29] block/export: Add BlockExport infrastructure and block-export-add
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (2 preceding siblings ...)
2020-09-07 18:19 ` [PATCH 03/29] qapi: Rename BlockExport to BlockExportOptions Kevin Wolf
@ 2020-09-07 18:19 ` Kevin Wolf
2020-09-10 10:16 ` Max Reitz
2020-09-07 18:19 ` [PATCH 05/29] qemu-storage-daemon: Use qmp_block_export_add() Kevin Wolf
` (25 subsequent siblings)
29 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
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 <kwolf@redhat.com>
---
qapi/block-export.json | 9 ++++++++
include/block/export.h | 33 +++++++++++++++++++++++++++
include/block/nbd.h | 5 ++++-
block/export/export.c | 48 ++++++++++++++++++++++++++++++++++++++++
blockdev-nbd.c | 28 +++++++++++++++++------
nbd/server.c | 15 ++++++++++++-
block/export/meson.build | 1 +
block/meson.build | 2 ++
meson.build | 2 +-
9 files changed, 133 insertions(+), 10 deletions(-)
create mode 100644 include/block/export.h
create mode 100644 block/export/export.c
create mode 100644 block/export/meson.build
diff --git a/qapi/block-export.json b/qapi/block-export.json
index 799b7e50e7..afb1af048d 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -173,3 +173,12 @@
'nbd': 'BlockExportOptionsNbd'
} }
+##
+# @block-export-add:
+#
+# Creates a new block export.
+#
+# Since: 5.2
+##
+{ 'command': 'block-export-add',
+ 'data': 'BlockExportOptions', 'boxed': true }
diff --git a/include/block/export.h b/include/block/export.h
new file mode 100644
index 0000000000..42e3c055fc
--- /dev/null
+++ b/include/block/export.h
@@ -0,0 +1,33 @@
+/*
+ * Declarations for block exports
+ *
+ * Copyright (c) 2012, 2020 Red Hat, Inc.
+ *
+ * Authors:
+ * Paolo Bonzini <pbonzini@redhat.com>
+ * Kevin Wolf <kwolf@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later. See the COPYING file in the top-level directory.
+ */
+
+#ifndef BLOCK_EXPORT_H
+#define BLOCK_EXPORT_H
+
+#include "qapi/qapi-types-block-export.h"
+
+typedef struct BlockExport BlockExport;
+
+typedef struct BlockExportDriver {
+ /* The export type that this driver services */
+ BlockExportType type;
+
+ /* Creates and starts a new block export */
+ BlockExport *(*create)(BlockExportOptions *, Error **);
+} BlockExportDriver;
+
+struct BlockExport {
+ const BlockExportDriver *drv;
+};
+
+#endif
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 262f6da2ce..7698453fb2 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -20,11 +20,13 @@
#ifndef NBD_H
#define NBD_H
-#include "qapi/qapi-types-block-export.h"
+#include "block/export.h"
#include "io/channel-socket.h"
#include "crypto/tlscreds.h"
#include "qapi/error.h"
+extern const BlockExportDriver blk_exp_nbd;
+
/* Handshake phase structs - this struct is passed on the wire */
struct NBDOption {
@@ -328,6 +330,7 @@ int nbd_errno_to_system_errno(int err);
typedef struct NBDExport NBDExport;
typedef struct NBDClient NBDClient;
+BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp);
NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
uint64_t size, const char *name, const char *desc,
const char *bitmap, bool readonly, bool shared,
diff --git a/block/export/export.c b/block/export/export.c
new file mode 100644
index 0000000000..fd65541963
--- /dev/null
+++ b/block/export/export.c
@@ -0,0 +1,48 @@
+/*
+ * Common block export infrastructure
+ *
+ * Copyright (c) 2012, 2020 Red Hat, Inc.
+ *
+ * Authors:
+ * Paolo Bonzini <pbonzini@redhat.com>
+ * Kevin Wolf <kwolf@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later. See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+
+#include "block/export.h"
+#include "block/nbd.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-block-export.h"
+
+static const BlockExportDriver *blk_exp_drivers[] = {
+ &blk_exp_nbd,
+};
+
+static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(blk_exp_drivers); i++) {
+ if (blk_exp_drivers[i]->type == type) {
+ return blk_exp_drivers[i];
+ }
+ }
+ return NULL;
+}
+
+void qmp_block_export_add(BlockExportOptions *export, Error **errp)
+{
+ const BlockExportDriver *drv;
+
+ drv = blk_exp_find_driver(export->type);
+ if (!drv) {
+ error_setg(errp, "No driver found for the requested export type");
+ return;
+ }
+
+ drv->create(export, errp);
+}
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 98ee1b6170..47b04f166a 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -148,17 +148,20 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
qapi_free_SocketAddress(addr_flat);
}
-void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
+BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
{
+ BlockExportOptionsNbd *arg = &exp_args->u.nbd;
BlockDriverState *bs = NULL;
BlockBackend *on_eject_blk;
- NBDExport *exp;
+ NBDExport *exp = NULL;
int64_t len;
AioContext *aio_context;
+ assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD);
+
if (!nbd_server) {
error_setg(errp, "NBD server not running");
- return;
+ return NULL;
}
if (!arg->has_name) {
@@ -167,24 +170,24 @@ void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
if (strlen(arg->name) > NBD_MAX_STRING_SIZE) {
error_setg(errp, "export name '%s' too long", arg->name);
- return;
+ return NULL;
}
if (arg->description && strlen(arg->description) > NBD_MAX_STRING_SIZE) {
error_setg(errp, "description '%s' too long", arg->description);
- return;
+ return NULL;
}
if (nbd_export_find(arg->name)) {
error_setg(errp, "NBD server already has export named '%s'", arg->name);
- return;
+ return NULL;
}
on_eject_blk = blk_by_name(arg->device);
bs = bdrv_lookup_bs(arg->device, arg->device, errp);
if (!bs) {
- return;
+ return NULL;
}
aio_context = bdrv_get_aio_context(bs);
@@ -217,6 +220,17 @@ void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
out:
aio_context_release(aio_context);
+ /* TODO Remove the cast: nbd_export_new() will return a BlockExport. */
+ return (BlockExport*) exp;
+}
+
+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);
}
void qmp_nbd_server_remove(const char *name,
diff --git a/nbd/server.c b/nbd/server.c
index bd53f7baea..f5af93c253 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;
int refcount;
void (*close)(NBDExport *exp);
@@ -1512,10 +1515,15 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
{
AioContext *ctx;
BlockBackend *blk;
- NBDExport *exp = g_new0(NBDExport, 1);
+ NBDExport *exp;
uint64_t perm;
int ret;
+ exp = g_new0(NBDExport, 1);
+ exp->common = (BlockExport) {
+ .drv = &blk_exp_nbd,
+ };
+
/*
* NBD exports are used for non-shared storage migration. Make sure
* that BDRV_O_INACTIVE is cleared and the image is ready for write
@@ -1731,6 +1739,11 @@ void nbd_export_put(NBDExport *exp)
}
}
+const BlockExportDriver blk_exp_nbd = {
+ .type = BLOCK_EXPORT_TYPE_NBD,
+ .create = nbd_export_create,
+};
+
void nbd_export_close_all(void)
{
NBDExport *exp, *next;
diff --git a/block/export/meson.build b/block/export/meson.build
new file mode 100644
index 0000000000..558ef35d38
--- /dev/null
+++ b/block/export/meson.build
@@ -0,0 +1 @@
+block_ss.add(files('export.c'))
diff --git a/block/meson.build b/block/meson.build
index a3e56b7cd1..0b38dc36f7 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -110,6 +110,8 @@ block_ss.add(module_block_h)
block_ss.add(files('stream.c'))
softmmu_ss.add(files('qapi-sysemu.c'))
+
+subdir('export')
subdir('monitor')
modules += {'block': block_modules}
diff --git a/meson.build b/meson.build
index 5aaa364730..7bd98cb3fb 100644
--- a/meson.build
+++ b/meson.build
@@ -767,6 +767,7 @@ subdir('dump')
block_ss.add(files(
'block.c',
+ 'blockdev-nbd.c',
'blockjob.c',
'job.c',
'qemu-io-cmds.c',
@@ -779,7 +780,6 @@ subdir('block')
blockdev_ss.add(files(
'blockdev.c',
- 'blockdev-nbd.c',
'iothread.c',
'job-qmp.c',
))
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 05/29] qemu-storage-daemon: Use qmp_block_export_add()
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (3 preceding siblings ...)
2020-09-07 18:19 ` [PATCH 04/29] block/export: Add BlockExport infrastructure and block-export-add Kevin Wolf
@ 2020-09-07 18:19 ` Kevin Wolf
2020-09-07 18:19 ` [PATCH 06/29] qemu-nbd: Use raw block driver for --offset Kevin Wolf
` (24 subsequent siblings)
29 siblings, 0 replies; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
No reason to duplicate the functionality locally, we can now just reuse
the QMP command block-export-add for --export.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
storage-daemon/qemu-storage-daemon.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index ed26097254..b6f678d3ab 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -150,17 +150,6 @@ static void init_qmp_commands(void)
qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG);
}
-static void init_export(BlockExportOptions *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 void process_options(int argc, char *argv[])
{
int c;
@@ -241,7 +230,7 @@ static void process_options(int argc, char *argv[])
visit_type_BlockExportOptions(v, NULL, &export, &error_fatal);
visit_free(v);
- init_export(export, &error_fatal);
+ qmp_block_export_add(export, &error_fatal);
qapi_free_BlockExportOptions(export);
break;
}
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 06/29] qemu-nbd: Use raw block driver for --offset
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (4 preceding siblings ...)
2020-09-07 18:19 ` [PATCH 05/29] qemu-storage-daemon: Use qmp_block_export_add() Kevin Wolf
@ 2020-09-07 18:19 ` Kevin Wolf
2020-09-07 18:19 ` [PATCH 07/29] block/export: Remove magic from block-export-add Kevin Wolf
` (23 subsequent siblings)
29 siblings, 0 replies; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Instead of implementing qemu-nbd --offset in the NBD code, just put a
raw block node with the requested offset on top of the user image and
rely on that doing the job.
This does not only simplify the nbd_export_new() interface and bring it
closer to the set of options that the nbd-server-add QMP command offers,
but in fact it also eliminates a potential source for bugs in the NBD
code which previously had to add the offset manually in all relevant
places.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
include/block/nbd.h | 4 ++--
blockdev-nbd.c | 9 +--------
nbd/server.c | 33 ++++++++++++++++-----------------
qemu-nbd.c | 27 ++++++++++++---------------
4 files changed, 31 insertions(+), 42 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 7698453fb2..451f399b0a 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -331,8 +331,8 @@ typedef struct NBDExport NBDExport;
typedef struct NBDClient NBDClient;
BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp);
-NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
- uint64_t size, const char *name, const char *desc,
+NBDExport *nbd_export_new(BlockDriverState *bs,
+ const char *name, const char *desc,
const char *bitmap, bool readonly, bool shared,
void (*close)(NBDExport *), bool writethrough,
BlockBackend *on_eject_blk, Error **errp);
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 47b04f166a..96cb0100e9 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -154,7 +154,6 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
BlockDriverState *bs = NULL;
BlockBackend *on_eject_blk;
NBDExport *exp = NULL;
- int64_t len;
AioContext *aio_context;
assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD);
@@ -192,12 +191,6 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
- len = bdrv_getlength(bs);
- if (len < 0) {
- error_setg_errno(errp, -len,
- "Failed to determine the NBD export's length");
- goto out;
- }
if (!arg->has_writable) {
arg->writable = false;
@@ -206,7 +199,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
arg->writable = false;
}
- exp = nbd_export_new(bs, 0, len, arg->name, arg->description, arg->bitmap,
+ exp = nbd_export_new(bs, arg->name, arg->description, arg->bitmap,
!arg->writable, !arg->writable,
NULL, false, on_eject_blk, errp);
if (!exp) {
diff --git a/nbd/server.c b/nbd/server.c
index f5af93c253..33aaca918c 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -89,7 +89,6 @@ struct NBDExport {
BlockBackend *blk;
char *name;
char *description;
- uint64_t dev_offset;
uint64_t size;
uint16_t nbdflags;
QTAILQ_HEAD(, NBDClient) clients;
@@ -1507,8 +1506,8 @@ static void nbd_eject_notifier(Notifier *n, void *data)
aio_context_release(aio_context);
}
-NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
- uint64_t size, const char *name, const char *desc,
+NBDExport *nbd_export_new(BlockDriverState *bs,
+ const char *name, const char *desc,
const char *bitmap, bool readonly, bool shared,
void (*close)(NBDExport *), bool writethrough,
BlockBackend *on_eject_blk, Error **errp)
@@ -1516,9 +1515,17 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
AioContext *ctx;
BlockBackend *blk;
NBDExport *exp;
+ int64_t size;
uint64_t perm;
int ret;
+ size = bdrv_getlength(bs);
+ if (size < 0) {
+ error_setg_errno(errp, -size,
+ "Failed to determine the NBD export's length");
+ return NULL;
+ }
+
exp = g_new0(NBDExport, 1);
exp->common = (BlockExport) {
.drv = &blk_exp_nbd,
@@ -1553,8 +1560,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
exp->refcount = 1;
QTAILQ_INIT(&exp->clients);
exp->blk = blk;
- assert(dev_offset <= INT64_MAX);
- exp->dev_offset = dev_offset;
exp->name = g_strdup(name);
assert(!desc || strlen(desc) <= NBD_MAX_STRING_SIZE);
exp->description = g_strdup(desc);
@@ -1569,7 +1574,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
exp->nbdflags |= (NBD_FLAG_SEND_TRIM | NBD_FLAG_SEND_WRITE_ZEROES |
NBD_FLAG_SEND_FAST_ZERO);
}
- assert(size <= INT64_MAX - dev_offset);
exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE);
if (bitmap) {
@@ -1928,8 +1932,7 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
stl_be_p(&chunk.length, pnum);
ret = nbd_co_send_iov(client, iov, 1, errp);
} else {
- ret = blk_pread(exp->blk, offset + progress + exp->dev_offset,
- data + progress, pnum);
+ ret = blk_pread(exp->blk, offset + progress, data + progress, pnum);
if (ret < 0) {
error_setg_errno(errp, -ret, "reading from file failed");
break;
@@ -2303,8 +2306,7 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request,
data, request->len, errp);
}
- ret = blk_pread(exp->blk, request->from + exp->dev_offset, data,
- request->len);
+ ret = blk_pread(exp->blk, request->from, data, request->len);
if (ret < 0) {
return nbd_send_generic_reply(client, request->handle, ret,
"reading from file failed", errp);
@@ -2339,7 +2341,7 @@ static coroutine_fn int nbd_do_cmd_cache(NBDClient *client, NBDRequest *request,
assert(request->type == NBD_CMD_CACHE);
- ret = blk_co_preadv(exp->blk, request->from + exp->dev_offset, request->len,
+ ret = blk_co_preadv(exp->blk, request->from, request->len,
NULL, BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH);
return nbd_send_generic_reply(client, request->handle, ret,
@@ -2370,8 +2372,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
if (request->flags & NBD_CMD_FLAG_FUA) {
flags |= BDRV_REQ_FUA;
}
- ret = blk_pwrite(exp->blk, request->from + exp->dev_offset,
- data, request->len, flags);
+ ret = blk_pwrite(exp->blk, request->from, data, request->len, flags);
return nbd_send_generic_reply(client, request->handle, ret,
"writing to file failed", errp);
@@ -2392,8 +2393,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
int align = client->check_align ?: 1;
int len = MIN(request->len, QEMU_ALIGN_DOWN(BDRV_REQUEST_MAX_BYTES,
align));
- ret = blk_pwrite_zeroes(exp->blk, request->from + exp->dev_offset,
- len, flags);
+ ret = blk_pwrite_zeroes(exp->blk, request->from, len, flags);
request->len -= len;
request->from += len;
}
@@ -2416,8 +2416,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
int align = client->check_align ?: 1;
int len = MIN(request->len, QEMU_ALIGN_DOWN(BDRV_REQUEST_MAX_BYTES,
align));
- ret = blk_co_pdiscard(exp->blk, request->from + exp->dev_offset,
- len);
+ ret = blk_co_pdiscard(exp->blk, request->from, len);
request->len -= len;
request->from += len;
}
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 33476a1000..1752daa4c4 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -524,7 +524,6 @@ int main(int argc, char **argv)
const char *port = NULL;
char *sockpath = NULL;
char *device = NULL;
- int64_t fd_size;
QemuOpts *sn_opts = NULL;
const char *sn_id_or_name = NULL;
const char *sopt = "hVb:o:p:rsnc:dvk:e:f:tl:x:T:D:B:L";
@@ -1037,6 +1036,17 @@ int main(int argc, char **argv)
}
bs = blk_bs(blk);
+ if (dev_offset) {
+ QDict *raw_opts = qdict_new();
+ qdict_put_str(raw_opts, "driver", "raw");
+ qdict_put_str(raw_opts, "file", bs->node_name);
+ qdict_put_int(raw_opts, "offset", dev_offset);
+ bs = bdrv_open(NULL, NULL, raw_opts, flags, &error_fatal);
+ blk_remove_bs(blk);
+ blk_insert_bs(blk, bs, &error_fatal);
+ bdrv_unref(bs);
+ }
+
blk_set_enable_write_cache(blk, !writethrough);
if (sn_opts) {
@@ -1054,21 +1064,8 @@ int main(int argc, char **argv)
}
bs->detect_zeroes = detect_zeroes;
- fd_size = blk_getlength(blk);
- if (fd_size < 0) {
- error_report("Failed to determine the image length: %s",
- strerror(-fd_size));
- exit(EXIT_FAILURE);
- }
-
- if (dev_offset >= fd_size) {
- error_report("Offset (%" PRIu64 ") has to be smaller than the image "
- "size (%" PRId64 ")", dev_offset, fd_size);
- exit(EXIT_FAILURE);
- }
- fd_size -= dev_offset;
- export = nbd_export_new(bs, dev_offset, fd_size, export_name,
+ export = nbd_export_new(bs, export_name,
export_description, bitmap, readonly, shared > 1,
nbd_export_closed, writethrough, NULL,
&error_fatal);
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 07/29] block/export: Remove magic from block-export-add
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (5 preceding siblings ...)
2020-09-07 18:19 ` [PATCH 06/29] qemu-nbd: Use raw block driver for --offset Kevin Wolf
@ 2020-09-07 18:19 ` Kevin Wolf
2020-09-10 10:53 ` Max Reitz
2020-09-07 18:19 ` [PATCH 08/29] nbd: Add max-connections to nbd-server-start Kevin Wolf
` (22 subsequent siblings)
29 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
nbd-server-add tries to be convenient and adds two questionable
features that we don't want to share in block-export-add, even for NBD
exports:
1. When requesting a writable export of a read-only device, the export
is silently downgraded to read-only. This should be an error in the
context of block-export-add.
2. When using a BlockBackend name, unplugging the device from the guest
will automatically stop the NBD server, too. This may sometimes be
what you want, but it could also be very surprising. Let's keep
things explicit with block-export-add. If the user wants to stop the
export, they should tell us so.
Move these things into the nbd-server-add QMP command handler so that
they apply only there.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/export.h | 2 ++
include/block/nbd.h | 3 ++-
block/export/export.c | 13 +++++++++---
blockdev-nbd.c | 47 +++++++++++++++++++++++++++++++++++-------
nbd/server.c | 20 +++++++++++-------
qemu-nbd.c | 3 +--
6 files changed, 67 insertions(+), 21 deletions(-)
diff --git a/include/block/export.h b/include/block/export.h
index 42e3c055fc..e7af2c7687 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -30,4 +30,6 @@ struct BlockExport {
const BlockExportDriver *drv;
};
+BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp);
+
#endif
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 451f399b0a..f55f5b710b 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -335,7 +335,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
const char *name, const char *desc,
const char *bitmap, bool readonly, bool shared,
void (*close)(NBDExport *), bool writethrough,
- BlockBackend *on_eject_blk, Error **errp);
+ Error **errp);
+void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk);
void nbd_export_close(NBDExport *exp);
void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
void nbd_export_get(NBDExport *exp);
diff --git a/block/export/export.c b/block/export/export.c
index fd65541963..05bc5e3744 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -13,6 +13,8 @@
#include "qemu/osdep.h"
+#include "block/block.h"
+#include "sysemu/block-backend.h"
#include "block/export.h"
#include "block/nbd.h"
#include "qapi/error.h"
@@ -34,15 +36,20 @@ static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
return NULL;
}
-void qmp_block_export_add(BlockExportOptions *export, Error **errp)
+BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
{
const BlockExportDriver *drv;
drv = blk_exp_find_driver(export->type);
if (!drv) {
error_setg(errp, "No driver found for the requested export type");
- return;
+ return NULL;
}
- drv->create(export, errp);
+ return drv->create(export, errp);
+}
+
+void qmp_block_export_add(BlockExportOptions *export, Error **errp)
+{
+ blk_exp_add(export, errp);
}
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 96cb0100e9..7bcca105f9 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -152,7 +152,6 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
{
BlockExportOptionsNbd *arg = &exp_args->u.nbd;
BlockDriverState *bs = NULL;
- BlockBackend *on_eject_blk;
NBDExport *exp = NULL;
AioContext *aio_context;
@@ -182,8 +181,6 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
return NULL;
}
- on_eject_blk = blk_by_name(arg->device);
-
bs = bdrv_lookup_bs(arg->device, arg->device, errp);
if (!bs) {
return NULL;
@@ -195,13 +192,14 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
if (!arg->has_writable) {
arg->writable = false;
}
- if (bdrv_is_read_only(bs)) {
- arg->writable = false;
+ if (bdrv_is_read_only(bs) && arg->writable) {
+ error_setg(errp, "Cannot export read-only node as writable");
+ goto out;
}
exp = nbd_export_new(bs, arg->name, arg->description, arg->bitmap,
!arg->writable, !arg->writable,
- NULL, false, on_eject_blk, errp);
+ NULL, false, errp);
if (!exp) {
goto out;
}
@@ -219,11 +217,44 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
{
- BlockExportOptions export = {
+ BlockExport *export;
+ BlockDriverState *bs;
+ BlockBackend *on_eject_blk;
+ BlockExportOptions export_opts;
+
+ bs = bdrv_lookup_bs(arg->device, arg->device, errp);
+ if (!bs) {
+ return;
+ }
+
+ export_opts = (BlockExportOptions) {
.type = BLOCK_EXPORT_TYPE_NBD,
.u.nbd = *arg,
};
- qmp_block_export_add(&export, errp);
+
+ /*
+ * nbd-server-add doesn't complain when a read-only device should be
+ * exported as writable, but simply downgrades it. This is an error with
+ * block-export-add.
+ */
+ if (bdrv_is_read_only(bs)) {
+ export_opts.u.nbd.has_writable = true;
+ export_opts.u.nbd.writable = false;
+ }
+
+ export = blk_exp_add(&export_opts, errp);
+ if (!export) {
+ return;
+ }
+
+ /*
+ * nbd-server-add removes the export when the named BlockBackend used for
+ * @device goes away.
+ */
+ on_eject_blk = blk_by_name(arg->device);
+ if (on_eject_blk) {
+ nbd_export_set_on_eject_blk(export, on_eject_blk);
+ }
}
void qmp_nbd_server_remove(const char *name,
diff --git a/nbd/server.c b/nbd/server.c
index 33aaca918c..23d9a53094 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1506,11 +1506,23 @@ static void nbd_eject_notifier(Notifier *n, void *data)
aio_context_release(aio_context);
}
+void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk)
+{
+ NBDExport *nbd_exp = container_of(exp, NBDExport, common);
+ assert(exp->drv == &blk_exp_nbd);
+ assert(nbd_exp->eject_notifier_blk == NULL);
+
+ blk_ref(blk);
+ nbd_exp->eject_notifier_blk = blk;
+ nbd_exp->eject_notifier.notify = nbd_eject_notifier;
+ blk_add_remove_bs_notifier(blk, &nbd_exp->eject_notifier);
+}
+
NBDExport *nbd_export_new(BlockDriverState *bs,
const char *name, const char *desc,
const char *bitmap, bool readonly, bool shared,
void (*close)(NBDExport *), bool writethrough,
- BlockBackend *on_eject_blk, Error **errp)
+ Error **errp)
{
AioContext *ctx;
BlockBackend *blk;
@@ -1617,12 +1629,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
exp->ctx = ctx;
blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
- if (on_eject_blk) {
- blk_ref(on_eject_blk);
- exp->eject_notifier_blk = on_eject_blk;
- exp->eject_notifier.notify = nbd_eject_notifier;
- blk_add_remove_bs_notifier(on_eject_blk, &exp->eject_notifier);
- }
QTAILQ_INSERT_TAIL(&exports, exp, next);
nbd_export_get(exp);
return exp;
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 1752daa4c4..2bba6c0db3 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1067,8 +1067,7 @@ int main(int argc, char **argv)
export = nbd_export_new(bs, export_name,
export_description, bitmap, readonly, shared > 1,
- nbd_export_closed, writethrough, NULL,
- &error_fatal);
+ nbd_export_closed, writethrough, &error_fatal);
if (device) {
#if HAVE_NBD_DEVICE
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 07/29] block/export: Remove magic from block-export-add
2020-09-07 18:19 ` [PATCH 07/29] block/export: Remove magic from block-export-add Kevin Wolf
@ 2020-09-10 10:53 ` Max Reitz
0 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2020-09-10 10:53 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1398 bytes --]
On 07.09.20 20:19, Kevin Wolf wrote:
> nbd-server-add tries to be convenient and adds two questionable
> features that we don't want to share in block-export-add, even for NBD
> exports:
>
> 1. When requesting a writable export of a read-only device, the export
> is silently downgraded to read-only. This should be an error in the
> context of block-export-add.
>
> 2. When using a BlockBackend name, unplugging the device from the guest
> will automatically stop the NBD server, too. This may sometimes be
> what you want, but it could also be very surprising. Let's keep
> things explicit with block-export-add. If the user wants to stop the
> export, they should tell us so.
>
> Move these things into the nbd-server-add QMP command handler so that
> they apply only there.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> include/block/export.h | 2 ++
> include/block/nbd.h | 3 ++-
> block/export/export.c | 13 +++++++++---
> blockdev-nbd.c | 47 +++++++++++++++++++++++++++++++++++-------
> nbd/server.c | 20 +++++++++++-------
> qemu-nbd.c | 3 +--
> 6 files changed, 67 insertions(+), 21 deletions(-)
[...]
> + if (bdrv_is_read_only(bs)) {
> + export_opts.u.nbd.has_writable = true;
Ah, yes, setting that might be nice. :)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 08/29] nbd: Add max-connections to nbd-server-start
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (6 preceding siblings ...)
2020-09-07 18:19 ` [PATCH 07/29] block/export: Remove magic from block-export-add Kevin Wolf
@ 2020-09-07 18:19 ` Kevin Wolf
2020-09-07 18:19 ` [PATCH 09/29] nbd: Add writethrough to block-export-add Kevin Wolf
` (21 subsequent siblings)
29 siblings, 0 replies; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
This is a QMP equivalent of qemu-nbd's --shared option, limiting the
maximum number of clients that can attach at the same time.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qapi/block-export.json | 10 +++++++--
include/block/nbd.h | 3 ++-
block/monitor/block-hmp-cmds.c | 2 +-
blockdev-nbd.c | 33 ++++++++++++++++++++++------
storage-daemon/qemu-storage-daemon.c | 4 ++--
5 files changed, 39 insertions(+), 13 deletions(-)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index afb1af048d..7dedd893ab 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -17,6 +17,8 @@
# is only resolved at time of use, so can be deleted and
# recreated on the fly while the NBD server is active.
# If missing, it will default to denying access (since 4.0).
+# @max-connections: The maximum number of connections to allow at the same
+# time, 0 for unlimited. (since 5.2; default: 0)
#
# Keep this type consistent with the nbd-server-start arguments. The only
# intended difference is using SocketAddress instead of SocketAddressLegacy.
@@ -26,7 +28,8 @@
{ 'struct': 'NbdServerOptions',
'data': { 'addr': 'SocketAddress',
'*tls-creds': 'str',
- '*tls-authz': 'str'} }
+ '*tls-authz': 'str',
+ '*max-connections': 'uint32' } }
##
# @nbd-server-start:
@@ -43,6 +46,8 @@
# is only resolved at time of use, so can be deleted and
# recreated on the fly while the NBD server is active.
# If missing, it will default to denying access (since 4.0).
+# @max-connections: The maximum number of connections to allow at the same
+# time, 0 for unlimited. (since 5.2; default: 0)
#
# Returns: error if the server is already running.
#
@@ -54,7 +59,8 @@
{ 'command': 'nbd-server-start',
'data': { 'addr': 'SocketAddressLegacy',
'*tls-creds': 'str',
- '*tls-authz': 'str'} }
+ '*tls-authz': 'str',
+ '*max-connections': 'uint32' } }
##
# @BlockExportOptionsNbd:
diff --git a/include/block/nbd.h b/include/block/nbd.h
index f55f5b710b..acccdb3180 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -354,7 +354,8 @@ void nbd_client_get(NBDClient *client);
void nbd_client_put(NBDClient *client);
void nbd_server_start(SocketAddress *addr, const char *tls_creds,
- const char *tls_authz, Error **errp);
+ const char *tls_authz, uint32_t max_connections,
+ Error **errp);
void nbd_server_start_options(NbdServerOptions *arg, Error **errp);
/* nbd_read
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index fb632b1189..662b7f7d00 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -411,7 +411,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
goto exit;
}
- nbd_server_start(addr, NULL, NULL, &local_err);
+ nbd_server_start(addr, NULL, NULL, 0, &local_err);
qapi_free_SocketAddress(addr);
if (local_err != NULL) {
goto exit;
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 7bcca105f9..41d5542987 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -23,23 +23,41 @@ typedef struct NBDServerData {
QIONetListener *listener;
QCryptoTLSCreds *tlscreds;
char *tlsauthz;
+ uint32_t max_connections;
+ uint32_t connections;
} NBDServerData;
static NBDServerData *nbd_server;
+static void nbd_update_server_watch(NBDServerData *s);
+
static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
{
nbd_client_put(client);
+ assert(nbd_server->connections > 0);
+ nbd_server->connections--;
+ nbd_update_server_watch(nbd_server);
}
static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,
gpointer opaque)
{
+ nbd_server->connections++;
+ nbd_update_server_watch(nbd_server);
+
qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server");
nbd_client_new(cioc, nbd_server->tlscreds, nbd_server->tlsauthz,
nbd_blockdev_client_closed);
}
+static void nbd_update_server_watch(NBDServerData *s)
+{
+ if (!s->max_connections || s->connections < s->max_connections) {
+ qio_net_listener_set_client_func(s->listener, nbd_accept, NULL, NULL);
+ } else {
+ qio_net_listener_set_client_func(s->listener, NULL, NULL, NULL);
+ }
+}
static void nbd_server_free(NBDServerData *server)
{
@@ -88,7 +106,8 @@ static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)
void nbd_server_start(SocketAddress *addr, const char *tls_creds,
- const char *tls_authz, Error **errp)
+ const char *tls_authz, uint32_t max_connections,
+ Error **errp)
{
if (nbd_server) {
error_setg(errp, "NBD server already running");
@@ -96,6 +115,7 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds,
}
nbd_server = g_new0(NBDServerData, 1);
+ nbd_server->max_connections = max_connections;
nbd_server->listener = qio_net_listener_new();
qio_net_listener_set_name(nbd_server->listener,
@@ -120,10 +140,7 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds,
nbd_server->tlsauthz = g_strdup(tls_authz);
- qio_net_listener_set_client_func(nbd_server->listener,
- nbd_accept,
- NULL,
- NULL);
+ nbd_update_server_watch(nbd_server);
return;
@@ -134,17 +151,19 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds,
void nbd_server_start_options(NbdServerOptions *arg, Error **errp)
{
- nbd_server_start(arg->addr, arg->tls_creds, arg->tls_authz, errp);
+ nbd_server_start(arg->addr, arg->tls_creds, arg->tls_authz,
+ arg->max_connections, errp);
}
void qmp_nbd_server_start(SocketAddressLegacy *addr,
bool has_tls_creds, const char *tls_creds,
bool has_tls_authz, const char *tls_authz,
+ bool has_max_connections, uint32_t max_connections,
Error **errp)
{
SocketAddress *addr_flat = socket_address_flatten(addr);
- nbd_server_start(addr_flat, tls_creds, tls_authz, errp);
+ nbd_server_start(addr_flat, tls_creds, tls_authz, max_connections, errp);
qapi_free_SocketAddress(addr_flat);
}
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index b6f678d3ab..0fcab6ed2d 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -101,9 +101,9 @@ static void help(void)
" configure a QMP monitor\n"
"\n"
" --nbd-server addr.type=inet,addr.host=<host>,addr.port=<port>\n"
-" [,tls-creds=<id>][,tls-authz=<id>]\n"
+" [,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]\n"
" --nbd-server addr.type=unix,addr.path=<path>\n"
-" [,tls-creds=<id>][,tls-authz=<id>]\n"
+" [,tls-creds=<id>][,tls-authz=<id>][,max-connections=<n>]\n"
" start an NBD server for exporting block nodes\n"
"\n"
" --object help list object types that can be added\n"
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 09/29] nbd: Add writethrough to block-export-add
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (7 preceding siblings ...)
2020-09-07 18:19 ` [PATCH 08/29] nbd: Add max-connections to nbd-server-start Kevin Wolf
@ 2020-09-07 18:19 ` Kevin Wolf
2020-09-10 11:15 ` Max Reitz
2020-09-07 18:19 ` [PATCH 10/29] nbd: Remove NBDExport.close callback Kevin Wolf
` (20 subsequent siblings)
29 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
qemu-nbd allows use of writethrough cache modes, which mean that write
requests made through NBD will cause a flush before they complete.
Expose the same functionality in block-export-add.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-export.json | 7 ++++++-
blockdev-nbd.c | 6 +++++-
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index 7dedd893ab..d95c082a33 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -170,10 +170,15 @@
# Describes a block export, i.e. how single node should be exported on an
# external interface.
#
+# @writethrough: If true, caches are flushed after every write request to the
+# export before completion is signalled. (since: 5.2;
+# default: false)
+#
# Since: 4.2
##
{ 'union': 'BlockExportOptions',
- 'base': { 'type': 'BlockExportType' },
+ 'base': { 'type': 'BlockExportType',
+ '*writethrough': 'bool' },
'discriminator': 'type',
'data': {
'nbd': 'BlockExportOptionsNbd'
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 41d5542987..09247a8ded 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -216,9 +216,13 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
goto out;
}
+ if (!exp_args->has_writethrough) {
+ exp_args->writethrough = false;
+ }
+
exp = nbd_export_new(bs, arg->name, arg->description, arg->bitmap,
!arg->writable, !arg->writable,
- NULL, false, errp);
+ NULL, exp_args->writethrough, errp);
if (!exp) {
goto out;
}
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 10/29] nbd: Remove NBDExport.close callback
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (8 preceding siblings ...)
2020-09-07 18:19 ` [PATCH 09/29] nbd: Add writethrough to block-export-add Kevin Wolf
@ 2020-09-07 18:19 ` Kevin Wolf
2020-09-07 18:19 ` [PATCH 11/29] qemu-nbd: Use blk_exp_add() to create the export Kevin Wolf
` (19 subsequent siblings)
29 siblings, 0 replies; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
The export close callback is unused by the built-in NBD server. qemu-nbd
uses it only during shutdown to wait for the unrefed export to actually
go away. It can just use nbd_export_close_all() instead and do without
the callback.
This removes the close callback from nbd_export_new() and makes both
callers of it more similar.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
include/block/nbd.h | 3 +--
blockdev-nbd.c | 2 +-
nbd/server.c | 9 +--------
qemu-nbd.c | 14 ++++----------
4 files changed, 7 insertions(+), 21 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index acccdb3180..9449b3dac4 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -334,8 +334,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp);
NBDExport *nbd_export_new(BlockDriverState *bs,
const char *name, const char *desc,
const char *bitmap, bool readonly, bool shared,
- void (*close)(NBDExport *), bool writethrough,
- Error **errp);
+ bool writethrough, Error **errp);
void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk);
void nbd_export_close(NBDExport *exp);
void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 09247a8ded..3a51b3e792 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -222,7 +222,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
exp = nbd_export_new(bs, arg->name, arg->description, arg->bitmap,
!arg->writable, !arg->writable,
- NULL, exp_args->writethrough, errp);
+ exp_args->writethrough, errp);
if (!exp) {
goto out;
}
diff --git a/nbd/server.c b/nbd/server.c
index 23d9a53094..1cc915f01d 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -84,7 +84,6 @@ struct NBDRequestData {
struct NBDExport {
BlockExport common;
int refcount;
- void (*close)(NBDExport *exp);
BlockBackend *blk;
char *name;
@@ -1521,8 +1520,7 @@ void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk)
NBDExport *nbd_export_new(BlockDriverState *bs,
const char *name, const char *desc,
const char *bitmap, bool readonly, bool shared,
- void (*close)(NBDExport *), bool writethrough,
- Error **errp)
+ bool writethrough, Error **errp)
{
AioContext *ctx;
BlockBackend *blk;
@@ -1625,7 +1623,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
}
- exp->close = close;
exp->ctx = ctx;
blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
@@ -1723,10 +1720,6 @@ void nbd_export_put(NBDExport *exp)
assert(exp->name == NULL);
assert(exp->description == NULL);
- if (exp->close) {
- exp->close(exp);
- }
-
if (exp->blk) {
if (exp->eject_notifier_blk) {
notifier_remove(&exp->eject_notifier);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 2bba6c0db3..cab65529a5 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -70,7 +70,7 @@ static int verbose;
static char *srcpath;
static SocketAddress *saddr;
static int persistent = 0;
-static enum { RUNNING, TERMINATE, TERMINATING, TERMINATED } state;
+static enum { RUNNING, TERMINATE, TERMINATED } state;
static int shared = 1;
static int nb_fds;
static QIONetListener *server;
@@ -332,12 +332,6 @@ static int nbd_can_accept(void)
return state == RUNNING && nb_fds < shared;
}
-static void nbd_export_closed(NBDExport *export)
-{
- assert(state == TERMINATING);
- state = TERMINATED;
-}
-
static void nbd_update_server_watch(void);
static void nbd_client_closed(NBDClient *client, bool negotiated)
@@ -1067,7 +1061,7 @@ int main(int argc, char **argv)
export = nbd_export_new(bs, export_name,
export_description, bitmap, readonly, shared > 1,
- nbd_export_closed, writethrough, &error_fatal);
+ writethrough, &error_fatal);
if (device) {
#if HAVE_NBD_DEVICE
@@ -1107,10 +1101,10 @@ int main(int argc, char **argv)
do {
main_loop_wait(false);
if (state == TERMINATE) {
- state = TERMINATING;
- nbd_export_close(export);
nbd_export_put(export);
+ nbd_export_close_all();
export = NULL;
+ state = TERMINATED;
}
} while (state != TERMINATED);
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 11/29] qemu-nbd: Use blk_exp_add() to create the export
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (9 preceding siblings ...)
2020-09-07 18:19 ` [PATCH 10/29] nbd: Remove NBDExport.close callback Kevin Wolf
@ 2020-09-07 18:19 ` Kevin Wolf
2020-09-07 18:19 ` [PATCH 12/29] nbd/server: Simplify export shutdown Kevin Wolf
` (18 subsequent siblings)
29 siblings, 0 replies; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
With this change, NBD exports are now only created through the
BlockExport interface. This allows us finally to move things from the
NBD layer to the BlockExport layer if they make sense for other export
types, too.
blk_exp_add() returns only a weak reference, so the explicit
nbd_export_put() goes away.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
include/block/nbd.h | 1 +
blockdev-nbd.c | 8 +++++++-
qemu-nbd.c | 28 ++++++++++++++++++++++------
3 files changed, 30 insertions(+), 7 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 9449b3dac4..1dafe70615 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -352,6 +352,7 @@ void nbd_client_new(QIOChannelSocket *sioc,
void nbd_client_get(NBDClient *client);
void nbd_client_put(NBDClient *client);
+void nbd_server_is_qemu_nbd(bool value);
void nbd_server_start(SocketAddress *addr, const char *tls_creds,
const char *tls_authz, uint32_t max_connections,
Error **errp);
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 3a51b3e792..7bb0b09697 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -28,9 +28,15 @@ typedef struct NBDServerData {
} NBDServerData;
static NBDServerData *nbd_server;
+static bool is_qemu_nbd;
static void nbd_update_server_watch(NBDServerData *s);
+void nbd_server_is_qemu_nbd(bool value)
+{
+ is_qemu_nbd = value;
+}
+
static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
{
nbd_client_put(client);
@@ -176,7 +182,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD);
- if (!nbd_server) {
+ if (!nbd_server && !is_qemu_nbd) {
error_setg(errp, "NBD server not running");
return NULL;
}
diff --git a/qemu-nbd.c b/qemu-nbd.c
index cab65529a5..a863741e8f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -65,7 +65,6 @@
#define MBR_SIZE 512
-static NBDExport *export;
static int verbose;
static char *srcpath;
static SocketAddress *saddr;
@@ -580,6 +579,7 @@ int main(int argc, char **argv)
int old_stderr = -1;
unsigned socket_activation;
const char *pid_file_name = NULL;
+ BlockExportOptions *export_opts;
#if HAVE_NBD_DEVICE
/* The client thread uses SIGTERM to interrupt the server. A signal
@@ -1059,9 +1059,27 @@ int main(int argc, char **argv)
bs->detect_zeroes = detect_zeroes;
- export = nbd_export_new(bs, export_name,
- export_description, bitmap, readonly, shared > 1,
- writethrough, &error_fatal);
+ nbd_server_is_qemu_nbd(true);
+
+ export_opts = g_new(BlockExportOptions, 1);
+ *export_opts = (BlockExportOptions) {
+ .type = BLOCK_EXPORT_TYPE_NBD,
+ .has_writethrough = true,
+ .writethrough = writethrough,
+ .u.nbd = {
+ .device = g_strdup(bdrv_get_node_name(bs)),
+ .has_name = true,
+ .name = g_strdup(export_name),
+ .has_description = !!export_description,
+ .description = g_strdup(export_description),
+ .has_writable = true,
+ .writable = !readonly,
+ .has_bitmap = !!bitmap,
+ .bitmap = g_strdup(bitmap),
+ },
+ };
+ blk_exp_add(export_opts, &error_fatal);
+ qapi_free_BlockExportOptions(export_opts);
if (device) {
#if HAVE_NBD_DEVICE
@@ -1101,9 +1119,7 @@ int main(int argc, char **argv)
do {
main_loop_wait(false);
if (state == TERMINATE) {
- nbd_export_put(export);
nbd_export_close_all();
- export = NULL;
state = TERMINATED;
}
} while (state != TERMINATED);
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 12/29] nbd/server: Simplify export shutdown
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (10 preceding siblings ...)
2020-09-07 18:19 ` [PATCH 11/29] qemu-nbd: Use blk_exp_add() to create the export Kevin Wolf
@ 2020-09-07 18:19 ` Kevin Wolf
2020-09-07 18:19 ` [PATCH 13/29] block/export: Move refcount from NBDExport to BlockExport Kevin Wolf
` (17 subsequent siblings)
29 siblings, 0 replies; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Closing export is somewhat convoluted because nbd_export_close() and
nbd_export_put() call each other and the ways they actually end up being
nested is not necessarily obvious.
However, it is not really necessary to call nbd_export_close() from
nbd_export_put() when putting the last reference because it only does
three things:
1. Close all clients. We're going to refcount 0 and all clients hold a
reference, so we know there is no active client any more.
2. Close the user reference (represented by exp->name being non-NULL).
The same argument applies: If the export were still named, we would
still have a reference.
3. Freeing exp->description. This is really cleanup work to be done when
the export is finally freed. There is no reason to already clear it
while clients are still in the process of shutting down.
So after moving the cleanup of exp->description, the code can be
simplified so that only nbd_export_close() calls nbd_export_put(), but
never the other way around.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
nbd/server.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 1cc915f01d..fb70374df5 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1678,8 +1678,6 @@ void nbd_export_close(NBDExport *exp)
QTAILQ_REMOVE(&exports, exp, next);
QTAILQ_INSERT_TAIL(&closed_exports, exp, next);
}
- g_free(exp->description);
- exp->description = NULL;
nbd_export_put(exp);
}
@@ -1706,19 +1704,12 @@ void nbd_export_get(NBDExport *exp)
void nbd_export_put(NBDExport *exp)
{
assert(exp->refcount > 0);
- if (exp->refcount == 1) {
- nbd_export_close(exp);
- }
-
- /* nbd_export_close() may theoretically reduce refcount to 0. It may happen
- * if someone calls nbd_export_put() on named export not through
- * nbd_export_set_name() when refcount is 1. So, let's assert that
- * it is > 0.
- */
- assert(exp->refcount > 0);
if (--exp->refcount == 0) {
assert(exp->name == NULL);
- assert(exp->description == NULL);
+ assert(QTAILQ_EMPTY(&exp->clients));
+
+ g_free(exp->description);
+ exp->description = NULL;
if (exp->blk) {
if (exp->eject_notifier_blk) {
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 13/29] block/export: Move refcount from NBDExport to BlockExport
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (11 preceding siblings ...)
2020-09-07 18:19 ` [PATCH 12/29] nbd/server: Simplify export shutdown Kevin Wolf
@ 2020-09-07 18:19 ` Kevin Wolf
2020-09-07 18:19 ` [PATCH 14/29] block/export: Move AioContext " Kevin Wolf
` (16 subsequent siblings)
29 siblings, 0 replies; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Having a refcount makes sense for all types of block exports. It is also
a prerequisite for keeping a list of all exports at the BlockExport
level.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
include/block/export.h | 15 +++++++++
include/block/nbd.h | 2 --
block/export/export.c | 14 ++++++++
blockdev-nbd.c | 2 +-
nbd/server.c | 72 +++++++++++++++++++-----------------------
5 files changed, 63 insertions(+), 42 deletions(-)
diff --git a/include/block/export.h b/include/block/export.h
index e7af2c7687..5236a35e12 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -24,12 +24,27 @@ typedef struct BlockExportDriver {
/* Creates and starts a new block export */
BlockExport *(*create)(BlockExportOptions *, Error **);
+
+ /*
+ * Frees a removed block export. This function is only called after all
+ * references have been dropped.
+ */
+ void (*delete)(BlockExport *);
} BlockExportDriver;
struct BlockExport {
const BlockExportDriver *drv;
+
+ /*
+ * Reference count for this block export. This includes strong references
+ * both from the owner (qemu-nbd or the monitor) and clients connected to
+ * the export.
+ */
+ int refcount;
};
BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp);
+void blk_exp_ref(BlockExport *exp);
+void blk_exp_unref(BlockExport *exp);
#endif
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 1dafe70615..e3bd112227 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -338,8 +338,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk);
void nbd_export_close(NBDExport *exp);
void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
-void nbd_export_get(NBDExport *exp);
-void nbd_export_put(NBDExport *exp);
AioContext *nbd_export_aio_context(NBDExport *exp);
NBDExport *nbd_export_find(const char *name);
diff --git a/block/export/export.c b/block/export/export.c
index 05bc5e3744..baba4e94ff 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -49,6 +49,20 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
return drv->create(export, errp);
}
+void blk_exp_ref(BlockExport *exp)
+{
+ assert(exp->refcount > 0);
+ exp->refcount++;
+}
+
+void blk_exp_unref(BlockExport *exp)
+{
+ assert(exp->refcount > 0);
+ if (--exp->refcount == 0) {
+ exp->drv->delete(exp);
+ }
+}
+
void qmp_block_export_add(BlockExportOptions *export, Error **errp)
{
blk_exp_add(export, errp);
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 7bb0b09697..e208324d42 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -236,7 +236,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
/* The list of named exports has a strong reference to this export now and
* our only way of accessing it is through nbd_export_find(), so we can drop
* the strong reference that is @exp. */
- nbd_export_put(exp);
+ blk_exp_unref((BlockExport*) exp);
out:
aio_context_release(aio_context);
diff --git a/nbd/server.c b/nbd/server.c
index fb70374df5..7be81c7bda 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -83,7 +83,6 @@ struct NBDRequestData {
struct NBDExport {
BlockExport common;
- int refcount;
BlockBackend *blk;
char *name;
@@ -499,7 +498,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
}
QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
- nbd_export_get(client->exp);
+ blk_exp_ref(&client->exp->common);
nbd_check_meta_export(client);
return 0;
@@ -707,7 +706,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
client->exp = exp;
client->check_align = check_align;
QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
- nbd_export_get(client->exp);
+ blk_exp_ref(&client->exp->common);
nbd_check_meta_export(client);
rc = 1;
}
@@ -1406,7 +1405,7 @@ void nbd_client_put(NBDClient *client)
g_free(client->tlsauthz);
if (client->exp) {
QTAILQ_REMOVE(&client->exp->clients, client, next);
- nbd_export_put(client->exp);
+ blk_exp_unref(&client->exp->common);
}
g_free(client);
}
@@ -1538,7 +1537,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
exp = g_new0(NBDExport, 1);
exp->common = (BlockExport) {
- .drv = &blk_exp_nbd,
+ .drv = &blk_exp_nbd,
+ .refcount = 1,
};
/*
@@ -1567,7 +1567,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
blk_set_enable_write_cache(blk, !writethrough);
blk_set_allow_aio_context_change(blk, true);
- exp->refcount = 1;
QTAILQ_INIT(&exp->clients);
exp->blk = blk;
exp->name = g_strdup(name);
@@ -1626,8 +1625,9 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
exp->ctx = ctx;
blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
+ blk_exp_ref(&exp->common);
QTAILQ_INSERT_TAIL(&exports, exp, next);
- nbd_export_get(exp);
+
return exp;
fail:
@@ -1660,7 +1660,7 @@ void nbd_export_close(NBDExport *exp)
{
NBDClient *client, *next;
- nbd_export_get(exp);
+ blk_exp_ref(&exp->common);
/*
* TODO: Should we expand QMP NbdServerRemoveNode enum to allow a
* close mode that stops advertising the export to new clients but
@@ -1672,13 +1672,13 @@ void nbd_export_close(NBDExport *exp)
client_close(client, true);
}
if (exp->name) {
- nbd_export_put(exp);
+ blk_exp_unref(&exp->common);
g_free(exp->name);
exp->name = NULL;
QTAILQ_REMOVE(&exports, exp, next);
QTAILQ_INSERT_TAIL(&closed_exports, exp, next);
}
- nbd_export_put(exp);
+ blk_exp_unref(&exp->common);
}
void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp)
@@ -1695,47 +1695,41 @@ void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp)
error_append_hint(errp, "Use mode='hard' to force client disconnect\n");
}
-void nbd_export_get(NBDExport *exp)
-{
- assert(exp->refcount > 0);
- exp->refcount++;
-}
-
-void nbd_export_put(NBDExport *exp)
+static void nbd_export_delete(BlockExport *blk_exp)
{
- assert(exp->refcount > 0);
- if (--exp->refcount == 0) {
- assert(exp->name == NULL);
- assert(QTAILQ_EMPTY(&exp->clients));
+ NBDExport *exp = container_of(blk_exp, NBDExport, common);
- g_free(exp->description);
- exp->description = NULL;
+ assert(exp->name == NULL);
+ assert(QTAILQ_EMPTY(&exp->clients));
- if (exp->blk) {
- if (exp->eject_notifier_blk) {
- notifier_remove(&exp->eject_notifier);
- blk_unref(exp->eject_notifier_blk);
- }
- blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
- blk_aio_detach, exp);
- blk_unref(exp->blk);
- exp->blk = NULL;
- }
+ g_free(exp->description);
+ exp->description = NULL;
- if (exp->export_bitmap) {
- bdrv_dirty_bitmap_set_busy(exp->export_bitmap, false);
- g_free(exp->export_bitmap_context);
+ if (exp->blk) {
+ if (exp->eject_notifier_blk) {
+ notifier_remove(&exp->eject_notifier);
+ blk_unref(exp->eject_notifier_blk);
}
+ blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
+ blk_aio_detach, exp);
+ blk_unref(exp->blk);
+ exp->blk = NULL;
+ }
- QTAILQ_REMOVE(&closed_exports, exp, next);
- g_free(exp);
- aio_wait_kick();
+ if (exp->export_bitmap) {
+ bdrv_dirty_bitmap_set_busy(exp->export_bitmap, false);
+ g_free(exp->export_bitmap_context);
}
+
+ QTAILQ_REMOVE(&closed_exports, exp, next);
+ g_free(exp);
+ aio_wait_kick();
}
const BlockExportDriver blk_exp_nbd = {
.type = BLOCK_EXPORT_TYPE_NBD,
.create = nbd_export_create,
+ .delete = nbd_export_delete,
};
void nbd_export_close_all(void)
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 14/29] block/export: Move AioContext from NBDExport to BlockExport
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (12 preceding siblings ...)
2020-09-07 18:19 ` [PATCH 13/29] block/export: Move refcount from NBDExport to BlockExport Kevin Wolf
@ 2020-09-07 18:19 ` Kevin Wolf
2020-09-10 11:52 ` Max Reitz
2020-09-07 18:19 ` [PATCH 15/29] block/export: Add node-name to BlockExportOptions Kevin Wolf
` (15 subsequent siblings)
29 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/export.h | 3 +++
block/export/export.c | 2 ++
nbd/server.c | 26 +++++++++++++-------------
3 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/include/block/export.h b/include/block/export.h
index 5236a35e12..e6f96f4e1e 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -41,6 +41,9 @@ struct BlockExport {
* the export.
*/
int refcount;
+
+ /* The AioContext whose lock protects this BlockExport object. */
+ AioContext *ctx;
};
BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp);
diff --git a/block/export/export.c b/block/export/export.c
index baba4e94ff..8635318ef1 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -49,12 +49,14 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
return drv->create(export, errp);
}
+/* Callers must hold exp->ctx lock */
void blk_exp_ref(BlockExport *exp)
{
assert(exp->refcount > 0);
exp->refcount++;
}
+/* Callers must hold exp->ctx lock */
void blk_exp_unref(BlockExport *exp)
{
assert(exp->refcount > 0);
diff --git a/nbd/server.c b/nbd/server.c
index 7be81c7bda..7cf81646fc 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -92,8 +92,6 @@ struct NBDExport {
QTAILQ_HEAD(, NBDClient) clients;
QTAILQ_ENTRY(NBDExport) next;
- AioContext *ctx;
-
BlockBackend *eject_notifier_blk;
Notifier eject_notifier;
@@ -1333,8 +1331,8 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
}
/* Attach the channel to the same AioContext as the export */
- if (client->exp && client->exp->ctx) {
- qio_channel_attach_aio_context(client->ioc, client->exp->ctx);
+ if (client->exp && client->exp->common.ctx) {
+ qio_channel_attach_aio_context(client->ioc, client->exp->common.ctx);
}
assert(!client->optlen);
@@ -1466,7 +1464,7 @@ static void blk_aio_attached(AioContext *ctx, void *opaque)
trace_nbd_blk_aio_attached(exp->name, ctx);
- exp->ctx = ctx;
+ exp->common.ctx = ctx;
QTAILQ_FOREACH(client, &exp->clients, next) {
qio_channel_attach_aio_context(client->ioc, ctx);
@@ -1484,13 +1482,13 @@ static void blk_aio_detach(void *opaque)
NBDExport *exp = opaque;
NBDClient *client;
- trace_nbd_blk_aio_detach(exp->name, exp->ctx);
+ trace_nbd_blk_aio_detach(exp->name, exp->common.ctx);
QTAILQ_FOREACH(client, &exp->clients, next) {
qio_channel_detach_aio_context(client->ioc);
}
- exp->ctx = NULL;
+ exp->common.ctx = NULL;
}
static void nbd_eject_notifier(Notifier *n, void *data)
@@ -1498,7 +1496,7 @@ static void nbd_eject_notifier(Notifier *n, void *data)
NBDExport *exp = container_of(n, NBDExport, eject_notifier);
AioContext *aio_context;
- aio_context = exp->ctx;
+ aio_context = exp->common.ctx;
aio_context_acquire(aio_context);
nbd_export_close(exp);
aio_context_release(aio_context);
@@ -1535,10 +1533,13 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
return NULL;
}
+ ctx = bdrv_get_aio_context(bs);
+
exp = g_new0(NBDExport, 1);
exp->common = (BlockExport) {
.drv = &blk_exp_nbd,
.refcount = 1,
+ .ctx = ctx,
};
/*
@@ -1548,7 +1549,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
* ctx was acquired in the caller.
*/
assert(name && strlen(name) <= NBD_MAX_STRING_SIZE);
- ctx = bdrv_get_aio_context(bs);
+
bdrv_invalidate_cache(bs, NULL);
/* Don't allow resize while the NBD server is running, otherwise we don't
@@ -1622,7 +1623,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
}
- exp->ctx = ctx;
blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
blk_exp_ref(&exp->common);
@@ -1653,7 +1653,7 @@ NBDExport *nbd_export_find(const char *name)
AioContext *
nbd_export_aio_context(NBDExport *exp)
{
- return exp->ctx;
+ return exp->common.ctx;
}
void nbd_export_close(NBDExport *exp)
@@ -1738,7 +1738,7 @@ void nbd_export_close_all(void)
AioContext *aio_context;
QTAILQ_FOREACH_SAFE(exp, &exports, next, next) {
- aio_context = exp->ctx;
+ aio_context = exp->common.ctx;
aio_context_acquire(aio_context);
nbd_export_close(exp);
aio_context_release(aio_context);
@@ -2537,7 +2537,7 @@ static void nbd_client_receive_next_request(NBDClient *client)
if (!client->recv_coroutine && client->nb_requests < MAX_NBD_REQUESTS) {
nbd_client_get(client);
client->recv_coroutine = qemu_coroutine_create(nbd_trip, client);
- aio_co_schedule(client->exp->ctx, client->recv_coroutine);
+ aio_co_schedule(client->exp->common.ctx, client->recv_coroutine);
}
}
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 15/29] block/export: Add node-name to BlockExportOptions
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (13 preceding siblings ...)
2020-09-07 18:19 ` [PATCH 14/29] block/export: Move AioContext " Kevin Wolf
@ 2020-09-07 18:19 ` Kevin Wolf
2020-09-10 12:35 ` Max Reitz
2020-09-07 18:19 ` [PATCH 16/29] block/export: Allocate BlockExport in blk_exp_add() Kevin Wolf
` (14 subsequent siblings)
29 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Every block export needs a block node to export, so add a 'node-name'
option to BlockExportOptions and remove the replaced option 'device'
from BlockExportOptionsNbd.
To maintain compatibility in nbd-server-add, BlockExportOptionsNbd needs
to be wrapped by a new type NbdServerAddOptions that adds 'device' back
because nbd-server-add doesn't use the BlockExportOptions base type at
all (so even without changing it to a 'node-name' option in
block-export-add, this compatibility code would be necessary).
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-export.json | 27 ++++++++++++++++-----
block/monitor/block-hmp-cmds.c | 6 ++---
blockdev-nbd.c | 44 +++++++++++++++++++++++++---------
qemu-nbd.c | 2 +-
4 files changed, 58 insertions(+), 21 deletions(-)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index d95c082a33..4d22f46c94 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -65,9 +65,8 @@
##
# @BlockExportOptionsNbd:
#
-# An NBD block export.
-#
-# @device: The device name or node name of the node to be exported
+# An NBD block export (options shared between nbd-server-add and the NBD branch
+# of block-export-add).
#
# @name: Export name. If unspecified, the @device parameter is used as the
# export name. (Since 2.12)
@@ -85,8 +84,21 @@
# Since: 5.0
##
{ 'struct': 'BlockExportOptionsNbd',
- 'data': {'device': 'str', '*name': 'str', '*description': 'str',
- '*writable': 'bool', '*bitmap': 'str' } }
+ 'data': { '*name': 'str', '*description': 'str',
+ '*writable': 'bool', '*bitmap': 'str' } }
+
+##
+# @NbdServerAddOptions:
+#
+# An NBD block export.
+#
+# @device: The device name or node name of the node to be exported
+#
+# Since: 5.0
+##
+{ 'struct': 'NbdServerAddOptions',
+ 'base': 'BlockExportOptionsNbd',
+ 'data': { 'device': 'str' } }
##
# @nbd-server-add:
@@ -99,7 +111,7 @@
# Since: 1.3.0
##
{ 'command': 'nbd-server-add',
- 'data': 'BlockExportOptionsNbd', 'boxed': true }
+ 'data': 'NbdServerAddOptions', 'boxed': true }
##
# @NbdServerRemoveMode:
@@ -170,6 +182,8 @@
# Describes a block export, i.e. how single node should be exported on an
# external interface.
#
+# @node-name: The node name of the block node to be exported (since: 5.2)
+#
# @writethrough: If true, caches are flushed after every write request to the
# export before completion is signalled. (since: 5.2;
# default: false)
@@ -178,6 +192,7 @@
##
{ 'union': 'BlockExportOptions',
'base': { 'type': 'BlockExportType',
+ 'node-name': 'str',
'*writethrough': 'bool' },
'discriminator': 'type',
'data': {
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 662b7f7d00..db357cafcb 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -398,7 +398,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
Error *local_err = NULL;
BlockInfoList *block_list, *info;
SocketAddress *addr;
- BlockExportOptionsNbd export;
+ NbdServerAddOptions export;
if (writable && !all) {
error_setg(&local_err, "-w only valid together with -a");
@@ -431,7 +431,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
continue;
}
- export = (BlockExportOptionsNbd) {
+ export = (NbdServerAddOptions) {
.device = info->value->device,
.has_writable = true,
.writable = writable,
@@ -458,7 +458,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
bool writable = qdict_get_try_bool(qdict, "writable", false);
Error *local_err = NULL;
- BlockExportOptionsNbd export = {
+ NbdServerAddOptions export = {
.device = (char *) device,
.has_name = !!name,
.name = (char *) name,
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index e208324d42..ef14303b25 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -188,7 +188,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
}
if (!arg->has_name) {
- arg->name = arg->device;
+ arg->name = exp_args->node_name;
}
if (strlen(arg->name) > NBD_MAX_STRING_SIZE) {
@@ -206,7 +206,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
return NULL;
}
- bs = bdrv_lookup_bs(arg->device, arg->device, errp);
+ bs = bdrv_lookup_bs(NULL, exp_args->node_name, errp);
if (!bs) {
return NULL;
}
@@ -244,21 +244,40 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
return (BlockExport*) exp;
}
-void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
+void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
{
BlockExport *export;
BlockDriverState *bs;
BlockBackend *on_eject_blk;
- BlockExportOptions export_opts;
+ BlockExportOptions *export_opts;
bs = bdrv_lookup_bs(arg->device, arg->device, errp);
if (!bs) {
return;
}
- export_opts = (BlockExportOptions) {
- .type = BLOCK_EXPORT_TYPE_NBD,
- .u.nbd = *arg,
+ /*
+ * block-export-add would default to the node-name, but we may have to use
+ * the device name as a default here for compatibility.
+ */
+ if (!arg->has_name) {
+ arg->name = arg->device;
+ }
+
+ export_opts = g_new(BlockExportOptions, 1);
+ *export_opts = (BlockExportOptions) {
+ .type = BLOCK_EXPORT_TYPE_NBD,
+ .node_name = g_strdup(bdrv_get_node_name(bs)),
+ .u.nbd = {
+ .has_name = true,
+ .name = g_strdup(arg->name),
+ .has_description = arg->has_description,
+ .description = g_strdup(arg->description),
+ .has_writable = arg->has_writable,
+ .writable = arg->writable,
+ .has_bitmap = arg->has_bitmap,
+ .bitmap = g_strdup(arg->bitmap),
+ },
};
/*
@@ -267,13 +286,13 @@ void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
* block-export-add.
*/
if (bdrv_is_read_only(bs)) {
- export_opts.u.nbd.has_writable = true;
- export_opts.u.nbd.writable = false;
+ export_opts->u.nbd.has_writable = true;
+ export_opts->u.nbd.writable = false;
}
- export = blk_exp_add(&export_opts, errp);
+ export = blk_exp_add(export_opts, errp);
if (!export) {
- return;
+ goto fail;
}
/*
@@ -284,6 +303,9 @@ void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
if (on_eject_blk) {
nbd_export_set_on_eject_blk(export, on_eject_blk);
}
+
+fail:
+ qapi_free_BlockExportOptions(export_opts);
}
void qmp_nbd_server_remove(const char *name,
diff --git a/qemu-nbd.c b/qemu-nbd.c
index a863741e8f..23c2b7da8e 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1064,10 +1064,10 @@ int main(int argc, char **argv)
export_opts = g_new(BlockExportOptions, 1);
*export_opts = (BlockExportOptions) {
.type = BLOCK_EXPORT_TYPE_NBD,
+ .node_name = g_strdup(bdrv_get_node_name(bs)),
.has_writethrough = true,
.writethrough = writethrough,
.u.nbd = {
- .device = g_strdup(bdrv_get_node_name(bs)),
.has_name = true,
.name = g_strdup(export_name),
.has_description = !!export_description,
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 15/29] block/export: Add node-name to BlockExportOptions
2020-09-07 18:19 ` [PATCH 15/29] block/export: Add node-name to BlockExportOptions Kevin Wolf
@ 2020-09-10 12:35 ` Max Reitz
0 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2020-09-10 12:35 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 960 bytes --]
On 07.09.20 20:19, Kevin Wolf wrote:
> Every block export needs a block node to export, so add a 'node-name'
> option to BlockExportOptions and remove the replaced option 'device'
> from BlockExportOptionsNbd.
>
> To maintain compatibility in nbd-server-add, BlockExportOptionsNbd needs
> to be wrapped by a new type NbdServerAddOptions that adds 'device' back
> because nbd-server-add doesn't use the BlockExportOptions base type at
> all (so even without changing it to a 'node-name' option in
> block-export-add, this compatibility code would be necessary).
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-export.json | 27 ++++++++++++++++-----
> block/monitor/block-hmp-cmds.c | 6 ++---
> blockdev-nbd.c | 44 +++++++++++++++++++++++++---------
> qemu-nbd.c | 2 +-
> 4 files changed, 58 insertions(+), 21 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 16/29] block/export: Allocate BlockExport in blk_exp_add()
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (14 preceding siblings ...)
2020-09-07 18:19 ` [PATCH 15/29] block/export: Add node-name to BlockExportOptions Kevin Wolf
@ 2020-09-07 18:19 ` Kevin Wolf
2020-09-16 10:56 ` Max Reitz
2020-09-07 18:19 ` [PATCH 17/29] block/export: Add blk_exp_close_all(_type) Kevin Wolf
` (13 subsequent siblings)
29 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Instead of letting the driver allocate and return the BlockExport
object, allocate it already in blk_exp_add() and pass it. This allows us
to initialise the generic part before calling into the driver so that
the driver can just use these values instead of having to parse the
options a second time.
For symmetry, move freeing the BlockExport to blk_exp_unref().
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
include/block/export.h | 8 +++++++-
include/block/nbd.h | 11 ++++++-----
block/export/export.c | 18 +++++++++++++++++-
blockdev-nbd.c | 26 ++++++++++++++------------
nbd/server.c | 30 +++++++++++++-----------------
5 files changed, 57 insertions(+), 36 deletions(-)
diff --git a/include/block/export.h b/include/block/export.h
index e6f96f4e1e..cf9b1c9dad 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -22,8 +22,14 @@ typedef struct BlockExportDriver {
/* The export type that this driver services */
BlockExportType type;
+ /*
+ * The size of the driver-specific state that contains BlockExport as its
+ * first field.
+ */
+ size_t instance_size;
+
/* Creates and starts a new block export */
- BlockExport *(*create)(BlockExportOptions *, Error **);
+ int (*create)(BlockExport *, BlockExportOptions *, Error **);
/*
* Frees a removed block export. This function is only called after all
diff --git a/include/block/nbd.h b/include/block/nbd.h
index e3bd112227..fc2c153d5b 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -330,11 +330,12 @@ int nbd_errno_to_system_errno(int err);
typedef struct NBDExport NBDExport;
typedef struct NBDClient NBDClient;
-BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp);
-NBDExport *nbd_export_new(BlockDriverState *bs,
- const char *name, const char *desc,
- const char *bitmap, bool readonly, bool shared,
- bool writethrough, Error **errp);
+int nbd_export_create(BlockExport *exp, BlockExportOptions *exp_args,
+ Error **errp);
+int nbd_export_new(BlockExport *blk_exp, BlockDriverState *bs,
+ const char *name, const char *desc,
+ const char *bitmap, bool readonly, bool shared,
+ bool writethrough, Error **errp);
void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk);
void nbd_export_close(NBDExport *exp);
void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
diff --git a/block/export/export.c b/block/export/export.c
index 8635318ef1..03ff155f97 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -39,6 +39,8 @@ static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
{
const BlockExportDriver *drv;
+ BlockExport *exp;
+ int ret;
drv = blk_exp_find_driver(export->type);
if (!drv) {
@@ -46,7 +48,20 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
return NULL;
}
- return drv->create(export, errp);
+ assert(drv->instance_size >= sizeof(BlockExport));
+ exp = g_malloc0(drv->instance_size);
+ *exp = (BlockExport) {
+ .drv = &blk_exp_nbd,
+ .refcount = 1,
+ };
+
+ ret = drv->create(exp, export, errp);
+ if (ret < 0) {
+ g_free(exp);
+ return NULL;
+ }
+
+ return exp;
}
/* Callers must hold exp->ctx lock */
@@ -62,6 +77,7 @@ void blk_exp_unref(BlockExport *exp)
assert(exp->refcount > 0);
if (--exp->refcount == 0) {
exp->drv->delete(exp);
+ g_free(exp);
}
}
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index ef14303b25..b34f159888 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -173,18 +173,19 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
qapi_free_SocketAddress(addr_flat);
}
-BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
+int nbd_export_create(BlockExport *exp, BlockExportOptions *exp_args,
+ Error **errp)
{
BlockExportOptionsNbd *arg = &exp_args->u.nbd;
BlockDriverState *bs = NULL;
- NBDExport *exp = NULL;
AioContext *aio_context;
+ int ret;
assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD);
if (!nbd_server && !is_qemu_nbd) {
error_setg(errp, "NBD server not running");
- return NULL;
+ return -EINVAL;
}
if (!arg->has_name) {
@@ -193,22 +194,22 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
if (strlen(arg->name) > NBD_MAX_STRING_SIZE) {
error_setg(errp, "export name '%s' too long", arg->name);
- return NULL;
+ return -EINVAL;
}
if (arg->description && strlen(arg->description) > NBD_MAX_STRING_SIZE) {
error_setg(errp, "description '%s' too long", arg->description);
- return NULL;
+ return -EINVAL;
}
if (nbd_export_find(arg->name)) {
error_setg(errp, "NBD server already has export named '%s'", arg->name);
- return NULL;
+ return -EEXIST;
}
bs = bdrv_lookup_bs(NULL, exp_args->node_name, errp);
if (!bs) {
- return NULL;
+ return -ENOENT;
}
aio_context = bdrv_get_aio_context(bs);
@@ -218,6 +219,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
arg->writable = false;
}
if (bdrv_is_read_only(bs) && arg->writable) {
+ ret = -EINVAL;
error_setg(errp, "Cannot export read-only node as writable");
goto out;
}
@@ -226,22 +228,22 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
exp_args->writethrough = false;
}
- exp = nbd_export_new(bs, arg->name, arg->description, arg->bitmap,
+ ret = nbd_export_new(exp, bs, arg->name, arg->description, arg->bitmap,
!arg->writable, !arg->writable,
exp_args->writethrough, errp);
- if (!exp) {
+ if (ret < 0) {
goto out;
}
/* The list of named exports has a strong reference to this export now and
* our only way of accessing it is through nbd_export_find(), so we can drop
* the strong reference that is @exp. */
- blk_exp_unref((BlockExport*) exp);
+ blk_exp_unref(exp);
+ ret = 0;
out:
aio_context_release(aio_context);
- /* TODO Remove the cast: nbd_export_new() will return a BlockExport. */
- return (BlockExport*) exp;
+ return ret;
}
void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
diff --git a/nbd/server.c b/nbd/server.c
index 7cf81646fc..f31d8bbb60 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1514,14 +1514,14 @@ void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk)
blk_add_remove_bs_notifier(blk, &nbd_exp->eject_notifier);
}
-NBDExport *nbd_export_new(BlockDriverState *bs,
- const char *name, const char *desc,
- const char *bitmap, bool readonly, bool shared,
- bool writethrough, Error **errp)
+int nbd_export_new(BlockExport *blk_exp, BlockDriverState *bs,
+ const char *name, const char *desc,
+ const char *bitmap, bool readonly, bool shared,
+ bool writethrough, Error **errp)
{
+ NBDExport *exp = container_of(blk_exp, NBDExport, common);
AioContext *ctx;
BlockBackend *blk;
- NBDExport *exp;
int64_t size;
uint64_t perm;
int ret;
@@ -1530,17 +1530,11 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
if (size < 0) {
error_setg_errno(errp, -size,
"Failed to determine the NBD export's length");
- return NULL;
+ return size;
}
ctx = bdrv_get_aio_context(bs);
-
- exp = g_new0(NBDExport, 1);
- exp->common = (BlockExport) {
- .drv = &blk_exp_nbd,
- .refcount = 1,
- .ctx = ctx,
- };
+ blk_exp->ctx = ctx;
/*
* NBD exports are used for non-shared storage migration. Make sure
@@ -1599,16 +1593,19 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
}
if (bm == NULL) {
+ ret = -ENOENT;
error_setg(errp, "Bitmap '%s' is not found", bitmap);
goto fail;
}
if (bdrv_dirty_bitmap_check(bm, BDRV_BITMAP_ALLOW_RO, errp)) {
+ ret = -EINVAL;
goto fail;
}
if (readonly && bdrv_is_writable(bs) &&
bdrv_dirty_bitmap_enabled(bm)) {
+ ret = -EINVAL;
error_setg(errp,
"Enabled bitmap '%s' incompatible with readonly export",
bitmap);
@@ -1628,14 +1625,13 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
blk_exp_ref(&exp->common);
QTAILQ_INSERT_TAIL(&exports, exp, next);
- return exp;
+ return 0;
fail:
blk_unref(blk);
g_free(exp->name);
g_free(exp->description);
- g_free(exp);
- return NULL;
+ return ret;
}
NBDExport *nbd_export_find(const char *name)
@@ -1722,12 +1718,12 @@ static void nbd_export_delete(BlockExport *blk_exp)
}
QTAILQ_REMOVE(&closed_exports, exp, next);
- g_free(exp);
aio_wait_kick();
}
const BlockExportDriver blk_exp_nbd = {
.type = BLOCK_EXPORT_TYPE_NBD,
+ .instance_size = sizeof(NBDExport),
.create = nbd_export_create,
.delete = nbd_export_delete,
};
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 16/29] block/export: Allocate BlockExport in blk_exp_add()
2020-09-07 18:19 ` [PATCH 16/29] block/export: Allocate BlockExport in blk_exp_add() Kevin Wolf
@ 2020-09-16 10:56 ` Max Reitz
0 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2020-09-16 10:56 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1878 bytes --]
On 07.09.20 20:19, Kevin Wolf wrote:
> Instead of letting the driver allocate and return the BlockExport
> object, allocate it already in blk_exp_add() and pass it. This allows us
> to initialise the generic part before calling into the driver so that
> the driver can just use these values instead of having to parse the
> options a second time.
>
> For symmetry, move freeing the BlockExport to blk_exp_unref().
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
> include/block/export.h | 8 +++++++-
> include/block/nbd.h | 11 ++++++-----
> block/export/export.c | 18 +++++++++++++++++-
> blockdev-nbd.c | 26 ++++++++++++++------------
> nbd/server.c | 30 +++++++++++++-----------------
> 5 files changed, 57 insertions(+), 36 deletions(-)
[...]
> diff --git a/block/export/export.c b/block/export/export.c
> index 8635318ef1..03ff155f97 100644
> --- a/block/export/export.c
> +++ b/block/export/export.c
> @@ -39,6 +39,8 @@ static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
> BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
> {
> const BlockExportDriver *drv;
> + BlockExport *exp;
> + int ret;
>
> drv = blk_exp_find_driver(export->type);
> if (!drv) {
> @@ -46,7 +48,20 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
> return NULL;
> }
>
> - return drv->create(export, errp);
> + assert(drv->instance_size >= sizeof(BlockExport));
> + exp = g_malloc0(drv->instance_size);
> + *exp = (BlockExport) {
> + .drv = &blk_exp_nbd,
Only noticed now when trying to base my FUSE stuff on this series:
s/blk_exp_nbd/drv/.
(I’d like to say “obviously”, but, well. Evidently not obvious enough
for me.)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 17/29] block/export: Add blk_exp_close_all(_type)
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (15 preceding siblings ...)
2020-09-07 18:19 ` [PATCH 16/29] block/export: Allocate BlockExport in blk_exp_add() Kevin Wolf
@ 2020-09-07 18:19 ` Kevin Wolf
2020-09-10 13:22 ` Max Reitz
2020-09-07 18:20 ` [PATCH 18/29] block/export: Add 'id' option to block-export-add Kevin Wolf
` (12 subsequent siblings)
29 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:19 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
This adds a function to shut down all block exports, and another one to
shut down the block exports of a single type. The latter is used for now
when stopping the NBD server. As soon as we implement support for
multiple NBD servers, we'll need a per-server list of exports and it
will be replaced by a function using that.
As a side effect, the BlockExport layer has a list tracking all existing
exports now. closed_exports loses its only user and can go away.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/export.h | 15 ++++++++
include/block/nbd.h | 2 --
block.c | 2 +-
block/export/export.c | 79 ++++++++++++++++++++++++++++++++++++++++--
blockdev-nbd.c | 2 +-
nbd/server.c | 34 +++---------------
qemu-nbd.c | 2 +-
7 files changed, 100 insertions(+), 36 deletions(-)
diff --git a/include/block/export.h b/include/block/export.h
index cf9b1c9dad..6fffcb5651 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -15,6 +15,7 @@
#define BLOCK_EXPORT_H
#include "qapi/qapi-types-block-export.h"
+#include "qemu/queue.h"
typedef struct BlockExport BlockExport;
@@ -36,6 +37,14 @@ typedef struct BlockExportDriver {
* references have been dropped.
*/
void (*delete)(BlockExport *);
+
+ /*
+ * Start to disconnect all clients and drop other references held
+ * internally by the export driver. When the function returns, there may
+ * still be active references while the export is in the process of
+ * shutting down.
+ */
+ void (*request_shutdown)(BlockExport *);
} BlockExportDriver;
struct BlockExport {
@@ -50,10 +59,16 @@ struct BlockExport {
/* The AioContext whose lock protects this BlockExport object. */
AioContext *ctx;
+
+ /* List entry for block_exports */
+ QLIST_ENTRY(BlockExport) next;
};
BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp);
void blk_exp_ref(BlockExport *exp);
void blk_exp_unref(BlockExport *exp);
+void blk_exp_request_shutdown(BlockExport *exp);
+void blk_exp_close_all(void);
+void blk_exp_close_all_type(BlockExportType type);
#endif
diff --git a/include/block/nbd.h b/include/block/nbd.h
index fc2c153d5b..0b9f3e5d4e 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -337,12 +337,10 @@ int nbd_export_new(BlockExport *blk_exp, BlockDriverState *bs,
const char *bitmap, bool readonly, bool shared,
bool writethrough, Error **errp);
void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk);
-void nbd_export_close(NBDExport *exp);
void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
AioContext *nbd_export_aio_context(NBDExport *exp);
NBDExport *nbd_export_find(const char *name);
-void nbd_export_close_all(void);
void nbd_client_new(QIOChannelSocket *sioc,
QCryptoTLSCreds *tlscreds,
diff --git a/block.c b/block.c
index 9538af4884..acde53f92a 100644
--- a/block.c
+++ b/block.c
@@ -4462,7 +4462,7 @@ static void bdrv_close(BlockDriverState *bs)
void bdrv_close_all(void)
{
assert(job_next(NULL) == NULL);
- nbd_export_close_all();
+ blk_exp_close_all();
/* Drop references from requests still in flight, such as canceled block
* jobs whose AIO context has not been polled yet */
diff --git a/block/export/export.c b/block/export/export.c
index 03ff155f97..1b36286010 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -24,6 +24,10 @@ static const BlockExportDriver *blk_exp_drivers[] = {
&blk_exp_nbd,
};
+/* Only accessed from the main thread */
+static QLIST_HEAD(, BlockExport) block_exports =
+ QLIST_HEAD_INITIALIZER(block_exports);
+
static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
{
int i;
@@ -61,6 +65,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
return NULL;
}
+ QLIST_INSERT_HEAD(&block_exports, exp, next);
return exp;
}
@@ -71,16 +76,86 @@ void blk_exp_ref(BlockExport *exp)
exp->refcount++;
}
+/* Runs in the main thread */
+static void blk_exp_delete_bh(void *opaque)
+{
+ BlockExport *exp = opaque;
+ AioContext *aio_context = exp->ctx;
+
+ aio_context_acquire(aio_context);
+
+ assert(exp->refcount == 0);
+ QLIST_REMOVE(exp, next);
+ exp->drv->delete(exp);
+ g_free(exp);
+
+ aio_context_release(aio_context);
+}
+
/* Callers must hold exp->ctx lock */
void blk_exp_unref(BlockExport *exp)
{
assert(exp->refcount > 0);
if (--exp->refcount == 0) {
- exp->drv->delete(exp);
- g_free(exp);
+ /* Touch the block_exports list only in the main thread */
+ aio_bh_schedule_oneshot(qemu_get_aio_context(), blk_exp_delete_bh,
+ exp);
}
}
+/* Acquires exp->ctx internally. Callers must *not* hold the lock. */
+void blk_exp_request_shutdown(BlockExport *exp)
+{
+ AioContext *aio_context = exp->ctx;
+
+ aio_context_acquire(aio_context);
+ exp->drv->request_shutdown(exp);
+ aio_context_release(aio_context);
+}
+
+/*
+ * Returns whether a block export of the given type exists.
+ * type == BLOCK_EXPORT_TYPE__MAX checks for an export of any type.
+ */
+static bool blk_exp_has_type(BlockExportType type)
+{
+ BlockExport *exp;
+
+ if (type == BLOCK_EXPORT_TYPE__MAX) {
+ return !QLIST_EMPTY(&block_exports);
+ }
+
+ QLIST_FOREACH(exp, &block_exports, next) {
+ if (exp->drv->type == type) {
+ return true;
+ }
+ }
+
+ return false;
+}
+
+/* type == BLOCK_EXPORT_TYPE__MAX for all types */
+void blk_exp_close_all_type(BlockExportType type)
+{
+ BlockExport *exp, *next;
+
+ assert(in_aio_context_home_thread(qemu_get_aio_context()));
+
+ QLIST_FOREACH_SAFE(exp, &block_exports, next, next) {
+ if (type != BLOCK_EXPORT_TYPE__MAX && exp->drv->type != type) {
+ continue;
+ }
+ blk_exp_request_shutdown(exp);
+ }
+
+ AIO_WAIT_WHILE(NULL, blk_exp_has_type(type));
+}
+
+void blk_exp_close_all(void)
+{
+ blk_exp_close_all_type(BLOCK_EXPORT_TYPE__MAX);
+}
+
void qmp_block_export_add(BlockExportOptions *export, Error **errp)
{
blk_exp_add(export, errp);
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index b34f159888..f927264777 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -345,7 +345,7 @@ void qmp_nbd_server_stop(Error **errp)
return;
}
- nbd_export_close_all();
+ blk_exp_close_all_type(BLOCK_EXPORT_TYPE_NBD);
nbd_server_free(nbd_server);
nbd_server = NULL;
diff --git a/nbd/server.c b/nbd/server.c
index f31d8bbb60..32147e4871 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -100,8 +100,6 @@ struct NBDExport {
};
static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
-static QTAILQ_HEAD(, NBDExport) closed_exports =
- QTAILQ_HEAD_INITIALIZER(closed_exports);
/* NBDExportMetaContexts represents a list of contexts to be exported,
* as selected by NBD_OPT_SET_META_CONTEXT. Also used for
@@ -1494,12 +1492,8 @@ static void blk_aio_detach(void *opaque)
static void nbd_eject_notifier(Notifier *n, void *data)
{
NBDExport *exp = container_of(n, NBDExport, eject_notifier);
- AioContext *aio_context;
- aio_context = exp->common.ctx;
- aio_context_acquire(aio_context);
- nbd_export_close(exp);
- aio_context_release(aio_context);
+ blk_exp_request_shutdown(&exp->common);
}
void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk)
@@ -1652,8 +1646,9 @@ nbd_export_aio_context(NBDExport *exp)
return exp->common.ctx;
}
-void nbd_export_close(NBDExport *exp)
+static void nbd_export_request_shutdown(BlockExport *blk_exp)
{
+ NBDExport *exp = container_of(blk_exp, NBDExport, common);
NBDClient *client, *next;
blk_exp_ref(&exp->common);
@@ -1672,7 +1667,6 @@ void nbd_export_close(NBDExport *exp)
g_free(exp->name);
exp->name = NULL;
QTAILQ_REMOVE(&exports, exp, next);
- QTAILQ_INSERT_TAIL(&closed_exports, exp, next);
}
blk_exp_unref(&exp->common);
}
@@ -1681,7 +1675,7 @@ void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp)
{
ERRP_GUARD();
if (mode == NBD_SERVER_REMOVE_MODE_HARD || QTAILQ_EMPTY(&exp->clients)) {
- nbd_export_close(exp);
+ nbd_export_request_shutdown(&exp->common);
return;
}
@@ -1716,9 +1710,6 @@ static void nbd_export_delete(BlockExport *blk_exp)
bdrv_dirty_bitmap_set_busy(exp->export_bitmap, false);
g_free(exp->export_bitmap_context);
}
-
- QTAILQ_REMOVE(&closed_exports, exp, next);
- aio_wait_kick();
}
const BlockExportDriver blk_exp_nbd = {
@@ -1726,24 +1717,9 @@ const BlockExportDriver blk_exp_nbd = {
.instance_size = sizeof(NBDExport),
.create = nbd_export_create,
.delete = nbd_export_delete,
+ .request_shutdown = nbd_export_request_shutdown,
};
-void nbd_export_close_all(void)
-{
- NBDExport *exp, *next;
- AioContext *aio_context;
-
- QTAILQ_FOREACH_SAFE(exp, &exports, next, next) {
- aio_context = exp->common.ctx;
- aio_context_acquire(aio_context);
- nbd_export_close(exp);
- aio_context_release(aio_context);
- }
-
- AIO_WAIT_WHILE(NULL, !(QTAILQ_EMPTY(&exports) &&
- QTAILQ_EMPTY(&closed_exports)));
-}
-
static int coroutine_fn nbd_co_send_iov(NBDClient *client, struct iovec *iov,
unsigned niov, Error **errp)
{
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 23c2b7da8e..c64f83618b 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1119,7 +1119,7 @@ int main(int argc, char **argv)
do {
main_loop_wait(false);
if (state == TERMINATE) {
- nbd_export_close_all();
+ blk_exp_close_all();
state = TERMINATED;
}
} while (state != TERMINATED);
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 17/29] block/export: Add blk_exp_close_all(_type)
2020-09-07 18:19 ` [PATCH 17/29] block/export: Add blk_exp_close_all(_type) Kevin Wolf
@ 2020-09-10 13:22 ` Max Reitz
0 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2020-09-10 13:22 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1567 bytes --]
On 07.09.20 20:19, Kevin Wolf wrote:
> This adds a function to shut down all block exports, and another one to
> shut down the block exports of a single type. The latter is used for now
> when stopping the NBD server. As soon as we implement support for
> multiple NBD servers, we'll need a per-server list of exports and it
> will be replaced by a function using that.
>
> As a side effect, the BlockExport layer has a list tracking all existing
> exports now. closed_exports loses its only user and can go away.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> include/block/export.h | 15 ++++++++
> include/block/nbd.h | 2 --
> block.c | 2 +-
> block/export/export.c | 79 ++++++++++++++++++++++++++++++++++++++++--
> blockdev-nbd.c | 2 +-
> nbd/server.c | 34 +++---------------
> qemu-nbd.c | 2 +-
> 7 files changed, 100 insertions(+), 36 deletions(-)
[...]
> /* Callers must hold exp->ctx lock */
> void blk_exp_unref(BlockExport *exp)
> {
> assert(exp->refcount > 0);
> if (--exp->refcount == 0) {
> - exp->drv->delete(exp);
> - g_free(exp);
> + /* Touch the block_exports list only in the main thread */
> + aio_bh_schedule_oneshot(qemu_get_aio_context(), blk_exp_delete_bh,
> + exp);
Looks safe.
Reviewed-by: Max Reitz <mreitz@redhat.com>
(The effort of special-casing this to delete the export immediately if
we already run in the main thread doesn’t look worth it.)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 18/29] block/export: Add 'id' option to block-export-add
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (16 preceding siblings ...)
2020-09-07 18:19 ` [PATCH 17/29] block/export: Add blk_exp_close_all(_type) Kevin Wolf
@ 2020-09-07 18:20 ` Kevin Wolf
2020-09-10 13:26 ` Max Reitz
2020-09-07 18:20 ` [PATCH 19/29] block/export: Move strong user reference to block_exports Kevin Wolf
` (11 subsequent siblings)
29 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:20 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
We'll need an id to identify block exports in monitor commands. This
adds one.
Note that this is different from the 'name' option in the NBD server,
which is the externally visible export name. While block export ids need
to be unique in the whole process, export names must be unique only for
the same server. Different export types or (potentially in the future)
multiple NBD servers can have the same export name externally, but still
need different block export ids internally.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-export.json | 5 +++++
include/block/export.h | 3 +++
block/export/export.c | 26 ++++++++++++++++++++++++++
blockdev-nbd.c | 1 +
qemu-nbd.c | 1 +
storage-daemon/qemu-storage-daemon.c | 2 +-
tests/qemu-iotests/223.out | 4 ++--
7 files changed, 39 insertions(+), 3 deletions(-)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index 4d22f46c94..9e05265b06 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -105,6 +105,8 @@
#
# Export a block node to QEMU's embedded NBD server.
#
+# The export name will be used as the id for the resulting block export.
+#
# Returns: error if the server is not running, or export with the same name
# already exists.
#
@@ -182,6 +184,8 @@
# Describes a block export, i.e. how single node should be exported on an
# external interface.
#
+# @id: A unique identifier for the block export (across all export types)
+#
# @node-name: The node name of the block node to be exported (since: 5.2)
#
# @writethrough: If true, caches are flushed after every write request to the
@@ -192,6 +196,7 @@
##
{ 'union': 'BlockExportOptions',
'base': { 'type': 'BlockExportType',
+ 'id': 'str',
'node-name': 'str',
'*writethrough': 'bool' },
'discriminator': 'type',
diff --git a/include/block/export.h b/include/block/export.h
index 6fffcb5651..cdc6e161ea 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -50,6 +50,9 @@ typedef struct BlockExportDriver {
struct BlockExport {
const BlockExportDriver *drv;
+ /* Unique identifier for the export */
+ char *id;
+
/*
* Reference count for this block export. This includes strong references
* both from the owner (qemu-nbd or the monitor) and clients connected to
diff --git a/block/export/export.c b/block/export/export.c
index 1b36286010..21e9013fb6 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -19,6 +19,7 @@
#include "block/nbd.h"
#include "qapi/error.h"
#include "qapi/qapi-commands-block-export.h"
+#include "qemu/id.h"
static const BlockExportDriver *blk_exp_drivers[] = {
&blk_exp_nbd,
@@ -28,6 +29,19 @@ static const BlockExportDriver *blk_exp_drivers[] = {
static QLIST_HEAD(, BlockExport) block_exports =
QLIST_HEAD_INITIALIZER(block_exports);
+static BlockExport *blk_exp_find(const char *id)
+{
+ BlockExport *exp;
+
+ QLIST_FOREACH(exp, &block_exports, next) {
+ if (strcmp(id, exp->id) == 0) {
+ return exp;
+ }
+ }
+
+ return NULL;
+}
+
static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
{
int i;
@@ -46,6 +60,15 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
BlockExport *exp;
int ret;
+ if (!id_wellformed(export->id)) {
+ error_setg(errp, "Invalid block export id");
+ return NULL;
+ }
+ if (blk_exp_find(export->id)) {
+ error_setg(errp, "Block export id '%s' is already in use", export->id);
+ return NULL;
+ }
+
drv = blk_exp_find_driver(export->type);
if (!drv) {
error_setg(errp, "No driver found for the requested export type");
@@ -57,10 +80,12 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
*exp = (BlockExport) {
.drv = &blk_exp_nbd,
.refcount = 1,
+ .id = g_strdup(export->id),
};
ret = drv->create(exp, export, errp);
if (ret < 0) {
+ g_free(exp->id);
g_free(exp);
return NULL;
}
@@ -87,6 +112,7 @@ static void blk_exp_delete_bh(void *opaque)
assert(exp->refcount == 0);
QLIST_REMOVE(exp, next);
exp->drv->delete(exp);
+ g_free(exp->id);
g_free(exp);
aio_context_release(aio_context);
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index f927264777..814554dd90 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -269,6 +269,7 @@ void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
export_opts = g_new(BlockExportOptions, 1);
*export_opts = (BlockExportOptions) {
.type = BLOCK_EXPORT_TYPE_NBD,
+ .id = g_strdup(arg->name),
.node_name = g_strdup(bdrv_get_node_name(bs)),
.u.nbd = {
.has_name = true,
diff --git a/qemu-nbd.c b/qemu-nbd.c
index c64f83618b..ac82acb4ac 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1064,6 +1064,7 @@ int main(int argc, char **argv)
export_opts = g_new(BlockExportOptions, 1);
*export_opts = (BlockExportOptions) {
.type = BLOCK_EXPORT_TYPE_NBD,
+ .id = g_strdup("qemu-nbd-export"),
.node_name = g_strdup(bdrv_get_node_name(bs)),
.has_writethrough = true,
.writethrough = writethrough,
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index 0fcab6ed2d..e6157ff518 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -92,7 +92,7 @@ static void help(void)
" --chardev <options> configure a character device backend\n"
" (see the qemu(1) man page for possible options)\n"
"\n"
-" --export [type=]nbd,device=<node-name>[,name=<export-name>]\n"
+" --export [type=]nbd,device=<node-name>,id=<id>,[,name=<export-name>]\n"
" [,writable=on|off][,bitmap=<name>]\n"
" export the specified block node over NBD\n"
" (requires --nbd-server)\n"
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index e1eaaedb55..31ce9e6fe0 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -45,7 +45,7 @@ exports available: 0
{"execute":"nbd-server-add", "arguments":{"device":"nosuch"}}
{"error": {"class": "GenericError", "desc": "Cannot find device=nosuch nor node_name=nosuch"}}
{"execute":"nbd-server-add", "arguments":{"device":"n"}}
-{"error": {"class": "GenericError", "desc": "NBD server already has export named 'n'"}}
+{"error": {"class": "GenericError", "desc": "Block export id 'n' is already in use"}}
{"execute":"nbd-server-add", "arguments":{"device":"n", "name":"n2", "bitmap":"b2"}}
{"error": {"class": "GenericError", "desc": "Enabled bitmap 'b2' incompatible with readonly export"}}
{"execute":"nbd-server-add", "arguments":{"device":"n", "name":"n2", "bitmap":"b3"}}
@@ -126,7 +126,7 @@ exports available: 0
{"execute":"nbd-server-add", "arguments":{"device":"nosuch"}}
{"error": {"class": "GenericError", "desc": "Cannot find device=nosuch nor node_name=nosuch"}}
{"execute":"nbd-server-add", "arguments":{"device":"n"}}
-{"error": {"class": "GenericError", "desc": "NBD server already has export named 'n'"}}
+{"error": {"class": "GenericError", "desc": "Block export id 'n' is already in use"}}
{"execute":"nbd-server-add", "arguments":{"device":"n", "name":"n2", "bitmap":"b2"}}
{"error": {"class": "GenericError", "desc": "Enabled bitmap 'b2' incompatible with readonly export"}}
{"execute":"nbd-server-add", "arguments":{"device":"n", "name":"n2", "bitmap":"b3"}}
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 18/29] block/export: Add 'id' option to block-export-add
2020-09-07 18:20 ` [PATCH 18/29] block/export: Add 'id' option to block-export-add Kevin Wolf
@ 2020-09-10 13:26 ` Max Reitz
0 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2020-09-10 13:26 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1080 bytes --]
On 07.09.20 20:20, Kevin Wolf wrote:
> We'll need an id to identify block exports in monitor commands. This
> adds one.
>
> Note that this is different from the 'name' option in the NBD server,
> which is the externally visible export name. While block export ids need
> to be unique in the whole process, export names must be unique only for
> the same server. Different export types or (potentially in the future)
> multiple NBD servers can have the same export name externally, but still
> need different block export ids internally.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-export.json | 5 +++++
> include/block/export.h | 3 +++
> block/export/export.c | 26 ++++++++++++++++++++++++++
> blockdev-nbd.c | 1 +
> qemu-nbd.c | 1 +
> storage-daemon/qemu-storage-daemon.c | 2 +-
> tests/qemu-iotests/223.out | 4 ++--
> 7 files changed, 39 insertions(+), 3 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 19/29] block/export: Move strong user reference to block_exports
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (17 preceding siblings ...)
2020-09-07 18:20 ` [PATCH 18/29] block/export: Add 'id' option to block-export-add Kevin Wolf
@ 2020-09-07 18:20 ` Kevin Wolf
2020-09-10 13:33 ` Max Reitz
2020-09-07 18:20 ` [PATCH 20/29] block/export: Add block-export-del Kevin Wolf
` (10 subsequent siblings)
29 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:20 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
The reference owned by the user/monitor that is created when adding the
export and dropped when removing it was tied to the 'exports' list in
nbd/server.c. Every block export will have a user reference, so move it
to the block export level and tie it to the 'block_exports' list in
block/export/export.c instead. This is necessary for introducing a QMP
command for removing exports.
Note that exports are present in block_exports even after the user has
requested shutdown. This is different from NBD's exports where exports
are immediately removed on a shutdown request, even if they are still in
the process of shutting down. In order to avoid that the user still
interacts with an export that is shutting down (and possibly removes it
a second time), we need to remember if the user actually still owns it.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/export.h | 8 ++++++++
block/export/export.c | 6 ++++++
blockdev-nbd.c | 5 -----
nbd/server.c | 2 --
4 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/include/block/export.h b/include/block/export.h
index cdc6e161ea..4833947e89 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -60,6 +60,14 @@ struct BlockExport {
*/
int refcount;
+ /*
+ * True if one of the references in refcount belongs to the user. After the
+ * user has dropped their reference, they may not e.g. remove the same
+ * export a second time (which would decrease the refcount without having
+ * it incremented first).
+ */
+ bool user_owned;
+
/* The AioContext whose lock protects this BlockExport object. */
AioContext *ctx;
diff --git a/block/export/export.c b/block/export/export.c
index 21e9013fb6..42aac867e9 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -80,6 +80,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
*exp = (BlockExport) {
.drv = &blk_exp_nbd,
.refcount = 1,
+ .user_owned = true,
.id = g_strdup(export->id),
};
@@ -136,6 +137,11 @@ void blk_exp_request_shutdown(BlockExport *exp)
aio_context_acquire(aio_context);
exp->drv->request_shutdown(exp);
+
+ assert(exp->user_owned);
+ exp->user_owned = false;
+ blk_exp_unref(exp);
+
aio_context_release(aio_context);
}
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 814554dd90..9efbaef8f7 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -235,11 +235,6 @@ int nbd_export_create(BlockExport *exp, BlockExportOptions *exp_args,
goto out;
}
- /* The list of named exports has a strong reference to this export now and
- * our only way of accessing it is through nbd_export_find(), so we can drop
- * the strong reference that is @exp. */
- blk_exp_unref(exp);
-
ret = 0;
out:
aio_context_release(aio_context);
diff --git a/nbd/server.c b/nbd/server.c
index 32147e4871..22a1d66168 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1616,7 +1616,6 @@ int nbd_export_new(BlockExport *blk_exp, BlockDriverState *bs,
blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
- blk_exp_ref(&exp->common);
QTAILQ_INSERT_TAIL(&exports, exp, next);
return 0;
@@ -1663,7 +1662,6 @@ static void nbd_export_request_shutdown(BlockExport *blk_exp)
client_close(client, true);
}
if (exp->name) {
- blk_exp_unref(&exp->common);
g_free(exp->name);
exp->name = NULL;
QTAILQ_REMOVE(&exports, exp, next);
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 19/29] block/export: Move strong user reference to block_exports
2020-09-07 18:20 ` [PATCH 19/29] block/export: Move strong user reference to block_exports Kevin Wolf
@ 2020-09-10 13:33 ` Max Reitz
2020-09-10 13:36 ` Max Reitz
0 siblings, 1 reply; 53+ messages in thread
From: Max Reitz @ 2020-09-10 13:33 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1926 bytes --]
On 07.09.20 20:20, Kevin Wolf wrote:
> The reference owned by the user/monitor that is created when adding the
> export and dropped when removing it was tied to the 'exports' list in
> nbd/server.c. Every block export will have a user reference, so move it
> to the block export level and tie it to the 'block_exports' list in
> block/export/export.c instead. This is necessary for introducing a QMP
> command for removing exports.
>
> Note that exports are present in block_exports even after the user has
> requested shutdown. This is different from NBD's exports where exports
> are immediately removed on a shutdown request, even if they are still in
> the process of shutting down. In order to avoid that the user still
> interacts with an export that is shutting down (and possibly removes it
> a second time), we need to remember if the user actually still owns it.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> include/block/export.h | 8 ++++++++
> block/export/export.c | 6 ++++++
> blockdev-nbd.c | 5 -----
> nbd/server.c | 2 --
> 4 files changed, 14 insertions(+), 7 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
> diff --git a/include/block/export.h b/include/block/export.h
> index cdc6e161ea..4833947e89 100644
> --- a/include/block/export.h
> +++ b/include/block/export.h
> @@ -60,6 +60,14 @@ struct BlockExport {
> */
> int refcount;
>
> + /*
> + * True if one of the references in refcount belongs to the user. After the
> + * user has dropped their reference, they may not e.g. remove the same
> + * export a second time (which would decrease the refcount without having
> + * it incremented first).
Not sure though whether this (or the request_shutdown documentation)
should mention that request_shutdown drops the user reference.
Max
> + */
> + bool user_owned;
> +
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 19/29] block/export: Move strong user reference to block_exports
2020-09-10 13:33 ` Max Reitz
@ 2020-09-10 13:36 ` Max Reitz
0 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2020-09-10 13:36 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 2162 bytes --]
On 10.09.20 15:33, Max Reitz wrote:
> On 07.09.20 20:20, Kevin Wolf wrote:
>> The reference owned by the user/monitor that is created when adding the
>> export and dropped when removing it was tied to the 'exports' list in
>> nbd/server.c. Every block export will have a user reference, so move it
>> to the block export level and tie it to the 'block_exports' list in
>> block/export/export.c instead. This is necessary for introducing a QMP
>> command for removing exports.
>>
>> Note that exports are present in block_exports even after the user has
>> requested shutdown. This is different from NBD's exports where exports
>> are immediately removed on a shutdown request, even if they are still in
>> the process of shutting down. In order to avoid that the user still
>> interacts with an export that is shutting down (and possibly removes it
>> a second time), we need to remember if the user actually still owns it.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>> include/block/export.h | 8 ++++++++
>> block/export/export.c | 6 ++++++
>> blockdev-nbd.c | 5 -----
>> nbd/server.c | 2 --
>> 4 files changed, 14 insertions(+), 7 deletions(-)
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
>> diff --git a/include/block/export.h b/include/block/export.h
>> index cdc6e161ea..4833947e89 100644
>> --- a/include/block/export.h
>> +++ b/include/block/export.h
>> @@ -60,6 +60,14 @@ struct BlockExport {
>> */
>> int refcount;
>>
>> + /*
>> + * True if one of the references in refcount belongs to the user. After the
>> + * user has dropped their reference, they may not e.g. remove the same
>> + * export a second time (which would decrease the refcount without having
>> + * it incremented first).
>
> Not sure though whether this (or the request_shutdown documentation)
> should mention that request_shutdown drops the user reference.
Er, no, it shouldn’t, because it doesn’t. Only
blk_exp_request_shutdown() does that. Though since that’s a public
function (used by nbd/server.c), maybe it should be documented there.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 20/29] block/export: Add block-export-del
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (18 preceding siblings ...)
2020-09-07 18:20 ` [PATCH 19/29] block/export: Move strong user reference to block_exports Kevin Wolf
@ 2020-09-07 18:20 ` Kevin Wolf
2020-09-07 18:20 ` [PATCH 21/29] block/export: Add BLOCK_EXPORT_DELETED event Kevin Wolf
` (9 subsequent siblings)
29 siblings, 0 replies; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:20 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Implement a new QMP command block-export-del and make nbd-server-remove
a wrapper around it.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
qapi/block-export.json | 32 ++++++++++++++++++++-----
include/block/export.h | 1 +
include/block/nbd.h | 1 -
block/export/export.c | 43 +++++++++++++++++++++++++++++++++-
block/monitor/block-hmp-cmds.c | 4 ++--
blockdev-nbd.c | 25 +++++---------------
nbd/server.c | 14 -----------
7 files changed, 77 insertions(+), 43 deletions(-)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index 9e05265b06..77a6b595e8 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -116,9 +116,9 @@
'data': 'NbdServerAddOptions', 'boxed': true }
##
-# @NbdServerRemoveMode:
+# @BlockExportRemoveMode:
#
-# Mode for removing an NBD export.
+# Mode for removing a block export.
#
# @safe: Remove export if there are no existing connections, fail otherwise.
#
@@ -134,16 +134,16 @@
#
# Since: 2.12
##
-{'enum': 'NbdServerRemoveMode', 'data': ['safe', 'hard']}
+{'enum': 'BlockExportRemoveMode', 'data': ['safe', 'hard']}
##
# @nbd-server-remove:
#
# Remove NBD export by name.
#
-# @name: Export name.
+# @name: Block export id.
#
-# @mode: Mode of command operation. See @NbdServerRemoveMode description.
+# @mode: Mode of command operation. See @BlockExportRemoveMode description.
# Default is 'safe'.
#
# Returns: error if
@@ -154,7 +154,7 @@
# Since: 2.12
##
{ 'command': 'nbd-server-remove',
- 'data': {'name': 'str', '*mode': 'NbdServerRemoveMode'} }
+ 'data': {'name': 'str', '*mode': 'BlockExportRemoveMode'} }
##
# @nbd-server-stop:
@@ -213,3 +213,23 @@
##
{ 'command': 'block-export-add',
'data': 'BlockExportOptions', 'boxed': true }
+
+##
+# @block-export-del:
+#
+# Request to remove a block export. This drops the user's reference to the
+# export, but the export may still stay around after this command returns until
+# the shutdown of the export has completed.
+#
+# @id: Block export id.
+#
+# @mode: Mode of command operation. See @BlockExportRemoveMode description.
+# Default is 'safe'.
+#
+# Returns: Error if the export is not found or @mode is 'safe' and the export
+# is still in use (e.g. by existing client connections)
+#
+# Since: 5.2
+##
+{ 'command': 'block-export-del',
+ 'data': { 'id': 'str', '*mode': 'BlockExportRemoveMode' } }
diff --git a/include/block/export.h b/include/block/export.h
index 4833947e89..ff54d35872 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -76,6 +76,7 @@ struct BlockExport {
};
BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp);
+BlockExport *blk_exp_find(const char *id);
void blk_exp_ref(BlockExport *exp);
void blk_exp_unref(BlockExport *exp);
void blk_exp_request_shutdown(BlockExport *exp);
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 0b9f3e5d4e..a4dc1f9e54 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -337,7 +337,6 @@ int nbd_export_new(BlockExport *blk_exp, BlockDriverState *bs,
const char *bitmap, bool readonly, bool shared,
bool writethrough, Error **errp);
void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk);
-void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
AioContext *nbd_export_aio_context(NBDExport *exp);
NBDExport *nbd_export_find(const char *name);
diff --git a/block/export/export.c b/block/export/export.c
index 42aac867e9..4af3b69186 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -29,7 +29,7 @@ static const BlockExportDriver *blk_exp_drivers[] = {
static QLIST_HEAD(, BlockExport) block_exports =
QLIST_HEAD_INITIALIZER(block_exports);
-static BlockExport *blk_exp_find(const char *id)
+BlockExport *blk_exp_find(const char *id)
{
BlockExport *exp;
@@ -136,12 +136,23 @@ void blk_exp_request_shutdown(BlockExport *exp)
AioContext *aio_context = exp->ctx;
aio_context_acquire(aio_context);
+
+ /*
+ * If the user doesn't own the export any more, it is already shutting
+ * down. We must not call .request_shutdown and decrease the refcount a
+ * second time.
+ */
+ if (!exp->user_owned) {
+ goto out;
+ }
+
exp->drv->request_shutdown(exp);
assert(exp->user_owned);
exp->user_owned = false;
blk_exp_unref(exp);
+out:
aio_context_release(aio_context);
}
@@ -192,3 +203,33 @@ void qmp_block_export_add(BlockExportOptions *export, Error **errp)
{
blk_exp_add(export, errp);
}
+
+void qmp_block_export_del(const char *id,
+ bool has_mode, BlockExportRemoveMode mode,
+ Error **errp)
+{
+ ERRP_GUARD();
+ BlockExport *exp;
+
+ exp = blk_exp_find(id);
+ if (exp == NULL) {
+ error_setg(errp, "Export '%s' is not found", id);
+ return;
+ }
+ if (!exp->user_owned) {
+ error_setg(errp, "Export '%s' is already shutting down", id);
+ return;
+ }
+
+ if (!has_mode) {
+ mode = BLOCK_EXPORT_REMOVE_MODE_SAFE;
+ }
+ if (mode == BLOCK_EXPORT_REMOVE_MODE_SAFE && exp->refcount > 1) {
+ error_setg(errp, "export '%s' still in use", exp->id);
+ error_append_hint(errp, "Use mode='hard' to force client "
+ "disconnect\n");
+ return;
+ }
+
+ blk_exp_request_shutdown(exp);
+}
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index db357cafcb..d15a2be827 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -476,8 +476,8 @@ void hmp_nbd_server_remove(Monitor *mon, const QDict *qdict)
bool force = qdict_get_try_bool(qdict, "force", false);
Error *err = NULL;
- /* Rely on NBD_SERVER_REMOVE_MODE_SAFE being the default */
- qmp_nbd_server_remove(name, force, NBD_SERVER_REMOVE_MODE_HARD, &err);
+ /* Rely on BLOCK_EXPORT_REMOVE_MODE_SAFE being the default */
+ qmp_nbd_server_remove(name, force, BLOCK_EXPORT_REMOVE_MODE_HARD, &err);
hmp_handle_error(mon, err);
}
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 9efbaef8f7..4a9a1be571 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -307,31 +307,18 @@ fail:
}
void qmp_nbd_server_remove(const char *name,
- bool has_mode, NbdServerRemoveMode mode,
+ bool has_mode, BlockExportRemoveMode mode,
Error **errp)
{
- NBDExport *exp;
- AioContext *aio_context;
-
- if (!nbd_server) {
- error_setg(errp, "NBD server not running");
- return;
- }
+ BlockExport *exp;
- exp = nbd_export_find(name);
- if (exp == NULL) {
- error_setg(errp, "Export '%s' is not found", name);
+ exp = blk_exp_find(name);
+ if (exp && exp->drv->type != BLOCK_EXPORT_TYPE_NBD) {
+ error_setg(errp, "Block export '%s' is not an NBD export", name);
return;
}
- if (!has_mode) {
- mode = NBD_SERVER_REMOVE_MODE_SAFE;
- }
-
- aio_context = nbd_export_aio_context(exp);
- aio_context_acquire(aio_context);
- nbd_export_remove(exp, mode, errp);
- aio_context_release(aio_context);
+ qmp_block_export_del(name, has_mode, mode, errp);
}
void qmp_nbd_server_stop(Error **errp)
diff --git a/nbd/server.c b/nbd/server.c
index 22a1d66168..1a0e1db401 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1669,20 +1669,6 @@ static void nbd_export_request_shutdown(BlockExport *blk_exp)
blk_exp_unref(&exp->common);
}
-void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp)
-{
- ERRP_GUARD();
- if (mode == NBD_SERVER_REMOVE_MODE_HARD || QTAILQ_EMPTY(&exp->clients)) {
- nbd_export_request_shutdown(&exp->common);
- return;
- }
-
- assert(mode == NBD_SERVER_REMOVE_MODE_SAFE);
-
- error_setg(errp, "export '%s' still in use", exp->name);
- error_append_hint(errp, "Use mode='hard' to force client disconnect\n");
-}
-
static void nbd_export_delete(BlockExport *blk_exp)
{
NBDExport *exp = container_of(blk_exp, NBDExport, common);
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 21/29] block/export: Add BLOCK_EXPORT_DELETED event
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (19 preceding siblings ...)
2020-09-07 18:20 ` [PATCH 20/29] block/export: Add block-export-del Kevin Wolf
@ 2020-09-07 18:20 ` Kevin Wolf
2020-09-10 14:04 ` Max Reitz
` (2 more replies)
2020-09-07 18:20 ` [PATCH 22/29] block/export: Move blk to BlockExport Kevin Wolf
` (8 subsequent siblings)
29 siblings, 3 replies; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:20 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Clients may want to know when an export has finally disappeard
(block-export-del returns earlier than that in the general case), so add
a QAPI event for it.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-export.json | 12 ++++++++++++
block/export/export.c | 2 ++
tests/qemu-iotests/140.out | 1 +
tests/qemu-iotests/223.out | 4 ++++
4 files changed, 19 insertions(+)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index 77a6b595e8..dac3250f08 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -233,3 +233,15 @@
##
{ 'command': 'block-export-del',
'data': { 'id': 'str', '*mode': 'BlockExportRemoveMode' } }
+
+##
+# @BLOCK_EXPORT_DELETED:
+#
+# Emitted when a block export is removed and it's id can be reused.
+#
+# @id: Block export id.
+#
+# Since: 5.2
+##
+{ 'event': 'BLOCK_EXPORT_DELETED',
+ 'data': { 'id': 'str' } }
diff --git a/block/export/export.c b/block/export/export.c
index 4af3b69186..ae7879e6a9 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -19,6 +19,7 @@
#include "block/nbd.h"
#include "qapi/error.h"
#include "qapi/qapi-commands-block-export.h"
+#include "qapi/qapi-events-block-export.h"
#include "qemu/id.h"
static const BlockExportDriver *blk_exp_drivers[] = {
@@ -113,6 +114,7 @@ static void blk_exp_delete_bh(void *opaque)
assert(exp->refcount == 0);
QLIST_REMOVE(exp, next);
exp->drv->delete(exp);
+ qapi_event_send_block_export_deleted(exp->id);
g_free(exp->id);
g_free(exp);
diff --git a/tests/qemu-iotests/140.out b/tests/qemu-iotests/140.out
index 86b985da75..f1db409b26 100644
--- a/tests/qemu-iotests/140.out
+++ b/tests/qemu-iotests/140.out
@@ -15,6 +15,7 @@ read 65536/65536 bytes at offset 0
qemu-io: can't open device nbd+unix:///drv?socket=SOCK_DIR/nbd: Requested export not available
server reported: export 'drv' not present
{ 'execute': 'quit' }
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "drv"}}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
*** done
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index 31ce9e6fe0..f6eac23f04 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -102,8 +102,10 @@ read 2097152/2097152 bytes at offset 2097152
{"execute":"nbd-server-remove", "arguments":{"name":"n"}}
{"return": {}}
{"execute":"nbd-server-remove", "arguments":{"name":"n2"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "n"}}
{"return": {}}
{"execute":"nbd-server-remove", "arguments":{"name":"n2"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "n2"}}
{"error": {"class": "GenericError", "desc": "Export 'n2' is not found"}}
{"execute":"nbd-server-stop"}
{"return": {}}
@@ -183,8 +185,10 @@ read 2097152/2097152 bytes at offset 2097152
{"execute":"nbd-server-remove", "arguments":{"name":"n"}}
{"return": {}}
{"execute":"nbd-server-remove", "arguments":{"name":"n2"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "n"}}
{"return": {}}
{"execute":"nbd-server-remove", "arguments":{"name":"n2"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "n2"}}
{"error": {"class": "GenericError", "desc": "Export 'n2' is not found"}}
{"execute":"nbd-server-stop"}
{"return": {}}
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 21/29] block/export: Add BLOCK_EXPORT_DELETED event
2020-09-07 18:20 ` [PATCH 21/29] block/export: Add BLOCK_EXPORT_DELETED event Kevin Wolf
@ 2020-09-10 14:04 ` Max Reitz
2020-09-10 15:12 ` Max Reitz
2020-09-16 14:57 ` Max Reitz
2 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2020-09-10 14:04 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 516 bytes --]
On 07.09.20 20:20, Kevin Wolf wrote:
> Clients may want to know when an export has finally disappeard
> (block-export-del returns earlier than that in the general case), so add
> a QAPI event for it.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-export.json | 12 ++++++++++++
> block/export/export.c | 2 ++
> tests/qemu-iotests/140.out | 1 +
> tests/qemu-iotests/223.out | 4 ++++
> 4 files changed, 19 insertions(+)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 21/29] block/export: Add BLOCK_EXPORT_DELETED event
2020-09-07 18:20 ` [PATCH 21/29] block/export: Add BLOCK_EXPORT_DELETED event Kevin Wolf
2020-09-10 14:04 ` Max Reitz
@ 2020-09-10 15:12 ` Max Reitz
2020-09-16 14:57 ` Max Reitz
2 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2020-09-10 15:12 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 985 bytes --]
On 07.09.20 20:20, Kevin Wolf wrote:
> Clients may want to know when an export has finally disappeard
> (block-export-del returns earlier than that in the general case), so add
> a QAPI event for it.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-export.json | 12 ++++++++++++
> block/export/export.c | 2 ++
> tests/qemu-iotests/140.out | 1 +
> tests/qemu-iotests/223.out | 4 ++++
> 4 files changed, 19 insertions(+)
It appears I had started a reply and for overlooked it when I gave my R-b:
> diff --git a/qapi/block-export.json b/qapi/block-export.json
> index 77a6b595e8..dac3250f08 100644
> --- a/qapi/block-export.json
> +++ b/qapi/block-export.json
> @@ -233,3 +233,15 @@
> ##
> { 'command': 'block-export-del',
> 'data': { 'id': 'str', '*mode': 'BlockExportRemoveMode' } }
> +
> +##
> +# @BLOCK_EXPORT_DELETED:
> +#
> +# Emitted when a block export is removed and it's id can be reused.
*its
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [PATCH 21/29] block/export: Add BLOCK_EXPORT_DELETED event
2020-09-07 18:20 ` [PATCH 21/29] block/export: Add BLOCK_EXPORT_DELETED event Kevin Wolf
2020-09-10 14:04 ` Max Reitz
2020-09-10 15:12 ` Max Reitz
@ 2020-09-16 14:57 ` Max Reitz
2 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2020-09-16 14:57 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 2655 bytes --]
On 07.09.20 20:20, Kevin Wolf wrote:
> Clients may want to know when an export has finally disappeard
> (block-export-del returns earlier than that in the general case), so add
> a QAPI event for it.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-export.json | 12 ++++++++++++
> block/export/export.c | 2 ++
> tests/qemu-iotests/140.out | 1 +
> tests/qemu-iotests/223.out | 4 ++++
> 4 files changed, 19 insertions(+)
>
> diff --git a/qapi/block-export.json b/qapi/block-export.json
> index 77a6b595e8..dac3250f08 100644
> --- a/qapi/block-export.json
> +++ b/qapi/block-export.json
> @@ -233,3 +233,15 @@
> ##
> { 'command': 'block-export-del',
> 'data': { 'id': 'str', '*mode': 'BlockExportRemoveMode' } }
> +
> +##
> +# @BLOCK_EXPORT_DELETED:
> +#
> +# Emitted when a block export is removed and it's id can be reused.
> +#
> +# @id: Block export id.
> +#
> +# Since: 5.2
> +##
> +{ 'event': 'BLOCK_EXPORT_DELETED',
> + 'data': { 'id': 'str' } }
> diff --git a/block/export/export.c b/block/export/export.c
> index 4af3b69186..ae7879e6a9 100644
> --- a/block/export/export.c
> +++ b/block/export/export.c
> @@ -19,6 +19,7 @@
> #include "block/nbd.h"
> #include "qapi/error.h"
> #include "qapi/qapi-commands-block-export.h"
> +#include "qapi/qapi-events-block-export.h"
> #include "qemu/id.h"
>
> static const BlockExportDriver *blk_exp_drivers[] = {
> @@ -113,6 +114,7 @@ static void blk_exp_delete_bh(void *opaque)
> assert(exp->refcount == 0);
> QLIST_REMOVE(exp, next);
> exp->drv->delete(exp);
> + qapi_event_send_block_export_deleted(exp->id);
> g_free(exp->id);
> g_free(exp);
>
> diff --git a/tests/qemu-iotests/140.out b/tests/qemu-iotests/140.out
> index 86b985da75..f1db409b26 100644
> --- a/tests/qemu-iotests/140.out
> +++ b/tests/qemu-iotests/140.out
> @@ -15,6 +15,7 @@ read 65536/65536 bytes at offset 0
> qemu-io: can't open device nbd+unix:///drv?socket=SOCK_DIR/nbd: Requested export not available
> server reported: export 'drv' not present
> { 'execute': 'quit' }
> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "drv"}}
> {"return": {}}
> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
> *** done
With -qed, this event appears immediately when the drive is ejected, so
this test now fails (for qed).
I’m not sure whether there’s much of a point in fixing it or just
marking qed as unsupported.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 22/29] block/export: Move blk to BlockExport
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (20 preceding siblings ...)
2020-09-07 18:20 ` [PATCH 21/29] block/export: Add BLOCK_EXPORT_DELETED event Kevin Wolf
@ 2020-09-07 18:20 ` Kevin Wolf
2020-09-07 18:20 ` [PATCH 23/29] block/export: Create BlockBackend in blk_exp_add() Kevin Wolf
` (7 subsequent siblings)
29 siblings, 0 replies; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:20 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Every block export has a BlockBackend representing the disk that is
exported. It should live in BlockExport therefore.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
include/block/export.h | 3 +++
block/export/export.c | 3 +++
nbd/server.c | 43 +++++++++++++++++++++---------------------
3 files changed, 28 insertions(+), 21 deletions(-)
diff --git a/include/block/export.h b/include/block/export.h
index ff54d35872..7feb02e10d 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -71,6 +71,9 @@ struct BlockExport {
/* The AioContext whose lock protects this BlockExport object. */
AioContext *ctx;
+ /* The block device to export */
+ BlockBackend *blk;
+
/* List entry for block_exports */
QLIST_ENTRY(BlockExport) next;
};
diff --git a/block/export/export.c b/block/export/export.c
index ae7879e6a9..40fc9d505f 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -92,6 +92,8 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
return NULL;
}
+ assert(exp->blk != NULL);
+
QLIST_INSERT_HEAD(&block_exports, exp, next);
return exp;
}
@@ -114,6 +116,7 @@ static void blk_exp_delete_bh(void *opaque)
assert(exp->refcount == 0);
QLIST_REMOVE(exp, next);
exp->drv->delete(exp);
+ blk_unref(exp->blk);
qapi_event_send_block_export_deleted(exp->id);
g_free(exp->id);
g_free(exp);
diff --git a/nbd/server.c b/nbd/server.c
index 1a0e1db401..f9af45c480 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -84,7 +84,6 @@ struct NBDRequestData {
struct NBDExport {
BlockExport common;
- BlockBackend *blk;
char *name;
char *description;
uint64_t size;
@@ -643,7 +642,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
* whether this is OPT_INFO or OPT_GO. */
/* minimum - 1 for back-compat, or actual if client will obey it. */
if (client->opt == NBD_OPT_INFO || blocksize) {
- check_align = sizes[0] = blk_get_request_alignment(exp->blk);
+ check_align = sizes[0] = blk_get_request_alignment(exp->common.blk);
} else {
sizes[0] = 1;
}
@@ -652,7 +651,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
* TODO: is blk_bs(blk)->bl.opt_transfer appropriate? */
sizes[1] = MAX(4096, sizes[0]);
/* maximum - At most 32M, but smaller as appropriate. */
- sizes[2] = MIN(blk_get_max_transfer(exp->blk), NBD_MAX_BUFFER_SIZE);
+ sizes[2] = MIN(blk_get_max_transfer(exp->common.blk), NBD_MAX_BUFFER_SIZE);
trace_nbd_negotiate_handle_info_block_size(sizes[0], sizes[1], sizes[2]);
sizes[0] = cpu_to_be32(sizes[0]);
sizes[1] = cpu_to_be32(sizes[1]);
@@ -684,7 +683,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
* tolerate all clients, regardless of alignments.
*/
if (client->opt == NBD_OPT_INFO && !blocksize &&
- blk_get_request_alignment(exp->blk) > 1) {
+ blk_get_request_alignment(exp->common.blk) > 1) {
return nbd_negotiate_send_rep_err(client,
NBD_REP_ERR_BLOCK_SIZE_REQD,
errp,
@@ -1557,7 +1556,7 @@ int nbd_export_new(BlockExport *blk_exp, BlockDriverState *bs,
blk_set_allow_aio_context_change(blk, true);
QTAILQ_INIT(&exp->clients);
- exp->blk = blk;
+ exp->common.blk = blk;
exp->name = g_strdup(name);
assert(!desc || strlen(desc) <= NBD_MAX_STRING_SIZE);
exp->description = g_strdup(desc);
@@ -1679,15 +1678,13 @@ static void nbd_export_delete(BlockExport *blk_exp)
g_free(exp->description);
exp->description = NULL;
- if (exp->blk) {
+ if (exp->common.blk) {
if (exp->eject_notifier_blk) {
notifier_remove(&exp->eject_notifier);
blk_unref(exp->eject_notifier_blk);
}
- blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
+ blk_remove_aio_context_notifier(exp->common.blk, blk_aio_attached,
blk_aio_detach, exp);
- blk_unref(exp->blk);
- exp->blk = NULL;
}
if (exp->export_bitmap) {
@@ -1840,7 +1837,7 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
while (progress < size) {
int64_t pnum;
- int status = bdrv_block_status_above(blk_bs(exp->blk), NULL,
+ int status = bdrv_block_status_above(blk_bs(exp->common.blk), NULL,
offset + progress,
size - progress, &pnum, NULL,
NULL);
@@ -1872,7 +1869,8 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
stl_be_p(&chunk.length, pnum);
ret = nbd_co_send_iov(client, iov, 1, errp);
} else {
- ret = blk_pread(exp->blk, offset + progress, data + progress, pnum);
+ ret = blk_pread(exp->common.blk, offset + progress,
+ data + progress, pnum);
if (ret < 0) {
error_setg_errno(errp, -ret, "reading from file failed");
break;
@@ -2136,7 +2134,8 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
}
if (request->type != NBD_CMD_CACHE) {
- req->data = blk_try_blockalign(client->exp->blk, request->len);
+ req->data = blk_try_blockalign(client->exp->common.blk,
+ request->len);
if (req->data == NULL) {
error_setg(errp, "No memory");
return -ENOMEM;
@@ -2232,7 +2231,7 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request,
/* XXX: NBD Protocol only documents use of FUA with WRITE */
if (request->flags & NBD_CMD_FLAG_FUA) {
- ret = blk_co_flush(exp->blk);
+ ret = blk_co_flush(exp->common.blk);
if (ret < 0) {
return nbd_send_generic_reply(client, request->handle, ret,
"flush failed", errp);
@@ -2246,7 +2245,7 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request,
data, request->len, errp);
}
- ret = blk_pread(exp->blk, request->from, data, request->len);
+ ret = blk_pread(exp->common.blk, request->from, data, request->len);
if (ret < 0) {
return nbd_send_generic_reply(client, request->handle, ret,
"reading from file failed", errp);
@@ -2281,7 +2280,7 @@ static coroutine_fn int nbd_do_cmd_cache(NBDClient *client, NBDRequest *request,
assert(request->type == NBD_CMD_CACHE);
- ret = blk_co_preadv(exp->blk, request->from, request->len,
+ ret = blk_co_preadv(exp->common.blk, request->from, request->len,
NULL, BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH);
return nbd_send_generic_reply(client, request->handle, ret,
@@ -2312,7 +2311,8 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
if (request->flags & NBD_CMD_FLAG_FUA) {
flags |= BDRV_REQ_FUA;
}
- ret = blk_pwrite(exp->blk, request->from, data, request->len, flags);
+ ret = blk_pwrite(exp->common.blk, request->from, data, request->len,
+ flags);
return nbd_send_generic_reply(client, request->handle, ret,
"writing to file failed", errp);
@@ -2333,7 +2333,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
int align = client->check_align ?: 1;
int len = MIN(request->len, QEMU_ALIGN_DOWN(BDRV_REQUEST_MAX_BYTES,
align));
- ret = blk_pwrite_zeroes(exp->blk, request->from, len, flags);
+ ret = blk_pwrite_zeroes(exp->common.blk, request->from, len, flags);
request->len -= len;
request->from += len;
}
@@ -2345,7 +2345,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
abort();
case NBD_CMD_FLUSH:
- ret = blk_co_flush(exp->blk);
+ ret = blk_co_flush(exp->common.blk);
return nbd_send_generic_reply(client, request->handle, ret,
"flush failed", errp);
@@ -2356,12 +2356,12 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
int align = client->check_align ?: 1;
int len = MIN(request->len, QEMU_ALIGN_DOWN(BDRV_REQUEST_MAX_BYTES,
align));
- ret = blk_co_pdiscard(exp->blk, request->from, len);
+ ret = blk_co_pdiscard(exp->common.blk, request->from, len);
request->len -= len;
request->from += len;
}
if (ret >= 0 && request->flags & NBD_CMD_FLAG_FUA) {
- ret = blk_co_flush(exp->blk);
+ ret = blk_co_flush(exp->common.blk);
}
return nbd_send_generic_reply(client, request->handle, ret,
"discard failed", errp);
@@ -2379,7 +2379,8 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
if (client->export_meta.base_allocation) {
ret = nbd_co_send_block_status(client, request->handle,
- blk_bs(exp->blk), request->from,
+ blk_bs(exp->common.blk),
+ request->from,
request->len, dont_fragment,
!client->export_meta.bitmap,
NBD_META_ID_BASE_ALLOCATION,
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 23/29] block/export: Create BlockBackend in blk_exp_add()
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (21 preceding siblings ...)
2020-09-07 18:20 ` [PATCH 22/29] block/export: Move blk to BlockExport Kevin Wolf
@ 2020-09-07 18:20 ` Kevin Wolf
2020-09-10 15:09 ` Max Reitz
2020-09-07 18:20 ` [PATCH 24/29] block/export: Add query-block-exports Kevin Wolf
` (6 subsequent siblings)
29 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:20 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Every export type will need a BlockBackend, so creating it centrally in
blk_exp_add() instead of the .create driver callback avoids duplication.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/nbd.h | 4 ++--
block/export/export.c | 49 +++++++++++++++++++++++++++++++++++++++----
blockdev-nbd.c | 33 ++++-------------------------
nbd/server.c | 38 +++++++++++----------------------
4 files changed, 63 insertions(+), 61 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index a4dc1f9e54..5270b7eadd 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -332,10 +332,10 @@ typedef struct NBDClient NBDClient;
int nbd_export_create(BlockExport *exp, BlockExportOptions *exp_args,
Error **errp);
-int nbd_export_new(BlockExport *blk_exp, BlockDriverState *bs,
+int nbd_export_new(BlockExport *blk_exp,
const char *name, const char *desc,
const char *bitmap, bool readonly, bool shared,
- bool writethrough, Error **errp);
+ Error **errp);
void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk);
AioContext *nbd_export_aio_context(NBDExport *exp);
diff --git a/block/export/export.c b/block/export/export.c
index 40fc9d505f..52ec00dfcf 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -58,7 +58,10 @@ static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
{
const BlockExportDriver *drv;
- BlockExport *exp;
+ BlockExport *exp = NULL;
+ BlockDriverState *bs;
+ BlockBackend *blk;
+ AioContext *ctx;
int ret;
if (!id_wellformed(export->id)) {
@@ -76,6 +79,33 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
return NULL;
}
+ bs = bdrv_lookup_bs(NULL, export->node_name, errp);
+ if (!bs) {
+ return NULL;
+ }
+
+ ctx = bdrv_get_aio_context(bs);
+ aio_context_acquire(ctx);
+
+ /*
+ * Block exports are used for non-shared storage migration. Make sure
+ * that BDRV_O_INACTIVE is cleared and the image is ready for write
+ * access since the export could be available before migration handover.
+ * ctx was acquired in the caller.
+ */
+ bdrv_invalidate_cache(bs, NULL);
+
+ blk = blk_new(ctx, BLK_PERM_CONSISTENT_READ, BLK_PERM_ALL);
+ ret = blk_insert_bs(blk, bs, errp);
+ if (ret < 0) {
+ goto fail;
+ }
+
+ if (!export->has_writethrough) {
+ export->writethrough = false;
+ }
+ blk_set_enable_write_cache(blk, !export->writethrough);
+
assert(drv->instance_size >= sizeof(BlockExport));
exp = g_malloc0(drv->instance_size);
*exp = (BlockExport) {
@@ -83,19 +113,30 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
.refcount = 1,
.user_owned = true,
.id = g_strdup(export->id),
+ .ctx = ctx,
+ .blk = blk,
};
ret = drv->create(exp, export, errp);
if (ret < 0) {
- g_free(exp->id);
- g_free(exp);
- return NULL;
+ goto fail;
}
assert(exp->blk != NULL);
QLIST_INSERT_HEAD(&block_exports, exp, next);
+
+ aio_context_release(ctx);
return exp;
+
+fail:
+ blk_unref(blk);
+ aio_context_release(ctx);
+ if (exp) {
+ g_free(exp->id);
+ g_free(exp);
+ }
+ return NULL;
}
/* Callers must hold exp->ctx lock */
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 4a9a1be571..cdbbcdb958 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -177,9 +177,6 @@ int nbd_export_create(BlockExport *exp, BlockExportOptions *exp_args,
Error **errp)
{
BlockExportOptionsNbd *arg = &exp_args->u.nbd;
- BlockDriverState *bs = NULL;
- AioContext *aio_context;
- int ret;
assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD);
@@ -207,38 +204,16 @@ int nbd_export_create(BlockExport *exp, BlockExportOptions *exp_args,
return -EEXIST;
}
- bs = bdrv_lookup_bs(NULL, exp_args->node_name, errp);
- if (!bs) {
- return -ENOENT;
- }
-
- aio_context = bdrv_get_aio_context(bs);
- aio_context_acquire(aio_context);
-
if (!arg->has_writable) {
arg->writable = false;
}
- if (bdrv_is_read_only(bs) && arg->writable) {
- ret = -EINVAL;
+ if (blk_is_read_only(exp->blk) && arg->writable) {
error_setg(errp, "Cannot export read-only node as writable");
- goto out;
- }
-
- if (!exp_args->has_writethrough) {
- exp_args->writethrough = false;
- }
-
- ret = nbd_export_new(exp, bs, arg->name, arg->description, arg->bitmap,
- !arg->writable, !arg->writable,
- exp_args->writethrough, errp);
- if (ret < 0) {
- goto out;
+ return -EINVAL;
}
- ret = 0;
- out:
- aio_context_release(aio_context);
- return ret;
+ return nbd_export_new(exp, arg->name, arg->description, arg->bitmap,
+ !arg->writable, !arg->writable, errp);
}
void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
diff --git a/nbd/server.c b/nbd/server.c
index f9af45c480..6c8532de23 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1507,56 +1507,42 @@ void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk)
blk_add_remove_bs_notifier(blk, &nbd_exp->eject_notifier);
}
-int nbd_export_new(BlockExport *blk_exp, BlockDriverState *bs,
+int nbd_export_new(BlockExport *blk_exp,
const char *name, const char *desc,
const char *bitmap, bool readonly, bool shared,
- bool writethrough, Error **errp)
+ Error **errp)
{
NBDExport *exp = container_of(blk_exp, NBDExport, common);
- AioContext *ctx;
- BlockBackend *blk;
+ BlockBackend *blk = blk_exp->blk;
int64_t size;
- uint64_t perm;
+ uint64_t perm, shared_perm;
int ret;
- size = bdrv_getlength(bs);
+ size = blk_getlength(blk);
if (size < 0) {
error_setg_errno(errp, -size,
"Failed to determine the NBD export's length");
return size;
}
- ctx = bdrv_get_aio_context(bs);
- blk_exp->ctx = ctx;
-
- /*
- * NBD exports are used for non-shared storage migration. Make sure
- * that BDRV_O_INACTIVE is cleared and the image is ready for write
- * access since the export could be available before migration handover.
- * ctx was acquired in the caller.
- */
assert(name && strlen(name) <= NBD_MAX_STRING_SIZE);
- bdrv_invalidate_cache(bs, NULL);
-
/* Don't allow resize while the NBD server is running, otherwise we don't
* care what happens with the node. */
- perm = BLK_PERM_CONSISTENT_READ;
+ blk_get_perm(blk, &perm, &shared_perm);
+
if (!readonly) {
perm |= BLK_PERM_WRITE;
}
- blk = blk_new(ctx, perm,
- BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
- BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD);
- ret = blk_insert_bs(blk, bs, errp);
+
+ ret = blk_set_perm(blk, perm, shared_perm & ~BLK_PERM_RESIZE, errp);
if (ret < 0) {
- goto fail;
+ return ret;
}
- blk_set_enable_write_cache(blk, !writethrough);
+
blk_set_allow_aio_context_change(blk, true);
QTAILQ_INIT(&exp->clients);
- exp->common.blk = blk;
exp->name = g_strdup(name);
assert(!desc || strlen(desc) <= NBD_MAX_STRING_SIZE);
exp->description = g_strdup(desc);
@@ -1574,6 +1560,7 @@ int nbd_export_new(BlockExport *blk_exp, BlockDriverState *bs,
exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE);
if (bitmap) {
+ BlockDriverState *bs = blk_bs(blk);
BdrvDirtyBitmap *bm = NULL;
while (bs) {
@@ -1620,7 +1607,6 @@ int nbd_export_new(BlockExport *blk_exp, BlockDriverState *bs,
return 0;
fail:
- blk_unref(blk);
g_free(exp->name);
g_free(exp->description);
return ret;
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 24/29] block/export: Add query-block-exports
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (22 preceding siblings ...)
2020-09-07 18:20 ` [PATCH 23/29] block/export: Create BlockBackend in blk_exp_add() Kevin Wolf
@ 2020-09-07 18:20 ` Kevin Wolf
2020-09-10 15:10 ` Max Reitz
2020-09-07 18:20 ` [PATCH 25/29] block/export: Move writable to BlockExportOptions Kevin Wolf
` (5 subsequent siblings)
29 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:20 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
This adds a simple QMP command to query the list of block exports.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-export.json | 32 ++++++++++++++++++++++++++++++++
block/export/export.c | 23 +++++++++++++++++++++++
2 files changed, 55 insertions(+)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index dac3250f08..d4e9290210 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -245,3 +245,35 @@
##
{ 'event': 'BLOCK_EXPORT_DELETED',
'data': { 'id': 'str' } }
+
+##
+# @BlockExportInfo:
+#
+# Information about a single block export.
+#
+# @id: The unique identifier for the block export
+#
+# @type: The block export type
+#
+# @node-name: The node name of the block node that is exported
+#
+# @shutting-down: True if the export is shutting down (e.g. after a
+# block-export-del command, but before the shutdown has
+# completed)
+#
+# Since: 5.2
+##
+{ 'struct': 'BlockExportInfo',
+ 'data': { 'id': 'str',
+ 'type': 'BlockExportType',
+ 'node-name': 'str',
+ 'shutting-down': 'bool' } }
+
+##
+# @query-block-exports:
+#
+# Returns: A list of BlockExportInfo describing all block exports
+#
+# Since: 5.2
+##
+{ 'command': 'query-block-exports', 'returns': ['BlockExportInfo'] }
diff --git a/block/export/export.c b/block/export/export.c
index 52ec00dfcf..bbdb256267 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -279,3 +279,26 @@ void qmp_block_export_del(const char *id,
blk_exp_request_shutdown(exp);
}
+
+BlockExportInfoList *qmp_query_block_exports(Error **errp)
+{
+ BlockExportInfoList *head = NULL, **p_next = &head;
+ BlockExport *exp;
+
+ QLIST_FOREACH(exp, &block_exports, next) {
+ BlockExportInfoList *entry = g_new0(BlockExportInfoList, 1);
+ BlockExportInfo *info = g_new(BlockExportInfo, 1);
+ *info = (BlockExportInfo) {
+ .id = g_strdup(exp->id),
+ .type = exp->drv->type,
+ .node_name = g_strdup(bdrv_get_node_name(blk_bs(exp->blk))),
+ .shutting_down = !exp->user_owned,
+ };
+
+ entry->value = info;
+ *p_next = entry;
+ p_next = &entry->next;
+ }
+
+ return head;
+}
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 25/29] block/export: Move writable to BlockExportOptions
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (23 preceding siblings ...)
2020-09-07 18:20 ` [PATCH 24/29] block/export: Add query-block-exports Kevin Wolf
@ 2020-09-07 18:20 ` Kevin Wolf
2020-09-10 15:15 ` Max Reitz
2020-09-07 18:20 ` [PATCH 26/29] nbd: Merge nbd_export_new() and nbd_export_create() Kevin Wolf
` (4 subsequent siblings)
29 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:20 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
The 'writable' option is a basic option that will probably be applicable
to most if not all export types that we will implement. Move it from NBD
to the generic BlockExport layer.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-export.json | 15 ++++++++++-----
block/export/export.c | 16 +++++++++++++++-
blockdev-nbd.c | 18 +++++-------------
nbd/server.c | 5 -----
qemu-nbd.c | 4 ++--
5 files changed, 32 insertions(+), 26 deletions(-)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index d4e9290210..ebdfa978e0 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -74,9 +74,6 @@
# @description: Free-form description of the export, up to 4096 bytes.
# (Since 5.0)
#
-# @writable: Whether clients should be able to write to the device via the
-# NBD connection (default false).
-#
# @bitmap: Also export the dirty bitmap reachable from @device, so the
# NBD client can use NBD_OPT_SET_META_CONTEXT with
# "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0)
@@ -85,7 +82,7 @@
##
{ 'struct': 'BlockExportOptionsNbd',
'data': { '*name': 'str', '*description': 'str',
- '*writable': 'bool', '*bitmap': 'str' } }
+ '*bitmap': 'str' } }
##
# @NbdServerAddOptions:
@@ -94,11 +91,15 @@
#
# @device: The device name or node name of the node to be exported
#
+# @writable: Whether clients should be able to write to the device via the
+# NBD connection (default false).
+#
# Since: 5.0
##
{ 'struct': 'NbdServerAddOptions',
'base': 'BlockExportOptionsNbd',
- 'data': { 'device': 'str' } }
+ 'data': { 'device': 'str',
+ '*writable': 'bool' } }
##
# @nbd-server-add:
@@ -188,6 +189,9 @@
#
# @node-name: The node name of the block node to be exported (since: 5.2)
#
+# @writable: True if clients should be able to write to the export
+# (default false)
+#
# @writethrough: If true, caches are flushed after every write request to the
# export before completion is signalled. (since: 5.2;
# default: false)
@@ -198,6 +202,7 @@
'base': { 'type': 'BlockExportType',
'id': 'str',
'node-name': 'str',
+ '*writable': 'bool',
'*writethrough': 'bool' },
'discriminator': 'type',
'data': {
diff --git a/block/export/export.c b/block/export/export.c
index bbdb256267..87516d1e05 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -62,6 +62,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
BlockDriverState *bs;
BlockBackend *blk;
AioContext *ctx;
+ uint64_t perm;
int ret;
if (!id_wellformed(export->id)) {
@@ -84,6 +85,14 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
return NULL;
}
+ if (!export->has_writable) {
+ export->writable = false;
+ }
+ if (bdrv_is_read_only(bs) && export->writable) {
+ error_setg(errp, "Cannot export read-only node as writable");
+ return NULL;
+ }
+
ctx = bdrv_get_aio_context(bs);
aio_context_acquire(ctx);
@@ -95,7 +104,12 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
*/
bdrv_invalidate_cache(bs, NULL);
- blk = blk_new(ctx, BLK_PERM_CONSISTENT_READ, BLK_PERM_ALL);
+ perm = BLK_PERM_CONSISTENT_READ;
+ if (export->writable) {
+ perm |= BLK_PERM_WRITE;
+ }
+
+ blk = blk_new(ctx, perm, BLK_PERM_ALL);
ret = blk_insert_bs(blk, bs, errp);
if (ret < 0) {
goto fail;
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index cdbbcdb958..30e165c23f 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -204,16 +204,8 @@ int nbd_export_create(BlockExport *exp, BlockExportOptions *exp_args,
return -EEXIST;
}
- if (!arg->has_writable) {
- arg->writable = false;
- }
- if (blk_is_read_only(exp->blk) && arg->writable) {
- error_setg(errp, "Cannot export read-only node as writable");
- return -EINVAL;
- }
-
return nbd_export_new(exp, arg->name, arg->description, arg->bitmap,
- !arg->writable, !arg->writable, errp);
+ !exp_args->writable, !exp_args->writable, errp);
}
void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
@@ -241,13 +233,13 @@ void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
.type = BLOCK_EXPORT_TYPE_NBD,
.id = g_strdup(arg->name),
.node_name = g_strdup(bdrv_get_node_name(bs)),
+ .has_writable = arg->has_writable,
+ .writable = arg->writable,
.u.nbd = {
.has_name = true,
.name = g_strdup(arg->name),
.has_description = arg->has_description,
.description = g_strdup(arg->description),
- .has_writable = arg->has_writable,
- .writable = arg->writable,
.has_bitmap = arg->has_bitmap,
.bitmap = g_strdup(arg->bitmap),
},
@@ -259,8 +251,8 @@ void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
* block-export-add.
*/
if (bdrv_is_read_only(bs)) {
- export_opts->u.nbd.has_writable = true;
- export_opts->u.nbd.writable = false;
+ export_opts->has_writable = true;
+ export_opts->writable = false;
}
export = blk_exp_add(export_opts, errp);
diff --git a/nbd/server.c b/nbd/server.c
index 6c8532de23..465ec9e762 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1530,11 +1530,6 @@ int nbd_export_new(BlockExport *blk_exp,
/* Don't allow resize while the NBD server is running, otherwise we don't
* care what happens with the node. */
blk_get_perm(blk, &perm, &shared_perm);
-
- if (!readonly) {
- perm |= BLK_PERM_WRITE;
- }
-
ret = blk_set_perm(blk, perm, shared_perm & ~BLK_PERM_RESIZE, errp);
if (ret < 0) {
return ret;
diff --git a/qemu-nbd.c b/qemu-nbd.c
index ac82acb4ac..6d7ac7490f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1068,13 +1068,13 @@ int main(int argc, char **argv)
.node_name = g_strdup(bdrv_get_node_name(bs)),
.has_writethrough = true,
.writethrough = writethrough,
+ .has_writable = true,
+ .writable = !readonly,
.u.nbd = {
.has_name = true,
.name = g_strdup(export_name),
.has_description = !!export_description,
.description = g_strdup(export_description),
- .has_writable = true,
- .writable = !readonly,
.has_bitmap = !!bitmap,
.bitmap = g_strdup(bitmap),
},
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 26/29] nbd: Merge nbd_export_new() and nbd_export_create()
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (24 preceding siblings ...)
2020-09-07 18:20 ` [PATCH 25/29] block/export: Move writable to BlockExportOptions Kevin Wolf
@ 2020-09-07 18:20 ` Kevin Wolf
2020-09-10 15:30 ` Max Reitz
2020-09-07 18:20 ` [PATCH 27/29] nbd: Deprecate nbd-server-add/remove Kevin Wolf
` (3 subsequent siblings)
29 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:20 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
There is no real reason any more why nbd_export_new() and
nbd_export_create() should be separate functions. The latter only
performs a few checks before it calls the former. Move all the checks to
nbd/server.c and make the resulting function static.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/nbd.h | 7 +-----
blockdev-nbd.c | 40 +++++----------------------------
nbd/server.c | 54 ++++++++++++++++++++++++++++++++-------------
3 files changed, 45 insertions(+), 56 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 5270b7eadd..3dd9a04546 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -330,12 +330,6 @@ int nbd_errno_to_system_errno(int err);
typedef struct NBDExport NBDExport;
typedef struct NBDClient NBDClient;
-int nbd_export_create(BlockExport *exp, BlockExportOptions *exp_args,
- Error **errp);
-int nbd_export_new(BlockExport *blk_exp,
- const char *name, const char *desc,
- const char *bitmap, bool readonly, bool shared,
- Error **errp);
void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk);
AioContext *nbd_export_aio_context(NBDExport *exp);
@@ -349,6 +343,7 @@ void nbd_client_get(NBDClient *client);
void nbd_client_put(NBDClient *client);
void nbd_server_is_qemu_nbd(bool value);
+bool nbd_server_is_running(void);
void nbd_server_start(SocketAddress *addr, const char *tls_creds,
const char *tls_authz, uint32_t max_connections,
Error **errp);
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 30e165c23f..8174023e5c 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -37,6 +37,11 @@ void nbd_server_is_qemu_nbd(bool value)
is_qemu_nbd = value;
}
+bool nbd_server_is_running(void)
+{
+ return nbd_server || is_qemu_nbd;
+}
+
static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
{
nbd_client_put(client);
@@ -173,41 +178,6 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
qapi_free_SocketAddress(addr_flat);
}
-int nbd_export_create(BlockExport *exp, BlockExportOptions *exp_args,
- Error **errp)
-{
- BlockExportOptionsNbd *arg = &exp_args->u.nbd;
-
- assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD);
-
- if (!nbd_server && !is_qemu_nbd) {
- error_setg(errp, "NBD server not running");
- return -EINVAL;
- }
-
- if (!arg->has_name) {
- arg->name = exp_args->node_name;
- }
-
- if (strlen(arg->name) > NBD_MAX_STRING_SIZE) {
- error_setg(errp, "export name '%s' too long", arg->name);
- return -EINVAL;
- }
-
- if (arg->description && strlen(arg->description) > NBD_MAX_STRING_SIZE) {
- error_setg(errp, "description '%s' too long", arg->description);
- return -EINVAL;
- }
-
- if (nbd_export_find(arg->name)) {
- error_setg(errp, "NBD server already has export named '%s'", arg->name);
- return -EEXIST;
- }
-
- return nbd_export_new(exp, arg->name, arg->description, arg->bitmap,
- !exp_args->writable, !exp_args->writable, errp);
-}
-
void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
{
BlockExport *export;
diff --git a/nbd/server.c b/nbd/server.c
index 465ec9e762..f74766add7 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1507,17 +1507,44 @@ void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk)
blk_add_remove_bs_notifier(blk, &nbd_exp->eject_notifier);
}
-int nbd_export_new(BlockExport *blk_exp,
- const char *name, const char *desc,
- const char *bitmap, bool readonly, bool shared,
- Error **errp)
+static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
+ Error **errp)
{
NBDExport *exp = container_of(blk_exp, NBDExport, common);
+ BlockExportOptionsNbd *arg = &exp_args->u.nbd;
BlockBackend *blk = blk_exp->blk;
int64_t size;
uint64_t perm, shared_perm;
+ bool readonly = !exp_args->writable;
+ bool shared = !exp_args->writable;
int ret;
+ assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD);
+
+ if (!nbd_server_is_running()) {
+ error_setg(errp, "NBD server not running");
+ return -EINVAL;
+ }
+
+ if (!arg->has_name) {
+ arg->name = exp_args->node_name;
+ }
+
+ if (strlen(arg->name) > NBD_MAX_STRING_SIZE) {
+ error_setg(errp, "export name '%s' too long", arg->name);
+ return -EINVAL;
+ }
+
+ if (arg->description && strlen(arg->description) > NBD_MAX_STRING_SIZE) {
+ error_setg(errp, "description '%s' too long", arg->description);
+ return -EINVAL;
+ }
+
+ if (nbd_export_find(arg->name)) {
+ error_setg(errp, "NBD server already has export named '%s'", arg->name);
+ return -EEXIST;
+ }
+
size = blk_getlength(blk);
if (size < 0) {
error_setg_errno(errp, -size,
@@ -1525,8 +1552,6 @@ int nbd_export_new(BlockExport *blk_exp,
return size;
}
- assert(name && strlen(name) <= NBD_MAX_STRING_SIZE);
-
/* Don't allow resize while the NBD server is running, otherwise we don't
* care what happens with the node. */
blk_get_perm(blk, &perm, &shared_perm);
@@ -1538,9 +1563,8 @@ int nbd_export_new(BlockExport *blk_exp,
blk_set_allow_aio_context_change(blk, true);
QTAILQ_INIT(&exp->clients);
- exp->name = g_strdup(name);
- assert(!desc || strlen(desc) <= NBD_MAX_STRING_SIZE);
- exp->description = g_strdup(desc);
+ exp->name = g_strdup(arg->name);
+ exp->description = g_strdup(arg->description);
exp->nbdflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_FLUSH |
NBD_FLAG_SEND_FUA | NBD_FLAG_SEND_CACHE);
if (readonly) {
@@ -1554,12 +1578,12 @@ int nbd_export_new(BlockExport *blk_exp,
}
exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE);
- if (bitmap) {
+ if (arg->bitmap) {
BlockDriverState *bs = blk_bs(blk);
BdrvDirtyBitmap *bm = NULL;
while (bs) {
- bm = bdrv_find_dirty_bitmap(bs, bitmap);
+ bm = bdrv_find_dirty_bitmap(bs, arg->bitmap);
if (bm != NULL) {
break;
}
@@ -1569,7 +1593,7 @@ int nbd_export_new(BlockExport *blk_exp,
if (bm == NULL) {
ret = -ENOENT;
- error_setg(errp, "Bitmap '%s' is not found", bitmap);
+ error_setg(errp, "Bitmap '%s' is not found", arg->bitmap);
goto fail;
}
@@ -1583,15 +1607,15 @@ int nbd_export_new(BlockExport *blk_exp,
ret = -EINVAL;
error_setg(errp,
"Enabled bitmap '%s' incompatible with readonly export",
- bitmap);
+ arg->bitmap);
goto fail;
}
bdrv_dirty_bitmap_set_busy(bm, true);
exp->export_bitmap = bm;
- assert(strlen(bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
+ assert(strlen(arg->bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
- bitmap);
+ arg->bitmap);
assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
}
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PATCH 27/29] nbd: Deprecate nbd-server-add/remove
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (25 preceding siblings ...)
2020-09-07 18:20 ` [PATCH 26/29] nbd: Merge nbd_export_new() and nbd_export_create() Kevin Wolf
@ 2020-09-07 18:20 ` Kevin Wolf
2020-09-10 15:34 ` Max Reitz
2020-09-07 18:20 ` [PATCH 28/29] iotests: Factor out qemu_tool_pipe_and_status() Kevin Wolf
` (2 subsequent siblings)
29 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:20 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
These QMP commands are replaced by block-export-add/del.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-export.json | 11 +++++++++--
docs/system/deprecated.rst | 8 ++++++++
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index ebdfa978e0..ace0d66e17 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -108,13 +108,16 @@
#
# The export name will be used as the id for the resulting block export.
#
+# Features:
+# @deprecated: This command is deprecated. Use @block-export-add instead.
+#
# Returns: error if the server is not running, or export with the same name
# already exists.
#
# Since: 1.3.0
##
{ 'command': 'nbd-server-add',
- 'data': 'NbdServerAddOptions', 'boxed': true }
+ 'data': 'NbdServerAddOptions', 'boxed': true, 'features': ['deprecated'] }
##
# @BlockExportRemoveMode:
@@ -147,6 +150,9 @@
# @mode: Mode of command operation. See @BlockExportRemoveMode description.
# Default is 'safe'.
#
+# Features:
+# @deprecated: This command is deprecated. Use @block-export-del instead.
+#
# Returns: error if
# - the server is not running
# - export is not found
@@ -155,7 +161,8 @@
# Since: 2.12
##
{ 'command': 'nbd-server-remove',
- 'data': {'name': 'str', '*mode': 'BlockExportRemoveMode'} }
+ 'data': {'name': 'str', '*mode': 'BlockExportRemoveMode'},
+ 'features': ['deprecated'] }
##
# @nbd-server-stop:
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 851dbdeb8a..9a2e083f00 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -303,6 +303,14 @@ chardev client socket with ``wait`` option (since 4.0)
Character devices creating sockets in client mode should not specify
the 'wait' field, which is only applicable to sockets in server mode
+``nbd-server-add`` and ``nbd-server-remove`` (since 5.2)
+''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Use the more generic commands ``block-export-add`` and ``block-export-del``
+instead.
+
+Use argument value ``null`` instead.
+
Human Monitor Protocol (HMP) commands
-------------------------------------
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 27/29] nbd: Deprecate nbd-server-add/remove
2020-09-07 18:20 ` [PATCH 27/29] nbd: Deprecate nbd-server-add/remove Kevin Wolf
@ 2020-09-10 15:34 ` Max Reitz
2020-09-23 16:19 ` Kevin Wolf
0 siblings, 1 reply; 53+ messages in thread
From: Max Reitz @ 2020-09-10 15:34 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1045 bytes --]
On 07.09.20 20:20, Kevin Wolf wrote:
> These QMP commands are replaced by block-export-add/del.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/block-export.json | 11 +++++++++--
> docs/system/deprecated.rst | 8 ++++++++
> 2 files changed, 17 insertions(+), 2 deletions(-)
OK, but:
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 851dbdeb8a..9a2e083f00 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -303,6 +303,14 @@ chardev client socket with ``wait`` option (since 4.0)
> Character devices creating sockets in client mode should not specify
> the 'wait' field, which is only applicable to sockets in server mode
>
> +``nbd-server-add`` and ``nbd-server-remove`` (since 5.2)
> +''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +Use the more generic commands ``block-export-add`` and ``block-export-del``
> +instead.
> +
> +Use argument value ``null`` instead.
I don’t understand this sentence.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 27/29] nbd: Deprecate nbd-server-add/remove
2020-09-10 15:34 ` Max Reitz
@ 2020-09-23 16:19 ` Kevin Wolf
0 siblings, 0 replies; 53+ messages in thread
From: Kevin Wolf @ 2020-09-23 16:19 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]
Am 10.09.2020 um 17:34 hat Max Reitz geschrieben:
> On 07.09.20 20:20, Kevin Wolf wrote:
> > These QMP commands are replaced by block-export-add/del.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > qapi/block-export.json | 11 +++++++++--
> > docs/system/deprecated.rst | 8 ++++++++
> > 2 files changed, 17 insertions(+), 2 deletions(-)
>
> OK, but:
>
> > diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> > index 851dbdeb8a..9a2e083f00 100644
> > --- a/docs/system/deprecated.rst
> > +++ b/docs/system/deprecated.rst
> > @@ -303,6 +303,14 @@ chardev client socket with ``wait`` option (since 4.0)
> > Character devices creating sockets in client mode should not specify
> > the 'wait' field, which is only applicable to sockets in server mode
> >
> > +``nbd-server-add`` and ``nbd-server-remove`` (since 5.2)
> > +''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> > +
> > +Use the more generic commands ``block-export-add`` and ``block-export-del``
> > +instead.
> > +
> > +Use argument value ``null`` instead.
>
> I don’t understand this sentence.
Me neither, it's wrong. Looks like a copy from a few lines further up...
I'll just remove this sentence.
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 28/29] iotests: Factor out qemu_tool_pipe_and_status()
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (26 preceding siblings ...)
2020-09-07 18:20 ` [PATCH 27/29] nbd: Deprecate nbd-server-add/remove Kevin Wolf
@ 2020-09-07 18:20 ` Kevin Wolf
2020-09-10 15:45 ` Max Reitz
2020-09-07 18:20 ` [PATCH 29/29] iotests: Test block-export-* QMP interface Kevin Wolf
2020-09-08 8:38 ` [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Markus Armbruster
29 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:20 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
We have three almost identical functions that call an external process
and return its output and return code. Refactor them into small wrappers
around a common function.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
tests/qemu-iotests/iotests.py | 50 +++++++++++++++++------------------
1 file changed, 24 insertions(+), 26 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 64ccaf9061..6fed77487e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -90,21 +90,30 @@ luks_default_secret_object = 'secret,id=keysec0,data=' + \
luks_default_key_secret_opt = 'key-secret=keysec0'
-def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
+def qemu_tool_pipe_and_status(tool: str, *args: str,
+ connect_stderr: bool = True) -> Tuple[str, int]:
"""
- Run qemu-img and return both its output and its exit code
+ Run a tool and return both its output and its exit code
"""
- subp = subprocess.Popen(qemu_img_args + list(args),
+ stderr = subprocess.STDOUT if connect_stderr else None
+ subp = subprocess.Popen(args,
stdout=subprocess.PIPE,
- stderr=subprocess.STDOUT,
+ stderr=stderr,
universal_newlines=True)
output = subp.communicate()[0]
if subp.returncode < 0:
- sys.stderr.write('qemu-img received signal %i: %s\n'
- % (-subp.returncode,
+ sys.stderr.write('%s received signal %i: %s\n'
+ % (tool, -subp.returncode,
' '.join(qemu_img_args + list(args))))
return (output, subp.returncode)
+def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
+ """
+ Run qemu-img and return both its output and its exit code
+ """
+ full_args = qemu_img_args + list(args)
+ return qemu_tool_pipe_and_status('qemu-img', *full_args)
+
def qemu_img(*args: str) -> int:
'''Run qemu-img and return the exit code'''
return qemu_img_pipe_and_status(*args)[1]
@@ -265,19 +274,14 @@ def qemu_nbd(*args):
'''Run qemu-nbd in daemon mode and return the parent's exit code'''
return subprocess.call(qemu_nbd_args + ['--fork'] + list(args))
-def qemu_nbd_early_pipe(*args):
+def qemu_nbd_early_pipe(*args: str) -> Tuple[int, str]:
'''Run qemu-nbd in daemon mode and return both the parent's exit code
and its output in case of an error'''
- subp = subprocess.Popen(qemu_nbd_args + ['--fork'] + list(args),
- stdout=subprocess.PIPE,
- universal_newlines=True)
- output = subp.communicate()[0]
- if subp.returncode < 0:
- sys.stderr.write('qemu-nbd received signal %i: %s\n' %
- (-subp.returncode,
- ' '.join(qemu_nbd_args + ['--fork'] + list(args))))
-
- return subp.returncode, output if subp.returncode else ''
+ full_args = qemu_nbd_args + ['--fork'] + list(args)
+ output, returncode = qemu_tool_pipe_and_status('qemu-nbd',
+ connect_stderr=False,
+ *full_args)
+ return returncode, output if returncode else ''
@contextmanager
def qemu_nbd_popen(*args):
@@ -1140,20 +1144,14 @@ def verify_working_luks():
if not working:
notrun(reason)
-def qemu_pipe(*args):
+def qemu_pipe(*args: str) -> str:
"""
Run qemu with an option to print something and exit (e.g. a help option).
:return: QEMU's stdout output.
"""
- args = [qemu_prog] + qemu_opts + list(args)
- subp = subprocess.Popen(args, stdout=subprocess.PIPE,
- stderr=subprocess.STDOUT,
- universal_newlines=True)
- output = subp.communicate()[0]
- if subp.returncode < 0:
- sys.stderr.write('qemu received signal %i: %s\n' %
- (-subp.returncode, ' '.join(args)))
+ full_args = [qemu_prog] + qemu_opts + list(args)
+ output, _ = qemu_tool_pipe_and_status('qemu', *full_args)
return output
def supported_formats(read_only=False):
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 28/29] iotests: Factor out qemu_tool_pipe_and_status()
2020-09-07 18:20 ` [PATCH 28/29] iotests: Factor out qemu_tool_pipe_and_status() Kevin Wolf
@ 2020-09-10 15:45 ` Max Reitz
0 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2020-09-10 15:45 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 1819 bytes --]
On 07.09.20 20:20, Kevin Wolf wrote:
> We have three almost identical functions that call an external process
> and return its output and return code. Refactor them into small wrappers
> around a common function.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> tests/qemu-iotests/iotests.py | 50 +++++++++++++++++------------------
> 1 file changed, 24 insertions(+), 26 deletions(-)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 64ccaf9061..6fed77487e 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -90,21 +90,30 @@ luks_default_secret_object = 'secret,id=keysec0,data=' + \
> luks_default_key_secret_opt = 'key-secret=keysec0'
>
>
> -def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
> +def qemu_tool_pipe_and_status(tool: str, *args: str,
If this is just an internal function not to be used outside of
iotests.py, I wonder if “args: Sequence[str]” wouldn’t be cleaner.
(Just because I feel like the *args notation is useful to provide a
nicer-looking external interface, but is somehow unclean, so I
personally avoid it unless it actually does make things look nicer.
Just a very personal feeling, though, of course.)
> + connect_stderr: bool = True) -> Tuple[str, int]:
> """
> - Run qemu-img and return both its output and its exit code
> + Run a tool and return both its output and its exit code
> """
> - subp = subprocess.Popen(qemu_img_args + list(args),
> + stderr = subprocess.STDOUT if connect_stderr else None
I really despise this notation. It’s like ternary operators in C
weren’t bad enough.
(Again with the personal feelings. Sorry.)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 29/29] iotests: Test block-export-* QMP interface
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (27 preceding siblings ...)
2020-09-07 18:20 ` [PATCH 28/29] iotests: Factor out qemu_tool_pipe_and_status() Kevin Wolf
@ 2020-09-07 18:20 ` Kevin Wolf
2020-09-10 16:11 ` Max Reitz
2020-09-08 8:38 ` [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Markus Armbruster
29 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2020-09-07 18:20 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
tests/qemu-iotests/iotests.py | 11 +++-
tests/qemu-iotests/307 | 117 ++++++++++++++++++++++++++++++++++
tests/qemu-iotests/307.out | 111 ++++++++++++++++++++++++++++++++
tests/qemu-iotests/group | 1 +
4 files changed, 239 insertions(+), 1 deletion(-)
create mode 100755 tests/qemu-iotests/307
create mode 100644 tests/qemu-iotests/307.out
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6fed77487e..a842a9f92d 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -67,7 +67,8 @@ if os.environ.get('QEMU_IO_OPTIONS_NO_FMT'):
qemu_io_args_no_fmt += \
os.environ['QEMU_IO_OPTIONS_NO_FMT'].strip().split(' ')
-qemu_nbd_args = [os.environ.get('QEMU_NBD_PROG', 'qemu-nbd')]
+qemu_nbd_prog = [os.environ.get('QEMU_NBD_PROG', 'qemu-nbd')]
+qemu_nbd_args = qemu_nbd_prog
if os.environ.get('QEMU_NBD_OPTIONS'):
qemu_nbd_args += os.environ['QEMU_NBD_OPTIONS'].strip().split(' ')
@@ -283,6 +284,14 @@ def qemu_nbd_early_pipe(*args: str) -> Tuple[int, str]:
*full_args)
return returncode, output if returncode else ''
+def qemu_nbd_list(*args: str) -> str:
+ '''Run qemu-nbd to list remote exports'''
+ full_args = qemu_nbd_prog + ['-L'] + list(args)
+ output, _ = qemu_tool_pipe_and_status('qemu-nbd', *full_args)
+ log(output, filters=[filter_testfiles])
+ return output
+
+
@contextmanager
def qemu_nbd_popen(*args):
'''Context manager running qemu-nbd within the context'''
diff --git a/tests/qemu-iotests/307 b/tests/qemu-iotests/307
new file mode 100755
index 0000000000..06b9eeb15f
--- /dev/null
+++ b/tests/qemu-iotests/307
@@ -0,0 +1,117 @@
+#!/usr/bin/env python3
+#
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+# Creator/Owner: Kevin Wolf <kwolf@redhat.com>
+#
+# Test the block export QAPI interfaces
+
+import iotests
+import os
+
+# Formats that have a request alignment != 1 result in different output for
+# qemu-nbd -L ("min block")
+iotests.script_initialize(
+ supported_fmts=['raw', 'qcow2', 'vmdk'],
+ supported_platforms=['linux'],
+)
+
+with iotests.FilePath('image') as img, \
+ iotests.FilePath('socket') as socket, \
+ iotests.VM() as vm:
+
+ iotests.qemu_img('create', '-f', iotests.imgfmt, img, '64M')
+ iotests.qemu_io_log('-f', iotests.imgfmt, '-c', 'write -P 0x11 0 4k', img)
+
+ iotests.log('=== Launch VM ===')
+
+ virtio_scsi_device = iotests.get_virtio_scsi_device()
+
+ vm.add_object('iothread,id=iothread0')
+ vm.add_blockdev('file,filename=%s,node-name=file' % (img))
+ vm.add_blockdev('%s,file=file,node-name=fmt' % (iotests.imgfmt))
+ vm.add_blockdev('raw,file=file,node-name=ro,read-only=on')
+ vm.add_device('id=scsi0,driver=%s,iothread=iothread0' % virtio_scsi_device)
+ vm.launch()
+
+ vm.qmp_log('nbd-server-start',
+ addr={'type': 'unix', 'data': {'path': socket}},
+ filters=(iotests.filter_qmp_testfiles, ))
+ vm.qmp_log('query-block-exports')
+
+ iotests.log('\n=== Create a read-only NBD export ===')
+
+ vm.qmp_log('block-export-add', id='export0', type='nbd', node_name='fmt')
+ vm.qmp_log('query-block-exports')
+
+ iotests.qemu_nbd_list('-k', socket)
+
+ iotests.log('\n=== Try a few invalid things ===')
+
+ vm.qmp_log('block-export-add', id='#invalid_id', type='nbd', node_name='fmt')
+ vm.qmp_log('block-export-add', id='export0', type='nbd', node_name='fmt')
+ vm.qmp_log('block-export-add', id='export1', type='nbd', node_name='ro',
+ writable=True)
+ vm.qmp_log('block-export-del', id='export1')
+ vm.qmp_log('query-block-exports')
+
+ iotests.log('\n=== Move export to an iothread ===')
+
+ vm.qmp_log('device_add', id='sda', driver='scsi-hd', drive='fmt')
+ vm.qmp_log('query-block-exports')
+ iotests.qemu_nbd_list('-k', socket)
+
+ iotests.log('\n=== Add a writable export ===')
+
+ # This fails because share-rw=off
+ vm.qmp_log('block-export-add', id='export1', type='nbd', node_name='fmt',
+ name='export1', writable=True, writethrough=True,
+ description='This is the writable second export')
+
+ vm.qmp_log('device_del', id='sda')
+ event = vm.event_wait(name="DEVICE_DELETED",
+ match={'data': {'device': 'sda'}})
+ iotests.log(event, filters=[iotests.filter_qmp_event])
+ vm.qmp_log('device_add', id='sda', driver='scsi-hd', drive='fmt',
+ share_rw=True)
+
+ # Now it should work
+ vm.qmp_log('block-export-add', id='export1', type='nbd', node_name='fmt',
+ name='export1', writable=True, writethrough=True,
+ description='This is the writable second export')
+
+ vm.qmp_log('query-block-exports')
+ iotests.qemu_nbd_list('-k', socket)
+
+ iotests.log('\n=== Connect qemu-io to export1, try removing exports ===')
+
+ nbd_url = 'nbd+unix:///export1?socket=%s' % socket
+ qemu_io = iotests.QemuIoInteractive('-f', 'raw', nbd_url)
+
+ iotests.log(qemu_io.cmd('read -P 0x11 0 4k'),
+ filters=[iotests.filter_qemu_io])
+ iotests.log(qemu_io.cmd('write -P 0x22 4k 4k'),
+ filters=[iotests.filter_qemu_io])
+
+ vm.qmp_log('block-export-del', id='export1')
+ vm.qmp_log('block-export-del', id='export0')
+ qemu_io.close()
+
+ vm.qmp_log('query-block-exports')
+ iotests.qemu_nbd_list('-k', socket)
+
+ iotests.log('\n=== Shut down QEMU ===')
+ vm.shutdown()
diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out
new file mode 100644
index 0000000000..d8e572cac0
--- /dev/null
+++ b/tests/qemu-iotests/307.out
@@ -0,0 +1,111 @@
+wrote 4096/4096 bytes at offset 0
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Launch VM ===
+{"execute": "nbd-server-start", "arguments": {"addr": {"data": {"path": "TEST_DIR/PID-socket"}, "type": "unix"}}}
+{"return": {}}
+{"execute": "query-block-exports", "arguments": {}}
+{"return": []}
+
+=== Create a read-only NBD export ===
+{"execute": "block-export-add", "arguments": {"id": "export0", "node-name": "fmt", "type": "nbd"}}
+{"return": {}}
+{"execute": "query-block-exports", "arguments": {}}
+{"return": [{"id": "export0", "node-name": "fmt", "shutting-down": false, "type": "nbd"}]}
+exports available: 1
+ export: 'fmt'
+ size: 67108864
+ flags: 0x58f ( readonly flush fua df multi cache )
+ min block: 1
+ opt block: 4096
+ max block: 33554432
+ available meta contexts: 1
+ base:allocation
+
+
+=== Try a few invalid things ===
+{"execute": "block-export-add", "arguments": {"id": "#invalid_id", "node-name": "fmt", "type": "nbd"}}
+{"error": {"class": "GenericError", "desc": "Invalid block export id"}}
+{"execute": "block-export-add", "arguments": {"id": "export0", "node-name": "fmt", "type": "nbd"}}
+{"error": {"class": "GenericError", "desc": "Block export id 'export0' is already in use"}}
+{"execute": "block-export-add", "arguments": {"id": "export1", "node-name": "ro", "type": "nbd", "writable": true}}
+{"error": {"class": "GenericError", "desc": "Cannot export read-only node as writable"}}
+{"execute": "block-export-del", "arguments": {"id": "export1"}}
+{"error": {"class": "GenericError", "desc": "Export 'export1' is not found"}}
+{"execute": "query-block-exports", "arguments": {}}
+{"return": [{"id": "export0", "node-name": "fmt", "shutting-down": false, "type": "nbd"}]}
+
+=== Move export to an iothread ===
+{"execute": "device_add", "arguments": {"drive": "fmt", "driver": "scsi-hd", "id": "sda"}}
+{"return": {}}
+{"execute": "query-block-exports", "arguments": {}}
+{"return": [{"id": "export0", "node-name": "fmt", "shutting-down": false, "type": "nbd"}]}
+exports available: 1
+ export: 'fmt'
+ size: 67108864
+ flags: 0x58f ( readonly flush fua df multi cache )
+ min block: 1
+ opt block: 4096
+ max block: 33554432
+ available meta contexts: 1
+ base:allocation
+
+
+=== Add a writable export ===
+{"execute": "block-export-add", "arguments": {"description": "This is the writable second export", "id": "export1", "name": "export1", "node-name": "fmt", "type": "nbd", "writable": true, "writethrough": true}}
+{"error": {"class": "GenericError", "desc": "Conflicts with use by sda as 'root', which does not allow 'write' on fmt"}}
+{"execute": "device_del", "arguments": {"id": "sda"}}
+{"return": {}}
+{"data": {"device": "sda", "path": "/machine/peripheral/sda"}, "event": "DEVICE_DELETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "device_add", "arguments": {"drive": "fmt", "driver": "scsi-hd", "id": "sda", "share-rw": true}}
+{"return": {}}
+{"execute": "block-export-add", "arguments": {"description": "This is the writable second export", "id": "export1", "name": "export1", "node-name": "fmt", "type": "nbd", "writable": true, "writethrough": true}}
+{"return": {}}
+{"execute": "query-block-exports", "arguments": {}}
+{"return": [{"id": "export1", "node-name": "fmt", "shutting-down": false, "type": "nbd"}, {"id": "export0", "node-name": "fmt", "shutting-down": false, "type": "nbd"}]}
+exports available: 2
+ export: 'fmt'
+ size: 67108864
+ flags: 0x58f ( readonly flush fua df multi cache )
+ min block: 1
+ opt block: 4096
+ max block: 33554432
+ available meta contexts: 1
+ base:allocation
+ export: 'export1'
+ description: This is the writable second export
+ size: 67108864
+ flags: 0xced ( flush fua trim zeroes df cache fast-zero )
+ min block: 1
+ opt block: 4096
+ max block: 33554432
+ available meta contexts: 1
+ base:allocation
+
+
+=== Connect qemu-io to export1, try removing exports ===
+read 4096/4096 bytes at offset 0
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 4096/4096 bytes at offset 4096
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+{"execute": "block-export-del", "arguments": {"id": "export1"}}
+{"error": {"class": "GenericError", "desc": "export 'export1' still in use"}}
+{"execute": "block-export-del", "arguments": {"id": "export0"}}
+{"return": {}}
+{"execute": "query-block-exports", "arguments": {}}
+{"return": [{"id": "export1", "node-name": "fmt", "shutting-down": false, "type": "nbd"}]}
+exports available: 1
+ export: 'export1'
+ description: This is the writable second export
+ size: 67108864
+ flags: 0xced ( flush fua trim zeroes df cache fast-zero )
+ min block: 1
+ opt block: 4096
+ max block: 33554432
+ available meta contexts: 1
+ base:allocation
+
+
+=== Shut down QEMU ===
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 5cad015231..f0e9030aba 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -313,3 +313,4 @@
302 quick
303 rw quick
304 rw quick
+307 rw quick export
--
2.25.4
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PATCH 29/29] iotests: Test block-export-* QMP interface
2020-09-07 18:20 ` [PATCH 29/29] iotests: Test block-export-* QMP interface Kevin Wolf
@ 2020-09-10 16:11 ` Max Reitz
0 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2020-09-10 16:11 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 8555 bytes --]
On 07.09.20 20:20, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> tests/qemu-iotests/iotests.py | 11 +++-
> tests/qemu-iotests/307 | 117 ++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/307.out | 111 ++++++++++++++++++++++++++++++++
> tests/qemu-iotests/group | 1 +
> 4 files changed, 239 insertions(+), 1 deletion(-)
> create mode 100755 tests/qemu-iotests/307
> create mode 100644 tests/qemu-iotests/307.out
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 6fed77487e..a842a9f92d 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -67,7 +67,8 @@ if os.environ.get('QEMU_IO_OPTIONS_NO_FMT'):
> qemu_io_args_no_fmt += \
> os.environ['QEMU_IO_OPTIONS_NO_FMT'].strip().split(' ')
>
> -qemu_nbd_args = [os.environ.get('QEMU_NBD_PROG', 'qemu-nbd')]
> +qemu_nbd_prog = [os.environ.get('QEMU_NBD_PROG', 'qemu-nbd')]
It seems counterintuitive to me that something called “*_prog” would be
a list.
> +qemu_nbd_args = qemu_nbd_prog
> if os.environ.get('QEMU_NBD_OPTIONS'):
> qemu_nbd_args += os.environ['QEMU_NBD_OPTIONS'].strip().split(' ')
>
> @@ -283,6 +284,14 @@ def qemu_nbd_early_pipe(*args: str) -> Tuple[int, str]:
> *full_args)
> return returncode, output if returncode else ''
>
> +def qemu_nbd_list(*args: str) -> str:
> + '''Run qemu-nbd to list remote exports'''
> + full_args = qemu_nbd_prog + ['-L'] + list(args)
> + output, _ = qemu_tool_pipe_and_status('qemu-nbd', *full_args)
> + log(output, filters=[filter_testfiles])
Not sure whether I like functions “silently” auto-logging the result.
I’d be happier if it were called *_log (i.e. qemu_nbd_list_log) like
most other functions that do.
> + return output
> +
> +
> @contextmanager
> def qemu_nbd_popen(*args):
> '''Context manager running qemu-nbd within the context'''
This would fit nicely into a dedicated patch.
*shrug*
> diff --git a/tests/qemu-iotests/307 b/tests/qemu-iotests/307
> new file mode 100755
> index 0000000000..06b9eeb15f
> --- /dev/null
> +++ b/tests/qemu-iotests/307
> @@ -0,0 +1,117 @@
> +#!/usr/bin/env python3
> +#
> +# Copyright (C) 2020 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +#
> +# Creator/Owner: Kevin Wolf <kwolf@redhat.com>
> +#
> +# Test the block export QAPI interfaces
> +
> +import iotests
> +import os
> +
> +# Formats that have a request alignment != 1 result in different output for
> +# qemu-nbd -L ("min block")
So? Why not filter that out?
I wonder anyway why the whole export output is logged. It seems to me
like it would be sufficient to just log the number of exports available
and their names.
(Sure, logging everything gives us the advantage of perhaps finding NBD
bugs with this test even though it isn’t really an NBD test but intended
largely as a test for the new interface. But when I see something like
“opt block” or “max block” in the output, I get a tingling sensation of
this potentially causing problems in the future depending on where this
test is running.)
((Which is funny, because now I notice I didn’t have this fear for “min
block”, but that’s where you actually did find an issue.))
Though all in all I don’t even know why it is that you write data at all
and use images, instead of just backing the export by a null-co node.
(I do admit I’m sending mixed signals here – first I ask for whether we
couldn’t make this test run on more formats, then I propose abandoning
physical images altogether. I’m just emitting the questions that pop
into my head, and my head can be a confusing place from time to time.)
> +iotests.script_initialize(
> + supported_fmts=['raw', 'qcow2', 'vmdk'],
> + supported_platforms=['linux'],
> +)
> +
> +with iotests.FilePath('image') as img, \
> + iotests.FilePath('socket') as socket, \
The second one should use base_dir=iotests.sock_dir.
> + iotests.VM() as vm:
> +
> + iotests.qemu_img('create', '-f', iotests.imgfmt, img, '64M')
> + iotests.qemu_io_log('-f', iotests.imgfmt, '-c', 'write -P 0x11 0 4k', img)
> +
> + iotests.log('=== Launch VM ===')
> +
> + virtio_scsi_device = iotests.get_virtio_scsi_device()
> +
> + vm.add_object('iothread,id=iothread0')
> + vm.add_blockdev('file,filename=%s,node-name=file' % (img))
> + vm.add_blockdev('%s,file=file,node-name=fmt' % (iotests.imgfmt))
> + vm.add_blockdev('raw,file=file,node-name=ro,read-only=on')
> + vm.add_device('id=scsi0,driver=%s,iothread=iothread0' % virtio_scsi_device)
Format strings would look nicer to me. Perhaps not to you, though.
> + vm.launch()
> +
> + vm.qmp_log('nbd-server-start',
> + addr={'type': 'unix', 'data': {'path': socket}},
> + filters=(iotests.filter_qmp_testfiles, ))
> + vm.qmp_log('query-block-exports')
> +
> + iotests.log('\n=== Create a read-only NBD export ===')
> +
> + vm.qmp_log('block-export-add', id='export0', type='nbd', node_name='fmt')
> + vm.qmp_log('query-block-exports')
> +
> + iotests.qemu_nbd_list('-k', socket)
> +
> + iotests.log('\n=== Try a few invalid things ===')
> +
> + vm.qmp_log('block-export-add', id='#invalid_id', type='nbd', node_name='fmt')
> + vm.qmp_log('block-export-add', id='export0', type='nbd', node_name='fmt')
> + vm.qmp_log('block-export-add', id='export1', type='nbd', node_name='ro',
> + writable=True)
> + vm.qmp_log('block-export-del', id='export1')
> + vm.qmp_log('query-block-exports')
> +
> + iotests.log('\n=== Move export to an iothread ===')
> +
> + vm.qmp_log('device_add', id='sda', driver='scsi-hd', drive='fmt')
> + vm.qmp_log('query-block-exports')
> + iotests.qemu_nbd_list('-k', socket)
> +
> + iotests.log('\n=== Add a writable export ===')
> +
> + # This fails because share-rw=off
> + vm.qmp_log('block-export-add', id='export1', type='nbd', node_name='fmt',
> + name='export1', writable=True, writethrough=True,
> + description='This is the writable second export')
> +
> + vm.qmp_log('device_del', id='sda')
> + event = vm.event_wait(name="DEVICE_DELETED",
> + match={'data': {'device': 'sda'}})
> + iotests.log(event, filters=[iotests.filter_qmp_event])
> + vm.qmp_log('device_add', id='sda', driver='scsi-hd', drive='fmt',
> + share_rw=True)
> +
> + # Now it should work
> + vm.qmp_log('block-export-add', id='export1', type='nbd', node_name='fmt',
> + name='export1', writable=True, writethrough=True,
> + description='This is the writable second export')
> +
> + vm.qmp_log('query-block-exports')
> + iotests.qemu_nbd_list('-k', socket)
> +
> + iotests.log('\n=== Connect qemu-io to export1, try removing exports ===')
> +
> + nbd_url = 'nbd+unix:///export1?socket=%s' % socket
> + qemu_io = iotests.QemuIoInteractive('-f', 'raw', nbd_url)
> +
> + iotests.log(qemu_io.cmd('read -P 0x11 0 4k'),
> + filters=[iotests.filter_qemu_io])
> + iotests.log(qemu_io.cmd('write -P 0x22 4k 4k'),
> + filters=[iotests.filter_qemu_io])
> +
> + vm.qmp_log('block-export-del', id='export1')
> + vm.qmp_log('block-export-del', id='export0')
Should we check for the BLOCK_EXPORT_DELETED event?
> + qemu_io.close()
> +
> + vm.qmp_log('query-block-exports')
> + iotests.qemu_nbd_list('-k', socket)
It might be nice to reconnect with qemu-io, and then see whether a hard
del works.
Max
> +
> + iotests.log('\n=== Shut down QEMU ===')
> + vm.shutdown()
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports
2020-09-07 18:19 [PATCH 00/29] block/export: Add infrastructure and QAPI for block exports Kevin Wolf
` (28 preceding siblings ...)
2020-09-07 18:20 ` [PATCH 29/29] iotests: Test block-export-* QMP interface Kevin Wolf
@ 2020-09-08 8:38 ` Markus Armbruster
2020-09-08 12:29 ` Kevin Wolf
29 siblings, 1 reply; 53+ messages in thread
From: Markus Armbruster @ 2020-09-08 8:38 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mreitz
Doesn't apply for me. Got something I could pull?
^ permalink raw reply [flat|nested] 53+ messages in thread