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 76BDDA0A0C; Thu, 1 Jul 2021 18:21:46 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F36734067C; Thu, 1 Jul 2021 18:21:45 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id D3F9040141 for ; Thu, 1 Jul 2021 18:21:44 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id 3233820B7178; Thu, 1 Jul 2021 09:21:44 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 3233820B7178 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1625156504; bh=gRZv0fjxx6GWoW8WftJ4utIhVPXZGKCfgcRvyUe1QiA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=qKjnjmbMwj2pg7mAzR+qEJEKAclue9WSjLEEG8Q041REg4z1x57SIwcinjbmNsR67 GEJkFAIU0KkVsjTXQ4etkGmRWuIu2kpicpCnJI5Opn4rIUMyxn1/D1Z/3YU3DlI21D 0Ms59Z5NTwIphkM4ZK9a6ilDNuxONRcE3B57xbOw= Date: Thu, 1 Jul 2021 09:21:44 -0700 From: Tyler Retzlaff To: Dmitry Kozlyuk Cc: Jie Zhou , dev@dpdk.org, roretzla@microsoft.com, talshn@nvidia.com, pallavi.kadam@intel.com, navasile@linux.microsoft.com, dmitrym@microsoft.com Message-ID: <20210701162144.GA9873@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1624495001-16613-1-git-send-email-jizh@linux.microsoft.com> <20210701023129.2ee5e076@sovereign> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210701023129.2ee5e076@sovereign> User-Agent: Mutt/1.5.21 (2010-09-15) 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" On Thu, Jul 01, 2021 at 02:31:29AM +0300, Dmitry Kozlyuk wrote: > 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 agree with your assessment. it's a bit silly to test a range constraint where the range is just some arbitrary range and not the real range. changing the implementation to calculate the "real" valid range isn't practical. i'm sure linux implementors would argue that catching some extremely out of range values is better than none? i guess a correct test would would calculate the valid range and test against that but as per above the implementation won't pass the test. so we are left with * matching the range imposed in the linux implementation so we pass the test as is. * don't run the test with the input data that exercises this bogus range constraint. i guess based on your comments you prefer the latter? so is our action here to submit a patch for the test that doesn't test this range conditionally on execenv windows? > 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? i'll discuss this subject to whether or not we remove the above range check. for now i'll defer discussion. > > > + 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