diff mbox series

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

Message ID 20210416080911.83197-3-vsementsov@virtuozzo.com (mailing list archive)
State New
Headers show
Series block/nbd: rework client connection | expand

Commit Message

Vladimir Sementsov-Ogievskiy April 16, 2021, 8:08 a.m. UTC
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(-)

Comments

Roman Kagan April 21, 2021, 2 p.m. UTC | #1
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);
>  }
>  
>  /*
Vladimir Sementsov-Ogievskiy April 21, 2021, 10:27 p.m. UTC | #2
21.04.2021 17:00, Roman Kagan wrote:
> 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.
> 

No, nbd_client_handshake() only calls yank_unregister_function(), not instance.
Roman Kagan April 22, 2021, 8:49 a.m. UTC | #3
On Thu, Apr 22, 2021 at 01:27:22AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 21.04.2021 17:00, Roman Kagan wrote:
> > On Fri, Apr 16, 2021 at 11:08:40AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > @@ -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.
> > 
> 
> No, nbd_client_handshake() only calls yank_unregister_function(), not instance.

Indeed.  Sorry for confusion.

Reviewed-by: Roman Kagan <rvkagan@yandex-team.ru>
diff mbox series

Patch

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);
     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);
 }
 
 /*