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 <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
Thanks Dmitry and Tyler. Will address these in V2.
