Re: [Qemu-devel] [PATCH v1] xen_disk: fix memory leak

[Anthony PERARD <anthony.perard@citrix.com>]

On Tue, Dec 11, 2018 at 05:02:24PM +0100, Olaf Hering wrote:
> There are some code paths that clobber ioreq->buf, which leads to a huge
> memory leak after a few hours of runtime. One code path is
> qemu_aio_complete, which might be called recursive. Another one is

I think it's s/recursive/recursively/.

> ioreq_reset, which might clobber ioreq->buf as well.
> 
> Add wrappers to free ioreq->buf before reassignment.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

That patch seems fine, with a few coding style issues, and is going to
be needed to be forward ported to Paul's reimplementation (not yet
merged).

> ---
>  hw/block/xen_disk.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 36eff94f84..e15eefe625 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -103,12 +103,24 @@ struct XenBlkDev {
>  
>  /* ------------------------------------------------------------- */
>  
> +static void ioreq_buf_alloc(struct ioreq *ioreq, size_t alignment)

You have the parameter `alignment` but don't actually use it, I don't
think it's needed.

> +{
> +    if (ioreq->buf)
> +        qemu_vfree(ioreq->buf);

You could call ioreq_buf_free here instead of duplicating the code.

> +    ioreq->buf = qemu_memalign(XC_PAGE_SIZE, ioreq->size);
> +}
> +static void ioreq_buf_free(struct ioreq *ioreq)
> +{
> +    if (ioreq->buf)
> +        qemu_vfree(ioreq->buf);
> +    ioreq->buf = NULL;
> +}

Thanks,

-- 
Anthony PERARD