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 <eespo...@redhat.com>
>> Reviewed-by: Kevin Wolf <kw...@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 <vsement...@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