[Qemu-devel] Re: [PATCH]hw/xen_disk: ioreq not finished on error

[Stefano Stabellini]

On Sun, 27 Mar 2011, Feiran Zheng wrote:
> Bug fix: routines 'ioreq_runio_qemu_sync' and 'ioreq_runio_qemu_aio'
> won't call 'ioreq_unmap' or 'ioreq_finish' on errors, leaving ioreq in
> the blkdev->inflight list and a leak.
> 
> Signed-off-by: Feiran Zheng <address@hidden>
> ---
>  hw/xen_disk.c |   22 +++++++++++++++++-----
>  1 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/xen_disk.c b/hw/xen_disk.c
> index 445bf03..7940fab 100644
> --- a/hw/xen_disk.c
> +++ b/hw/xen_disk.c
> @@ -309,8 +309,10 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
>      int i, rc, len = 0;
>      off_t pos;
> 
> -    if (ioreq->req.nr_segments && ioreq_map(ioreq) == -1)
> -     goto err;
> +    if (ioreq->req.nr_segments) {
> +     if (ioreq_map(ioreq) == -1)
> +         goto err_no_map;
> +    }
>      if (ioreq->presync)
>       bdrv_flush(blkdev->bs);
> 

this change is just to make the code clearer and easier to read, right?


> @@ -364,6 +366,9 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
>      return 0;
> 
>  err:
> +    ioreq_unmap(ioreq);
> +err_no_map:
> +    ioreq_finish(ioreq);
>      ioreq->status = BLKIF_RSP_ERROR;
>      return -1;
>  }
> @@ -392,8 +397,10 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
>  {
>      struct XenBlkDev *blkdev = ioreq->blkdev;
> 
> -    if (ioreq->req.nr_segments && ioreq_map(ioreq) == -1)
> -     goto err;
> +    if (ioreq->req.nr_segments) {
> +     if (ioreq_map(ioreq) == -1)
> +         goto err_no_map;
> +    }
> 
>      ioreq->aio_inflight++;
>      if (ioreq->presync)
> @@ -425,9 +432,14 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
>      qemu_aio_complete(ioreq, 0);
> 
>      return 0;
> +
> +err_no_map:
> +    ioreq_finish(ioreq);
> +    ioreq->status = BLKIF_RSP_ERROR;
> +    return -1;
> 

why aren't you calling ioreq_unmap before ioreq_finish, like in the
previous case?