Re: [dpdk-dev] [PATCH v1] lib/eal: enforce alarm APIs parameters check
From: Dmitry Kozlyuk <hidden>
Date: 2021-06-30 23:31:34
Hi Jie, 2021-06-23 17:36 (UTC-0700), Jie Zhou:
From: Jie Zhou <redacted> 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.
quoted hunk ↗ jump to hunk
Signed-off-by: Jie Zhou <redacted> Signed-off-by: Jie Zhou <redacted> --- 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.
quoted hunk ↗ jump to hunk
+ 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