Hi Jie,
2021-06-23 17:36 (UTC-0700), Jie Zhou:
> From: Jie Zhou <j...@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 <j...@microsoft.com>
> Signed-off-by: Jie Zhou <j...@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
