Hi Bruce,
Thanks for your suggestions, will send v2 patch.
Regards
Zhichao
> -----Original Message-----
> From: Richardson, Bruce <bruce.richard...@intel.com>
> Sent: Friday, November 1, 2024 5:36 PM
> To: Zeng, ZhichaoX <zhichaox.z...@intel.com>
> Cc: dev@dpdk.org; Burakov, Anatoly <anatoly.bura...@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.z...@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.
