Re: [PATCH v3 02/33] block/nbd: fix how state is cleared on nbd_open() failure paths

[Roman Kagan]

On Fri, Apr 16, 2021 at 11:08:40AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 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 <vsementsov@virtuozzo.com>
> ---
>  block/nbd.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 739ae2941f..a407a3814b 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;
> @@ -2279,9 +2283,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;
>  }
> @@ -2292,11 +2293,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);
> @@ -2305,20 +2301,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);

Not that this was introduced by this patch, but once you're at it:
AFAICT nbd_client_handshake() calls yank_unregister_instance() on some
error path(s); I assume this needs to go too, otherwise it's called
twice (and asserts).

Roman.

>      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;
> @@ -2330,6 +2329,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)
> @@ -2373,11 +2376,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);
>  }
>  
>  /*