diff mbox series

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

Message ID 20221125133518.418328-8-eesposit@redhat.com
State New
Headers show
Series Still more coroutine and various fixes in block layer | expand

Commit Message

Emanuele Giuseppe Esposito Nov. 25, 2022, 1:35 p.m. UTC 
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(-)

Comments

Vladimir Sementsov-Ogievskiy Nov. 25, 2022, 6:03 p.m. UTC | #1 
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?
Emanuele Giuseppe Esposito Nov. 28, 2022, 9:11 a.m. UTC | #2 
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
diff mbox series

Patch

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);
+
+    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);
 
+    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);
 }
 
 int bdrv_create(BlockDriver *drv, const char* filename,
                 QemuOpts *opts, Error **errp)
 {
-    int ret;
-
     GLOBAL_STATE_CODE();
 
-    Coroutine *co;
-    CreateCo cco = {
-        .drv = drv,
-        .filename = filename,
-        .opts = opts,
-        .ret = NOT_DONE,
-        .err = NULL,
-    };
-
-    if (!drv->bdrv_co_create_opts) {
-        error_setg(errp, "Driver '%s' does not support image creation", drv->format_name);
-        return -ENOTSUP;
-    }
-
     if (qemu_in_coroutine()) {
         /* Fast-path if already in coroutine context */
-        bdrv_create_co_entry(&cco);
+        return bdrv_co_create(drv, filename, opts, errp);
     } else {
+        Coroutine *co;
+        CreateCo cco = {
+            .drv = drv,
+            .filename = filename,
+            .opts = opts,
+            .ret = NOT_DONE,
+            .err = NULL,
+        };
+
         co = qemu_coroutine_create(bdrv_create_co_entry, &cco);
         qemu_coroutine_enter(co);
         while (cco.ret == NOT_DONE) {
             aio_poll(qemu_get_aio_context(), true);
         }
+        error_propagate(errp, cco.err);
+        return cco.ret;
     }
-
-    ret = cco.ret;
-    if (ret < 0) {
-        if (cco.err) {
-            error_propagate(errp, cco.err);
-        } else {
-            error_setg_errno(errp, -ret, "Could not create image");
-        }
-    }
-
-    return ret;
 }
 
 /**