diff mbox series

app: fix redundant comparison

Message ID 1618835391-54882-1-git-send-email-humin29@huawei.com (mailing list archive)
State Rejected, archived
Delegated to: David Marchand
Headers
Series app: fix redundant comparison |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/travis-robot success travis build: passed
ci/github-robot success github build: passed
ci/iol-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS

Commit Message

humin (Q) April 19, 2021, 12:29 p.m. UTC 
  The return value of 'rte_eal_cleanup' is always zero, so comparison
with zero is redundant.

This patch fixed it by deleting the redundant comparison.

Fixes: 67684d1e87b6 ("app/procinfo: call EAL cleanup before exit")
Fixes: b68a82425da4 ("app/compress-perf: add performance measurement")
Fixes: 5e516c89830a ("app/testpmd: call cleanup on exit")
Fixes: 613ce6691c0d ("examples/l3fwd-power: implement proper shutdown")
Fixes: 48be180de631 ("eal: move OS common debug functions to single file")
Cc: stable@dpdk.org

Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 app/pdump/main.c                         | 4 +---
 app/proc-info/main.c                     | 4 +---
 app/test-compress-perf/main.c            | 7 +------
 app/test-pmd/testpmd.c                   | 5 +----
 examples/l3fwd-power/main.c              | 3 +--
 lib/librte_eal/common/eal_common_debug.c | 4 +---
 6 files changed, 6 insertions(+), 21 deletions(-)

Comments

Hunt, David April 19, 2021, 12:44 p.m. UTC | #1 
Hi Connor,

On 19/4/2021 1:29 PM, Min Hu (Connor) wrote:
> The return value of 'rte_eal_cleanup' is always zero, so comparison
> with zero is redundant.
>
> This patch fixed it by deleting the redundant comparison.
>
> Fixes: 67684d1e87b6 ("app/procinfo: call EAL cleanup before exit")
> Fixes: b68a82425da4 ("app/compress-perf: add performance measurement")
> Fixes: 5e516c89830a ("app/testpmd: call cleanup on exit")
> Fixes: 613ce6691c0d ("examples/l3fwd-power: implement proper shutdown")
> Fixes: 48be180de631 ("eal: move OS common debug functions to single file")
> Cc: stable@dpdk.org
>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>   app/pdump/main.c                         | 4 +---
>   app/proc-info/main.c                     | 4 +---
>   app/test-compress-perf/main.c            | 7 +------
>   app/test-pmd/testpmd.c                   | 5 +----
>   examples/l3fwd-power/main.c              | 3 +--
>   lib/librte_eal/common/eal_common_debug.c | 4 +---
>   6 files changed, 6 insertions(+), 21 deletions(-)
>
> diff --git a/app/pdump/main.c b/app/pdump/main.c
> index 63bbe65..33523c5 100644
> --- a/app/pdump/main.c
> +++ b/app/pdump/main.c
> @@ -1014,9 +1014,7 @@ main(int argc, char **argv)
>   	/* dump debug stats */
>   	print_pdump_stats();
>   
> -	ret = rte_eal_cleanup();
> -	if (ret)
> -		printf("Error from rte_eal_cleanup(), %d\n", ret);
> +	rte_eal_cleanup();
>   
>   	return 0;
>   }
> diff --git a/app/proc-info/main.c b/app/proc-info/main.c
> index b9587f7..9498490 100644
> --- a/app/proc-info/main.c
> +++ b/app/proc-info/main.c
> @@ -1458,9 +1458,7 @@ main(int argc, char **argv)
>   	RTE_ETH_FOREACH_DEV(i)
>   		rte_eth_dev_close(i);
>   
> -	ret = rte_eal_cleanup();
> -	if (ret)
> -		printf("Error from rte_eal_cleanup(), %d\n", ret);
> +	rte_eal_cleanup();
>   
>   	return 0;
>   }
> diff --git a/app/test-compress-perf/main.c b/app/test-compress-perf/main.c
> index cc9951a..03ee385 100644
> --- a/app/test-compress-perf/main.c
> +++ b/app/test-compress-perf/main.c
> @@ -474,12 +474,7 @@ main(int argc, char **argv)
>   		/* fallthrough */
>   	case ST_CLEAR:
>   	default:
> -		i = rte_eal_cleanup();
> -		if (i) {
> -			RTE_LOG(ERR, USER1,
> -				"Error from rte_eal_cleanup(), %d\n", i);
> -			ret = i;
> -		}
> +		rte_eal_cleanup();
>   		break;
>   	}
>   	return ret;
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index d4be23f..1f8a7b8 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3972,10 +3972,7 @@ main(int argc, char** argv)
>   			return 1;
>   	}
>   
> -	ret = rte_eal_cleanup();
> -	if (ret != 0)
> -		rte_exit(EXIT_FAILURE,
> -			 "EAL cleanup failed: %s\n", strerror(-ret));
> +	rte_eal_cleanup();
>   
>   	return EXIT_SUCCESS;
>   }
> diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
> index 0af8810..84464d5 100644
> --- a/examples/l3fwd-power/main.c
> +++ b/examples/l3fwd-power/main.c
> @@ -2914,8 +2914,7 @@ main(int argc, char **argv)
>   			deinit_power_library())
>   		rte_exit(EXIT_FAILURE, "deinit_power_library failed\n");
>   
> -	if (rte_eal_cleanup() < 0)
> -		RTE_LOG(ERR, L3FWD_POWER, "EAL cleanup failed\n");
> +	rte_eal_cleanup();
>   
>   	return 0;
>   }
> diff --git a/lib/librte_eal/common/eal_common_debug.c b/lib/librte_eal/common/eal_common_debug.c
> index 15418e9..04b1fed 100644
> --- a/lib/librte_eal/common/eal_common_debug.c
> +++ b/lib/librte_eal/common/eal_common_debug.c
> @@ -37,8 +37,6 @@ rte_exit(int exit_code, const char *format, ...)
>   	rte_vlog(RTE_LOG_CRIT, RTE_LOGTYPE_EAL, format, ap);
>   	va_end(ap);
>   
> -	if (rte_eal_cleanup() != 0)
> -		RTE_LOG(CRIT, EAL,
> -			"EAL could not release all resources\n");
> +	rte_eal_cleanup();
>   	exit(exit_code);
>   }


