Re: [RFC PATCH 13/22] block/export: Move refcount from NBDExport to BlockExport

[Eric Blake]

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 <kwolf@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