Re: [dpdk-dev] [PATCH v1] lib/eal: enforce alarm APIs parameters check

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

Hi Jie,

2021-06-23 17:36 (UTC-0700), Jie Zhou:
> From: Jie Zhou <jizh@microsoft.com>
> 
> lib/eal alarm APIs rte_eal_alarm_set and rte_eal_alarm_cancel
> on Windows do not check parameters to fail fast for invalid
> parameters, which captured by DPDK UT alarm_autotest.

Please use past tense to describe situation before the patch.
A nit, but browsing the log, I see that errors are usually "caught"
rather then "captured"; consistency would be nice.

> 
> Enforce Windows lib/eal alarm APIs parameters check and log
> invalid parameter info.

Fixes tag needed.

> Signed-off-by: Jie Zhou <jizh@microsoft.com>
> Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> 
> ---
>  lib/eal/windows/eal_alarm.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/lib/eal/windows/eal_alarm.c b/lib/eal/windows/eal_alarm.c
> index f5bf88715a..7bb79ae869 100644
> --- a/lib/eal/windows/eal_alarm.c
> +++ b/lib/eal/windows/eal_alarm.c
> @@ -4,6 +4,7 @@
>  
>  #include <stdatomic.h>
>  #include <stdbool.h>
> +#include <inttypes.h>
>  
>  #include <rte_alarm.h>
>  #include <rte_spinlock.h>
> @@ -91,6 +92,22 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
>  	LARGE_INTEGER deadline;
>  	int ret;
>  
> +	/* Check if us is valid */
> +	if (us < 1 || us >(UINT64_MAX - US_PER_S)) {

This condition is specific to Linux EAL. In fact, it's not very useful even
there, because actual upper bound for `us` depends on current time.
No bounds are specified in API description at all.
Windows check would be different, but these considerations remain valid.

Maybe it's alarm_autotest or API description that needs adjustments,
but not the implementation. I understand that you're enabling UT for Windows
and not correcting tests themselves, but I'm against inserting checks known
to be incorrect.

> +		RTE_LOG(ERR, EAL, "Invalid us: %" PRIu64 "\n"
> +			"Valid us range is 1 to (UINT64_MAX - US_PER_S)\n",
> +			us);

Why does Windows need these messages, while Linux and FreeBSD don't?
How will printing API contract here help the user who gets the message?

> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
> +	/* Check if callback is not NULL */
> +	if (!cb_fn) {

Pointers (`cb_fn`) must be checked for `NULL` explicitly.
You won't need an obvious comment after that.

> +		RTE_LOG(ERR, EAL, "NULL callback\n");
> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
>  	/* Calculate deadline ASAP, unit of measure = 100ns. */
>  	GetSystemTimePreciseAsFileTime(&ft);
>  	deadline.LowPart = ft.dwLowDateTime;
> @@ -180,6 +197,12 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
>  	bool executing;
>  
>  	removed = 0;
> +
> +	if (!cb_fn) {
> +		RTE_LOG(ERR, EAL, "NULL callback\n");
> +		return -EINVAL;
> +	}
> +
>  	do {
>  		executing = false;
>  

Please also fix other style issues:
http://mails.dpdk.org/archives/test-report/2021-June/200580.html