Since the function is defined as "int rte_eal_cleanup(void)" and the 
documentation says:

  *  - 0 Successfully released all internal EAL resources.
  *  - -EFAULT There was an error in releasing all resources.

We really should check for non zero values. Just because it's currently 
coded to only return zero may not hold in the future. The code may 
change in the future to return non-zero values.

Rgds,
Dave.
Stephen Hemminger April 19, 2021, 3:06 p.m. UTC | #2 
On Mon, 19 Apr 2021 20:29:51 +0800
"Min Hu (Connor)" <humin29@huawei.com> wrote:

> The return value of 'rte_eal_cleanup' is always zero, so comparison
> with zero is redundant.
> 
> This patch fixed it by deleting the redundant comparison.
> 
> Fixes: 67684d1e87b6 ("app/procinfo: call EAL cleanup before exit")
> Fixes: b68a82425da4 ("app/compress-perf: add performance measurement")
> Fixes: 5e516c89830a ("app/testpmd: call cleanup on exit")
> Fixes: 613ce6691c0d ("examples/l3fwd-power: implement proper shutdown")
> Fixes: 48be180de631 ("eal: move OS common debug functions to single file")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

Please keep it in testpmd, which should be checking everything.
For the others, the message doesn't add any value to the user and can be removed.
Andrew Rybchenko July 23, 2021, 8:16 p.m. UTC | #3 
On 4/19/21 3:44 PM, David Hunt wrote:
> Hi Connor,
> 
> On 19/4/2021 1:29 PM, Min Hu (Connor) wrote:
>> The return value of 'rte_eal_cleanup' is always zero, so comparison
>> with zero is redundant.
>>
>> This patch fixed it by deleting the redundant comparison.
>>
>> Fixes: 67684d1e87b6 ("app/procinfo: call EAL cleanup before exit")
>> Fixes: b68a82425da4 ("app/compress-perf: add performance measurement")
>> Fixes: 5e516c89830a ("app/testpmd: call cleanup on exit")
>> Fixes: 613ce6691c0d ("examples/l3fwd-power: implement proper shutdown")
>> Fixes: 48be180de631 ("eal: move OS common debug functions to single 
>> file")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>>   app/pdump/main.c                         | 4 +---
>>   app/proc-info/main.c                     | 4 +---
>>   app/test-compress-perf/main.c            | 7 +------
>>   app/test-pmd/testpmd.c                   | 5 +----
>>   examples/l3fwd-power/main.c              | 3 +--
>>   lib/librte_eal/common/eal_common_debug.c | 4 +---
>>   6 files changed, 6 insertions(+), 21 deletions(-)
>>
>> diff --git a/app/pdump/main.c b/app/pdump/main.c
>> index 63bbe65..33523c5 100644
>> --- a/app/pdump/main.c
>> +++ b/app/pdump/main.c
>> @@ -1014,9 +1014,7 @@ main(int argc, char **argv)
>>       /* dump debug stats */
>>       print_pdump_stats();
>> -    ret = rte_eal_cleanup();
>> -    if (ret)
>> -        printf("Error from rte_eal_cleanup(), %d\n", ret);
>> +    rte_eal_cleanup();
>>       return 0;
>>   }
>> diff --git a/app/proc-info/main.c b/app/proc-info/main.c
>> index b9587f7..9498490 100644
>> --- a/app/proc-info/main.c
>> +++ b/app/proc-info/main.c
>> @@ -1458,9 +1458,7 @@ main(int argc, char **argv)
>>       RTE_ETH_FOREACH_DEV(i)
>>           rte_eth_dev_close(i);
>> -    ret = rte_eal_cleanup();
>> -    if (ret)
>> -        printf("Error from rte_eal_cleanup(), %d\n", ret);
>> +    rte_eal_cleanup();
>>       return 0;
>>   }
>> diff --git a/app/test-compress-perf/main.c 
>> b/app/test-compress-perf/main.c
>> index cc9951a..03ee385 100644
>> --- a/app/test-compress-perf/main.c
>> +++ b/app/test-compress-perf/main.c
>> @@ -474,12 +474,7 @@ main(int argc, char **argv)
>>           /* fallthrough */
>>       case ST_CLEAR:
>>       default:
>> -        i = rte_eal_cleanup();
>> -        if (i) {
>> -            RTE_LOG(ERR, USER1,
>> -                "Error from rte_eal_cleanup(), %d\n", i);
>> -            ret = i;
>> -        }
>> +        rte_eal_cleanup();
>>           break;
>>       }
>>       return ret;
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index d4be23f..1f8a7b8 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -3972,10 +3972,7 @@ main(int argc, char** argv)
>>               return 1;
>>       }
>> -    ret = rte_eal_cleanup();
>> -    if (ret != 0)
>> -        rte_exit(EXIT_FAILURE,
>> -             "EAL cleanup failed: %s\n", strerror(-ret));
>> +    rte_eal_cleanup();
>>       return EXIT_SUCCESS;
>>   }
>> diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
>> index 0af8810..84464d5 100644
>> --- a/examples/l3fwd-power/main.c
>> +++ b/examples/l3fwd-power/main.c
>> @@ -2914,8 +2914,7 @@ main(int argc, char **argv)
>>               deinit_power_library())
>>           rte_exit(EXIT_FAILURE, "deinit_power_library failed\n");
>> -    if (rte_eal_cleanup() < 0)
>> -        RTE_LOG(ERR, L3FWD_POWER, "EAL cleanup failed\n");
>> +    rte_eal_cleanup();
>>       return 0;
>>   }
>> diff --git a/lib/librte_eal/common/eal_common_debug.c 
>> b/lib/librte_eal/common/eal_common_debug.c
>> index 15418e9..04b1fed 100644
>> --- a/lib/librte_eal/common/eal_common_debug.c
>> +++ b/lib/librte_eal/common/eal_common_debug.c
>> @@ -37,8 +37,6 @@ rte_exit(int exit_code, const char *format, ...)
>>       rte_vlog(RTE_LOG_CRIT, RTE_LOGTYPE_EAL, format, ap);
>>       va_end(ap);
>> -    if (rte_eal_cleanup() != 0)
>> -        RTE_LOG(CRIT, EAL,
>> -            "EAL could not release all resources\n");
>> +    rte_eal_cleanup();
>>       exit(exit_code);
>>   }
> 
> 
> Since the function is defined as "int rte_eal_cleanup(void)" and the 
> documentation says:
> 
>   *  - 0 Successfully released all internal EAL resources.
>   *  - -EFAULT There was an error in releasing all resources.
> 
> We really should check for non zero values. Just because it's currently 
> coded to only return zero may not hold in the future. The code may 
> change in the future to return non-zero values.

