From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
by inbox.dpdk.org (Postfix) with ESMTP id 6341AA0A0C;
Thu, 1 Jul 2021 01:31:33 +0200 (CEST)
Received: from [217.70.189.124] (localhost [127.0.0.1])
by mails.dpdk.org (Postfix) with ESMTP id DB81840141;
Thu, 1 Jul 2021 01:31:32 +0200 (CEST)
Received: from mail-lf1-f41.google.com (mail-lf1-f41.google.com
[209.85.167.41]) by mails.dpdk.org (Postfix) with ESMTP id CBB9740040
for <dev@dpdk.org>; Thu, 1 Jul 2021 01:31:31 +0200 (CEST)
Received: by mail-lf1-f41.google.com with SMTP id n14so8239193lfu.8
for <dev@dpdk.org>; Wed, 30 Jun 2021 16:31:31 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;
h=date:from:to:cc:subject:message-id:in-reply-to:references
:mime-version:content-transfer-encoding;
bh=ELX2HSbdyV+pH7NsOt1o7JniFL2+JRjCGCrQprL7se8=;
b=dQ3alcAzhsRZj42/5xR1Xuk2efhtVott2F6rXMi5gg1CeI+uG6XD+oejOjqdeBor5X
tDcq5z4quJdskGUPkEps36eSRytSjr2SapGdXQTJSYHKPQzm8a3EewOBpW+v7fCgvWsi
RB9C7SAF4Rfd1yCdHe7bTKtLL2PBMQwHdf9xje6N+XPREQhDubKS5UFCFfKHt4kEc5Dj
1G7a8X4F0Mk/Jk6+nAU8fjS8n0gRh2ycPa5gwdmehWbzPjraSTME9PLmFegWqUU6Fpli
9yMjLr2YNH6c/GGKChpUFb9AljmPotUupMqiMmRZXGpBMg50IiKHCBS/N+Ol47FFMxGg
k6rw==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20161025;
h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to
:references:mime-version:content-transfer-encoding;
bh=ELX2HSbdyV+pH7NsOt1o7JniFL2+JRjCGCrQprL7se8=;
b=LxvUWlydx8uou8Rz8RrFUpOKmJsP+hP4izVoAFEzS3x3DMnpLDs2Ynbj+UMH+iMajA
aB7G/gVuDmP4HMP32IiBgWmtp8fm52UaHL0vS1BHVFoyAGs5jNmaJzk+TfJ/OQ+3RMNz
FsQCQQMmVlhl3MdxpzmOu2yjdYKwMANpiInar3JuyNaLzU5nLnEQ0O80sUZpU9yoDp+A
ik8HZyucTTo3XNKz912zP4+wN1SXygqNNKP69q1OUN+SiHO3fYPveyxhpRvNNG1prdwX
9Lv4F3uukzUiM6e5otRGj7KUHIenl2L3FTdjSxQe94OVCKf7VaqcOwy1vfGbRfSpL02W
1tGw==
X-Gm-Message-State: AOAM531ku8LtJXs8vQfL3jO8sblMCVjCVNn0B0QsueTBqutxJ0mY3wKK
QzAaPyS1o/u9PfPN22Wj8Rk=
X-Google-Smtp-Source: ABdhPJzvT45mA5sl5jrqfcUc/Vwzt94g1yY8v6Hl7z3sT5MU4iTVSOmA1bDAhBZGVI1kXKp1msDksg==
X-Received: by 2002:a05:6512:203b:: with SMTP id
s27mr23856lfs.205.1625095891470;
Wed, 30 Jun 2021 16:31:31 -0700 (PDT)
Received: from sovereign (broadband-37-110-65-23.ip.moscow.rt.ru.
[37.110.65.23])
by smtp.gmail.com with ESMTPSA id c9sm2279886ljr.104.2021.06.30.16.31.30
(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
Wed, 30 Jun 2021 16:31:30 -0700 (PDT)
Date: Thu, 1 Jul 2021 02:31:29 +0300
From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
To: Jie Zhou <jizh@linux.microsoft.com>
Cc: dev@dpdk.org, roretzla@microsoft.com, talshn@nvidia.com,
pallavi.kadam@intel.com, navasile@linux.microsoft.com,
dmitrym@microsoft.com
Message-ID: <20210701023129.2ee5e076@sovereign>
In-Reply-To: <1624495001-16613-1-git-send-email-jizh@linux.microsoft.com>
References: <1624495001-16613-1-git-send-email-jizh@linux.microsoft.com>
X-Mailer: Claws Mail 3.17.8 (GTK+ 2.24.32; x86_64-pc-linux-gnu)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Subject: Re: [dpdk-dev] [PATCH v1] lib/eal: enforce alarm APIs parameters
check
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
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