[Qemu-devel] [PATCH] block: Drop bdrv_ioctl_bh_cb

[Qemu-devel] [PATCH] block: Drop bdrv_ioctl_bh_cb

[Fam Zheng]

Similar to the "!drv || !drv->bdrv_aio_ioctl" case above, here it is
okay to set co.ret and return. As pointed out by Paolo, a BH will be
created as necessary by the caller (bdrv_co_maybe_schedule_bh).
Besides, as pointed out by Kevin, "data" was leaked before.

Reported-by: Kevin Wolf <kwolf@redhat.com>
Reported-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/block/io.c b/block/io.c
index 2d832aa..c32f5b7 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2309,19 +2309,6 @@ int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors)
     return rwco.ret;
 }
 
-typedef struct {
-    CoroutineIOCompletion *co;
-    QEMUBH *bh;
-} BdrvIoctlCompletionData;
-
-static void bdrv_ioctl_bh_cb(void *opaque)
-{
-    BdrvIoctlCompletionData *data = opaque;
-
-    bdrv_co_io_em_complete(data->co, -ENOTSUP);
-    qemu_bh_delete(data->bh);
-}
-
 static int bdrv_co_do_ioctl(BlockDriverState *bs, int req, void *buf)
 {
     BlockDriver *drv = bs->drv;
@@ -2339,11 +2326,8 @@ static int bdrv_co_do_ioctl(BlockDriverState *bs, int req, void *buf)
 
     acb = drv->bdrv_aio_ioctl(bs, req, buf, bdrv_co_io_em_complete, &co);
     if (!acb) {
-        BdrvIoctlCompletionData *data = g_new(BdrvIoctlCompletionData, 1);
-        data->bh = aio_bh_new(bdrv_get_aio_context(bs),
-                                bdrv_ioctl_bh_cb, data);
-        data->co = &co;
-        qemu_bh_schedule(data->bh);
+        co.ret = -ENOTSUP;
+        goto out;
     }
     qemu_coroutine_yield();
 out:
-- 
2.8.3

Re: [Qemu-devel] [PATCH] block: Drop bdrv_ioctl_bh_cb

[Paolo Bonzini]

On 01/06/2016 03:52, Fam Zheng wrote:
> Similar to the "!drv || !drv->bdrv_aio_ioctl" case above, here it is
> okay to set co.ret and return. As pointed out by Paolo, a BH will be
> created as necessary by the caller (bdrv_co_maybe_schedule_bh).
> Besides, as pointed out by Kevin, "data" was leaked before.
> 
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Reported-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/io.c | 20 ++------------------
>  1 file changed, 2 insertions(+), 18 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 2d832aa..c32f5b7 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2309,19 +2309,6 @@ int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors)
>      return rwco.ret;
>  }
>  
> -typedef struct {
> -    CoroutineIOCompletion *co;
> -    QEMUBH *bh;
> -} BdrvIoctlCompletionData;
> -
> -static void bdrv_ioctl_bh_cb(void *opaque)
> -{
> -    BdrvIoctlCompletionData *data = opaque;
> -
> -    bdrv_co_io_em_complete(data->co, -ENOTSUP);
> -    qemu_bh_delete(data->bh);
> -}
> -
>  static int bdrv_co_do_ioctl(BlockDriverState *bs, int req, void *buf)
>  {
>      BlockDriver *drv = bs->drv;
> @@ -2339,11 +2326,8 @@ static int bdrv_co_do_ioctl(BlockDriverState *bs, int req, void *buf)
>  
>      acb = drv->bdrv_aio_ioctl(bs, req, buf, bdrv_co_io_em_complete, &co);
>      if (!acb) {
> -        BdrvIoctlCompletionData *data = g_new(BdrvIoctlCompletionData, 1);
> -        data->bh = aio_bh_new(bdrv_get_aio_context(bs),
> -                                bdrv_ioctl_bh_cb, data);
> -        data->co = &co;
> -        qemu_bh_schedule(data->bh);
> +        co.ret = -ENOTSUP;
> +        goto out;
>      }
>      qemu_coroutine_yield();
>  out:
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH] block: Drop bdrv_ioctl_bh_cb

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 678 bytes --]

On Wed, Jun 01, 2016 at 09:52:23AM +0800, Fam Zheng wrote:
> Similar to the "!drv || !drv->bdrv_aio_ioctl" case above, here it is
> okay to set co.ret and return. As pointed out by Paolo, a BH will be
> created as necessary by the caller (bdrv_co_maybe_schedule_bh).
> Besides, as pointed out by Kevin, "data" was leaked before.
> 
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Reported-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/io.c | 20 ++------------------
>  1 file changed, 2 insertions(+), 18 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]