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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 1/22/21 10:24 AM, Xia, Chenbo wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Wednesday, January 20, 2021 5:25 AM
>> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; olivier.matz@6wind.com;
>> amorenoz@redhat.com; david.marchand@redhat.com
>> Cc: Maxime Coquelin <maxime.coquelin@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@redhat.com>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@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?

I agree, I'm reworking it for v3.

Kick and call FDs will be initialized to -1 at virtio_user_dev_init()
time. I introduce a virtio_user_dev_uninit_notify that will close all
kick and call FDs is opened and reset their value to -1.

Thenb I call this function in the error path of virtio_user_dev_setup()
and also in virtio_user_dev_uninit().

Doing that, I can also simplifies virtio_user_dev_init_notify() and only
loop for max queue pairs instead of VIRTIO_MAX_VIRTQUEUES and so avoid
setting FDs to -1 a second time.

Thanks,
Maxime

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