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

[Stefan Hajnoczi]

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 
> Reported-by: Paolo Bonzini 
> Signed-off-by: Fam Zheng 
> ---
>  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


signature.asc
Description: PGP signature

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 
> Reported-by: Paolo Bonzini 
> Signed-off-by: Fam Zheng 
> ---
>  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, );
>  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 = 
> -qemu_bh_schedule(data->bh);
> +co.ret = -ENOTSUP;
> +goto out;
>  }
>  qemu_coroutine_yield();
>  out:
> 

Reviewed-by: Paolo Bonzini 

[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 
Reported-by: Paolo Bonzini 
Signed-off-by: Fam Zheng 
---
 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, );
 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 = 
-qemu_bh_schedule(data->bh);
+co.ret = -ENOTSUP;
+goto out;
 }
 qemu_coroutine_yield();
 out:
-- 
2.8.3