From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
To: Tyler Retzlaff <roretzla@linux.microsoft.com>
Cc: Jie Zhou <jizh@linux.microsoft.com>,
dev@dpdk.org, roretzla@microsoft.com, talshn@nvidia.com,
pallavi.kadam@intel.com, navasile@linux.microsoft.com,
dmitrym@microsoft.com
Subject: Re: [dpdk-dev] [PATCH v1] lib/eal: enforce alarm APIs parameters check
Date: Fri, 2 Jul 2021 00:36:45 +0300 [thread overview]
Message-ID: <20210702003645.13813371@sovereign> (raw)
In-Reply-To: <20210701162144.GA9873@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
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.
> * 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.
next prev parent reply other threads:[~2021-07-01 21:36 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-24 0:36 Jie Zhou
2021-06-30 23:31 ` Dmitry Kozlyuk
2021-07-01 16:21 ` Tyler Retzlaff
2021-07-01 21:36 ` Dmitry Kozlyuk [this message]
2021-07-01 21:45 ` Tyler Retzlaff
2021-07-07 17:29 ` Jie Zhou
2021-07-07 20:25 ` [dpdk-dev] [PATCH v2] eal/windows: enforce alarm APIs parameter check Jie Zhou
2021-07-21 15:28 ` Dmitry Kozlyuk
2021-07-22 20:06 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210702003645.13813371@sovereign \
--to=dmitry.kozliuk@gmail.com \
--cc=dev@dpdk.org \
--cc=dmitrym@microsoft.com \
--cc=jizh@linux.microsoft.com \
--cc=navasile@linux.microsoft.com \
--cc=pallavi.kadam@intel.com \
--cc=roretzla@linux.microsoft.com \
--cc=roretzla@microsoft.com \
--cc=talshn@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).