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

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Roman Kagan <rvkagan@yandex-team.ru>,
	qemu-block@nongnu.org, qemu-devel@nongnu.org, eblake@redhat.com,
	mreitz@redhat.com, kwolf@redhat.com, den@openvz.org
Subject: Re: [PATCH v3 02/33] block/nbd: fix how state is cleared on nbd_open() failure paths
Date: Thu, 22 Apr 2021 01:27:22 +0300	[thread overview]
Message-ID: <2f1afa65-fb15-7ccd-7285-dee9fe41161c@virtuozzo.com> (raw)
In-Reply-To: <YIAwEYbLpqzzFzd4@rvkaganb.lan>

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.

-- 
Best regards,
Vladimir