diff mbox series

[1/2] net/tap: fix log loss when state fails to be restored

Message ID 1619090834-14643-2-git-send-email-humin29@huawei.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series bugfix for tap device |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

humin (Q) April 22, 2021, 11:27 a.m. UTC 
  From: Chengchang Tang <tangchengchang@huawei.com>

After restoring the remote states, the return value of ioctl() is not
checked. Therefore, users cannot know whether the remote state is
restored successfully.

This patch add log for restoring failure.

Fixes: 4810d3af8343 ("net/tap: restore state of remote device when closing")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 drivers/net/tap/rte_eth_tap.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Ferruh Yigit April 26, 2021, 3:30 p.m. UTC | #1 
On 4/22/2021 12:27 PM, Min Hu (Connor) wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> After restoring the remote states, the return value of ioctl() is not
> checked. Therefore, users cannot know whether the remote state is
> restored successfully.
> 
> This patch add log for restoring failure.
> 
> Fixes: 4810d3af8343 ("net/tap: restore state of remote device when closing")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>  drivers/net/tap/rte_eth_tap.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 68baa18..6007c78 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -1101,6 +1101,7 @@ tap_dev_close(struct rte_eth_dev *dev)
>  	struct pmd_internals *internals = dev->data->dev_private;
>  	struct pmd_process_private *process_private = dev->process_private;
>  	struct rx_queue *rxq;
> +	int ret;
>  
>  	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
>  		rte_free(dev->process_private);
> @@ -1133,8 +1134,11 @@ tap_dev_close(struct rte_eth_dev *dev)
>  
>  	if (internals->remote_if_index) {
>  		/* Restore initial remote state */
> -		ioctl(internals->ioctl_sock, SIOCSIFFLAGS,
> +		ret = ioctl(internals->ioctl_sock, SIOCSIFFLAGS,
>  				&internals->remote_initial_flags);
> +		if (ret)
> +			TAP_LOG(ERR, "restore remote state failed: %d", ret);
> +

'ret' is used only in this scope, can you please move the variable declaration
at the beginning of the this if block?
You can do something like "int ret = ioctl(...."


>  	}
>  
>  	rte_mempool_free(internals->gso_ctx_mp);
>
humin (Q) April 27, 2021, 12:54 a.m. UTC | #2 
在 2021/4/26 23:30, Ferruh Yigit 写道:
> On 4/22/2021 12:27 PM, Min Hu (Connor) wrote:
>> From: Chengchang Tang <tangchengchang@huawei.com>
>>
>> After restoring the remote states, the return value of ioctl() is not
>> checked. Therefore, users cannot know whether the remote state is
>> restored successfully.
>>
>> This patch add log for restoring failure.
>>
>> Fixes: 4810d3af8343 ("net/tap: restore state of remote device when closing")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>>   drivers/net/tap/rte_eth_tap.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index 68baa18..6007c78 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -1101,6 +1101,7 @@ tap_dev_close(struct rte_eth_dev *dev)
>>   	struct pmd_internals *internals = dev->data->dev_private;
>>   	struct pmd_process_private *process_private = dev->process_private;
>>   	struct rx_queue *rxq;
>> +	int ret;
>>   
>>   	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
>>   		rte_free(dev->process_private);
>> @@ -1133,8 +1134,11 @@ tap_dev_close(struct rte_eth_dev *dev)
>>   
>>   	if (internals->remote_if_index) {
>>   		/* Restore initial remote state */
>> -		ioctl(internals->ioctl_sock, SIOCSIFFLAGS,
>> +		ret = ioctl(internals->ioctl_sock, SIOCSIFFLAGS,
>>   				&internals->remote_initial_flags);
>> +		if (ret)
>> +			TAP_LOG(ERR, "restore remote state failed: %d", ret);
>> +
> 
> 'ret' is used only in this scope, can you please move the variable declaration
> at the beginning of the this if block?
> You can do something like "int ret = ioctl(...."
> 
Hi, fixed in v2, thanks.
> 
>>   	}
>>   
>>   	rte_mempool_free(internals->gso_ctx_mp);
>>
> 
> .
>
diff mbox series

Patch

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 68baa18..6007c78 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -1101,6 +1101,7 @@  tap_dev_close(struct rte_eth_dev *dev)
 	struct pmd_internals *internals = dev->data->dev_private;
 	struct pmd_process_private *process_private = dev->process_private;
 	struct rx_queue *rxq;
+	int ret;
 
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
 		rte_free(dev->process_private);
@@ -1133,8 +1134,11 @@  tap_dev_close(struct rte_eth_dev *dev)
 
 	if (internals->remote_if_index) {
 		/* Restore initial remote state */
-		ioctl(internals->ioctl_sock, SIOCSIFFLAGS,
+		ret = ioctl(internals->ioctl_sock, SIOCSIFFLAGS,
 				&internals->remote_initial_flags);
+		if (ret)
+			TAP_LOG(ERR, "restore remote state failed: %d", ret);
+
 	}
 
 	rte_mempool_free(internals->gso_ctx_mp);