+1

I think the patch should be rejected.
humin (Q) July 24, 2021, 12:35 a.m. UTC | #4 
Agreed, This patch can be rejected, thanks.

在 2021/7/24 4:16, Andrew Rybchenko 写道:
> On 4/19/21 3:44 PM, David Hunt wrote:
>> Hi Connor,
>>
>> On 19/4/2021 1:29 PM, Min Hu (Connor) wrote:
>>> The return value of 'rte_eal_cleanup' is always zero, so comparison
>>> with zero is redundant.
>>>
>>> This patch fixed it by deleting the redundant comparison.
>>>
>>> Fixes: 67684d1e87b6 ("app/procinfo: call EAL cleanup before exit")
>>> Fixes: b68a82425da4 ("app/compress-perf: add performance measurement")
>>> Fixes: 5e516c89830a ("app/testpmd: call cleanup on exit")
>>> Fixes: 613ce6691c0d ("examples/l3fwd-power: implement proper shutdown")
>>> Fixes: 48be180de631 ("eal: move OS common debug functions to single 
>>> file")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> ---
>>>   app/pdump/main.c                         | 4 +---
>>>   app/proc-info/main.c                     | 4 +---
>>>   app/test-compress-perf/main.c            | 7 +------
>>>   app/test-pmd/testpmd.c                   | 5 +----
>>>   examples/l3fwd-power/main.c              | 3 +--
>>>   lib/librte_eal/common/eal_common_debug.c | 4 +---
>>>   6 files changed, 6 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/app/pdump/main.c b/app/pdump/main.c
>>> index 63bbe65..33523c5 100644
>>> --- a/app/pdump/main.c
>>> +++ b/app/pdump/main.c
>>> @@ -1014,9 +1014,7 @@ main(int argc, char **argv)
>>>       /* dump debug stats */
>>>       print_pdump_stats();
>>> -    ret = rte_eal_cleanup();
>>> -    if (ret)
>>> -        printf("Error from rte_eal_cleanup(), %d\n", ret);
>>> +    rte_eal_cleanup();
>>>       return 0;
>>>   }
>>> diff --git a/app/proc-info/main.c b/app/proc-info/main.c
>>> index b9587f7..9498490 100644
>>> --- a/app/proc-info/main.c
>>> +++ b/app/proc-info/main.c
>>> @@ -1458,9 +1458,7 @@ main(int argc, char **argv)
>>>       RTE_ETH_FOREACH_DEV(i)
>>>           rte_eth_dev_close(i);
>>> -    ret = rte_eal_cleanup();
>>> -    if (ret)
>>> -        printf("Error from rte_eal_cleanup(), %d\n", ret);
>>> +    rte_eal_cleanup();
>>>       return 0;
>>>   }
>>> diff --git a/app/test-compress-perf/main.c 
>>> b/app/test-compress-perf/main.c
>>> index cc9951a..03ee385 100644
>>> --- a/app/test-compress-perf/main.c
>>> +++ b/app/test-compress-perf/main.c
>>> @@ -474,12 +474,7 @@ main(int argc, char **argv)
>>>           /* fallthrough */
>>>       case ST_CLEAR:
>>>       default:
>>> -        i = rte_eal_cleanup();
>>> -        if (i) {
>>> -            RTE_LOG(ERR, USER1,
>>> -                "Error from rte_eal_cleanup(), %d\n", i);
>>> -            ret = i;
>>> -        }
>>> +        rte_eal_cleanup();
>>>           break;
>>>       }
>>>       return ret;
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>>> index d4be23f..1f8a7b8 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -3972,10 +3972,7 @@ main(int argc, char** argv)
>>>               return 1;
>>>       }
>>> -    ret = rte_eal_cleanup();
>>> -    if (ret != 0)
>>> -        rte_exit(EXIT_FAILURE,
>>> -             "EAL cleanup failed: %s\n", strerror(-ret));
>>> +    rte_eal_cleanup();
>>>       return EXIT_SUCCESS;
>>>   }
>>> diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
>>> index 0af8810..84464d5 100644
>>> --- a/examples/l3fwd-power/main.c
>>> +++ b/examples/l3fwd-power/main.c
>>> @@ -2914,8 +2914,7 @@ main(int argc, char **argv)
>>>               deinit_power_library())
>>>           rte_exit(EXIT_FAILURE, "deinit_power_library failed\n");
>>> -    if (rte_eal_cleanup() < 0)
>>> -        RTE_LOG(ERR, L3FWD_POWER, "EAL cleanup failed\n");
>>> +    rte_eal_cleanup();
>>>       return 0;
>>>   }
>>> diff --git a/lib/librte_eal/common/eal_common_debug.c 
>>> b/lib/librte_eal/common/eal_common_debug.c
>>> index 15418e9..04b1fed 100644
>>> --- a/lib/librte_eal/common/eal_common_debug.c
>>> +++ b/lib/librte_eal/common/eal_common_debug.c
>>> @@ -37,8 +37,6 @@ rte_exit(int exit_code, const char *format, ...)
>>>       rte_vlog(RTE_LOG_CRIT, RTE_LOGTYPE_EAL, format, ap);
>>>       va_end(ap);
>>> -    if (rte_eal_cleanup() != 0)
>>> -        RTE_LOG(CRIT, EAL,
>>> -            "EAL could not release all resources\n");
>>> +    rte_eal_cleanup();
>>>       exit(exit_code);
>>>   }
>>
>>
>> Since the function is defined as "int rte_eal_cleanup(void)" and the 
>> documentation says:
>>
>>   *  - 0 Successfully released all internal EAL resources.
>>   *  - -EFAULT There was an error in releasing all resources.
>>
>> We really should check for non zero values. Just because it's 
>> currently coded to only return zero may not hold in the future. The 
>> code may change in the future to return non-zero values.
> 
> +1
> 
> I think the patch should be rejected.
> .
diff mbox series

