diff mbox series

[RFC,16/22] block/export: Allocate BlockExport in blk_exp_add()

Message ID 20200813162935.210070-17-kwolf@redhat.com
State New
Headers show
Series block/export: Add infrastructure and QAPI for block exports | expand

Commit Message

Kevin Wolf Aug. 13, 2020, 4:29 p.m. UTC 
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>
---
 include/block/export.h |  3 ++-
 include/block/nbd.h    | 11 ++++++-----
 block/export/export.c  | 17 ++++++++++++++++-
 blockdev-nbd.c         | 24 +++++++++++++-----------
 nbd/server.c           | 30 +++++++++++++-----------------
 5 files changed, 50 insertions(+), 35 deletions(-)

Comments

Max Reitz Aug. 18, 2020, 2:25 p.m. UTC | #1 
On 13.08.20 18:29, 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>
> ---
>  include/block/export.h |  3 ++-
>  include/block/nbd.h    | 11 ++++++-----
>  block/export/export.c  | 17 ++++++++++++++++-
>  blockdev-nbd.c         | 24 +++++++++++++-----------
>  nbd/server.c           | 30 +++++++++++++-----------------
>  5 files changed, 50 insertions(+), 35 deletions(-)

[...]

> diff --git a/block/export/export.c b/block/export/export.c
> index ef86bf892b..9de108cbc1 100644
> --- a/block/export/export.c
> +++ b/block/export/export.c

[...]

> @@ -46,7 +48,19 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
>          return NULL;
>      }
>  
> -    return drv->create(export, errp);

assert(drv->instance_size >= sizeof(BlockExport)) might be nice before
dereferencing *exp.

With that:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +    exp = g_malloc0(drv->instance_size);
> +    *exp = (BlockExport) {
> +        .drv        = &blk_exp_nbd,
> +        .refcount   = 1,
> +    };
diff mbox series

Patch

diff --git a/include/block/export.h b/include/block/export.h
index 5459f79469..597fc58245 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -20,7 +20,8 @@  typedef struct BlockExport BlockExport;
 
 typedef struct BlockExportDriver {
     BlockExportType type;
-    BlockExport *(*create)(BlockExportOptions *, Error **);
+    size_t instance_size;
+    int (*create)(BlockExport *, BlockExportOptions *, Error **);
     void (*delete)(BlockExport *);
 } BlockExportDriver;
 
diff --git a/include/block/nbd.h b/include/block/nbd.h
index af8509ab70..602536feb2 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -328,11 +328,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 ef86bf892b..9de108cbc1 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,19 @@  BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
         return NULL;
     }
 
-    return drv->create(export, errp);
+    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;
 }
 
 void blk_exp_ref(BlockExport *exp)
@@ -60,6 +74,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 5e97975c80..f97deba424 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(exp_args->device, exp_args->device, errp);
     if (!bs) {
-        return NULL;
+        return -ENOENT;
     }
 
     aio_context = bdrv_get_aio_context(bs);
@@ -218,14 +219,15 @@  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;
     }
 
-    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;
     }
 
@@ -234,10 +236,10 @@  BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
      * the strong reference that is @exp. */
     blk_exp_unref((BlockExport*) exp);
 
+    ret = 0;
  out:
     aio_context_release(aio_context);
-    /* TODO Remove the cast: Move to server.c which can access fields of exp */
-    return (BlockExport*) exp;
+    return ret;
 }
 
 void qmp_nbd_server_remove(const char *name,
diff --git a/nbd/server.c b/nbd/server.c
index b735a68429..6a58297557 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1513,14 +1513,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;
@@ -1529,17 +1529,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,
 };