Re: [PATCH V2 06/45] vfio/container: reform vfio_connect_container cleanup

["Cédric Le Goater" <clg@redhat.com>]

On 2/14/25 15:13, Steve Sistare wrote:
> Replace the proliferation of exit labels in vfio_connect_container with
> conditionals for cleaning each piece of state.  No functional change.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/vfio/container.c | 61 +++++++++++++++++++++++++++++------------------------
>   1 file changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 0f17d53..c668d07 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -587,9 +587,12 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>   {
>       VFIOContainer *container;
>       VFIOContainerBase *bcontainer;
> -    int ret, fd;
> +    int ret, fd = -1;
>       VFIOAddressSpace *space;
> -    VFIOIOMMUClass *vioc;
> +    VFIOIOMMUClass *vioc = NULL;
> +    bool new_container = false;
> +    bool group_was_added = false;
> +    bool discard_disabled = false;
>   
>       space = vfio_get_address_space(as);
>   
> @@ -608,35 +611,37 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>   
>       fd = qemu_open("/dev/vfio/vfio", O_RDWR, errp);
>       if (fd < 0) {
> -        goto put_space_exit;
> +        goto fail;
>       }
>   
>       ret = ioctl(fd, VFIO_GET_API_VERSION);
>       if (ret != VFIO_API_VERSION) {
>           error_setg(errp, "supported vfio version: %d, "
>                      "reported version: %d", VFIO_API_VERSION, ret);
> -        goto close_fd_exit;
> +        goto fail;
>       }
>   
>       container = vfio_create_container(fd, group, errp);
>       if (!container) {
> -        goto close_fd_exit;
> +        goto fail;
>       }
> +    new_container = true;
>       bcontainer = &container->bcontainer;>   
>       if (!vfio_cpr_register_container(bcontainer, errp)) {
> -        goto free_container_exit;
> +        goto fail;
>       }
>   
>       if (!vfio_attach_discard_disable(container, group, errp)) {
> -        goto unregister_container_exit;
> +        goto fail;
>       }
> +    discard_disabled = true;
>   
>       vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
>       assert(vioc->setup);
>   
>       if (!vioc->setup(bcontainer, errp)) {
> -        goto enable_discards_exit;
> +        goto fail;
>       }
>   
>       vfio_kvm_device_add_group(group);
> @@ -645,6 +650,7 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>   
>       group->container = container;
>       QLIST_INSERT_HEAD(&container->group_list, group, container_next);
> +    group_was_added = true;
>   
>       bcontainer->listener = vfio_memory_listener;
>       memory_listener_register(&bcontainer->listener, bcontainer->space->as);
> @@ -652,35 +658,34 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>       if (bcontainer->error) {
>           error_propagate_prepend(errp, bcontainer->error,
>               "memory listener initialization failed: ");
> -        goto listener_release_exit;
> +        goto fail;
>       }
>   
>       bcontainer->initialized = true;
>   
>       return true;
> -listener_release_exit:
> -    QLIST_REMOVE(group, container_next);
> -    vfio_kvm_device_del_group(group);
> +
> +fail:
>       memory_listener_unregister(&bcontainer->listener);
> -    if (vioc->release) {
> +
> +    if (group_was_added) {
> +        QLIST_REMOVE(group, container_next);
> +        vfio_kvm_device_del_group(group);
> +    }
> +    if (vioc && vioc->release) {
>           vioc->release(bcontainer);
>       }
> -
> -enable_discards_exit:
> -    vfio_ram_block_discard_disable(container, false);
> -
> -unregister_container_exit:
> -    vfio_cpr_unregister_container(bcontainer);
> -
> -free_container_exit:
> -    object_unref(container);
> -
> -close_fd_exit:
> -    close(fd);
> -
> -put_space_exit:
> +    if (discard_disabled) {
> +        vfio_ram_block_discard_disable(container, false);
> +    }
> +    if (new_container) {
> +        vfio_cpr_unregister_container(bcontainer);
> +        object_unref(container);
> +    }
> +    if (fd >= 0) {
> +        close(fd);
> +    }
>       vfio_put_address_space(space);
> -
>       return false;
>   }
>