From bbba1c376b8b1ba5171bd14eb6bf212fa1173ddb Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Thu, 10 Jun 2021 13:07:33 +0300 Subject: [PATCH] block/nbd: fix how state is cleared on nbd_open() failure paths We have two "return error" paths in nbd_open() after nbd_process_options(). Actually we should call nbd_clear_bdrvstate() on these paths. Interesting that nbd_process_options() calls nbd_clear_bdrvstate() by itself. Let's fix leaks and refactor things to be more obvious: - intialize yank at top of nbd_open() - move yank cleanup to nbd_clear_bdrvstate() - refactor nbd_open() so that all failure paths except for yank-register goes through nbd_clear_bdrvstate() Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Roman Kagan Reviewed-by: Eric Blake Message-Id: <20210610100802.5888-4-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- block/nbd.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/block/nbd.c b/block/nbd.c index f4b3407587..01d2c2efad 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -152,8 +152,12 @@ static void nbd_co_establish_connection_cancel(BlockDriverState *bs, static int nbd_client_handshake(BlockDriverState *bs, Error **errp); static void nbd_yank(void *opaque); -static void nbd_clear_bdrvstate(BDRVNBDState *s) +static void nbd_clear_bdrvstate(BlockDriverState *bs) { + BDRVNBDState *s = (BDRVNBDState *)bs->opaque; + + yank_unregister_instance(BLOCKDEV_YANK_INSTANCE(bs->node_name)); + object_unref(OBJECT(s->tlscreds)); qapi_free_SocketAddress(s->saddr); s->saddr = NULL; @@ -2275,9 +2279,6 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options, ret = 0; error: - if (ret < 0) { - nbd_clear_bdrvstate(s); - } qemu_opts_del(opts); return ret; } @@ -2288,11 +2289,6 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags, int ret; BDRVNBDState *s = (BDRVNBDState *)bs->opaque; - ret = nbd_process_options(bs, options, errp); - if (ret < 0) { - return ret; - } - s->bs = bs; qemu_co_mutex_init(&s->send_mutex); qemu_co_queue_init(&s->free_sema); @@ -2301,20 +2297,23 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags, return -EEXIST; } + ret = nbd_process_options(bs, options, errp); + if (ret < 0) { + goto fail; + } + /* * establish TCP connection, return error if it fails * TODO: Configurable retry-until-timeout behaviour. */ if (nbd_establish_connection(bs, s->saddr, errp) < 0) { - yank_unregister_instance(BLOCKDEV_YANK_INSTANCE(bs->node_name)); - return -ECONNREFUSED; + ret = -ECONNREFUSED; + goto fail; } ret = nbd_client_handshake(bs, errp); if (ret < 0) { - yank_unregister_instance(BLOCKDEV_YANK_INSTANCE(bs->node_name)); - nbd_clear_bdrvstate(s); - return ret; + goto fail; } /* successfully connected */ s->state = NBD_CLIENT_CONNECTED; @@ -2326,6 +2325,10 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags, aio_co_schedule(bdrv_get_aio_context(bs), s->connection_co); return 0; + +fail: + nbd_clear_bdrvstate(bs); + return ret; } static int nbd_co_flush(BlockDriverState *bs) @@ -2369,11 +2372,8 @@ static void nbd_refresh_limits(BlockDriverState *bs, Error **errp) static void nbd_close(BlockDriverState *bs) { - BDRVNBDState *s = bs->opaque; - nbd_client_close(bs); - yank_unregister_instance(BLOCKDEV_YANK_INSTANCE(bs->node_name)); - nbd_clear_bdrvstate(s); + nbd_clear_bdrvstate(bs); } /*