Re: [PATCH v6 07/14] block: distinguish between bdrv_create running in coroutine and not

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>,
	qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Hanna Reitz <hreitz@redhat.com>, John Snow <jsnow@redhat.com>,
	Eric Blake <eblake@redhat.com>, Fam Zheng <fam@euphon.net>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	"Denis V. Lunev" <den@openvz.org>, Stefan Weil <sw@weilnetz.de>,
	Jeff Cody <codyprime@gmail.com>, Cleber Rosa <crosa@redhat.com>,
	qemu-devel@nongnu.org
Subject: Re: [PATCH v6 07/14] block: distinguish between bdrv_create running in coroutine and not
Date: Mon, 28 Nov 2022 10:11:59 +0100	[thread overview]
Message-ID: <4b36c016-e26b-796f-a5bd-bcc58b5bcba7@redhat.com> (raw)
In-Reply-To: <e45ea640-294d-201b-7b0d-94755eaccd30@yandex-team.ru>

Am 25/11/2022 um 19:03 schrieb Vladimir Sementsov-Ogievskiy:
> On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
>> Call two different functions depending on whether bdrv_create
>> is in coroutine or not, following the same pattern as
>> generated_co_wrapper functions.
>>
>> This allows to also call the coroutine function directly,
>> without using CreateCo or relying in bdrv_create().
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   block.c | 71 +++++++++++++++++++++++++++++----------------------------
>>   1 file changed, 36 insertions(+), 35 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 9d51e7b6e5..2cf50b37c4 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -528,63 +528,64 @@ typedef struct CreateCo {
>>       Error *err;
>>   } CreateCo;
>>   -static void coroutine_fn bdrv_create_co_entry(void *opaque)
>> +static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char
>> *filename,
>> +                                       QemuOpts *opts, Error **errp)
>>   {
>> -    Error *local_err = NULL;
>>       int ret;
>> +    GLOBAL_STATE_CODE();
>> +    ERRP_GUARD();
>> +    assert(*errp == NULL);
>> +    assert(drv);
> 
> Why we need these two assertions? These are general assumptions, and we
> don't assert it in all functions. Dereference of NULL will crash not
> worse than assertion. I'd drop them.
> 
>> +
>> +    if (!drv->bdrv_co_create_opts) {
>> +        error_setg(errp, "Driver '%s' does not support image creation",
>> +                   drv->format_name);
>> +        return -ENOTSUP;
>> +    }
>> +
>> +    ret = drv->bdrv_co_create_opts(drv, filename, opts, errp);
>>   
> 
> and this empty line, looks accidental.
> 
> Offtopic: hope one day we fix *open* functions to always set errp on
> error paths.
> 
>> +    if (ret < 0 && !*errp) {
>> +        error_setg_errno(errp, -ret, "Could not create image");
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static void coroutine_fn bdrv_create_co_entry(void *opaque)
>> +{
>>       CreateCo *cco = opaque;
>> -    assert(cco->drv);
>>       GLOBAL_STATE_CODE();
>>   -    ret = cco->drv->bdrv_co_create_opts(cco->drv,
>> -                                        cco->filename, cco->opts,
>> &local_err);
>> -    error_propagate(&cco->err, local_err);
>> -    cco->ret = ret;
>> +    cco->ret = bdrv_co_create(cco->drv, cco->filename, cco->opts,
>> &cco->err);
> 
> We need aio_wait_kick() call here, like in other co_entry() functions.
> Otherwise we may stuck in aio_poll()
> 
> with it:
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> Hmm actually, we can simply merge this patch into patch 13 (and move 08
> to be after 13). Why to refactor bdrv_create twice?
> 

I think I'll leave the patches separate, otherwise I am worried that it
will be difficult to understand what is happending and why when we merge
these 2 operations (move all logic to _co_ and using co_wrapper) together.

I agree with the rest.

Thank you,
Emanuele