Re: [dpdk-dev] [PATCH] app/test-compress-perf: fix reliance on integer endianness

[Shally Verma <shallyv@marvell.com>]

HI Tomasz

> -----Original Message-----
> From: Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>
> Sent: Tuesday, May 21, 2019 12:18 PM
> To: Trahe, Fiona <fiona.trahe@intel.com>; dev@dpdk.org; Shally Verma
> <shallyv@marvell.com>; stable@dpdk.org
> Cc: Trybula, ArturX <arturx.trybula@intel.com>
> Subject: [EXT] RE: [PATCH] app/test-compress-perf: fix reliance on integer
> endianness
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Fiona,
> 
> Outlook issue :D  , so once again
> 
> > -----Original Message-----
> > From: Trahe, Fiona
> > Sent: Monday, May 20, 2019 4:06 PM
> > To: Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>; dev@dpdk.org;
> > shallyv@marvell.com; stable@dpdk.org
> > Cc: Trahe, Fiona <fiona.trahe@intel.com>; Trybula, ArturX
> > <arturx.trybula@intel.com>
> > Subject: RE: [PATCH] app/test-compress-perf: fix reliance on integer
> > endianness
> >
> > HI Tomasz,
> >
> > > -----Original Message-----
> > > From: Jozwiak, TomaszX
> > > Sent: Monday, May 20, 2019 2:26 PM
> > > To: dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>; Jozwiak,
> > > TomaszX <tomaszx.jozwiak@intel.com>; shallyv@marvell.com;
> > > stable@dpdk.org
> > > Subject: [PATCH] app/test-compress-perf: fix reliance on integer
> > > endianness
> > >
> > > This patch fixes coverity issue:
> > > Reliance on integer endianness (INCOMPATIBLE_CAST) in
> > parse_window_sz
> > > function.
> > >
> > > Coverity issue: 328524
> > > Fixes: e0b6287c035d ("app/compress-perf: add parser")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
> > > ---
> > >  app/test-compress-perf/comp_perf_options_parse.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/app/test-compress-perf/comp_perf_options_parse.c
> > > b/app/test-compress- perf/comp_perf_options_parse.c index
> > > 2fb6fb4..56ca580 100644
> > > --- a/app/test-compress-perf/comp_perf_options_parse.c
> > > +++ b/app/test-compress-perf/comp_perf_options_parse.c
> > > @@ -364,13 +364,15 @@ parse_max_num_sgl_segs(struct
> > comp_test_data
> > > *test_data, const char *arg)  static int  parse_window_sz(struct
> > > comp_test_data *test_data, const char *arg)  {
> > > -	int ret = parse_uint16_t((uint16_t *)&test_data->window_sz, arg);
> > > +	uint16_t tmp;
> > > +	int ret = parse_uint16_t(&tmp, arg);
> > >
> > >  	if (ret) {
> > >  		RTE_LOG(ERR, USER1, "Failed to parse window size\n");
> > >  		return -1;
> > >  	}
> > >
> > > +	test_data->window_sz = (int)tmp;
> > >  	return 0;
> > >  }
> > [Fiona] I expect this fixes this coverity issue - but will it result in another one?
> > window_sz on the xform is uint8_t - so this int will get truncated
> > later, and there's no cast done at that point.
> > Would it be better to add a new parse_uint8_t fn and change test-data-
> > >window_sz to a unit8_t?
> > Or add that cast?
> [Tomek] I measn it's ok. There's a check inside comp_perf_check_capabilities
> function.
> If the value from test_data->window_sz > cap->window_size we have a fail.
> Also during parsing there's a check is value from command line between 0 and
> UINT16_MAX, so in my opinion all cases are tested. The point is there's only
> one place where we're parsing uint8_t value.  parse_uint8_t function will be
> especially for that.
[Shally] What is window_sz in test data ?is it base 2 log of (actual window length) or actual window length in bytes? lib spec mention this as struct rte_param_log2_range, so
If test window size is actual window length in bytes then I assume test perf should check for test_data->window_sz > 2 pow cap->window_size but that doesn't look like the case. 
So if it is log value, then coding wise typecasting here doesn't look right. Though it add need for extra function to parse_uint8, but that looks like cleaner approach to use.