RE: [PATCH] net/ice: fix wrong DDP search path

["Zeng, ZhichaoX" <zhichaox.zeng@intel.com>]

Hi Bruce,

Thanks for your suggestions, will send v2 patch.

Regards
Zhichao

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Friday, November 1, 2024 5:36 PM
> To: Zeng, ZhichaoX <zhichaox.zeng@intel.com>
> Cc: dev@dpdk.org; Burakov, Anatoly <anatoly.burakov@intel.com>
> Subject: Re: [PATCH] net/ice: fix wrong DDP search path
> 
> On Fri, Nov 01, 2024 at 04:44:43PM +0800, Zhichao Zeng wrote:
> > In the previous implementation, when the user did not enter any value
> > in "/sys/module/firmware_class/parameters/path", it would incorrectly
> > search for DDP packages under "/". This commit fixes this issue.
> >
> > Fixes: 9207f93640a7 ("net/ice: support custom search path for DDP
> > package")
> >
> > Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
> > ---
> >  drivers/net/ice/ice_ethdev.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ice/ice_ethdev.c
> > b/drivers/net/ice/ice_ethdev.c index d5e94a6685..0705f8e961 100644
> > --- a/drivers/net/ice/ice_ethdev.c
> > +++ b/drivers/net/ice/ice_ethdev.c
> > @@ -1922,8 +1922,11 @@ static int ice_read_customized_path(char
> *pkg_file, uint16_t buff_len)
> >  		return -EIO;
> >  	}
> >
> > -	if (pkg_file[n - 1] == '\n')
> > +	if (pkg_file[n - 1] == '\n') {
> >  		n--;
> > +		if (n == 0)
> > +			return -EINVAL;
> > +	}
> >
> >  	pkg_file[n] = '\0';
> >
> 
> May I suggest a slightly alternative fix, that I think it a little shorter and neater
> (assuming it works - if not, let me know.)
> 
> Rather than adding an explicit check for n==0 and returning error, I think we
> can instead change the return value of the function to be the length of the
> data read. So at line 1931 "return 0" we can change that to "return n".
> Then at the call site for the function we can change:
> 
> 	if (ice_read_customized_path(....) == 0) {
> 
> to
> 	if (ice_read_customized_path(....) > 0) {
> 
> What do you think?
> 
> /Bruce
> 
> PS: if you do take this approach, we can also slightly shorten the function by
> changing/removing the block:
> 
> 	if (n == 0) {
> 		close(fp);
> 		return -EIO;
> 	}
> 	... /* length adjust and zeroing */
> 	return n;
> 
> to be an inverted check with the length adjustment and zeroing inside it:
> 	if (n > 0){
> 		if (pkg_file[n -1] == '\n')
> 			n--;
> 		pkg_file[n] = '\0';
> 	}
> 	close(fp);
> 	return n;
> 
> That gives us a zero return value too in case of reading zero bytes. It also
> handles the currently unhandled case of read returning < 0.