Patch

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 63bbe65..33523c5 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -1014,9 +1014,7 @@  main(int argc, char **argv)
 	/* dump debug stats */
 	print_pdump_stats();
 
-	ret = rte_eal_cleanup();
-	if (ret)
-		printf("Error from rte_eal_cleanup(), %d\n", ret);
+	rte_eal_cleanup();
 
 	return 0;
 }
diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index b9587f7..9498490 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -1458,9 +1458,7 @@  main(int argc, char **argv)
 	RTE_ETH_FOREACH_DEV(i)
 		rte_eth_dev_close(i);
 
-	ret = rte_eal_cleanup();
-	if (ret)
-		printf("Error from rte_eal_cleanup(), %d\n", ret);
+	rte_eal_cleanup();
 
 	return 0;
 }
diff --git a/app/test-compress-perf/main.c b/app/test-compress-perf/main.c
index cc9951a..03ee385 100644
--- a/app/test-compress-perf/main.c
+++ b/app/test-compress-perf/main.c
@@ -474,12 +474,7 @@  main(int argc, char **argv)
 		/* fallthrough */
 	case ST_CLEAR:
 	default:
-		i = rte_eal_cleanup();
-		if (i) {
-			RTE_LOG(ERR, USER1,
-				"Error from rte_eal_cleanup(), %d\n", i);
-			ret = i;
-		}
+		rte_eal_cleanup();
 		break;
 	}
 	return ret;
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index d4be23f..1f8a7b8 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3972,10 +3972,7 @@  main(int argc, char** argv)
 			return 1;
 	}
 
