block/export: Remove magic from block-export-add

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>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-Id: <20200924152717.287415-8-kwolf@redhat.com>
Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>

6 files changed, 67 insertions, 21 deletions

diff --git a/block/export/export.c b/block/export/export.c
index fd65541..05bc5e3 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 96cb010..7bcca10 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/include/block/export.h b/include/block/export.h
index 42e3c05..e7af2c7 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 451f399..f55f5b7 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/nbd/server.c b/nbd/server.c
index 33aaca9..23d9a53 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 16473d8..b2a0ea6 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