Re: [Qemu-devel] [PATCH v2 2/6] nbd: Merge nbd_export_set_name into nbd_export_new

[Vladimir Sementsov-Ogievskiy]

10.01.2019 10:13, Eric Blake wrote:
> The existing NBD code had a weird split where nbd_export_new()
> created an export but did not add it to the list of exported
> names until a later nbd_export_set_name() came along and grabbed
> a second reference on the object; later, nbd_export_close()
> drops the second reference.  But since we never change the
> name of an NBD export while it is exposed, it is easier to just
> inline the process of setting the name as part of creating the
> export.
> 
> Inline the contents of nbd_export_set_name() and
> nbd_export_set_description() into the two points in an export
> lifecycle where they matter, then adjust both callers to pass
> the name up front.  Note that all callers pass a non-NULL name,
> (passing NULL at creation was for old style servers, but we
> removed support for that in commit 7f7dfe2a).
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>   include/block/nbd.h |  3 +--
>   blockdev-nbd.c      |  5 ++---
>   nbd/server.c        | 45 ++++++++++++++++-----------------------------
>   qemu-nbd.c          |  5 ++---
>   4 files changed, 21 insertions(+), 37 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 65402d33964..2f9a2aeb73c 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -295,6 +295,7 @@ typedef struct NBDExport NBDExport;
>   typedef struct NBDClient NBDClient;
> 
>   NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t 
> size,
> +                          const char *name, const char *description,
>                             uint16_t nbdflags, void (*close)(NBDExport *),
>                             bool writethrough, BlockBackend *on_eject_blk,
>                             Error **errp);
> @@ -306,8 +307,6 @@ void nbd_export_put(NBDExport *exp);
>   BlockBackend *nbd_export_get_blockdev(NBDExport *exp);
> 
>   NBDExport *nbd_export_find(const char *name);
> -void nbd_export_set_name(NBDExport *exp, const char *name);
> -void nbd_export_set_description(NBDExport *exp, const char *description);
>   void nbd_export_close_all(void);
> 
>   void nbd_client_new(QIOChannelSocket *sioc,
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 1d170c80b82..f5edbc27d88 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -174,14 +174,13 @@ void qmp_nbd_server_add(const char *device, bool 
> has_name, const char *name,
>           writable = false;
>       }
> 
> -    exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY,
> +    exp = nbd_export_new(bs, 0, -1, name, NULL,
> +                         writable ? 0 : NBD_FLAG_READ_ONLY,
>                            NULL, false, on_eject_blk, errp);
>       if (!exp) {
>           return;
>       }
> 
> -    nbd_export_set_name(exp, name);
> -
>       /* 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. */
> diff --git a/nbd/server.c b/nbd/server.c
> index 98327088cb4..676fb4886d0 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1456,6 +1456,7 @@ static void nbd_eject_notifier(Notifier *n, void *data)
>   }
> 
>   NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t 
> size,
> +                          const char *name, const char *description,
>                             uint16_t nbdflags, void (*close)(NBDExport *),
>                             bool writethrough, BlockBackend *on_eject_blk,
>                             Error **errp)
> @@ -1471,6 +1472,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t 
> dev_offset, off_t size,
>        * that BDRV_O_INACTIVE is cleared and the image is ready for write
>        * access since the export could be available before migration handover.
>        */
> +    assert(name);
>       ctx = bdrv_get_aio_context(bs);
>       aio_context_acquire(ctx);
>       bdrv_invalidate_cache(bs, NULL);
> @@ -1494,6 +1496,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t 
> dev_offset, off_t size,
>       QTAILQ_INIT(&exp->clients);
>       exp->blk = blk;
>       exp->dev_offset = dev_offset;
> +    exp->name = g_strdup(name);
> +    exp->description = g_strdup(description);
>       exp->nbdflags = nbdflags;
>       exp->size = size < 0 ? blk_getlength(blk) : size;
>       if (exp->size < 0) {
> @@ -1513,10 +1517,14 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t 
> dev_offset, off_t size,
>           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;
> 
>   fail:
>       blk_unref(blk);
> +    g_free(exp->name);
> +    g_free(exp->description);
>       g_free(exp);
>       return NULL;
>   }
> @@ -1533,33 +1541,6 @@ NBDExport *nbd_export_find(const char *name)
>       return NULL;
>   }
> 
> -void nbd_export_set_name(NBDExport *exp, const char *name)
> -{
> -    if (exp->name == name) {
> -        return;
> -    }
> -
> -    nbd_export_get(exp);
> -    if (exp->name != NULL) {
> -        g_free(exp->name);
> -        exp->name = NULL;
> -        QTAILQ_REMOVE(&exports, exp, next);
> -        nbd_export_put(exp);
> -    }
> -    if (name != NULL) {
> -        nbd_export_get(exp);
> -        exp->name = g_strdup(name);
> -        QTAILQ_INSERT_TAIL(&exports, exp, next);
> -    }
> -    nbd_export_put(exp);
> -}
> -
> -void nbd_export_set_description(NBDExport *exp, const char *description)
> -{
> -    g_free(exp->description);
> -    exp->description = g_strdup(description);
> -}
> -
>   void nbd_export_close(NBDExport *exp)
>   {
>       NBDClient *client, *next;
> @@ -1568,8 +1549,14 @@ void nbd_export_close(NBDExport *exp)
>       QTAILQ_FOREACH_SAFE(client, &exp->clients, next, next) {
>           client_close(client, true);
>       }
> -    nbd_export_set_name(exp, NULL);
> -    nbd_export_set_description(exp, NULL);
> +    if (exp->name) {
> +        nbd_export_put(exp);
> +        g_free(exp->name);
> +        exp->name = NULL;
> +        QTAILQ_REMOVE(&exports, exp, next);

exp->name is always non-zero, and _get and _INSERT_TAIL were done independently 
from name,
so with these four lines done unconditionally:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> +    }
> +    g_free(exp->description);
> +    exp->description = NULL;
>       nbd_export_put(exp);
>   }
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 2807e132396..696bd78a2e2 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -1015,10 +1015,9 @@ int main(int argc, char **argv)
>           }
>       }
> 
> -    exp = nbd_export_new(bs, dev_offset, fd_size, nbdflags, 
> nbd_export_closed,
> +    exp = nbd_export_new(bs, dev_offset, fd_size, export_name,
> +                         export_description, nbdflags, nbd_export_closed,
>                            writethrough, NULL, &error_fatal);
> -    nbd_export_set_name(exp, export_name);
> -    nbd_export_set_description(exp, export_description);
> 
>       if (device) {
>   #if HAVE_NBD_DEVICE
> 


-- 
Best regards,
Vladimir