Re: [Qemu-devel] [RFC PATCH 1/9] block: Add bdrv_aio_cancel_async

[Paolo Bonzini <pbonzini@redhat.com>]

Il 21/08/2014 13:56, Fam Zheng ha scritto:
> +        BlockDriverAIOCB *save = g_new(BlockDriverAIOCB, 1);
> +        save->cb               = acb->cb;
> +        save->opaque           = acb->opaque;
> +        acb->cb                = bdrv_aio_cancel_async_cb;
> +        acb->opaque            = acb;
> +        acb->cancel_acb_save   = save;

No need for this extra field.  You can make a struct with {acb->cb,
acb->opaque, acb} and pass it as the opaque.  You also do not need to
allocate it on the heap, since everything is synchronous.

> +
> +        /* Use the synchronous version and hope our cb is called. */
> +        acb->aiocb_info->cancel(acb);

Unfortunately, acb has been freed at this point, so you'll be accessing
a dangling pointer.  I think you need to modify all cancel
implementations.  For example:

- return whether they have called the callback

- only free the acb if they have called the callback

- otherwise, free the acb in bdrv_aio_cancel

Paolo

> +        if (acb->cancel_acb_save) {
> +            /* cb is not called yet, let's call it */
> +            bdrv_aio_cancel_async_cb(acb->opaque, -ECANCELED);
> +        }
> +    }
> +}
> +
>  /**************************************************************/
>  /* async block device emulation */
>  
> diff --git a/include/block/aio.h b/include/block/aio.h
> index c23de3c..fcc5c87 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -27,6 +27,7 @@ typedef void BlockDriverCompletionFunc(void *opaque, int ret);
>  
>  typedef struct AIOCBInfo {
>      void (*cancel)(BlockDriverAIOCB *acb);
> +    void (*cancel_async)(BlockDriverAIOCB *acb);
>      size_t aiocb_size;
>  } AIOCBInfo;
>  
> @@ -35,6 +36,7 @@ struct BlockDriverAIOCB {
>      BlockDriverState *bs;
>      BlockDriverCompletionFunc *cb;
>      void *opaque;
> +    BlockDriverAIOCB *cancel_acb_save;
>  };