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

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

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>
> > 
> > lib/eal alarm APIs rte_eal_alarm_set and rte_eal_alarm_cancel
> > on Windows do not check parameters to fail fast for invalid
> > parameters, which captured by DPDK UT alarm_autotest.
> 
> Please use past tense to describe situation before the patch.
> A nit, but browsing the log, I see that errors are usually "caught"
> rather then "captured"; consistency would be nice.
> 
> > 
> > Enforce Windows lib/eal alarm APIs parameters check and log
> > invalid parameter info.
> 
> Fixes tag needed.
> 
> > Signed-off-by: Jie Zhou <jizh@microsoft.com>
> > Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> > 
> > ---
> >  lib/eal/windows/eal_alarm.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/lib/eal/windows/eal_alarm.c b/lib/eal/windows/eal_alarm.c
> > index f5bf88715a..7bb79ae869 100644
> > --- a/lib/eal/windows/eal_alarm.c
> > +++ b/lib/eal/windows/eal_alarm.c
> > @@ -4,6 +4,7 @@
> >  
> >  #include <stdatomic.h>
> >  #include <stdbool.h>
> > +#include <inttypes.h>
> >  
> >  #include <rte_alarm.h>
> >  #include <rte_spinlock.h>
> > @@ -91,6 +92,22 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
> >  	LARGE_INTEGER deadline;
> >  	int ret;
> >  
> > +	/* 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?

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.

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

> I understand that you're enabling UT for Windows
> and not correcting tests themselves, but I'm against inserting checks known
> to be incorrect.
> 
> > +		RTE_LOG(ERR, EAL, "Invalid us: %" PRIu64 "\n"
> > +			"Valid us range is 1 to (UINT64_MAX - US_PER_S)\n",
> > +			us);
> 
> Why does Windows need these messages, while Linux and FreeBSD don't?
> How will printing API contract here help the user who gets the message?

i'll discuss this subject to whether or not we remove the above range
check. for now i'll defer discussion.

> 
> > +		ret = -EINVAL;
> > +		goto exit;
> > +	}
> > +
> > +	/* Check if callback is not NULL */
> > +	if (!cb_fn) {
> 
> Pointers (`cb_fn`) must be checked for `NULL` explicitly.
> You won't need an obvious comment after that.
> 
> > +		RTE_LOG(ERR, EAL, "NULL callback\n");
> > +		ret = -EINVAL;
> > +		goto exit;
> > +	}
> > +
> >  	/* Calculate deadline ASAP, unit of measure = 100ns. */
> >  	GetSystemTimePreciseAsFileTime(&ft);
> >  	deadline.LowPart = ft.dwLowDateTime;
> > @@ -180,6 +197,12 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
> >  	bool executing;
> >  
> >  	removed = 0;
> > +
> > +	if (!cb_fn) {
> > +		RTE_LOG(ERR, EAL, "NULL callback\n");
> > +		return -EINVAL;
> > +	}
> > +
> >  	do {
> >  		executing = false;
> >  
> 
> Please also fix other style issues:
> http://mails.dpdk.org/archives/test-report/2021-June/200580.html