diff mbox series

[v2,44/44] net/virtio: handle Virtio-user setup failure properly

Message ID 20210119212507.1043636-45-maxime.coquelin@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series net/virtio: Virtio PMD rework |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Functional success Functional Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-testing warning Testing issues

Commit Message

Maxime Coquelin Jan. 19, 2021, 9:25 p.m. UTC 
  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(-)

Comments

Chenbo Xia Jan. 22, 2021, 9:24 a.m. UTC | #1 
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?

Thanks,
Chenbo

> +destroy:
> +	dev->ops->destroy(dev);
> +
> +	return -1;
>  }
> 
>  /* Use below macro to filter features from vhost backend */
> --
> 2.29.2
Maxime Coquelin Jan. 25, 2021, 4:16 p.m. UTC | #2 
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
>
diff mbox series

Patch

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);
+destroy:
+	dev->ops->destroy(dev);
+
+	return -1;
 }
 
 /* Use below macro to filter features from vhost backend */