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.
> * 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.
