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

Tyler Retzlaff roretzla at linux.microsoft.com
Thu Jul 1 23:45:24 CEST 2021


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 at 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


More information about the dev mailing list