On 8/13/20 11:29 AM, Kevin Wolf wrote:
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 <kw...@redhat.com>
---
+++ b/include/block/export.h
@@ -21,14 +21,24 @@ typedef struct BlockExport BlockExport;
typedef struct BlockExportDriver {
BlockExportType type;
BlockExport *(*create)(BlockExportOptions *, Error **);
+ 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.
I guess 'the monitor' includes qemu-storage-daemon.
+ */
+ int refcount;
};
extern const BlockExportDriver blk_exp_nbd;
BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp);
+void blk_exp_ref(BlockExport *exp);
+void blk_exp_unref(BlockExport *exp);
Yay, I think this naming is more consistent with the rest of qemu...
#endif
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 23030db3f1..af8509ab70 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -336,8 +336,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);
...as opposed to this which is common in kernel but less so in this
project. No hard feelings from me :)
+++ b/blockdev-nbd.c
@@ -232,7 +232,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);
Even a helper function that converts NBDBlockExport* to BlockExport*
rather than a cast might be nicer, but then again, I see from Max's
review that this may be a temporary state of things.
(The QAPI contains such type-safe container casts, such as
qapi_DriveBackup_base(), if that helps...)
@@ -1537,7 +1536,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,
I'm not sure whether trying to align the '=' is good, because the moment
you add a longer field name, every other line has to be touched. I'm
fine with just one space on both side of =, even if it is more ragged to
read. But you're the author, so you get to pick.
@@ -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);
+
Is there any consequence to this changed ordering in grabbing the
reference vs. updating the list?
Overall looks nice.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
