net/ice: fix wrong DDP search path
Checks
Commit Message
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(-)
Comments
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.
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.
@@ -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';