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