[dpdk-dev] [PATCH v2 44/44] net/virtio: handle Virtio-user setup failure properly

[Xia, Chenbo]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin at redhat.com>
> Sent: Wednesday, January 20, 2021 5:25 AM
> To: dev at dpdk.org; Xia, Chenbo <chenbo.xia at intel.com>; olivier.matz at 6wind.com;
> amorenoz at redhat.com; david.marchand at redhat.com
> Cc: Maxime Coquelin <maxime.coquelin at redhat.com>
> Subject: [PATCH v2 44/44] net/virtio: handle Virtio-user setup failure
> properly
> 
> This patch fixes virtio_user_dev_setup() error path,
> by cleaning all resources it allocates.
> 
> Suggested-by: Adrian Moreno <amorenoz at redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin at redhat.com>
> ---
>  drivers/net/virtio/virtio_user/virtio_user_dev.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index a1e32158bb..2f7dc574b6 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -427,15 +427,22 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
> 
>  	if (virtio_user_dev_init_notify(dev) < 0) {
>  		PMD_INIT_LOG(ERR, "(%s) Failed to init notifiers\n", dev->path);
> -		return -1;
> +		goto destroy;
>  	}
> 
>  	if (virtio_user_fill_intr_handle(dev) < 0) {
>  		PMD_INIT_LOG(ERR, "(%s) Failed to init interrupt handler\n", dev-
> >path);
> -		return -1;
> +		goto uninit;
>  	}
> 
>  	return 0;
> +
> +uninit:
> +	virtio_user_dev_uninit(dev);

IMHO, it may not be the best choice to call virtio_user_dev_uninit here..

Logically when virtio_user_fill_intr_handle fails, we want to release the resources
which is allocated in virtio_user_dev_init_notify (i.e., fds), but virtio_user_dev_uninit
does more. For example, unregister the event callback that have not been registered yet and
it also leads to destroy callback called twice. Although things done but not needed will
not lead to error, but it looks not in the best way. What do you think?

Thanks,
Chenbo

> +destroy:
> +	dev->ops->destroy(dev);
> +
> +	return -1;
>  }
> 
>  /* Use below macro to filter features from vhost backend */
> --
> 2.29.2