Re: [dpdk-dev] [PATCH v1] lib/eal: enforce alarm APIs parameters check

[Jie Zhou <jizh@linux.microsoft.com>]

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 <jizh@microsoft.com>
> > > [...]
> > > > > +	/* 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.