-	ret = rte_eal_cleanup();
-	if (ret != 0)
-		rte_exit(EXIT_FAILURE,
-			 "EAL cleanup failed: %s\n", strerror(-ret));
+	rte_eal_cleanup();
 
 	return EXIT_SUCCESS;
 }
diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index 0af8810..84464d5 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -2914,8 +2914,7 @@  main(int argc, char **argv)
 			deinit_power_library())
 		rte_exit(EXIT_FAILURE, "deinit_power_library failed\n");
 
-	if (rte_eal_cleanup() < 0)
-		RTE_LOG(ERR, L3FWD_POWER, "EAL cleanup failed\n");
+	rte_eal_cleanup();
 
 	return 0;
 }
diff --git a/lib/librte_eal/common/eal_common_debug.c b/lib/librte_eal/common/eal_common_debug.c
index 15418e9..04b1fed 100644
--- a/lib/librte_eal/common/eal_common_debug.c
+++ b/lib/librte_eal/common/eal_common_debug.c
@@ -37,8 +37,6 @@  rte_exit(int exit_code, const char *format, ...)
 	rte_vlog(RTE_LOG_CRIT, RTE_LOGTYPE_EAL, format, ap);
 	va_end(ap);
 
-	if (rte_eal_cleanup() != 0)
-		RTE_LOG(CRIT, EAL,
-			"EAL could not release all resources\n");
+	rte_eal_cleanup();
 	exit(exit_code);
 }