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 56521A0A0C; Thu, 1 Jul 2021 23:45:26 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 136BC40141; Thu, 1 Jul 2021 23:45:26 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id D91514003E for ; Thu, 1 Jul 2021 23:45:24 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id 36F5120B7178; Thu, 1 Jul 2021 14:45:24 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 36F5120B7178 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1625175924; bh=oX7tbJzD5k+QFcAwtF8xF8Lg1rIJWh4prdCHfUIYZFg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=LqKJ7OwjW87d/3qwgX3NcDOdM7GlOx/tq0RvJ3zhgqtE9fmXDOg7eJtHZG9n8W/Y3 KW9sSx9fDUyocl1xXjTJE6eRg1eOF0veun/JaN0r1Td9uzogMlZYvorjcGpwGh2fqg P74DD8PGWAjlE1QwyvjrxnwuxPZXuR0fU9nRo+eg= Date: Thu, 1 Jul 2021 14:45:24 -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: <20210701214524.GA2068@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1624495001-16613-1-git-send-email-jizh@linux.microsoft.com> <20210701023129.2ee5e076@sovereign> <20210701162144.GA9873@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <20210702003645.13813371@sovereign> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210702003645.13813371@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 Fri, Jul 02, 2021 at 12:36:45AM +0300, Dmitry Kozlyuk wrote: > 2021-07-01 09:21 (UTC-0700), Tyler Retzlaff: > > 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 > > [...] > > > > + /* 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? > > Why we care about overflow? > 1. It likely indicates a bug in the app. > 2. Alarm is can to be scheduled to some time in the past and fire > immediately, which means API did not do what it promises. > I see the only way to tell the interval is incorrect if it gives > (would give) time in the past when added to the current time. > But this is an implementation detail and does not need testing. > A separate patch can be submitted to change behavior for all OS. > > > > > 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. > > It would be the worst thing one could do, defying the purpose of unit tests. agreed. > > > * 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? > > Yes, please remove this check from the test as it verifies no contract. we'll go with this. thanks