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 D53E8A0C4B; Wed, 7 Jul 2021 19:29:54 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 96FAD413DB; Wed, 7 Jul 2021 19:29:54 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 10B65413B6 for ; Wed, 7 Jul 2021 19:29:53 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1061) id 67E9C20B7188; Wed, 7 Jul 2021 10:29:52 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 67E9C20B7188 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1625678992; bh=q07BpQ1YFR7AL8ncbKYwGGPUqCFwH4PZdYazs1YnNW4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=MaSX8yFRflaQdCSq2LaVjN5Aag+MqzUQh5g/gnll8TAtZ3j8hm5cxNemDhklAZ7jn EuH23Y/q11/9JYEA8Z5hdoEPLwR5jlXB2KHOn5B7CArfkkBLHyh0TW63EE+pjvUcFJ aORtXw1So5mKG6cB3l3ZE0tUxuJDGxqq4IMnZckQ= Date: Wed, 7 Jul 2021 10:29:52 -0700 From: Jie Zhou To: Tyler Retzlaff Cc: Dmitry Kozlyuk , dev@dpdk.org, roretzla@microsoft.com, talshn@nvidia.com, pallavi.kadam@intel.com, navasile@linux.microsoft.com, dmitrym@microsoft.com Message-ID: <20210707172952.GA11852@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> <20210701214524.GA2068@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210701214524.GA2068@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> 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:45:24PM -0700, Tyler Retzlaff wrote: > 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 Thanks Dmitry and Tyler. Will address these in V2.