From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; Thu, 1 Jul 2021 01:31:31 +0200 (CEST) Received: by mail-lf1-f41.google.com with SMTP id n14so8239193lfu.8 for ; 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 To: Jie Zhou 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Jie, 2021-06-23 17:36 (UTC-0700), Jie Zhou: > From: Jie Zhou > > 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 > Signed-off-by: Jie Zhou > > --- > 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 > #include > +#include > > #include > #include > @@ -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