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

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

[Zhichao Zeng]

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';
 
-- 
2.34.1

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

[Bruce Richardson]

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.