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 <j...@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
