Re: [dpdk-dev] [PATCH] app: fix redundant comparison

[David Hunt <david.hunt@intel.com>]

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.