Re: [PATCH] vfio-helpers: Free QEMUVFIOState in qemu_vfio_close()

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Michal,

On 4/22/20 12:30 PM, Michal Privoznik wrote:
> The qemu_vfio_open_pci() allocates this QEMUVFIOState structure
> but free counterpart is missing. Since we already have
> qemu_vfio_close() which does cleanup of the state, it looks like
> a perfect place to free the structure too. However, to avoid
> confusing rename the function to make it explicit that the passed
> structure is also freed.
> 
> ==167296== 528 (360 direct, 168 indirect) bytes in 1 blocks are definitely lost in loss record 8,504 of 8,908
> ==167296==    at 0x4837B86: calloc (vg_replace_malloc.c:762)
> ==167296==    by 0x4B8F6A0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.7)
> ==167296==    by 0xA7F532: qemu_vfio_open_pci (vfio-helpers.c:428)
> ==167296==    by 0x989595: nvme_init (nvme.c:606)
> ==167296==    by 0x989EB0: nvme_file_open (nvme.c:795)
> ==167296==    by 0x8F9D04: bdrv_open_driver (block.c:1466)
> ==167296==    by 0x8FA6E1: bdrv_open_common (block.c:1744)
> ==167296==    by 0x8FDC73: bdrv_open_inherit (block.c:3291)
> ==167296==    by 0x8FE1B5: bdrv_open (block.c:3384)
> ==167296==    by 0x5EE828: bds_tree_init (blockdev.c:663)
> ==167296==    by 0x5F57F8: qmp_blockdev_add (blockdev.c:3746)
> ==167296==    by 0x5666DC: configure_blockdev (vl.c:1047)
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>   block/nvme.c                | 2 +-
>   include/qemu/vfio-helpers.h | 2 +-
>   util/vfio-helpers.c         | 3 ++-
>   3 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 7b7c0cc5d6..7e00c4f1a7 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -766,7 +766,7 @@ static void nvme_close(BlockDriverState *bs)
>                              false, NULL, NULL);
>       event_notifier_cleanup(&s->irq_notifier);
>       qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
> -    qemu_vfio_close(s->vfio);
> +    qemu_vfio_close_and_free(s->vfio);
>   
>       g_free(s->device);
>   }
> diff --git a/include/qemu/vfio-helpers.h b/include/qemu/vfio-helpers.h
> index 1f057c2b9e..c96a0b1963 100644
> --- a/include/qemu/vfio-helpers.h
> +++ b/include/qemu/vfio-helpers.h
> @@ -16,7 +16,7 @@
>   typedef struct QEMUVFIOState QEMUVFIOState;
>   
>   QEMUVFIOState *qemu_vfio_open_pci(const char *device, Error **errp);
> -void qemu_vfio_close(QEMUVFIOState *s);
> +void qemu_vfio_close_and_free(QEMUVFIOState *s);
>   int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size,
>                         bool temporary, uint64_t *iova_list);
>   int qemu_vfio_dma_reset_temporary(QEMUVFIOState *s);
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index ddd9a96e76..4c525d245b 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -706,7 +706,7 @@ static void qemu_vfio_reset(QEMUVFIOState *s)
>   }
>   
>   /* Close and free the VFIO resources. */

The comment already says 'close and free', I don't think it is worth 
renaming the function.

> -void qemu_vfio_close(QEMUVFIOState *s)
> +void qemu_vfio_close_and_free(QEMUVFIOState *s)
>   {
>       int i;
>   
> @@ -721,4 +721,5 @@ void qemu_vfio_close(QEMUVFIOState *s)
>       close(s->device);
>       close(s->group);
>       close(s->container);
> +    g_free(s);

Good catch.

Preferably not renaming:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>   }
>